search

Linaro / lite-lava-docker-compose

LITE Team LAVA docker dispatcher
MIT License
5 stars 10 forks source link

[RFC] Makefile: clean: Use "docker-compose down". #108

Closed pfalcon closed 4 years ago

pfalcon commented 4 years ago

Instead of removing containers and volumes "manually". The benefit is avoiding to enumerate which volumes to remove, as their set and names may change over time.

Another difference of "docket-compose down" is that it also removes related network interfaces (required for containers, etc. to communicate among themselves).

Signed-off-by: Paul Sokolovsky paul.sokolovsky@linaro.org

pfalcon commented 4 years ago

@galak: I'm not sure if you had specific reasons to do it originally like you did, so would like to run this thru you. I otherwise ran it like that last few weeks, and everything seems to work as expected. The motivation for this change is otherwise is (slow) motion towards re-syncing with upstream container/volume/etc. naming (which is "native" docker-compose naming (directory-namespaced)), which in turn will potentially allow to run 2 docker-compose services concurrently (e.g. latest release vs from-git devel setup).

vanti commented 4 years ago

@pfalcon There is one benefit I came across with the two step approach. It makes it clear which volumes need to be deleted, so if I want to upgrade LAVA without wiping out existing account and test result information it is as simple as commenting out the line

docker volume rm -f lava-server-pgdata lava-server-joboutput lava-server-devices lava-server-health-checks

and running make clean before the upgrade/rebase. Not sure if this outweighs what you are trying to achieve, but just thought I'd point it out since I have done this recently. Maybe it'd be good to at least leave the command with the list of volumes somewhere in the documentation in case someone needs it.

pfalcon commented 4 years ago

@vanti: Thanks for keeping an eye on this stuff. And let me start with saying that I in no way push for this patch, it's a kind of "slow motion" towards being able to (scalably) reconcile with some upstream changes which we didn't pick up. (I skipped them earlier exactly because they would require patching our Makefile and/or other scripts, and I didn't want to introduce possible instability at that time, but now trying to slowly catch up).

what you say makes sense and is a good usecase to keep it as is, at least for now. My counter-arguments would be:

It makes it clear which volumes need to be deleted

Kind of, but then it also kinda duplicates information in docker-compose.yaml (arguably, it contains too much information, so specifically the list of volumes is "less clear" in it), and we'd need to keep both in sync.

if I want to upgrade LAVA without wiping out existing account and test result information it is as simple as commenting out the line ... and running make clean.

Well, the purpose of "make clean" is to clean the environment completely, to start with another deployment from scratch. Upgrading is definitely an important usecase, but piggybacking it on top of "make clean" is a bit hacky.

With counter-arguments like that, it's still all not a big deal, so I'd be happy to postpone this patch for as a long as it makes sense.

pfalcon commented 4 years ago

and we'd need to keep both in sync.

I was looking for a recent patch I saw to prove the point, and here's what I see: https://git.lavasoftware.org/lava/pkg/docker-compose/-/merge_requests/26/diffs . D'oh! The explicit docker volume rm command in the Makefile wasn't added by @galak, like I thought, but comes directly from upstream! Well, let's treat it as an upstream "feature" then, given that there're usecases for it, as showed by @vanti.

Closing as irrelevant then.