Skip to content

runtime/v2: Fix defer cleanup for TaskManager.Create#5148

Merged
mxpv merged 1 commit intocontainerd:masterfrom
wzshiming:fix/defer-cleanup
Mar 20, 2021
Merged

runtime/v2: Fix defer cleanup for TaskManager.Create#5148
mxpv merged 1 commit intocontainerd:masterfrom
wzshiming:fix/defer-cleanup

Conversation

@wzshiming
Copy link
Copy Markdown
Contributor

@wzshiming wzshiming commented Mar 10, 2021

No description provided.

@k8s-ci-robot
Copy link
Copy Markdown

k8s-ci-robot commented Mar 10, 2021

Hi @wzshiming. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@wzshiming wzshiming changed the title runtime/v2: Fix defer cleanup runtime/v2: Fix defer cleanup for TaskManager.Create Mar 10, 2021
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Mar 10, 2021

Build succeeded.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Mar 15, 2021

Build succeeded.

AkihiroSuda
AkihiroSuda reviewed Mar 18, 2021
View reviewed changes
Comment thread runtime/v2/manager.go Outdated
@AkihiroSuda
Copy link
Copy Markdown
Member

AkihiroSuda commented Mar 18, 2021

Can we have a test?

@wzshiming wzshiming force-pushed the fix/defer-cleanup branch 2 times, most recently from ba5d4f1 to 0ba9247 Compare March 18, 2021 12:19
@wzshiming
Copy link
Copy Markdown
Contributor Author

wzshiming commented Mar 18, 2021

There is no test for this piece of code. If you want to add it, may need to add it completely. there is no way to add only this function. need some time, maybe I open a new PR to add tests?

crosbymichael
crosbymichael approved these changes Mar 18, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@crosbymichael crosbymichael left a comment

Choose a reason for hiding this comment

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

LGTM

mxpv
mxpv reviewed Mar 19, 2021
View reviewed changes
Comment thread runtime/v2/manager.go Outdated
@wzshiming
runtime/v2: Fix defer cleanup
30e1e66 
Signed-off-by: Shiming Zhang <[email protected]>
@wzshiming wzshiming requested review from AkihiroSuda and mxpv March 20, 2021 17:03
@mxpv mxpv merged commit 56f17a0 into containerd:master Mar 20, 2021
@wzshiming wzshiming deleted the fix/defer-cleanup branch March 21, 2021 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@crosbymichael crosbymichael crosbymichael approved these changes

@mxpv mxpv mxpv approved these changes

@AkihiroSuda AkihiroSuda Awaiting requested review from AkihiroSuda

Assignees

No one assigned

Labels

needs-ok-to-test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@wzshiming @k8s-ci-robot @AkihiroSuda @crosbymichael @mxpv