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

Feature/prevent zombie containers #53

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

abbesBenayache
Copy link
Contributor

@abbesBenayache abbesBenayache commented Jan 16, 2025

🔄 Scénarios gérés :

  1. Build long + interruption (Ctrl+C) Le build est interrompu proprement.

Screenshot from 2025-01-29 15-22-33

  1. Build réussi, mais run long + interruption (Ctrl+C)

Screenshot from 2025-01-29 15-37-48
L'utilisateur peut choisir :
"Yes" image
"No" image

  1. Build réussi, run long, interruption forcée (Ctrl+C x2)

image

@PierreJeanjacquot
Copy link
Member

A few remarks

  • we can pass an AbortSignal to dockerode methods, I think it is interesting to abort some methods such as build and push
  • we need to give some feedback to the user while running the graceful shutdown procedure
  • we need to let the user kill the command (by sending a second SIGINT) when the graceful shutdown is running
  • SIGINT handlers must be cleaned as soon as the part of the code requiring a graceful shutdown finished to run
  • long build can be tested by adding a RUN sleep x step to the Dockerfile

Copy link
Member

@PierreJeanjacquot PierreJeanjacquot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comments

cli/src/cmd/test.js Outdated Show resolved Hide resolved
Comment on lines 9 to 11
process.on('SIGINT', () => {
abortController.abort();
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Registering the listener in the module scope and never cleaning it is problematic because it prevents SIGINT from stopping the process in the whole app.

This creates this bug:
image
I cannot cancel the command while a long async step is running (in the example TEE transformation step)

We need to register the SIGINT listener just before calling docker and clean it as soon as the docker interaction ends (successfully or with an error).

cli/src/execDocker/docker.js Outdated Show resolved Hide resolved
cli/src/execDocker/docker.js Outdated Show resolved Hide resolved
cli/src/execDocker/docker.js Outdated Show resolved Hide resolved
cli/src/execDocker/docker.js Outdated Show resolved Hide resolved
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