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

Implement a graceful container shutdown #1946

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

TrevorBenson
Copy link
Contributor

Description

This PR implements a change to the buildImage for the cardano-db-sync container.

  1. Uses Dockerfile instruction stopsignal to change the default SIGTERM to SIGINT
  2. Uses exec so that the primary process is the cardano-db-sync binary process, instead of the bash shell script.
  3. Added an entry to the CHANGELOG as checklist suggested.

Should resolve issue cardano-db-sync container doesn't gracefully shut down

Checklist

  • Commit sequence broadly makes sense
  • Commits have useful messages
  • New tests are added if needed and existing tests are updated
  • Any changes are noted in the changelog
  • Code is formatted with fourmolu on version 0.10.1.0 (which can be run with scripts/fourmolize.sh)
  • Self-reviewed the diff

Migrations

  • The pr causes a breaking change of type a,b or c
  • If there is a breaking change, the pr includes a database migration and/or a fix process for old values, so that upgrade is possible
  • Resyncing and running the migrations provided will result in the same database semantically

If there is a breaking change, especially a big one, please add a justification here. Please elaborate
more what the migration achieves, what it cannot achieve or why a migration is not possible.

Additional Details / Empty Checklist Items

  • New tests added or existing updated if needed. I didn't find tests specific to building the container image. I left it empty in case I overlooked something and reviewer points me at tests that require fixing.
  • Code is formatted with fourmolu. PR changes nix files. The instructions for using fourmolu appeared to be a specific haskell code, so did not check this (yet).

@TrevorBenson TrevorBenson requested review from a team as code owners February 21, 2025 01:13
Use stopsignal with buildImage
Use exec so binary process is primary pid
@TrevorBenson TrevorBenson force-pushed the graceful-container-shutdown-stopsignal branch from 04a13f9 to 7c68622 Compare February 21, 2025 23:43
@Cmdv
Copy link
Contributor

Cmdv commented Feb 24, 2025

@TrevorBenson Thanks for the PR, we're just trying to fix the CI failures which aren't actually related to this PR once those are fixed we'll be able to put this into master.

@TrevorBenson
Copy link
Contributor Author

@TrevorBenson Thanks for the PR, we're just trying to fix the CI failures which aren't actually related to this PR once those are fixed we'll be able to put this into master.

Thanks. I took a quick look at them the other day and noticed they seemed to exist prior to this PR, so figured I could skip looking for root causes.

Glad to know I wasn't mistaken 👍🏼

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.

2 participants