-
Notifications
You must be signed in to change notification settings - Fork 65
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
crd: Add and use RuntimeClasses struct to associate a runtime to a snapshotter #250
crd: Add and use RuntimeClasses struct to associate a runtime to a snapshotter #250
Conversation
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.
LGTM, thanks @fidencio !
@bpradipt, @jensfr, we never ever discussed the possibility of having "experimental features", or things that we know they'll end up being dropped / rewritten in the long term. The approach I've decided to take is just naming it as ExperimentalFeature${FOO}, but I'd like to hear from you and understand what would be the best approach, considering your experience, to deal with such cases. |
We never really discussed how to handle experimental features. For this specific case, since there could be different snapshotter configuration for a specific RuntimeClass, I was thinking should we change the RuntimeClassNames to a struct and add specifics like name, snapshotter config and any other required configs ? operator/api/v1beta1/ccruntime_types.go Line 163 in cce4aac
|
RuntimeClassNames has been added as part of the commit e0ab5ae, but right now we need to actually map a runtime with the snapshotter it'll be using, forcing us to drop the RuntimeClassNames string from the CRD, and add a struct which will hold the Name and the Snapshotter to be used. This suggestion came from Pradipta Banerjee, and I appreciate a lot the way of handling this, and I'm assuming here changes on the CRD are not too problematic, as long as those are not done too often, while we're in the v1beta1 stage. Signed-off-by: Fabiano Fidêncio <[email protected]>
cce4aac
to
5262e4a
Compare
I still have to learn how to properly patch a struct, but that's for Tomorrow. @bpradipt, thanks a ton for the suggestion, and I hope breaking backwards compat in v1beta1 is not a huge deal. |
5262e4a
to
81b7b77
Compare
81b7b77
to
82c9936
Compare
/test |
82c9936
to
240f8e9
Compare
/test |
Yeah, till we release v1, we can be flexible |
For now, let's use "overlayfs", which is the default containerd snapshotter, and exactly what's been used till now as the snapshotter for the runtimeclasses. Later on, this will be adapted to using nydus / tardev. Signed-off-by: Fabiano Fidêncio <[email protected]>
240f8e9
to
6c39d1c
Compare
/test |
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.
/lgtm
Thanks @fidencio
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.
LGTM. Thanks!
This is the Operator counter part of the following Kata Containers PR kata-containers/kata-containers#7715
The idea here is to expose a way that users can set a snapshotter to be used with the containerd runtime handler.
The reason I'm marking this feature, right now, as "ExperimentalFeature" is a way to indicate this will be changed, or even dropped, in the coming releases, as Kata Containers will actually expose a way to map a specific runtime handler to a specific snapshotter, rather than "set this snapshotter for all the runtime handlers" as currently done.
This will require folks using a new enough version of containerd (v1.7+), but I do believe this will be the case anyways with the work going on as part of removing the dependency on the containerd fork.