Skip to content

Fix error case in Windows layer cleanup#5328

Merged
crosbymichael merged 1 commit intocontainerd:masterfrom
kevpar:win-ci-cleanup
Jun 3, 2021
Merged

Fix error case in Windows layer cleanup#5328
crosbymichael merged 1 commit intocontainerd:masterfrom
kevpar:win-ci-cleanup

Conversation

@kevpar
Copy link
Copy Markdown
Member

@kevpar kevpar commented Apr 9, 2021

ForceRemoveAll has special logic on Windows for cleaning up a Windows
snapshotter directory. The logic was missing proper handling for an
error case that can be returned in some cases.

This fixes the CI failure seen in #5326.

Signed-off-by: Kevin Parsons [email protected]

@kevpar
Copy link
Copy Markdown
Member Author

kevpar commented Apr 9, 2021

@TBBle FYI in case you are interested

ForceRemoveAll has special logic on Windows for cleaning up a Windows
snapshotter directory. The logic was missing proper handling for an
error case that can be returned in some cases.

This fixes the CI failure seen in containerd#5326.

Signed-off-by: Kevin Parsons <[email protected]>
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Apr 9, 2021

Build succeeded.

@TBBle
Copy link
Copy Markdown
Contributor

TBBle commented Apr 9, 2021

Oh weird, I thought I tested that case, and it silently succeeded for me. Perhaps this case is different on Windows 10 20H2 (where I tested it) vs RS5 on CI. It's possible I'm misremembering, but I believe I tested it by bailing out of a test between Activate and Prepare.

Either way, seems reasonable to me.

Comment thread sys/filesys_windows.go
@thaJeztah
Copy link
Copy Markdown
Member

close/reopen to trigger CI

@thaJeztah thaJeztah closed this Apr 13, 2021
@thaJeztah thaJeztah reopened this Apr 13, 2021
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Apr 13, 2021

Build succeeded.

@kzys
Copy link
Copy Markdown
Member

kzys commented May 27, 2021

@containerd/committers Can someone take a look?

@crosbymichael crosbymichael merged commit ada96ec into containerd:master Jun 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants