Skip to content

runtime/v2/runc: fix the defer cleanup of the NewContainer#5206

Merged
fuweid merged 1 commit intocontainerd:masterfrom
Iceber:fix-new-container
Mar 17, 2021
Merged

runtime/v2/runc: fix the defer cleanup of the NewContainer#5206
fuweid merged 1 commit intocontainerd:masterfrom
Iceber:fix-new-container

Conversation

@Iceber
Copy link
Copy Markdown
Member

@Iceber Iceber commented Mar 16, 2021 •
edited
Loading

The cleanup operation does not take effect in many cases

e.g.

containerd/runtime/v2/runc/container.go

Lines 112 to 116 in 2755ead

if err := m.Mount(rootfs); err != nil {
return nil, errors.Wrapf(err, "failed to mount rootfs component %v", m)
}
}

containerd/runtime/v2/runc/container.go

Lines 130 to 132 in 2755ead

if err := p.Create(ctx, config); err != nil {
return nil, errdefs.ToGRPC(err)
}

containerd/runtime/v2/runc/container.go

Lines 143 to 152 in 2755ead

if cgroups.Mode() == cgroups.Unified {
g, err := cgroupsv2.PidGroupPath(pid)
if err != nil {
logrus.WithError(err).Errorf("loading cgroup2 for %d", pid)
return container, nil
}
cg, err = cgroupsv2.LoadManager("/sys/fs/cgroup", g)
if err != nil {
logrus.WithError(err).Errorf("loading cgroup2 for %d", pid)
}

@k8s-ci-robot
Copy link
Copy Markdown

k8s-ci-robot commented Mar 16, 2021

Hi @Iceber. 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.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Mar 16, 2021

Build succeeded.

@Iceber Iceber force-pushed the fix-new-container branch from 86ceac2 to 5e484c9 Compare March 16, 2021 03:42
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Mar 16, 2021

Build succeeded.

crosbymichael
crosbymichael approved these changes Mar 16, 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

Zyqsempai
Zyqsempai approved these changes Mar 16, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@Zyqsempai Zyqsempai left a comment

Choose a reason for hiding this comment

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

LG

fuweid
fuweid approved these changes Mar 17, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM

@fuweid fuweid merged commit 1a0973d into containerd:master Mar 17, 2021
@Iceber
Copy link
Copy Markdown
Member Author

Iceber commented Mar 17, 2021

@fuweid Is it necessary to fix release/1.4? This looks useful for the previous version.

@fuweid
Copy link
Copy Markdown
Member

fuweid commented Mar 17, 2021

@fuweid Is it necessary to fix release/1.4? This looks useful for the previous version.

yeah. please

@Iceber Iceber mentioned this pull request Apr 8, 2021
Merged
@thaJeztah thaJeztah added cherry-picked/1.4.x PR commits are cherry picked into the release/1.4 branch and removed cherry-pick/1.4.x labels Apr 9, 2021
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

@fuweid fuweid fuweid approved these changes

+1 more reviewer

@Zyqsempai Zyqsempai Zyqsempai approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cherry-picked/1.4.x PR commits are cherry picked into the release/1.4 branch needs-ok-to-test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Iceber @k8s-ci-robot @fuweid @crosbymichael @Zyqsempai @thaJeztah