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

chore: kill docker container #1927

Merged
merged 10 commits into from
Jun 5, 2024
Merged

chore: kill docker container #1927

merged 10 commits into from
Jun 5, 2024

Conversation

shivamsouravjha
Copy link
Contributor

Solution

  • Instead of killing children kill the container directly
  • Wait for 25 second before sending sigkill
  • Listen to docker events for 30seconds, if not killed log that application isn't killed

Closes: #1904

Signed-off-by: shivamsouravjha <shivamsouravjha@gmail.com>
Signed-off-by: shivamsouravjha <shivamsouravjha@gmail.com>
Signed-off-by: shivamsouravjha <shivamsouravjha@gmail.com>
@@ -414,6 +414,34 @@ func (a *App) Run(ctx context.Context, inodeChan chan uint64) models.AppError {
}
return a.run(ctx)
}
func (a *App) getDockerMetaSync() {
Copy link
Member

Choose a reason for hiding this comment

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

Change this name, as it is not getting something and also don't mention sync, if it is a blocking call then it is already in sync.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated the same

Signed-off-by: shivamsouravjha <shivamsouravjha@gmail.com>
Copy link
Member

@gouravkrosx gouravkrosx left a comment

Choose a reason for hiding this comment

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

LGTM!

containerID := a.container
for {
select {
case <-logTicker.C:
Copy link
Member

Choose a reason for hiding this comment

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

also add ctx.done check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why is it needed when we're running it after context done? If we do so would be unable to inspect the docker as context is already cancelled.

Signed-off-by: shivamsouravjha <shivamsouravjha@gmail.com>
Signed-off-by: shivamsouravjha <shivamsouravjha@gmail.com>
Signed-off-by: shivamsouravjha <shivamsouravjha@gmail.com>
Signed-off-by: shivamsouravjha <shivamsouravjha@gmail.com>
Signed-off-by: shivamsouravjha <shivamsouravjha@gmail.com>
@gouravkrosx gouravkrosx self-requested a review June 5, 2024 15:27
@gouravkrosx gouravkrosx merged commit 07dc5e7 into main Jun 5, 2024
17 checks passed
@gouravkrosx gouravkrosx deleted the shivam/dockerKill branch June 5, 2024 15:29
@github-actions github-actions bot locked and limited conversation to collaborators Jun 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug]: wait for docker container to be stopped before exiting from keploy
2 participants