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

ws-manager: Allow extra time for workspace to receive SIGKILL #14498

Merged
merged 2 commits into from
Nov 15, 2022
Merged

Conversation

utam0k
Copy link
Contributor

@utam0k utam0k commented Nov 8, 2022

Description

By default, pod accepts SIGKILL 30 seconds after receiving SIGTERM.
This means that workspace processes must be processed for termination within 30 seconds.
This may be too short for large software such as dockerd, which will clean up the overlay directory and other files when it receives a SIGTERM. (maybe)
https://github.com/moby/moby/blob/0910306bf970603ce787466a98e4294ba81af841/cmd/dockerd/trap/trap.go#L41
If the cleanup is not done in time within 30 seconds, the container's overlay directory will remain uncluttered and causes #11183
Therefore, set this to 3 minutes, which gives you a little more time. There is currently no basis for this number, so we are looking for great ideas.

Related Issue(s)

Mitigation #11183

How to test

Please give me the idea

Release Notes

The processes in the workspace are given up to 3 minutes after receiving SIGTERM.

Documentation

Werft options:

  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview
  • /werft with-large-vm
  • /werft with-integration-tests=all
    Valid options are all, workspace, webapp, ide

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-to-grace.1 because the annotations in the pull request description changed
(with .werft/ from main)

Volumes: volumes,
RestartPolicy: corev1.RestartPolicyNever,
Volumes: volumes,
TerminationGracePeriodSeconds: &graceSec,
Copy link
Contributor

Choose a reason for hiding this comment

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

🤯 this is a good idea, I think. For example, Kubernetes even recommends to do exactly this (like when saving content for a stopping container).

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI @utam0k tests are currently failing.

Copy link
Contributor

Choose a reason for hiding this comment

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

@aledbf this seems like a good idea, to add a grace period. Can you think of a reason not to add?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm also wondering if I missed something important and haven't fixed the test. I will first listen to your opinions and if it looks good, I will fix the test. That's why I chose Draft.

@utam0k
Copy link
Contributor Author

utam0k commented Nov 15, 2022

/werft run with-integration-tests=workspace

👍 started the job as gitpod-build-to-grace.3
(with .werft/ from main)

@utam0k utam0k marked this pull request as ready for review November 15, 2022 01:11
@utam0k utam0k requested a review from a team November 15, 2022 01:11
@utam0k utam0k requested a review from aledbf November 15, 2022 01:12
@github-actions github-actions bot added the team: workspace Issue belongs to the Workspace team label Nov 15, 2022
Copy link
Contributor

@sagor999 sagor999 left a comment

Choose a reason for hiding this comment

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

Good idea indeed.
As a follow up PR: can we detect somehow if that grace period is enough? 🤔 Can we maybe allow to configure it via gitpod.yml?
Different workloads might require different term grace period.

@roboquat roboquat merged commit d27715d into main Nov 15, 2022
@roboquat roboquat deleted the to/grace branch November 15, 2022 01:30
@jenting
Copy link
Contributor

jenting commented Nov 15, 2022

Good idea indeed. As a follow up PR: can we detect somehow if that grace period is enough? 🤔 Can we maybe allow to configure it via gitpod.yml? Different workloads might require different term grace period.

We had a PR before but not merged. We could consider revisiting it.

But as a user, to be honest, I don't know how much time the graceful period is needed.
So, it would be good to auto-adjust the graceful period by Gitpod (if possible) rather than by the user's configuration.

@roboquat roboquat added deployed: workspace Workspace team change is running in production deployed Change is completely running in production labels Dec 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed: workspace Workspace team change is running in production deployed Change is completely running in production release-note size/M team: workspace Issue belongs to the Workspace team
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants