Skip to content

bug fix: make sure cri image is pinned when it is pulled outside cri#9732

Merged
dims merged 1 commit intocontainerd:mainfrom
henry118:big9726
Feb 7, 2024
Merged

bug fix: make sure cri image is pinned when it is pulled outside cri#9732
dims merged 1 commit intocontainerd:mainfrom
henry118:big9726

Conversation

@henry118
Copy link
Copy Markdown
Member

@henry118 henry118 commented Feb 1, 2024

Fixes: #9726

Update the CRIImageService.UpdateImage routine to pass image ref to getLabels so that a CRI image could be correctly pinned if it's pulled outside of CRI, e.g. via ctr image pull.

@dmcgowan dmcgowan added this to the 2.0 milestone Feb 2, 2024
dims
dims approved these changes Feb 2, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@dims dims left a comment

Choose a reason for hiding this comment

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

LGTM

mikebrow
mikebrow reviewed Feb 2, 2024
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.

see comments.. Thanks for doing this PR!

ruiwen-zhao
ruiwen-zhao reviewed Feb 2, 2024
View reviewed changes
adisky
adisky reviewed Feb 5, 2024
View reviewed changes
@henry118 henry118 force-pushed the big9726 branch 2 times, most recently from c024a0b to 977e17e Compare February 5, 2024 18:03
@henry118
Copy link
Copy Markdown
Member Author

henry118 commented Feb 5, 2024

/retest

@henry118
Copy link
Copy Markdown
Member Author

henry118 commented Feb 5, 2024

/retest

@henry118
Copy link
Copy Markdown
Member Author

henry118 commented Feb 5, 2024

/test pull-containerd-node-e2e

@henry118
Copy link
Copy Markdown
Member Author

henry118 commented Feb 5, 2024

Thanks @mikebrow @ruiwen-zhao @adisky. The PR is updated, please take another look. thanks!

mikebrow
mikebrow reviewed Feb 6, 2024
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.

see comments

@henry118 henry118 force-pushed the big9726 branch 2 times, most recently from 666b8ef to 5cef2f5 Compare February 6, 2024 21:34
@henry118
Copy link
Copy Markdown
Member Author

henry118 commented Feb 6, 2024

/retest

dmcgowan
dmcgowan reviewed Feb 7, 2024
View reviewed changes
internal/cri/server/images/image_pull.go
if oldImg.Target.Digest == img.Target.Digest && oldImg.Labels[crilabels.ImageLabelKey] == labels[crilabels.ImageLabelKey] {
// Retrieve oldImg from image store here because Create routine returns an
// empty image on ErrAlreadyExists
oldImg, err := c.images.Get(ctx, name)
Copy link
Copy Markdown
Member

@dmcgowan dmcgowan Feb 7, 2024

Choose a reason for hiding this comment

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

Could just move this into the else case to avoid unnecessary Get

mikebrow
mikebrow approved these changes Feb 7, 2024
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

@dims dims added the area/cri Container Runtime Interface (CRI) label Feb 7, 2024
@mikebrow mikebrow added this pull request to the merge queue Feb 7, 2024
github-merge-queue bot pushed a commit that referenced this pull request Feb 7, 2024
@mikebrow
Merge pull request #9732 from henry118/big9726
4882955 
bug fix: make sure cri image is pinned when it is pulled outside cri
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Feb 7, 2024
@dmcgowan dmcgowan added this pull request to the merge queue Feb 7, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 7, 2024
@dmcgowan dmcgowan added this pull request to the merge queue Feb 7, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Feb 7, 2024
@henry118
bug fix: make sure cri image is pinned when it is pulled outside cri
1eaf0c1 
Signed-off-by: Henry Wang <henwang@amazon.com>
@dims dims enabled auto-merge February 7, 2024 17:55
dims
dims approved these changes Feb 7, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@dims dims left a comment

Choose a reason for hiding this comment

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

LGTM

@dims dims added this pull request to the merge queue Feb 7, 2024
Merged via the queue into containerd:main with commit 69dff41 Feb 7, 2024
@henry118 henry118 deleted the big9726 branch February 7, 2024 18:52
This was referenced Feb 7, 2024
Merged
Merged
@SergeyKanzhelev SergeyKanzhelev mentioned this pull request Aug 14, 2024
Merged
@neolit123 neolit123 mentioned this pull request Sep 9, 2024
Closed
3 tasks
@wangyuan0916
Copy link
Copy Markdown

wangyuan0916 commented Oct 25, 2024 •
edited
Loading

Hi folks, the pause image is still not pinned when upgrading from 1.6.28 to 1.6.33. During the upgrade, the pause image remained unchanged. Although I use ctr image import pause.tar to load the same pause image again, it won't add the pinned label.
containerd log:
time="2024-10-24T09:34:11.679108113Z" level=info msg="ImageUpdate event &ImageUpdate{Name:registry.k8s.io/pause:3.9,Labels:map[string]string{io.cri-containerd.image: managed,},XXX_unrecognized:[],}"

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

Reviewers

@ruiwen-zhao ruiwen-zhao ruiwen-zhao left review comments

@dims dims dims approved these changes

@dmcgowan dmcgowan dmcgowan approved these changes

@mikebrow mikebrow mikebrow approved these changes

+1 more reviewer

@adisky adisky adisky left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area/cri Container Runtime Interface (CRI) size/M

Projects

None yet

Milestone

2.0

Development

Successfully merging this pull request may close these issues.

CRI pause image is not pinned when it's pulled by ctr

8 participants

@henry118 @wangyuan0916 @dims @dmcgowan @ruiwen-zhao @adisky @mikebrow @k8s-ci-robot