[ws-manager] config terminationGracePeriodSeconds #13352

svenefftinge · 2022-09-27T08:26:30Z

Description

Allow extending the termination grace period of workspace pods through gitpod.yml configuration.

Related Issue(s)

Fixes #

How to test

Start a workspace on this branch : https://github.com/svenefftinge/termination-test/tree/se-extended-race
run ignoreSigterm > out.txt
stop workspace. It should stay in stopping close to 100 seconds.
when starting again, the out.txt should contain log messages that show the process ran almost 100 seconds after receiving the SIGHUP

Release Notes

Allow configuration of 'terminationGracePeriodSeconds' to get more time for gracefully stopping services in workspaces

Documentation

Werft options:

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

werft-gitpod-dev-com · 2022-09-27T08:26:51Z

started the job as gitpod-build-se-flexible-termination-period.1 because the annotations in the pull request description changed
(with .werft/ from main)

svenefftinge · 2022-09-27T09:07:11Z

/werft run

👍 started the job as gitpod-build-se-flexible-termination-period.3
(with .werft/ from main)

easyCZ

What's the general release plan for this feature? Are we looking to control access to the termination grace period somehow?

/hold for Qs

easyCZ · 2022-09-27T11:27:01Z

components/gitpod-protocol/data/gitpod-schema.json

@@ -114,6 +114,10 @@
                "additionalProperties": false
            }
        },
+        "terminationGracePeriodSeconds": {


In the protocol it's marked as optional, does it need to be marked optional here too?

By making this change, all requests will now be able to set this despite it not being actually implemented by workspaces. Is that intended? Do we want to control release of this in some way?

In the protocol it's marked as optional, does it need to be marked optional here too?

it is optional by default

Ah, it used to have optional in the v2 version of the spec. in v4 it only has required. Thanks!

easyCZ · 2022-09-27T11:28:59Z

components/ws-manager-api/core.proto

@@ -567,6 +567,9 @@ message StartWorkspaceSpec {

    // ssh_public_keys is user's uploaded ssh public keys
    repeated string ssh_public_keys = 15;
+
+    // termination_grace_period_seconds sets the TerminationGracePeriodSeconds, defaults to 30 seconds
+    int64 termination_grace_period_seconds = 16;


Worth keeping in mind that ws-manager running in WS clusters will have a different rollout cadence to webapp. This change is backwards compatible so it shouldn't be a system problem, just an expectation.

components/ws-manager/pkg/manager/create.go

svenefftinge · 2022-09-27T11:37:34Z

btw. the bash process we are starting doesn't seem to propagate SIGINT to the process it is waiting for.

csweichel · 2022-09-27T15:18:26Z

We have quite a few places where we pass in a termination grace period, e.g. all call sites of stopWorkspace. It's not clear which value (pod or arg) gets precedence.

components/gitpod-protocol/data/gitpod-schema.json

sagor999 · 2022-09-28T02:09:11Z

@csweichel
https://github.com/kubernetes/apiserver/blob/710ad8da8e0ae8c5dcb4a12744d231c26c7e9905/pkg/registry/rest/delete.go#L123-L128
According to this only shorter grace period can be provided into delete. Which means our stopWorkspace call (and any other place where we do this) can effectively be a maximum grace period? 🤔
though it changes current behaviour quite a bit.

csweichel · 2022-09-28T12:09:29Z

@csweichel https://github.com/kubernetes/apiserver/blob/710ad8da8e0ae8c5dcb4a12744d231c26c7e9905/pkg/registry/rest/delete.go#L123-L128 According to this only shorter grace period can be provided into delete. Which means our stopWorkspace call (and any other place where we do this) can effectively be a maximum grace period? 🤔 though it changes current behaviour quite a bit.

Which indeed means that the value we use for stopWorkspace would take precedence, because it's either the default 30s or less than that.

We could either (in descending order of implementation complexity)

use the actual termination grace period of the pod,
use the configurable max termination grace period,
just use a very long constant time (999 * time.Hour).

svenefftinge · 2022-09-28T12:12:30Z

I've set it to the same hard-coded max (5 minutes). (see)

components/ws-manager/pkg/manager/create.go

werft-gitpod-dev-com · 2022-09-29T13:39:46Z

started the job as gitpod-build-se-flexible-termination-period.9 because the annotations in the pull request description changed
(with .werft/ from main)

sagor999 · 2022-09-29T18:17:07Z

/hold
for conflicts and failed build

svenefftinge · 2022-10-07T15:49:39Z

/werft run with-clean-slate-deployment

👍 started the job as gitpod-build-se-flexible-termination-period.14
(with .werft/ from main)

stale · 2022-10-28T22:12:40Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

roboquat added do-not-merge/work-in-progress do-not-merge/release-note-label-needed size/S labels Sep 27, 2022

svenefftinge force-pushed the se/flexible-termination-period branch from 4d83eb9 to 65b686f Compare September 27, 2022 08:46

roboquat added size/XL and removed size/S labels Sep 27, 2022

roboquat added release-note and removed do-not-merge/release-note-label-needed labels Sep 27, 2022

svenefftinge marked this pull request as ready for review September 27, 2022 11:23

svenefftinge requested a review from a team September 27, 2022 11:23

svenefftinge requested review from aledbf and sagor999 as code owners September 27, 2022 11:23

svenefftinge requested a review from a team September 27, 2022 11:23

roboquat removed the do-not-merge/work-in-progress label Sep 27, 2022

github-actions bot added team: webapp Issue belongs to the WebApp team team: workspace Issue belongs to the Workspace team labels Sep 27, 2022

easyCZ approved these changes Sep 27, 2022

View reviewed changes

roboquat added the do-not-merge/hold label Sep 27, 2022

aledbf reviewed Sep 27, 2022

View reviewed changes

components/ws-manager/pkg/manager/create.go Outdated Show resolved Hide resolved

svenefftinge mentioned this pull request Sep 27, 2022

Base usage measurement on stoppedTime #13369

Closed

sagor999 reviewed Sep 28, 2022

View reviewed changes

components/gitpod-protocol/data/gitpod-schema.json Outdated Show resolved Hide resolved

svenefftinge force-pushed the se/flexible-termination-period branch from 65b686f to 616fb25 Compare September 28, 2022 06:36

svenefftinge requested a review from a team September 28, 2022 06:36

roboquat added size/XXL and removed size/XL labels Sep 28, 2022

github-actions bot added the team: IDE label Sep 28, 2022

svenefftinge force-pushed the se/flexible-termination-period branch 2 times, most recently from 55caad1 to 0e9d536 Compare September 28, 2022 09:05

sagor999 reviewed Sep 28, 2022

View reviewed changes

components/ws-manager/pkg/manager/create.go Outdated Show resolved Hide resolved

svenefftinge force-pushed the se/flexible-termination-period branch 2 times, most recently from 377dfde to 222dbfb Compare September 29, 2022 13:33

sagor999 approved these changes Sep 29, 2022

View reviewed changes

svenefftinge force-pushed the se/flexible-termination-period branch 3 times, most recently from e5b13f1 to 5939f9c Compare October 7, 2022 14:59

[ws-manager] config terminationGracePeriodSeconds

36923c6

svenefftinge force-pushed the se/flexible-termination-period branch from 5939f9c to 36923c6 Compare October 7, 2022 15:23

geropl marked this pull request as draft October 13, 2022 07:41

roboquat added the do-not-merge/work-in-progress label Oct 13, 2022

stale bot added the meta: stale This issue/PR is stale and will be closed soon label Oct 28, 2022

stale bot closed this Nov 9, 2022

jenting mentioned this pull request Nov 15, 2022

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

Merged

4 tasks

svenefftinge deleted the se/flexible-termination-period branch December 9, 2022 08:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ws-manager] config terminationGracePeriodSeconds #13352

[ws-manager] config terminationGracePeriodSeconds #13352

svenefftinge commented Sep 27, 2022 •

edited

Loading

werft-gitpod-dev-com bot commented Sep 27, 2022

svenefftinge commented Sep 27, 2022 •

edited by werft-gitpod-dev-com bot

Loading

easyCZ left a comment

easyCZ Sep 27, 2022

easyCZ Sep 27, 2022

svenefftinge Sep 27, 2022

easyCZ Sep 27, 2022

easyCZ Sep 27, 2022

svenefftinge commented Sep 27, 2022

csweichel commented Sep 27, 2022

sagor999 commented Sep 28, 2022

csweichel commented Sep 28, 2022 •

edited by werft-gitpod-dev-com bot

Loading

svenefftinge commented Sep 28, 2022 •

edited

Loading

werft-gitpod-dev-com bot commented Sep 29, 2022

sagor999 commented Sep 29, 2022

svenefftinge commented Oct 7, 2022 •

edited by werft-gitpod-dev-com bot

Loading

stale bot commented Oct 28, 2022

[ws-manager] config terminationGracePeriodSeconds #13352

[ws-manager] config terminationGracePeriodSeconds #13352

Conversation

svenefftinge commented Sep 27, 2022 • edited Loading

Description

Related Issue(s)

How to test

Release Notes

Documentation

Werft options:

werft-gitpod-dev-com bot commented Sep 27, 2022

svenefftinge commented Sep 27, 2022 • edited by werft-gitpod-dev-com bot Loading

easyCZ left a comment

Choose a reason for hiding this comment

easyCZ Sep 27, 2022

Choose a reason for hiding this comment

easyCZ Sep 27, 2022

Choose a reason for hiding this comment

svenefftinge Sep 27, 2022

Choose a reason for hiding this comment

easyCZ Sep 27, 2022

Choose a reason for hiding this comment

easyCZ Sep 27, 2022

Choose a reason for hiding this comment

svenefftinge commented Sep 27, 2022

csweichel commented Sep 27, 2022

sagor999 commented Sep 28, 2022

csweichel commented Sep 28, 2022 • edited by werft-gitpod-dev-com bot Loading

svenefftinge commented Sep 28, 2022 • edited Loading

werft-gitpod-dev-com bot commented Sep 29, 2022

sagor999 commented Sep 29, 2022

svenefftinge commented Oct 7, 2022 • edited by werft-gitpod-dev-com bot Loading

stale bot commented Oct 28, 2022

svenefftinge commented Sep 27, 2022 •

edited

Loading

svenefftinge commented Sep 27, 2022 •

edited by werft-gitpod-dev-com bot

Loading

csweichel commented Sep 28, 2022 •

edited by werft-gitpod-dev-com bot

Loading

svenefftinge commented Sep 28, 2022 •

edited

Loading

svenefftinge commented Oct 7, 2022 •

edited by werft-gitpod-dev-com bot

Loading