Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docker:stop stops containers it shouldn't #87

Closed
jgangemi opened this issue Feb 4, 2015 · 24 comments
Closed

docker:stop stops containers it shouldn't #87

jgangemi opened this issue Feb 4, 2015 · 24 comments
Assignees
Milestone

Comments

@jgangemi
Copy link
Collaborator

jgangemi commented Feb 4, 2015

docker:stop will stop running containers that share the same image name but are completely unrelated to the configuration specified in the pom.

given maven is stateless, i'm not really sure how this would be tracked short of writing a file containing the actual container names some place the plugin could later reference when the goal runs.

i'm not sure it's kosher to write the file in ${basedir}, so it could be written to target but if the user were to run a mvn clean before a docker:stop, they'd end up trashing the file and the stop would fail requiring manual cleanup.

perhaps as a fallback it could default to current behavior if the file isn't found?

or it could fail and the user could do something like mvn docker:stop -DforceCleanup=true to cause the current behavior to be applied.

@rhuss
Copy link
Collaborator

rhuss commented Feb 5, 2015

Yes, I know it works a bit like a sledge hammer ;-)

Initially docker:stop was supposed to be called only together with docker:start for integration tests in the same process so that they can communicate in-process via the plugin context.

The current behaviour was introduced to make things a bit easier (but the best way to stop a container is still docker stop on the CLI).

Also, what should be stopped when

mvn docker:start &
mvn docker:start &
mvn docker:stop

Both containers, the last one only or the first ?

What I wonder, whether it could be possible to attach Meta-Data when starting a container, which could be queried when doing a docker:stop. I think, this would be a better solution than storing sth. in the file system.

@jgangemi
Copy link
Collaborator Author

jgangemi commented Feb 5, 2015

yeah - i'm not a huge fan of writing out a file either.

i think the behavior would continue to be the same as it is today, if you run mvn docker:stop in a project, it stops all containers that are associated w/ that project. otherwise you could override the property used to tie all the containers together via a -D parameter and then invoke your maven goals.

docker doesn't currently support this but it looks like there is a proposal/pull request (moby/moby#9882) that is trying to push this forward, so i guess we can back burner this for a little bit.

depending on if we wanted to support older versions of docker, we could use environment variables to store this info and query it using inspect.

at the same time, there's no reason we couldn't support both and then those users that can't upgrade wouldn't be left in the lurch if they really needed this.

i'll keep an eye on that proposal and give this some more thought.

@jgangemi
Copy link
Collaborator Author

jgangemi commented Feb 5, 2015

there's also this moby/moby#10599 which i just discovered today.

@rhuss
Copy link
Collaborator

rhuss commented Feb 6, 2015

What's about setting an artificial environment variable during start holding the meta data which then is then looked up when stopping a container ?

E.g. an env var DOCKER_MAVEN_PLUGIN_ID=<group-id>:<artifact-id>:<version> with the maven coordinates of the current pom.

@jgangemi
Copy link
Collaborator Author

jgangemi commented Feb 6, 2015

i actually thought about doing that and it would work for ppl who can't immediately upgrade once docker decides on how it will handle meta-data. i'll start putting something together for this.

@rhuss
Copy link
Collaborator

rhuss commented Mar 28, 2015

@jgangemi if you don't mind, I would to start to work on that so that I can include it into 0.11.3 (there are more people now requesting it)

@rhuss rhuss modified the milestones: 0.11.3, 0.12.0 Mar 28, 2015
@rhuss rhuss added the prio1 label Mar 28, 2015
@jgangemi
Copy link
Collaborator Author

go ahead - i've been tied up w/ the day job and some other things as of late so i haven't had the time to work on this.

@rhuss
Copy link
Collaborator

rhuss commented Mar 28, 2015

no problem, I know how this is ;-)

ok, will take over this issue ....

@rhuss rhuss assigned rhuss and unassigned jgangemi Mar 28, 2015
@rhuss rhuss modified the milestones: 0.11.3, 0.11.4 Apr 21, 2015
@vauvenal5
Copy link

I am not entirely sure if it is the same issue but...
I got a multi-module project and have configured the plugin to execute docker:start in the pre-integration-phase and docker:stop is bound to the clean phase.
No when I start up all the containers and change into a sub-module directory of my project I expect with mvn clean to stop only the one docker container which is related to this module however all my containers are stopped.

The containers do share the same image name indeed they are all java:8 containers but what I don't quite understand is why the plugin doesn't simply relay on the pom file on which it runs?
I mean I have clear container-names on every container that is executed which are assigned by the pom file responsible for that module. Why does the stop not relay on this names but goes instead after the image names?

@jgangemi
Copy link
Collaborator Author

jgangemi commented May 5, 2015

i think it's the same issue. stop uses the image name b/c that's how it's implemented. as @rhuss said in a previous comment, it's a bit of a sledgehammer.

i worked around this by just packaging the artifacts into the jdk container, the disk usage is the same and after i had a data-volume get deleted leaving me unable to restart an app, it seemed a bit safer :) but i realize that may not work for you.

i'm not sure how far along the implementation is - i've been super busy w/ some other things but they are starting to circle back around to where i'll find some time to address some issues that are paining me. :)

@rhuss just started a new job (congrats!) so i'm not sure what his status is. once he responds we can go from there.

@rhuss
Copy link
Collaborator

rhuss commented May 13, 2015

I'm on it, currently I'm experimenting by (mis-)using environment variables as lifecycle tags so that container can be correlated back to the pom.xml (even for a future process). It's a tricky business, though. I'm afraid it won't make it to 0.11.4 but to 0.12.0 soon after (prototype is working).

Thanks for the congratulations and since I'm now working for Red Hat if can spent my whole time on working on Open Source ;-). I won't work fulltime on this plugin, of course, but can hopefully be much more reactive than the last half year. Thank you for your patience ;-)

@rhuss
Copy link
Collaborator

rhuss commented Jun 14, 2015

Now that labels are supported by Docker on various levels, I should switch my env based implementation before releasing it. will take probably still yet another release.

@rhuss
Copy link
Collaborator

rhuss commented Jul 24, 2015

Not yet, just rediscovered this issue :)

@jgangemi
Copy link
Collaborator Author

any further updates on this one? if you don't have the time, if you pushed the branch i could pick it up.

@rhuss
Copy link
Collaborator

rhuss commented Aug 19, 2015

Sorry, not yet. But I will rebase my branch I had (if it is not to far way already) and will push it so that you could pick it up.

rhuss added a commit that referenced this issue Aug 19, 2015
This env variable WHALE_DMP_LABEL is used to mark containers to belong to a certain
build so that can be recognized when calling 'docker:stop' #87
rhuss added a commit that referenced this issue Aug 19, 2015
Related to #87, still missing: Query for labels and stopping based on maven coordinates.
@rhuss
Copy link
Collaborator

rhuss commented Aug 19, 2015

Just pushed branch 87-docker-stop

Should work:

  • Setting a POM specific label when starting a container

Still missing:

  • Querying for containers to stop via label
  • Implementation in StopMojo

Shouldn't be to much to do (hope so ;-)

@rhuss rhuss modified the milestones: 0.13.3, 0.13.7 Oct 13, 2015
rhuss added a commit that referenced this issue Oct 13, 2015
This env variable WHALE_DMP_LABEL is used to mark containers to belong to a certain
build so that can be recognized when calling 'docker:stop' #87
rhuss added a commit that referenced this issue Oct 13, 2015
Related to #87, still missing: Query for labels and stopping based on maven coordinates.
@jgangemi
Copy link
Collaborator Author

this is finally going to get fixed - we can't run tests in parallel at the day job w/o this so it just became my top priority!

@jgangemi
Copy link
Collaborator Author

as part of fixing this, i think we should move forward one release in the docker api to v1.18 (truthfully i'd like to push us all the way to the current version but that can happen later).

as of v1.18, the /containers/json call returns any labels assigned to the container which means we can avoid the extra call to get the images and then inspect each one for labels.

rhuss added a commit that referenced this issue Dec 3, 2015
This env variable WHALE_DMP_LABEL is used to mark containers to belong to a certain
build so that can be recognized when calling 'docker:stop' #87
rhuss added a commit that referenced this issue Dec 3, 2015
Related to #87, still missing: Query for labels and stopping based on maven coordinates.
rhuss added a commit that referenced this issue Dec 3, 2015
* Label is renamed to 'dmp.coordinates'
* `runId` removed from label
* renamed property for stopping all containers from the images to 'docker.allContainers' so that a 'mvn docker:stop -Ddocker.allContainers' will reproduce the old behaviour.
* Some minor refactorings

See also #87.
@rhuss rhuss added the fixed label Dec 3, 2015
@rhuss
Copy link
Collaborator

rhuss commented Dec 3, 2015

Thanks a lot for the PR. I did some cleanup and refactorings, hope that's ok. See the commit message rhuss@229584c for details.

@jgangemi
Copy link
Collaborator Author

jgangemi commented Dec 3, 2015

aww, i really wanted to keep sledgehammer as the option name? can we add it back as an easter egg alias? :)

@rhuss
Copy link
Collaborator

rhuss commented Dec 3, 2015

Yeah, thought that you would insist on it :)

Let's add it as alias, no problem :) I will do this.

@jgangemi
Copy link
Collaborator Author

jgangemi commented Dec 3, 2015

haha - thanks!

@rhuss
Copy link
Collaborator

rhuss commented Dec 8, 2015

Oops. Forgot the alias ;-( Will add it for 0.14.0 ...

sorry.

@rhuss rhuss closed this as completed Dec 8, 2015
rhuss added a commit that referenced this issue Dec 8, 2015
leusonmario pushed a commit to leusonmario/docker-maven-plugin that referenced this issue Aug 18, 2018
Added support for encrypted passwords
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants