Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ctr: fix the cleanup of task #8174

Merged
merged 1 commit into from
May 31, 2023
Merged

ctr: fix the cleanup of task #8174

merged 1 commit into from
May 31, 2023

Conversation

Iceber
Copy link
Member

@Iceber Iceber commented Feb 28, 2023

Containers and tasks are not cleaned up when ctr run --rm exits due to a cni configuration error.

$ # No more logs, and containers and tasks are not cleaned up
$ ctr run --rm --cni docker.io/library/nginx:1.23  test-1 ls
ERRO[0000] network review                                error="plugin type=\"bridge\" failed (delete): incompatible CNI versions; config is \"1.0.0\", plugin supports [\"0.1.0\" \"0.2.0\" \"0.3.0\" \"0.3.1\" \"0.4.0\"]"
ctr: plugin type="portmap" failed (add): incompatible CNI versions; config is "1.0.0", plugin supports ["0.1.0" "0.2.0" "0.3.0" "0.3.1" "0.4.0"]

$ ctr containers ls
test-1       docker.io/library/nginx:1.23                                             io.containerd.runc.v2
$ ctr tasks ls
test-1     2964087    CREATED

@k8s-ci-robot
Copy link

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.

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.

@Iceber Iceber force-pushed the fix_ctr_run branch 5 times, most recently from 601ef2f to 956b7f4 Compare March 6, 2023 08:53
defer container.Delete(ctx, containerd.WithSnapshotCleanup)
defer func() {
if err := container.Delete(ctx, containerd.WithSnapshotCleanup); err != nil {
logrus.WithError(err).Error("delete container")
Copy link
Member

Choose a reason for hiding this comment

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

suggest to use failed to cleanup container

task.Delete(ctx)

if _, err := task.Delete(ctx, containerd.WithProcessKill); err != nil && !errdefs.IsNotFound(err) {
logrus.WithError(err).Error("delete task")
Copy link
Member

Choose a reason for hiding this comment

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

suggest to use failed to cleanup task

Copy link
Member Author

Choose a reason for hiding this comment

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

This uses the same style of error message as the rest of the run command, eg. logrus.WithError(err).Error("console resize")

do we need to change the rest together?

cmd/ctr/commands/run/run.go Outdated Show resolved Hide resolved
@Iceber
Copy link
Member Author

Iceber commented Mar 17, 2023

The message style of WithError(err).Error("msg") in ctr is not uniform, here it is just to keep the message of the run command cleanup uniform

In the future I can try to standardize the message style of all WithError(err).Error in ctr

@fuweid PTAL

@Iceber Iceber changed the title ctr/run: fix the cleanup of task ctr: fix the cleanup of task Mar 17, 2023
@fuweid
Copy link
Member

fuweid commented May 12, 2023

@Iceber please rebase. thanks!

Signed-off-by: Iceber Gu <wei.cai-nat@daocloud.io>
@Iceber
Copy link
Member Author

Iceber commented May 12, 2023

@fuweid updated

@Iceber
Copy link
Member Author

Iceber commented May 12, 2023

/retest

Copy link
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

@Iceber Iceber requested a review from dmcgowan May 15, 2023 03:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants