Skip to content

Conversation

@kiashok
Copy link
Contributor

@kiashok kiashok commented May 6, 2025

Adds support for image pull per runtime
class with the help of gc labels.
The image pull code path remains the same
but during UpdateImage() call, the image index
is walked to find the appropriate platform
and snapshot the image was unpacked for.
Then it finds all corresponding runtime handlers
with identical platform and snapshotter from
CriConfig.RuntimePlatforms.
Then new entries with image tuple (ref, runtimehandler) are added to containerd and cri image stores.
Appropriate GC labels and references are added to the root image from new entries to handle cleanup from gc correctly.
This approach also makes the image delete and reload code paths simple.

Note to reviewers:

  • Main logic lies in the following files:

For image pull:

internal/cri/server/images/image_pull.go,
core/images/image.go
internal/cri/server/store/image.go
internal/cri/server/image/check.go

Image deletion:

internal/cri/server/images/image_remove.go

containerd restart aka image reload:

internal/cri/server/images/check.go

  • This PR is missing unit tests for the logic being added. Will add anther commit with validation tests once there is consensus on the approach taken.

@samuelkarp
Copy link
Member

Looks like TestImageLoad is failing pretty consistently in CI.

@kiashok
Copy link
Contributor Author

kiashok commented May 22, 2025

Looks like TestImageLoad is failing pretty consistently in CI.

taking a look now

@kiashok kiashok force-pushed the kep4216-gc-labels branch 3 times, most recently from 6d08cf6 to 1c2e937 Compare May 22, 2025 23:08
Copy link

@miz060 miz060 left a comment

Choose a reason for hiding this comment

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

The PR description mentions this implements KEP-4216. Do you know what the migration strategy would look like?

// Id of the image. Normally the digest of image config.
ID string
// Key of the image - (ID, RuntimeHandler)
Key ImageIDKey
Copy link

Choose a reason for hiding this comment

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

Question: Would this be a breaking change in any way? What would the path for images pulled before this change look like after the changes are in?

)

var (
imageNameWithRuntimeHandler = "%s,%s"
Copy link

Choose a reason for hiding this comment

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

Question: Could an image name contain a comma? Is there a guidance on the standard for image names?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a new format as now image name has to only be thought of as a tuple of (imageName,runtimehandler). Please refer to KEP 4126 for more details.

@kiashok
Copy link
Contributor Author

kiashok commented Jul 1, 2025

Due to reprioritization and higher priority tasks, I will not be able to actively work on this change to fit the schedule @miz060 is looking for. Mitch will be taking over this commit and following up till merge.
@miz060 please add me or yourself as co-author and take it over. Rope me in for any help or questions that you might have. Would be happy to help. Thanks.

Adds support for image pull per runtime
class with the help of gc labels.
The image pull code path remains the same
but during UpdateImage() call, the image index
is walked to find the appropriate platform
and snapshot the image was unpacked for.
Then it finds all corresponding runtime handlers
with identical platform and snapshotter from
CriConfig.RuntimePlatforms.
Then new entries with image tuple (ref, runtimehandler)
are added to containerd and cri image stores.
Appropriate GC labels and references are added to the
root image from new entries to handle cleanup from
gc correctly.
This approach also makes the image delete and reload code
paths simple.

Signed-off-by: Kirtana Ashok <kiashok@microsoft.com>
@k8s-ci-robot
Copy link

@kiashok: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-containerd-k8s-e2e-ec2 fb321af link false /test pull-containerd-k8s-e2e-ec2
pull-containerd-node-e2e fb321af link true /test pull-containerd-node-e2e

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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-sigs/prow repository. I understand the commands that are listed here.

@kiashok
Copy link
Contributor Author

kiashok commented Aug 1, 2025

@miz060 Will close this PR for now as it is not being actively worked on anymore. Feel free to take my branch and create a PR as needed. Thanks!

@kiashok kiashok closed this Aug 1, 2025
@github-project-automation github-project-automation bot moved this from Needs Triage to Done in Pull Request Review Aug 1, 2025
@ChengyuZhu6
Copy link
Member

@miz060 I'm wondering if you're still working on this PR. I have the same image issue when trying to switch snapshotters. If you're not actively working on this, would it be okay if I take it over and continue the work? cc @kiashok

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

Labels

area/cri Container Runtime Interface (CRI) kind/feature size/XXL

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants