Skip to content

[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

Merged

Conversation

iholder101
Copy link
Contributor

  • One-line PR description: Report ImageVolume's image digest as part of the pod's status.
  • Other comments:

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/node Categorizes an issue or PR as relevant to SIG Node. labels Jun 5, 2025
@k8s-ci-robot k8s-ci-robot requested review from dchen1107 and mrunalp June 5, 2025 10:26
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jun 5, 2025
@iholder101
Copy link
Contributor Author

/cc @haircommander

I'd be happy if we can track this as alpha for v1.34.
WDYT?

@iholder101 iholder101 force-pushed the kep/1st-image-volume-with-digest branch 2 times, most recently from cf69b47 to eeef5a3 Compare June 9, 2025 12:07
@iholder101
Copy link
Contributor Author

Thanks a lot @bitoku!

Which do we want ImageDigest or ImageRef?

As a reference, imageID in v1.ContainerStatus refers to imageRef in CRI ContainerStatus.

imageRef then! The same one already used for container statuses.
Thanks for this clarification! PTAL

// +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.
Copy link
Contributor

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

Copy link
Contributor Author

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?

Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor

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 ?

Copy link
Contributor

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

@kannon92
Copy link
Contributor

If you are interested in getting this merged for 1.34, please add a PRR file and request review from PRR team.

Copy link
Member

@SergeyKanzhelev SergeyKanzhelev left a 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

@iholder101 iholder101 force-pushed the kep/1st-image-volume-with-digest branch 3 times, most recently from 08b7bed to 754db04 Compare June 15, 2025 11:35
@iholder101
Copy link
Contributor Author

If you are interested in getting this merged for 1.34, please add a PRR file and request review from PRR team.

Done, thanks for the reminder!

ping @kubernetes/prod-readiness-reviewers for a review :)
I've assigned @deads2k. If someone else wishes to become a review/approver, let me know!

@iholder101 iholder101 force-pushed the kep/1st-image-volume-with-digest branch 2 times, most recently from 876e051 to 741eb29 Compare June 17, 2025 06:51
Signed-off-by: Itamar Holder <iholder@redhat.com>
Signed-off-by: Itamar Holder <iholder@redhat.com>
@iholder101 iholder101 force-pushed the kep/1st-image-volume-with-digest branch from 741eb29 to 7fe7bae Compare June 17, 2025 06:52
Copy link
Contributor

@deads2k deads2k left a 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.
Copy link
Contributor

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?

Copy link
Member

@SergeyKanzhelev SergeyKanzhelev left a 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>
@iholder101 iholder101 force-pushed the kep/1st-image-volume-with-digest branch from 7fe7bae to 22d6171 Compare June 18, 2025 14:28
@iholder101
Copy link
Contributor Author

Thanks a lot @deads2k!
I believe I have addressed all comments. Could you kindly PTAL?


##### e2e tests

Not sure there's a need for e2e tests for this feature.
Copy link
Contributor

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

@deads2k
Copy link
Contributor

deads2k commented Jun 18, 2025

PRR lgtm

/approve

@haircommander
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 18, 2025
@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 18, 2025
@k8s-ci-robot k8s-ci-robot merged commit 97ebcc9 into kubernetes:master Jun 18, 2025
4 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.34 milestone Jun 18, 2025
Barakmor1 added a commit to Barakmor1/kubevirt that referenced this pull request Jun 22, 2025
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/node Categorizes an issue or PR as relevant to SIG Node. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants