-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Support image pull per runtime class #11807
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
Conversation
f482f62 to
3056a17
Compare
|
Looks like |
taking a look now |
6d08cf6 to
1c2e937
Compare
miz060
left a comment
There was a problem hiding this 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 |
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
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. |
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>
|
@kiashok: The following tests failed, say
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. DetailsInstructions 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. |
|
@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! |
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:
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