Wait for waitInitExit() to return #1249

gabriel-samfira · 2021-12-20T16:16:22Z

This change gives waitInitExit() a chance to cleanup resource when DeleteExec() is called, before returning.

TL;DR

This should fix situations where the shim exits before releasing container resources.

TS;NM

I am not sure if this is a proper fix, or if it is just a band-aid over something that should be fixed elsewhere, but it did fix the TestRestartMonitor test in containerd. This issue also seems to cause flakiness in the ```TestContainerdRestart```` test due to the fact that lingering sandboxes are picked up by the test and fails.

The restart monitor in containerd watches for situations where tasks end, and attempts to restart them. The TestRestartMonitor test will spawn a task, then send a Kill() signal to that task and wait for the restart monitor to respawn it.

The problem is that after the task is killed, it is properly marked as terminated, but the shim exits before waitInitExit() returns, so it doesn't have a chance to cleanup container resources (unprepare layers, etc). The test issues a DeleteTask(), which, due to the fact that the task has been marked as terminated does not fail any precondition and proceeds. Once the task is deleted, the shim exits (from my understanding), which if the waitInitExit() goroutine has not finished cleaning up container resources, will mean that lingering layers will remain mounted and in use:

This gives the shim a chance to cleanup resources before deleting the task.

Signed-off-by: Gabriel Adrian Samfira gsamfira@cloudbasesolutions.com

dcantah · 2021-12-20T18:16:40Z

@anmaxvl @helsaawy Assigning us three for this, small change but in a critical path. I'm going to run the cri-containerd tests with this and see if I can spot anything that would trip this up. Gabriel has ran the full integration test suite in upstream containerd (integration/client tests) and he's ran the TestRestartMonitor tests in a loop for a couple hours with no failures iirc

dcantah · 2021-12-20T22:59:36Z

Ok running this locally has the test fail for me, albeit for a new reason than before. Perhaps I'm missing a containerd side change needed or there's a difference in OS behavior on 20H2. @gabriel-samfira You tested this on ws2019 I assume?

gabriel-samfira · 2021-12-22T09:22:32Z

Ok running this locally has the test fail for me, albeit for a new reason than before. Perhaps I'm missing a containerd side change needed or there's a difference in OS behavior on 20H2. @gabriel-samfira You tested this on ws2019 I assume?

Yes, there is more than this issue making the restart monitor test fail. The other issue is that in some situations, sending a second Kill() to the same process handle will return an error 5, which is Access is denied.. I can't say for sure why this happens, but my guess is that the process is still waiting on some I/O to finish or there are still handles open to it, and it's in a state where a Kill signal will return access denied. From my tests, ignoring the error is safe, because the process does eventually die due to the first Kill that was sent, and handles are released, wait() goroutines return and resources are released.

I will open a separate issue detailing the error returned by Kill().

gabriel-samfira · 2021-12-22T10:43:45Z

I wonder if this select statement would be better placed outside of that switch statement. There is a call to ForceExit() if the state is shimExecStateCreated. That in turn can potentially send a Kill(). We might want to wait for that as well.

What do you think?

cmd/containerd-shim-runhcs-v1/task_hcs.go

helsaawy · 2021-12-22T21:03:56Z

cmd/containerd-shim-runhcs-v1/task_hcs.go

@@ -588,6 +589,27 @@ func (ht *hcsTask) DeleteExec(ctx context.Context, eid string) (int, uint32, tim
 	case shimExecStateRunning:
 		return 0, 0, time.Time{}, newExecInvalidStateError(ht.id, eid, state, "delete")
 	}
+
+	if eid == "" {


If we are cleaning up resources here when deleting the init task (since this triggers ht.close()), should we also delete everything in ht.execs here?

Good question. Let me dig a bit deeper and see if the entire hcsTask object is not reaped somewhere after init dies and the init task gets deleted.

I updated the PR to remove the task object after successfully deleting the init exec:

https://github.com/microsoft/hcsshim/pull/1249/files#diff-3524c9e80b115f6d1dd47e517ed3861db8c2fb7c7857224b2fc5e11bbf50f722R232

cmd/containerd-shim-runhcs-v1/service_internal.go

helsaawy · 2021-12-27T16:53:29Z

cmd/containerd-shim-runhcs-v1/service_internal.go

+		// We've successfully deleted the init exec. Compute resources
+		// should be closed, layers umounted and resources released.
+		// This task should now be done.
+		s.taskOrPod.Store(getEmptyPodOrTask(s.isSandbox))


If the service is tracking a pod (s.isSandbox == true), then there could be multiple tasks stored under s.getPod().workloadTasks, and emptying the taskOrPod store could prematurely close out other running tasks in the pod.
(I have a draft PR to address this: #1234)

Ahh! Then I'll remove this bit and simply clean up the execs. Your branch will take care of this properly.

Done. Please have another look.

helsaawy · 2021-12-27T23:51:42Z

lgtm

dcantah · 2022-01-04T22:08:31Z

@gabriel-samfira Sorry for the delay on check-in, we wanted to have one of us run some tests on this to verify the (now correct funnily enough 😆) behavior doesn't break any assumptions we may have built around the old behavior. Plan is to check this in today and get it backported to release/0.9 if everything's smooth. I'll also run the RestartMonitor test as well to see if that works for me now

dcantah · 2022-01-05T02:34:45Z

So TestRestartMonitor still fails for me but with restart_monitor_test.go:114: failed to remove C:\Users\danny\AppData\Local\Temp\containerd-test-new-daemon-with-config459297417: remove C:\Users\danny\AppData\Local\Temp\containerd-test-new-daemon-with-config459297417\root\io.containerd.snapshotter.v1.windows\snapshots\1\Files\Windows\Branding\Basebrd\basebrd.dll: Access is denied. which is while cleaning up the root directory for the test instance of containerd that's launched; the actual logic for the task restart looks to go through fine. I'm on 20H2, but let me try this on a ws2019 machine and my win11 machine.

Unless there was an extra fix needed in the integration tests that I'm forgetting @gabriel-samfira?

gabriel-samfira · 2022-01-05T05:24:21Z

So TestRestartMonitor still fails for me but with restart_monitor_test.go:114: failed to remove C:\Users\danny\AppData\Local\Temp\containerd-test-new-daemon-with-config459297417: remove C:\Users\danny\AppData\Local\Temp\containerd-test-new-daemon-with-config459297417\root\io.containerd.snapshotter.v1.windows\snapshots\1\Files\Windows\Branding\Basebrd\basebrd.dll: Access is denied. which is while cleaning up the root directory for the test instance of containerd that's launched; the actual logic for the task restart looks to go through fine. I'm on 20H2, but let me try this on a ws2019 machine and my win11 machine.

This looks like a filesystem permissions issue. Try the following:

icacls.exe "C:\Users\danny\AppData\Local\Temp" /grant "CREATOR OWNER":(OI)(CI)(IO)F /T

This will give "CREATOR OWNER" full access rights on files they create. Similar to what the sticky bit does on Linux. It's possible that once created, files inherit access rights from "C:\Users\danny\AppData\Local\Temp" and containerd can no longer remove them.

We had to do the same thing in the CI, here:

https://github.com/containerd/containerd/blob/main/script/test/utils.sh#L157-L162

dcantah · 2022-01-05T23:06:05Z

Ok this has been running in a loop on two machines, one ws2019 and one ws2022, with no issues for over an hour. I just ran the cri-containerd tests we have in /test in this repo and no issues observed either so things look A-Ok!

The function passed into the Range function of sync.Map will stop the iteration if false is returned. This commit makes sure we iterate through all elements in the map. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>

This change gives waitInitExit() a chance to cleanup resource when DeleteExec() is called, before returning. This should fix situations where the shim exits before releasing container resources. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>

dcantah

LGTM

…exit Wait for waitInitExit() to return (cherry picked from commit 6241c53)

…exit Wait for waitInitExit() to return (cherry picked from commit 6241c53) Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>

[release/0.9] Wait for waitInitExit() to return #1249

gabriel-samfira requested a review from a team as a code owner December 20, 2021 16:16

gabriel-samfira mentioned this pull request Dec 20, 2021

Fix restart container test containerd/containerd#6390

Merged

dcantah assigned anmaxvl, dcantah and helsaawy Dec 20, 2021

dcantah mentioned this pull request Dec 20, 2021

Windows Containerd: Kubelet processes not cleaned up due to container_log_manager cleanup issues, results in hung exec processes and memory explosion over time kubernetes/kubernetes#98102

Closed

gabriel-samfira mentioned this pull request Dec 22, 2021

Sending a second Kill() returns "Access is denied." #1251

Closed

gabriel-samfira force-pushed the wait-for-init-exit branch 2 times, most recently from e31f95a to 511321a Compare December 22, 2021 16:01

helsaawy reviewed Dec 22, 2021

View reviewed changes

cmd/containerd-shim-runhcs-v1/task_hcs.go Show resolved Hide resolved

helsaawy reviewed Dec 22, 2021

View reviewed changes

gabriel-samfira force-pushed the wait-for-init-exit branch from 0fb1e80 to 57417fb Compare December 23, 2021 11:46

gabriel-samfira commented Dec 23, 2021

View reviewed changes

cmd/containerd-shim-runhcs-v1/service_internal.go Outdated Show resolved Hide resolved

gabriel-samfira force-pushed the wait-for-init-exit branch 2 times, most recently from b4dd6c7 to 6bf7520 Compare December 23, 2021 13:21

helsaawy reviewed Dec 27, 2021

View reviewed changes

gabriel-samfira force-pushed the wait-for-init-exit branch 2 times, most recently from d2cf3e8 to 2d07914 Compare December 27, 2021 21:51

anmaxvl approved these changes Dec 29, 2021

View reviewed changes

gabriel-samfira force-pushed the wait-for-init-exit branch from 2d07914 to 6a00fc5 Compare January 6, 2022 06:36

Fix Range() iteration

76881a2

The function passed into the Range function of sync.Map will stop the iteration if false is returned. This commit makes sure we iterate through all elements in the map. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>

gabriel-samfira force-pushed the wait-for-init-exit branch from 6a00fc5 to a6edb25 Compare January 6, 2022 06:51

dcantah approved these changes Jan 6, 2022

View reviewed changes

dcantah merged commit 6241c53 into microsoft:master Jan 6, 2022

helsaawy pushed a commit to helsaawy/hcsshim that referenced this pull request Jan 6, 2022

Merge pull request microsoft#1249 from gabriel-samfira/wait-for-init-…

71ef4a3

…exit Wait for waitInitExit() to return (cherry picked from commit 6241c53)

jsturtevant mentioned this pull request Jan 6, 2022

Improve Windows logging and general stability for upstream e2e tests kubernetes-sigs/cluster-api-provider-azure#1932

Merged

3 tasks

helsaawy added a commit that referenced this pull request Jan 6, 2022

Merge pull request #1264 from helsaawy/he/backport-waitinitexit

51b804f

[release/0.9] Wait for waitInitExit() to return #1249

gabriel-samfira mentioned this pull request Mar 1, 2023

Mount snapshots on Windows containerd/containerd#8043

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wait for waitInitExit() to return #1249

Wait for waitInitExit() to return #1249

gabriel-samfira commented Dec 20, 2021

dcantah commented Dec 20, 2021 •

edited

Loading

dcantah commented Dec 20, 2021

gabriel-samfira commented Dec 22, 2021

gabriel-samfira commented Dec 22, 2021

helsaawy Dec 22, 2021

gabriel-samfira Dec 23, 2021

gabriel-samfira Dec 23, 2021 •

edited

Loading

helsaawy Dec 27, 2021

gabriel-samfira Dec 27, 2021

gabriel-samfira Dec 27, 2021

helsaawy commented Dec 27, 2021

dcantah commented Jan 4, 2022 •

edited

Loading

dcantah commented Jan 5, 2022 •

edited

Loading

gabriel-samfira commented Jan 5, 2022 •

edited

Loading

dcantah commented Jan 5, 2022

dcantah left a comment

Wait for waitInitExit() to return #1249

Wait for waitInitExit() to return #1249

Conversation

gabriel-samfira commented Dec 20, 2021

dcantah commented Dec 20, 2021 • edited Loading

dcantah commented Dec 20, 2021

gabriel-samfira commented Dec 22, 2021

gabriel-samfira commented Dec 22, 2021

helsaawy Dec 22, 2021

Choose a reason for hiding this comment

gabriel-samfira Dec 23, 2021

Choose a reason for hiding this comment

gabriel-samfira Dec 23, 2021 • edited Loading

Choose a reason for hiding this comment

helsaawy Dec 27, 2021

Choose a reason for hiding this comment

gabriel-samfira Dec 27, 2021

Choose a reason for hiding this comment

gabriel-samfira Dec 27, 2021

Choose a reason for hiding this comment

helsaawy commented Dec 27, 2021

dcantah commented Jan 4, 2022 • edited Loading

dcantah commented Jan 5, 2022 • edited Loading

gabriel-samfira commented Jan 5, 2022 • edited Loading

dcantah commented Jan 5, 2022

dcantah left a comment

Choose a reason for hiding this comment

dcantah commented Dec 20, 2021 •

edited

Loading

gabriel-samfira Dec 23, 2021 •

edited

Loading

dcantah commented Jan 4, 2022 •

edited

Loading

dcantah commented Jan 5, 2022 •

edited

Loading

gabriel-samfira commented Jan 5, 2022 •

edited

Loading