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

TINKERPOP-2950 Add docker shutdown handling to close containers gracefully #2397

Merged
merged 3 commits into from
Dec 19, 2023

Conversation

ryn5
Copy link
Contributor

@ryn5 ryn5 commented Dec 12, 2023

JIRA: https://issues.apache.org/jira/browse/TINKERPOP-2950. Handles SIGTERM when shutting down docker container by sending SIGINT to server processes.

@codecov-commenter
Copy link

codecov-commenter commented Dec 12, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c2e2086) 75.19% compared to head (eff17cd) 75.19%.
Report is 4 commits behind head on 3.6-dev.

Additional details and impacted files
@@              Coverage Diff              @@
##             3.6-dev    #2397      +/-   ##
=============================================
- Coverage      75.19%   75.19%   -0.01%     
- Complexity     12318    12319       +1     
=============================================
  Files           1057     1057              
  Lines          63450    63450              
  Branches        6935     6935              
=============================================
- Hits           47713    47709       -4     
- Misses         13169    13176       +7     
+ Partials        2568     2565       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Cole-Greer
Copy link
Contributor

Thanks Ryan, I checked out the branch and tested out the new image, it's working great!

VOTE +1

@ryn5 ryn5 changed the title Add docker shutdown handling to close containers gracefully TINKERPOP-2950 Add docker shutdown handling to close containers gracefully Dec 13, 2023
@xiazcy
Copy link
Contributor

xiazcy commented Dec 15, 2023

VOTE +1

1 similar comment
@vkagamlyk
Copy link
Contributor

VOTE +1

@xiazcy xiazcy merged commit 1e10331 into apache:3.6-dev Dec 19, 2023
23 checks passed
@rcbyron
Copy link

rcbyron commented Aug 5, 2024

I'm still having issues with graceful shutdown even with this code.

I use the default scripts/empty-sample.groovy which has:

globals << [hook : [
        onStartUp: { LifeCycleHook.Context ctx ->
            ctx.logger.info("Executed once at startup of Gremlin Server.")
        },
        onShutDown: { LifeCycleHook.Context ctx ->
            ctx.logger.info("Executed once at shutdown of Gremlin Server.")
            graph.close()  // <-- I ADDED THIS
        }
] as LifeCycleHook]

And Executed once at shutdown... is never called I tried stopping/starting/restarting from Docker Desktop.

@danielcweber
Copy link
Contributor

This is an issue for me as well. It seems that docker-entrypoint.sh forwards SIGTERMS as SIGINTS to /opt/gremlin-server/bin/gremlin-server.sh but it's ignored there. What needs to be sent the signal instead is the java process that gremlin-server.sh starts.

@Cole-Greer
Copy link
Contributor

I'm seeing the same behaviour as well now. The change here is preventing docker from hanging due to /opt/gremlin-server/bin/gremlin-server.sh not terminating, but it does not properly terminate the gremlin server JVM process.

@danielcweber As you've already dug into this, would you be willing to submit a PR with a fix? If not let's reopen the JIRA for tracking and ensure it gets fixed for the next release.

@danielcweber
Copy link
Contributor

I'm basically clueless about bash files but I'll give it a try nonetheless.

@danielcweber
Copy link
Contributor

danielcweber commented Aug 8, 2024

@Cole-Greer My naive attempt that is working for me would be this change. This is not even PR quality because I don't know what this will break for other users. Basically, through this change, the 'java' process will end up as PID 1 and receive all the signals.

@Cole-Greer
Copy link
Contributor

Thanks @danielcweber. I don't see anything that would break just from looking at the code. I will find some time to run some tests with it, if everything checks out I think we can merge that in.

@danielcweber
Copy link
Contributor

@Cole-Greer If everything is fine, here's a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants