Skip to content
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

Add support for containerd v3 configs #805

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

alam0rt
Copy link

@alam0rt alam0rt commented Nov 18, 2024

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.

@alam0rt alam0rt force-pushed the add-v3-containerd-config branch from 91269ac to 96934b2 Compare November 18, 2024 06:00
)

// ConfigV3 represents a version 3 containerd config
type ConfigV3 Config
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is technically the defaultVersion since the version in the config file overwrites this.

@elezar elezar force-pushed the add-v3-containerd-config branch from 96934b2 to c11d433 Compare November 29, 2024 14:41
@elezar elezar changed the title Work in progress: Add v3 containerd config Add support for containerd v3 configs Nov 29, 2024
@elezar
Copy link
Member

elezar commented Nov 29, 2024

@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.

@elezar elezar self-assigned this Nov 29, 2024
@elezar
Copy link
Member

elezar commented Nov 29, 2024

@cdesiniotis @tariq1890 this is something that will be required for the next GPU Operator release and we could consider backporting this too.

@elezar elezar marked this pull request as ready for review November 29, 2024 14:45
elezar and others added 3 commits November 29, 2024 16:54
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 elezar force-pushed the add-v3-containerd-config branch from c11d433 to 5d073fb Compare November 29, 2024 16:13
@alam0rt
Copy link
Author

alam0rt commented Nov 29, 2024

@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.

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]>
@cdesiniotis
Copy link
Contributor

It looks like io.containerd.grpc.v1.cri was renamed to io.containerd.cri.v1.runtime in the v3 configuration file. I was able to get things to work with this commit: cdesiniotis@16b5376.

@cdesiniotis
Copy link
Contributor

@alam0rt I have gone ahead and updated this PR with my commit.

@elezar @tariq1890 please take a look.

@cdesiniotis cdesiniotis requested a review from elezar December 19, 2024 06:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants