OCPNODE-2929: proposal of OCI image volume #1792
Conversation
|
/cc mrunalp |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: saschagrunert The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@mrunalp @haircommander PTAL for approval. |
bitoku
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@bitoku: This pull request references OCPNODE-2929 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.20.0" version, but no target version was set. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
/lgtm |
|
|
||
| ### Implementation Details/Notes/Constraints | ||
|
|
||
| To separate graduation of OCI image and OCI artifact, we are going to add a new option to disable OCI artifact in CRI-O and disable OCI artifact mount by default via MCO. |
There was a problem hiding this comment.
Would this not be done via an API change?
There was a problem hiding this comment.
why expose an API we don't expect a user to toggle? Either we're comfortable users using it OOTB or it's off IMO
There was a problem hiding this comment.
So all the APIs to enable/disable for MCO/Feature gates are present?
There was a problem hiding this comment.
The feature gate will be enabled by default for OCP.
But because we disable OCI artifacts in MCO cri-o config, the OCP users can't use OCI artifacts, and we're not going to give the users a way to toggle OCI artifact. The only way to enable it is to change cri-o config directly until it's available.
When we think we're ready for GA (or TP?), we're gonna enable it by changing the crio config template.
|
@kannon92 @haircommander I updated the doc. PTAL |
|
|
||
| - This proposal does not aim to replace existing VolumeSource types. | ||
| - This proposal does not address other use cases for OCI objects beyond directory sharing among containers in a pod. | ||
| - Mounting thousands of images in a single pod. |
There was a problem hiding this comment.
I kinda find it odd this is here tbh.are hundreds supported? seems odd to set such an arbitrarily high limit with no testing of those limits
There was a problem hiding this comment.
I might have copied from the original KEP. https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/4639-oci-volume-source#non-goals
Do we want to just delete this item or completely replace this section to the link to the upstream KEP?
| 2. Separate Graduation Approach between OCI Images and OCI Artifacts. | ||
|
|
||
| - **OCI Image Mount**: Promote to GA | ||
| - **OCI Artifact Mount**: Begin as DevPreview |
There was a problem hiding this comment.
This is a confusing to me..
Are we including the DevPreview of OCI Artifact Mount in this KEP?
Or are we aiming for this enhancement to me ImageVolume"
There was a problem hiding this comment.
Yes it's including OCI artifacts. The name of this enhancement is the same as the original KEP, which also includes OCI images and OCI artifacts.
There's not many differences feature-wise, I'd like to include OCI artfiacts rather than creating a new one for them, but it's true it's a bit confusing.
There was a problem hiding this comment.
I’m not sure if this is a big deal but I see that this KEP will have two different feature gates that will progress differently.
ImageVolume will be used to toggle the upstream feature gate so we can GA earlier. I think this gate could be dropped once ImageVolume is turned on by default.
For OCI artifacts we are starting with dev preview with no feature gates but eventually we would have a separate feature gate. I don’t know when we can promote this to GA?
writing this out here because I think we need to cover this in this KEP if you want to combine these two features but graduate at different times.
There was a problem hiding this comment.
there aren't gates that are being introduced here. there's one upstream featuregate enabling oci volume source, and then there's a cri-o toggle that says whether we'll allow artifacts or not. "dev preview" for artifacts is a support stance and talks about enablement. We on the node team rarely codify a dev preview with an API field, and we don't plan on doing so here
There was a problem hiding this comment.
The graduation process is going to be what @haircommander described.
But thank you for pointing it out @kannon92. That point should be explained in this section. I'll add the explanation.
There was a problem hiding this comment.
@bitoku thank you for your updates. This is much clearer.
"dev preview" for artifacts is a support stance and talks about enablement. We on the node team rarely codify a dev preview with an API field, and we don't plan on doing so here
My main point is do we want to call out graduation criteria for OCI artifacts in this KEP? Or are we going to draft a new KEP or update this one once we eventually want to take OCI artifacts out of dev preview?
There was a problem hiding this comment.
artifacts are out of scope for KEPs, do you mean openshift enhancement? I think we can append this one with graduation criteria when it's more clear, we're still defining what artifact support looks generally
There was a problem hiding this comment.
I think we could update the criteria if needed after this PR's merged, and if we think we should open a new draft when we graduate it to TP or GA, we can think about it at that time.
|
|
||
| #### OCI Image | ||
|
|
||
| Since OCI image mount will be GA in 4.20. No feature gate configuration is needed by users. Creating a pod with volumes.image will pull the image and mount in the container. |
There was a problem hiding this comment.
Given that this is a pod API and both windows / linux users could use this API. We should comment on windows containers here and what kind of UX we would have for this feature for windows.
I think it is okay if windows does not support this if containerd does not but we should still call it out.
There was a problem hiding this comment.
Apparently windows machines use containerd. OCI image mount might be supported, but I'll make it clear it's out-of-scope.
bitoku
force-pushed
the
oci-image-volume
branch
2 times, most recently
from
1a7f82b to
5d245a4
Compare
|
|
||
| #### OCI Artifact | ||
|
|
||
| While OCI artifact is in dev preview, it fails to mount if a user specifies OCI artifact in reference. To enable the mount, users need to add this drop-in CRI-O configuration. |
There was a problem hiding this comment.
I think noting both this is done via MachineConfig and that it makes their cluster unsupported would be good things to mention
| The mitigation is: | ||
|
|
||
| - CRI API is easier to change if needed since it's internal to the implementation | ||
| - [The Kubernetes API for this feature](https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/4639-oci-volume-source#kubernetes-api) is stable and unlikely to change as it's already in beta |
There was a problem hiding this comment.
nit: as it's already in beta and is built on a very stable volume API?
Signed-off-by: Ayato Tokubi <atokubi@redhat.com>
|
@bitoku: all tests passed! Full PR test history. Your PR dashboard. 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. |
|
@haircommander PTAL |
|
/lgtm |
|
|
||
| #### OCI Artifact | ||
|
|
||
| While OCI artifact is in dev preview, it fails to mount if a user specifies OCI artifact in reference. |
There was a problem hiding this comment.
Can we describe how OCI Artifact support will work? I am looking for details around how they are mounted, if they will be GC'ed by kubelet, and whether we set names from annotations?
/assign haircommander
/assign saschagrunert
This is a proposal for how to integrate OCI image volume into OpenShift.