wcow: support graceful termination of servercore containers

[kiashok]

Graceful termination was already supported for nanoserver containers,
but those based on servercore still use a 5 second timeout which is
controlled by Windows. The reason behind this behavior is due to the
different implementation of SrvEndTask() in Servercore image which is
as follows:
(1) Deliver CTRL_SHUTDOWN_EVENT to the appropriate process
(2) Wait for 'WaitToKillServiceTimeout' amount of time (5 seconds by default) and kill the process before returning back to the caller.
This causes the containers with servercore base images to be killed in 5 seconds immaterial of the timeout specified with the container stop command.

We fix this by instead sending the termination signal for WCOW on a
background goroutine, and then returning immediately without waiting
for 'WaitToKillServiceTimeout' to expire.

In the future, if the underlying issue in Windows is fixed, this change could be reverted.

[katiewasnothere]

Tried to capture the time elapsed since the container exited using container stats but this is not supported for a container that has already existed.

Could you clarify what the problem with this is? I would think getting stats on an exited container (so long as it's not cleaned up) would be supported.

Tried to capture the elapsed time before and after container was stopped with a timeout but even without this fix, I see that containerStopWithTimeout() in test/cri-containerd waits for the entire timeout period even if the container exits before the specified timeout has elapsed.

I wonder if this is a bug in our cri? Do you know if that's the case?

In Hcsshim, we override value of reg key 'WaitToKillServiceTimeout' from 5 seconds to 30 minutes

Did you mean 30 seconds? Do we know if this will impact container users if they rely on this 5 second timeout?

[kiashok]

Tried to capture the time elapsed since the container exited using container stats but this is not supported for a container that has already existed.

Could you clarify what the problem with this is? I would think getting stats on an exited container (so long as it's not cleaned up) would be supported.

I am using the StopContainerWithTimeout() function to stop the container with timeout in the cri test. This function currently kills the container after the timeout has passed and trying to get status after that results in the following error :
=== RUN Test_Gracefultermination_WCOW_Hypervisor_Servercore
error getting container container stats after stop: rpc error: code = Unknown desc = unexpected metrics response: []
stopcontainer_test.go:68: error getting container container stats after stop: rpc error: code = Unknown desc = unexpected metrics response: []

Tried to capture the elapsed time before and after container was stopped with a timeout but even without this fix, I see that containerStopWithTimeout() in test/cri-containerd waits for the entire timeout period even if the container exits before the specified timeout has elapsed.

I wonder if this is a bug in our cri? Do you know if that's the case?

I am not sure of this but it probably is something with the implementation as capturing the time right before and after the StopContainerWithTimeout() function with a timeout of 't' always returns a diff of 't' seconds even without my fix (but when I run it manually crictl.exe stop -t of any value kills the container in 5 seconds on servercore base images).

In Hcsshim, we override value of reg key 'WaitToKillServiceTimeout' from 5 seconds to 30 minutes

Did you mean 30 seconds? Do we know if this will impact container users if they rely on this 5 second timeout?

There are a lot of complaints saying we are not supporting timeout >5 seconds so am not sure if too many folks rely on the 5 second timer. I can double check with Brandon about this. The idea was to make behavior similar to nanoserver and linux which honor graceful timeout. Maybe adding release notes about this fix would help any customer upgrading to the latest package whenever its released?

[kevpar]

A few notes on Git commit style:

• The commit title should be fairly short (a lot of places recommend 50 characters max, but I think going a bit over this is still fine).
• The commit title should be written using imperative mood.
• If it is helpful, the title can be prefixed with a brief descriptor indicating the scope of the change (e.g. internal/foo: fix bug bar).
• The commit title should be followed by a blank line, and then a mutli-line commit body with more details on the change. It's good to describe here what changed and why the change is important, follow-up work that may be needed, link to more info on the change, etc. The commit body should also not use super long lines when it can be avoided. Personally I try to keep my lines to 72 characters max.

Taken together, you might consider a commit description like the following:

wcow: Support graceful termination of servercore containers

Graceful termination was already supported for nanoserver containers,
but those based on servercore still use a 5 second timeout which is
controlled by Windows. The reason behind this behavior is <reason>.

We fix this by instead sending the termination signal for WCOW on a
background goroutine, and then returning. <more details on fix>.

In the future, if the underlying issue is fixed in Windows, this change
could be reverted.

There's a lot of guidance on the internet for how to write a good Git commit. One guide I like is here. Note that here we are not super prescriptive on the exact line length to use. :)

[kevpar]

In Hcsshim, we override value of reg key 'WaitToKillServiceTimeout' from 5 seconds to 30 minutes

Did you mean 30 seconds? Do we know if this will impact container users if they rely on this 5 second timeout?

This is probably worth explicitly calling out in the PR/commit description. If a customer (for whatever reason) explicitly depended on this WaitToKillServiceTimeout value, this would be a breaking change for them. For instance, if a customer sent a termination signal between apps in their container, they would observe a difference in behavior now.

[kevpar]

In Hcsshim, we override value of reg key 'WaitToKillServiceTimeout' from 5 seconds to 30 minutes

Did you mean 30 seconds? Do we know if this will impact container users if they rely on this 5 second timeout?

This is probably worth explicitly calling out in the PR/commit description. If a customer (for whatever reason) explicitly depended on this WaitToKillServiceTimeout value, this would be a breaking change for them. For instance, if a customer sent a termination signal between apps in their container, they would observe a difference in behavior now.

Actually, @jsturtevant or @marosset interested in your opinions on if this will be a problem for customers. This change will fix graceful termination for servercore containers, but could have negative impact if a customer is relying on the default termination timeout in servercore.

[marosset]

In Hcsshim, we override value of reg key 'WaitToKillServiceTimeout' from 5 seconds to 30 minutes

Did you mean 30 seconds? Do we know if this will impact container users if they rely on this 5 second timeout?

This is probably worth explicitly calling out in the PR/commit description. If a customer (for whatever reason) explicitly depended on this WaitToKillServiceTimeout value, this would be a breaking change for them. For instance, if a customer sent a termination signal between apps in their container, they would observe a difference in behavior now.

Actually, @jsturtevant or @marosset interested in your opinions on if this will be a problem for customers. This change will fix graceful termination for servercore containers, but could have negative impact if a customer is relying on the default termination timeout in servercore.

IMO there are a lot of downsides w/ relying on the default termination timeout in servercore (especially for K8s scenarios) and users are unhappy with current solution.
I agree we should call this out as a breaking change but think it might be more important to label this as a breaking change in containerd when we update decencies there.

[kiashok]

In Hcsshim, we override value of reg key 'WaitToKillServiceTimeout' from 5 seconds to 30 minutes

Did you mean 30 seconds? Do we know if this will impact container users if they rely on this 5 second timeout?

This is probably worth explicitly calling out in the PR/commit description. If a customer (for whatever reason) explicitly depended on this WaitToKillServiceTimeout value, this would be a breaking change for them. For instance, if a customer sent a termination signal between apps in their container, they would observe a difference in behavior now.

Actually, @jsturtevant or @marosset interested in your opinions on if this will be a problem for customers. This change will fix graceful termination for servercore containers, but could have negative impact if a customer is relying on the default termination timeout in servercore.

IMO there are a lot of downsides w/ relying on the default termination timeout in servercore (especially for K8s scenarios) and users are unhappy with current solution. I agree we should call this out as a breaking change but think it might be more important to label this as a breaking change in containerd when we update decencies there.

I am not sure I understand why this is a breaking change. Currently, when crictl.exe stop is specified, the target pid is directly killed (that is SIGKILL is the only signal sent). The WaitToKillServiceTimeout reg key is used only when crictl.exe stop -t is specified. Currently no matter what value of 't' is specified, it is always killed in 5 seconds. This fix is only going to help honor the time specified. Am I missing something here?

[kiashok]

wcow: Support graceful termination of servercore containers

A few notes on Git commit style:

• The commit title should be fairly short (a lot of places recommend 50 characters max, but I think going a bit over this is still fine).
• The commit title should be written using imperative mood.
• If it is helpful, the title can be prefixed with a brief descriptor indicating the scope of the change (e.g. internal/foo: fix bug bar).
• The commit title should be followed by a blank line, and then a mutli-line commit body with more details on the change. It's good to describe here what changed and why the change is important, follow-up work that may be needed, link to more info on the change, etc. The commit body should also not use super long lines when it can be avoided. Personally I try to keep my lines to 72 characters max.

Taken together, you might consider a commit description like the following:

wcow: Support graceful termination of servercore containers

Graceful termination was already supported for nanoserver containers,
but those based on servercore still use a 5 second timeout which is
controlled by Windows. The reason behind this behavior is <reason>.

We fix this by instead sending the termination signal for WCOW on a
background goroutine, and then returning. <more details on fix>.

In the future, if the underlying issue is fixed in Windows, this change
could be reverted.

There's a lot of guidance on the internet for how to write a good Git commit. One guide I like is here. Note that here we are not super prescriptive on the exact line length to use. :)

A few notes on Git commit style:

• The commit title should be fairly short (a lot of places recommend 50 characters max, but I think going a bit over this is still fine).
• The commit title should be written using imperative mood.
• If it is helpful, the title can be prefixed with a brief descriptor indicating the scope of the change (e.g. internal/foo: fix bug bar).
• The commit title should be followed by a blank line, and then a mutli-line commit body with more details on the change. It's good to describe here what changed and why the change is important, follow-up work that may be needed, link to more info on the change, etc. The commit body should also not use super long lines when it can be avoided. Personally I try to keep my lines to 72 characters max.

Taken together, you might consider a commit description like the following:

wcow: Support graceful termination of servercore containers

Graceful termination was already supported for nanoserver containers,
but those based on servercore still use a 5 second timeout which is
controlled by Windows. The reason behind this behavior is <reason>.

We fix this by instead sending the termination signal for WCOW on a
background goroutine, and then returning. <more details on fix>.

In the future, if the underlying issue is fixed in Windows, this change
could be reverted.

There's a lot of guidance on the internet for how to write a good Git commit. One guide I like is here. Note that here we are not super prescriptive on the exact line length to use. :)

Sure I'll keep this in mind, thanks Kevin :) Also, do we follow a template for the PR description section? Does that section look ok?

[marosset]

In Hcsshim, we override value of reg key 'WaitToKillServiceTimeout' from 5 seconds to 30 minutes

Did you mean 30 seconds? Do we know if this will impact container users if they rely on this 5 second timeout?

This is probably worth explicitly calling out in the PR/commit description. If a customer (for whatever reason) explicitly depended on this WaitToKillServiceTimeout value, this would be a breaking change for them. For instance, if a customer sent a termination signal between apps in their container, they would observe a difference in behavior now.

Actually, @jsturtevant or @marosset interested in your opinions on if this will be a problem for customers. This change will fix graceful termination for servercore containers, but could have negative impact if a customer is relying on the default termination timeout in servercore.

IMO there are a lot of downsides w/ relying on the default termination timeout in servercore (especially for K8s scenarios) and users are unhappy with current solution. I agree we should call this out as a breaking change but think it might be more important to label this as a breaking change in containerd when we update decencies there.

In Kubernetes it is a very common p

In Hcsshim, we override value of reg key 'WaitToKillServiceTimeout' from 5 seconds to 30 minutes

Did you mean 30 seconds? Do we know if this will impact container users if they rely on this 5 second timeout?

This is probably worth explicitly calling out in the PR/commit description. If a customer (for whatever reason) explicitly depended on this WaitToKillServiceTimeout value, this would be a breaking change for them. For instance, if a customer sent a termination signal between apps in their container, they would observe a difference in behavior now.

Actually, @jsturtevant or @marosset interested in your opinions on if this will be a problem for customers. This change will fix graceful termination for servercore containers, but could have negative impact if a customer is relying on the default termination timeout in servercore.

IMO there are a lot of downsides w/ relying on the default termination timeout in servercore (especially for K8s scenarios) and users are unhappy with current solution. I agree we should call this out as a breaking change but think it might be more important to label this as a breaking change in containerd when we update decencies there.

I am not sure I understand why this is a breaking change. Currently, when _crictl.exe stop _ is specified, the target pid is directly killed (that is SIGKILL is the only signal sent). The WaitToKillServiceTimeout reg key is used only when crictl.exe stop **-t ** is specified. Currently no matter what value of 't' is specified, it is always killed in 5 seconds. This fix is only going to help honor the time specified. Am I missing something here?

In Kubernetes it is common for pods to be stopped with a termination grace period.
When this happens a shutdown signal is sent to the containers and that after a configurable timeout a kill signal is sent.
This is commonly used during scale-down operations, and it gives and containers that are selected for scale-down a chance to complete any in-progress operations and the configurable timeouts are controlled are set by Kubernetes and can be changed at any time.
My understanding is that the currently shutdown behavior for servercore based containers is that it must be set at container build time. This does not provide the flexibility cluster operators expect.
I think it would be a breaking change because shutdown logic would no longer be honoring registry keys which previously affected shutdown duration/behaviour.

[kiashok]

In Hcsshim, we override value of reg key 'WaitToKillServiceTimeout' from 5 seconds to 30 minutes

Did you mean 30 seconds? Do we know if this will impact container users if they rely on this 5 second timeout?

This is probably worth explicitly calling out in the PR/commit description. If a customer (for whatever reason) explicitly depended on this WaitToKillServiceTimeout value, this would be a breaking change for them. For instance, if a customer sent a termination signal between apps in their container, they would observe a difference in behavior now.

Actually, @jsturtevant or @marosset interested in your opinions on if this will be a problem for customers. This change will fix graceful termination for servercore containers, but could have negative impact if a customer is relying on the default termination timeout in servercore.

IMO there are a lot of downsides w/ relying on the default termination timeout in servercore (especially for K8s scenarios) and users are unhappy with current solution. I agree we should call this out as a breaking change but think it might be more important to label this as a breaking change in containerd when we update decencies there.

I am not sure I understand why this is a breaking change. Currently, when _crictl.exe stop _ is specified, the target pid is directly killed (that is SIGKILL is the only signal sent). The WaitToKillServiceTimeout reg key is used only when crictl.exe stop **-t ** is specified. Currently no matter what value of 't' is specified, it is always killed in 5 seconds. This fix is only going to help honor the time specified. Am I missing something here?

Discussed this with Kevin today and I understand how this could be a breaking change. I will make sure to call this out specifically.

[kevpar]

If you are just using a channel to wait on once, you can do chan struct{}, and close(wait) instead of sending to the channel. If you want to send/receive multiple times, you can still do chan struct{} and use wait <- struct{}{} to send. This is probably a little more efficient than sending an int.

[kevpar]

Not a big deal to change it here if you don't want to, just letting you know.

[kiashok]

Sure will keep this in mind

[kevpar]

LGTM

[kevpar]

I've signed off on this PR. If there's any additional testing we can do before merging that would be ideal, though. Maybe you can run the upstream containerd integration tests with an updated shim binary?

[kiashok]

I've signed off on this PR. If there's any additional testing we can do before merging that would be ideal, though. Maybe you can run the upstream containerd integration tests with an updated shim binary?

Sure will do

[katiewasnothere]

A small question, otherwise LGTM