Add support to stop docker containers during VM shutdown

[balazs-zsoldos]

Fix #915

I tried several approaches and this hacky one worked. The problem is that the classloader of the Mojo is closed at the time of the shutdown hook.

What did not work:

• catching when maven session ends. It is removed from maven API since 3.x.
• creating a new URLClassLoader and using it as parent classloader of the Mojo ClassRealm. Some classes of Apache HTTPClient is already imported, so there is an error that not the same interface is implemented

This works as in case StartMojo was called before, only HttpDelete is not loaded by the classloader. In case no goal is called before this Mojo, more classes should be loaded programmatically, but I did not want to check how many, and by changing the dependencies, the list of classes could change.

Please check if this solution is OK for you!

[sonarcloud]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
2 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

[codecov]

Codecov Report

Merging #1492 (c82f5c7) into master (d3d3618) will decrease coverage by 0.10%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##             master    #1492      +/-   ##
============================================
- Coverage     62.12%   62.02%   -0.11%     
  Complexity     2174     2174              
============================================
  Files           166      166              
  Lines          9559     9575      +16     
  Branches       1442     1443       +1     
============================================
  Hits           5939     5939              
- Misses         3108     3124      +16     
  Partials        512      512              

Impacted Files	Coverage Δ
...rc/main/java/io/fabric8/maven/docker/StopMojo.java	43.24% <0.00%> (-7.29%)	⬇️

[balazs-zsoldos]

I am not sure how to test VM shutdown hooks in unit tests.

[rohanKanojia]

Thanks a lot for your PR, I'll test and review this within this week. I'm okay with merging this if this fixes the issue.

[rohanKanojia]

Would appreciate if you could add documentation regarding this field in docker:stop configuration[0]. Source for this doc section is located here[1]

[0] http://dmp.fabric8.io/#docker:stop
[1] https://github.com/fabric8io/docker-maven-plugin/blob/master/src/main/asciidoc/inc/_docker-stop.adoc

[rohanKanojia]

Sonar reports this as a security hotspot: https://sonarcloud.io/project/security_hotspots?id=fabric8io_docker-maven-plugin&pullRequest=1492&hotspots=AXv_23_LXkHjdTy7bc6z

Do you think we can avoid this by just logging some message instead of printing whole stacktrace?

[rohanKanojia]

All goals in this plugin override executeInternal method which contains goal related functionality. I'm not sure why you're overriding execute method. Is this possible to move this stop on VM shutdown logic to executeInternal method instead?

[rohanKanojia]

We do have some integration tests which run on CircleCI on a docker daemon located in it/ folder. Do you think we can add an integration test for this scenario?

[balazs-zsoldos]

I will have a look on the comments.

About IT tests. This functionality can be checked if the JVM is actually shutting down. So I must check how these IT tests are written. If it is possible to do forking the Mojo execution somehow, there is a chance. But finding the time to go through all comments and this topic will take several weeks to me.

[balazs-zsoldos]

@rohanKanojia Sorry, I had no time in the last weeks to take care of the comments. I am not sure when I can find the time. Do you still expect the PR to be upgraded?

[rohanKanojia]

I'm planning to cut a release this weekend and was thinking of adding this to the release.

If it's difficult to add tests, maybe we can merge this as it is and ask users for feedback. We can revert in case it's not working for them.

[balazs-zsoldos]

Thanks you very much!

[sonarcloud]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
2 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

[alahaouas]

@rohanKanojia
Issue is still happening with v0.38.1

To test, have a docker-maven-plugin coupled with flyway with a faulty SQL migration script.
You will notice that flyway fails the maven build but the docker container still runs.

[alahaouas]

The stop execution is never reached because the build will fail before

[balazs-zsoldos]

@alahaouas Please use the property docker.executeStopOnVMShutdown and set it to true. This will call the stop goal of the plugin in a shutdown hook when the JVM stops gracefully.

[alahaouas]

@balazs-zsoldos I mentioned that the issue is still not fixed, even with docker.executeStopOnVMShutdown set to true.
Can you please investigate this ?

[balazs-zsoldos]

@alahaouas Can you share a git repo (or at least a pom file) that has the project that does not work for you and some description how to reproduce?

[alahaouas]

@balazs-zsoldos I can't share the git repo because it is private but plz find attached the edited pom.xml (I modified it to remove our internal dependencies).
(Rename extension file to XML)

To reproduce, have a basic unit test + faulty or missing liquibase changeset for example. Any cause at the pre-test phase that will fail the build.
The unit tests won't start, the build will stop and the container will stay alive.

pom.txt

[hho]

I believe @alahaouas probably has it resolved by now, but for everyone else finding this via Google:

You still need to execute the stop goal for the shutdown hook to be registered with the JVM. If your build only executes stop in a later lifecycle phase, the container still might not stop, because if the build never gets to that phase, then the hook never gets registered.

Basically, you can issue start and stop immediately after one another (i.e. in the same Maven lifecycle phase) – and with the executeStopOnVMShutdown flag, the container will then be stopped once the JVM exits.