Skip to content

[24.0 backport] libcontainerd/windows: Fix cleanup on newIOFromProcess error#46505

Merged
thaJeztah merged 3 commits intomoby:24.0from
vvoland:libcontainerd-windows-reap-fix-24
Sep 19, 2023
Merged

[24.0 backport] libcontainerd/windows: Fix cleanup on newIOFromProcess error#46505
thaJeztah merged 3 commits intomoby:24.0from
vvoland:libcontainerd-windows-reap-fix-24

Conversation

@vvoland
Copy link
Copy Markdown
Contributor

@vvoland vvoland commented Sep 18, 2023

- What I did
Fixed new process not being killed and deleted when newIOFromProcess would fail.

- How I did it
See commits.

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

… failure

Error check in defer block used wrong error variable which is always nil
if the flow reaches the defer. This caused the `newProcess.Kill` to be
never called if the subsequent attemp to attach to the stdio failed.
Although this only happens in Exec (as Start does overwrite the error),
this also adjusts the Start to also use the returned error to avoid this
kind of mistake in future changes.

Signed-off-by: Paweł Gronowski <[email protected]>
(cherry picked from commit 55b6640)
Signed-off-by: Paweł Gronowski <[email protected]>
The cleanup defer uses an `outErr` now, so we don't need to worry about
shadowing.

Signed-off-by: Paweł Gronowski <[email protected]>
(cherry picked from commit b805599)
Signed-off-by: Paweł Gronowski <[email protected]>
Synchronize the code to do the same thing as Exec.
reap doesn't need to be called before the start event was sent.
There's already a defer block which cleans up the process in case where
an error occurs.

Signed-off-by: Paweł Gronowski <[email protected]>
(cherry picked from commit 0937aef)
Signed-off-by: Paweł Gronowski <[email protected]>
@vvoland vvoland added this to the 24.0.7 milestone Sep 18, 2023
@thaJeztah
Copy link
Copy Markdown
Member

Flaky test; wondering if this is one that's still polling, or needs timeouts adjusted for Windows

=== FAIL: github.com/docker/docker/integration-cli TestDockerAPISuite/TestContainersAPICreateMountsCreate/0_config:_{volume__c:\foo_false__<nil>_<nil>_<nil>_<nil>} (12.36s)
    docker_api_containers_test.go:2136: timeout hit after 10s: container 05819d858b23900cd45179cdb84a0f47b6093b4b896494667725ce95d94708cf is running, waiting for exit

=== FAIL: github.com/docker/docker/integration-cli TestDockerAPISuite/TestContainersAPICreateMountsCreate (33.72s)

=== FAIL: github.com/docker/docker/integration-cli TestDockerAPISuite (842.41s)

Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants