-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[KEP-5365] ImageVolume with an image digest #5375
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
[KEP-5365] ImageVolume with an image digest #5375
Conversation
iholder101
commented
Jun 5, 2025
- One-line PR description: Report ImageVolume's image digest as part of the pod's status.
- Issue link: ImageVolume with an image digest #5365.
- Other comments:
/cc @haircommander I'd be happy if we can track this as alpha for v1.34. |
cf69b47
to
eeef5a3
Compare
Thanks a lot @bitoku!
|
// +featureGate=RecursiveReadOnlyMounts | ||
// +optional | ||
RecursiveReadOnly *RecursiveReadOnlyMode `json:"recursiveReadOnly,omitempty" protobuf:"bytes,4,opt,name=recursiveReadOnly,casttype=RecursiveReadOnlyMode"` | ||
// ImageRef is the digest of the image used for this volume. |
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.
is the expectation this is FQDN + SHA? we should make clear what is expected, imageRef is kind of vague unfortunately and that vaguness has gotten us into trouble in the past
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.
@haircommander I think it would be best to align with the same information as in containerStatuses.imageID
, so something like:
imageID: docker.io/library/busybox@sha256:f85340bf132ae937d2c2a763b8335c9bab35d6e8293f70f606b9c6178d84f42b
Does this make sense?
Do you think it's a good idea to add this as documentation here, or elsewhere in the KEP?
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.
nack on imageID name though, it's a really ambiguous title unfortunately and has caused issues in the past
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.
you can leave it in the documentation of the CRI field I think, so CRI implementations can see what to do. that can eventually be validated by critest
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.
Makes sense, done!
I've added:
// It should have a value that's similar to the pod's status.containerStatuses[i].imageID.
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.
drive-by on way to PRR. Pod volumes have a discriminated union so that values only pertinent to one volume type are only associated with one volume type. Why would status here be different?
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.
are you suggesting we add an intermediary struct for image volume specific information and nest the imageRef in there @deads2k ?
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.
are you suggesting we add an intermediary struct for image volume specific information and nest the imageRef in there @deads2k ?
Yes, that would match the structure used when defining the values and provide clarity about what can be expected without reading docs
If you are interested in getting this merged for 1.34, please add a PRR file and request review from PRR team. |
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.
small and useful KEP. it will be great if we can fit it into 1.34
08b7bed
to
754db04
Compare
Done, thanks for the reminder! ping @kubernetes/prod-readiness-reviewers for a review :) |
876e051
to
741eb29
Compare
Signed-off-by: Itamar Holder <iholder@redhat.com>
Signed-off-by: Itamar Holder <iholder@redhat.com>
741eb29
to
7fe7bae
Compare
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.
Please complete the PRR questionairre.
// +featureGate=RecursiveReadOnlyMounts | ||
// +optional | ||
RecursiveReadOnly *RecursiveReadOnlyMode `json:"recursiveReadOnly,omitempty" protobuf:"bytes,4,opt,name=recursiveReadOnly,casttype=RecursiveReadOnlyMode"` | ||
// ImageRef is the digest of the image used for this volume. |
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.
drive-by on way to PRR. Pod volumes have a discriminated union so that values only pertinent to one volume type are only associated with one volume type. Why would status here be different?
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.
content lgtm, PRR needs work as @deads2k pointed out
Signed-off-by: Itamar Holder <iholder@redhat.com>
7fe7bae
to
22d6171
Compare
Thanks a lot @deads2k! |
|
||
##### e2e tests | ||
|
||
Not sure there's a need for e2e tests for this feature. |
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.
we should have some e2e tests to verify it's working e2e when we expect it to
PRR lgtm /approve |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bitoku, deads2k, iholder101, mrunalp The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
As described in the ImageVolume VEP (https://github.com/kubevirt/enhancements/blob/main/veps/sig-compute/image-volume-proposal.md), we need the ImageVolume to report the image digest, similar to how imageID is exposed in containerStatus. There is currently a KEP in Kubernetes to address this need (kubernetes/enhancements#5375), but it depends on CRI-O support and may take time to GA. To avoid delaying ImageVolume graduation in kubevirt, this commit adds a noop containers using the images from containerDisks and kernel boot. To ensure that the image digest is fetched and reported, enabling support during live migration. Signed-off-by: bmordeha <bmordeha@redhat.com>