Skip to content

diff/walking: fix defer cleanup#5551

Merged
crosbymichael merged 1 commit intocontainerd:masterfrom
Iceber:fix-diff
Jun 3, 2021
Merged

diff/walking: fix defer cleanup#5551
crosbymichael merged 1 commit intocontainerd:masterfrom
Iceber:fix-diff

Conversation

@Iceber
Copy link
Copy Markdown
Member

@Iceber Iceber commented Jun 1, 2021

err may be overwritten

containerd/diff/walking/differ.go

Lines 143 to 147 in c8b33ba

if err := cw.Commit(ctx, 0, dgst, commitopts...); err != nil {
if !errdefs.IsAlreadyExists(err) {
return errors.Wrap(err, "failed to commit")
}
}

containerd/diff/walking/differ.go

Lines 156 to 162 in c8b33ba

// Set uncompressed label if digest already existed without label
if _, ok := info.Labels[uncompressed]; !ok {
info.Labels[uncompressed] = config.Labels[uncompressed]
if _, err := s.store.Update(ctx, info, "labels."+uncompressed); err != nil {
return errors.Wrap(err, "error setting uncompressed label")
}
}

@k8s-ci-robot
Copy link
Copy Markdown

k8s-ci-robot commented Jun 1, 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.

@Iceber Iceber closed this Jun 1, 2021
@Iceber Iceber reopened this Jun 1, 2021
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Jun 1, 2021

Build succeeded.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Jun 1, 2021

Build succeeded.

@crosbymichael crosbymichael requested a review from dmcgowan June 3, 2021 15:02
@crosbymichael
Copy link
Copy Markdown
Member

crosbymichael commented Jun 3, 2021

/ok-to-test

mikebrow
mikebrow approved these changes Jun 3, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

LGTM

@crosbymichael crosbymichael merged commit 6ae9090 into containerd:master Jun 3, 2021
@dmcgowan
Copy link
Copy Markdown
Member

dmcgowan commented Jun 3, 2021

#5242 (comment), err here was intentionally used to avoid cleanup after Commit was called. Was there an issue which this was causing. This code should either be reverted and documented or cleaned up to avoid the confusion around err re-use.

@dmcgowan dmcgowan mentioned this pull request Jun 3, 2021
Merged
@Iceber
Copy link
Copy Markdown
Member Author

Iceber commented Jun 4, 2021

err here was intentionally used to avoid cleanup after Commit was called.

@dmcgowan If we want the defer cleanup to be called only before Commit, the failure of the info, err := s.store.Info(ctx, dgst) may also trigger the call, but of course #5566 can fix this issue

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

Reviewers

@samuelkarp samuelkarp samuelkarp approved these changes

@crosbymichael crosbymichael crosbymichael approved these changes

@mikebrow mikebrow mikebrow approved these changes

@dmcgowan dmcgowan Awaiting requested review from dmcgowan

Assignees

No one assigned

Labels

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 @crosbymichael @dmcgowan @samuelkarp @mikebrow