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 CRI Service plugin type #9681

Merged
merged 5 commits into from
Jan 29, 2024
Merged

Conversation

dmcgowan
Copy link
Member

Now that the CRI image configuration is split out from the runtime configuration, there is no need to enforce plugin order between the images and runtime. Without the need for order, they can be more cleanly organized as a runtime and images plugins both of type CRI Image Service. This is a cleaner alternative to the CRI base plugin, which (mis)used the internal plugin type only to hold configuration.

@k8s-ci-robot
Copy link

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@dmcgowan
Copy link
Member Author

/test all

@dmcgowan dmcgowan force-pushed the cri-runtime-plugin branch 3 times, most recently from bc20433 to 1363b12 Compare January 24, 2024 16:51
@dmcgowan
Copy link
Member Author

/test all

@dmcgowan dmcgowan marked this pull request as ready for review January 24, 2024 21:31
@dmcgowan dmcgowan added this to the 2.0 milestone Jan 25, 2024
@@ -25,13 +26,13 @@ import (
"github.com/containerd/plugin/registry"

containerd "github.com/containerd/containerd/v2/client"
srvconfig "github.com/containerd/containerd/v2/cmd/containerd/server/config"
Copy link
Member

Choose a reason for hiding this comment

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

this felt a tad wrong ... importing from cmd/containerd into here. but we have done this elsewhere so it's ok.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can probably safely move that config package out from there. It got moved as a subpackage but considering it is our main configuration object, it could find a better home.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll move the configuration version out into the version package in a follow up. That's only thing I saw being imported from config

Copy link
Member

Choose a reason for hiding this comment

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

thanks!

Copy link
Member

@dims dims left a comment

Choose a reason for hiding this comment

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

LGTM

pkg/cri/server/podsandbox/controller.go Outdated Show resolved Hide resolved
pkg/cri/server/service.go Outdated Show resolved Hide resolved
pkg/cri/server/service.go Show resolved Hide resolved
plugins/cri/runtime/plugin.go Outdated Show resolved Hide resolved
@dmcgowan
Copy link
Member Author

Thanks @henry118 , updated

Copy link
Member

@dims dims left a comment

Choose a reason for hiding this comment

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

LGTM

pkg/cri/config/config.go Show resolved Hide resolved
pkg/cri/config/config.go Outdated Show resolved Hide resolved
pkg/cri/config/config.go Outdated Show resolved Hide resolved
plugins/cri/cri.go Outdated Show resolved Hide resolved
plugins/types.go Outdated Show resolved Hide resolved
Create new plugin type for CRI runtime and image services.

Signed-off-by: Derek McGowan <derek@mcg.dev>
Signed-off-by: Derek McGowan <derek@mcg.dev>
Signed-off-by: Derek McGowan <derek@mcg.dev>
pkg/cri/config/streaming.go Fixed Show resolved Hide resolved
@dmcgowan dmcgowan force-pushed the cri-runtime-plugin branch 2 times, most recently from c8c0794 to 5816c89 Compare January 29, 2024 06:47
Signed-off-by: Derek McGowan <derek@mcg.dev>
Signed-off-by: Derek McGowan <derek@mcg.dev>
Copy link
Member

@akhilerm akhilerm left a comment

Choose a reason for hiding this comment

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

LGTM

@dmcgowan dmcgowan added this pull request to the merge queue Jan 29, 2024
Merged via the queue into containerd:main with commit 1b6019b Jan 29, 2024
46 checks passed
@dmcgowan dmcgowan deleted the cri-runtime-plugin branch January 30, 2024 17:17
@AkihiroSuda
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants