Skip to content

Conversation

@dmcgowan
Copy link
Member

@dmcgowan dmcgowan commented Sep 26, 2023

Completely removes dependency of CRI image service on other CRI plugins. The CRI image plugin does not import or load any other CRI plugin and the main CRI plugin does not import the images package.

Separates CRI image config and adds migration.

Adds runtime platforms for resolving images based on runtime name by platform and snapshotter. This configuration and logic is now completely in the image service. Other CRI plugins can call the image service to get the snapshotter info.

Updates the pinned image logic to use a key pair where the key is used to lookup a specific pinned image, such as "sandbox".

Related to removing client dependency from internal plugins and preparing for CRI to use transfer service.

@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 dmcgowan force-pushed the cri-split-image-service branch 11 times, most recently from fd5aae6 to 514a072 Compare September 27, 2023 13:59
@mikebrow
Copy link
Member

/ok-to-test

@dmcgowan dmcgowan force-pushed the cri-split-image-service branch 2 times, most recently from 2beef9d to a8cceef Compare October 11, 2023 22:50
@dmcgowan dmcgowan changed the title WIP: Split CRI image service from GRPC handler Split CRI image service from GRPC handler Oct 11, 2023
@dmcgowan
Copy link
Member Author

We can move forward with this one. There are multiple changes as part of cleaning up the CRI dependencies, it will be easier to get them done in pieces. This should unblock the work to split up into multiple plugins as well as more of the migration off the client.

@dmcgowan dmcgowan marked this pull request as ready for review October 11, 2023 23:34
@dmcgowan dmcgowan added this to the 2.0 milestone Oct 11, 2023
@dmcgowan dmcgowan force-pushed the cri-split-image-service branch 4 times, most recently from 0b6b463 to 0ae618a Compare October 14, 2023 06:34
@dmcgowan dmcgowan force-pushed the cri-split-image-service branch 4 times, most recently from c7ab5ff to 0c32a4d Compare October 25, 2023 03:47
// NOTE: We don't need to start the CRI plugin here because we just need the
// ImageService API.
func initLocalCRIPlugin(client *containerd.Client, tmpDir string, registryCfg criconfig.Registry) (criserver.CRIService, error) {
func initLocalCRIPlugin(client *containerd.Client, tmpDir string, registryCfg criconfig.Registry) (criserver.ImageService, 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 can change that name to initLocalCRIImageService.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated, need to rebase anyway

func init() {
config := criconfig.DefaultImageConfig()

registry.Register(&plugin.Registration{
Copy link
Member

Choose a reason for hiding this comment

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

I think we should export config from image service because crictl info can dump CRI setting.
In this pull request, we lost this part.

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 could add another key to the info, agreed it could be in a follow up. It is not well defined what should actually be considered the config, since the configuration of other plugins may have related behavior. Can create a follow up issue for this.

Copy link
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM

The comments can be handled in that follow-up.

@dmcgowan
Copy link
Member Author

dmcgowan commented Jan 9, 2024

Rebased

Prepares the CRI image service for splitting CRI into multiple plugins.
Also prepares for config migration which will spread across multiple
different plugins.

Signed-off-by: Derek McGowan <derek@mcg.dev>
Signed-off-by: Derek McGowan <derek@mcg.dev>
Avoid requiring the whole image store interface for
the image store cache which only needs Get.

Signed-off-by: Derek McGowan <derek@mcg.dev>
Updates the CRI image service to own image related configuration and
separate it from the runtime configuration.

Signed-off-by: Derek McGowan <derek@mcg.dev>
This change simplifies the CRI plugin dependencies by not requiring the
CRI image plugin to depend on any other CRI components. Since other CRI
plugins depend on the image plugin, this allows prevents a dependency
cycle for CRI configurations on a base plugin.

Signed-off-by: Derek McGowan <derek@mcg.dev>
Avoids importing image service for utility function.

Signed-off-by: Derek McGowan <derek@mcg.dev>
Signed-off-by: Derek McGowan <derek@mcg.dev>
Signed-off-by: Derek McGowan <derek@mcg.dev>
@dmcgowan dmcgowan force-pushed the cri-split-image-service branch from 747fcc0 to 5a3d3f6 Compare January 11, 2024 18:03
@dmcgowan
Copy link
Member Author

Rebased again with latest go version.

Copy link
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants