Add support for containerd v3 configs #805
Conversation
alam0rt
force-pushed
the
add-v3-containerd-config
branch
from
91269ac to
96934b2
Compare
| ) | ||
|
|
||
| // ConfigV3 represents a version 3 containerd config | ||
| type ConfigV3 Config |
There was a problem hiding this comment.
If the v3 config is the same as the v2 config then we should not duplicate the logic for the functions here.
Are there any differences that are relevant to the runtime sections?
| @@ -89,14 +91,16 @@ func New(opts ...Option) (engine.Interface, error) { | |||
| return (*ConfigV1)(cfg), nil | |||
| case 2: | |||
| return cfg, nil | |||
| case 3: | |||
| return (*ConfigV3)(cfg), nil | |||
| } | |||
|
|
|||
| return nil, fmt.Errorf("unsupported config version: %v", version) | |||
| } | |||
|
|
|||
| // parseVersion returns the version of the config | |||
| func (c *Config) parseVersion(useLegacyConfig bool) (int, error) { | |||
There was a problem hiding this comment.
We've essentially replaced the useLegacyConfig variable with the configVersion option. Would it make sense to update the implementation here to something like:
defaultVersion := 3
if c.configVersion != 0 {
defaultVersion = c.configVersion
}
and drop the useLegacyConfig argument?
| @@ -30,7 +30,7 @@ type builder struct { | |||
| configSource toml.Loader | |||
| path string | |||
| runtimeType string | |||
| useLegacyConfig bool | |||
| configVersion int64 | |||
There was a problem hiding this comment.
This is technically the defaultVersion since the version in the config file overwrites this.
elezar
force-pushed
the
add-v3-containerd-config
branch
from
96934b2 to
c11d433
Compare
elezar
changed the title
|
@alam0rt I have taken these changes and reworked them. I hope you don't have an issue with me rebasing your commits to get them a little more streamlined. Let me know what you think. |
|
@cdesiniotis @tariq1890 this is something that will be required for the next GPU Operator release and we could consider backporting this too. |
Signed-off-by: Evan Lezar <[email protected]>
Signed-off-by: Evan Lezar <[email protected]>
This change adds support for containerd configs with version=3. From the perspective of the runtime configuration the contents of the config are the same. This means that we just have to load the new version and ensure that this is propagated to the generated config. Note that we still use a default config of version=2 since we need to ensure compatibility with older containerd versions (1.6.x and 1.7.x). Signed-off-by: Sam Lockart <[email protected]> Signed-off-by: Evan Lezar <[email protected]>
elezar
force-pushed
the
add-v3-containerd-config
branch
from
c11d433 to
5d073fb
Compare
I absolutely do not mind. Thanks for taking a look! Have been a bit inundated so I haven't had a chance to rework it myself |
…cri' in v3 containerd configs The fully qualified name of the containerd plugin for the CRI runtime service was changed to io.containerd.cri.v1.runtime in the v3 configuration. References: https://github.com/containerd/containerd/blob/v2.0.0/docs/PLUGINS.md containerd/containerd#10132 Signed-off-by: Christopher Desiniotis <[email protected]>
|
It looks like |
|
@alam0rt I have gone ahead and updated this PR with my commit. @elezar @tariq1890 please take a look. |
| config.SetPath([]string{"plugins", "cri", "containerd", "default_runtime", "runtime_engine"}, "") | ||
| config.SetPath([]string{"plugins", "cri", "containerd", "default_runtime", "privileged_without_host_devices"}, false) | ||
| if setAsDefault { | ||
| if !c.UseLegacyConfig { |
There was a problem hiding this comment.
Can we invert the if-else block here? It would look more readable if we avoid the NOT statement when possible
| RemoveRuntime(string) error | ||
| Save(string) (int64, error) | ||
| GetRuntimeConfig(string) (RuntimeConfig, error) | ||
| Set(string, interface{}) | ||
| String() string |
There was a problem hiding this comment.
I see that we are only using this in unit tests, do we really need to add a new interface method for this?
With the release of containerd 2.0 comes a new default config version. This PR attempts to support the new version 3 of the containerd configuration.
Version 3 is fairly similar to 2 and for all intents and purposes, a valid version 3 config is a valid version 2 config (bar the version of course). So for now I've just gone and copied the V2 logic and changed things here and there.