-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Split CRI image service from GRPC handler #9152
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
Conversation
|
Skipping CI for Draft Pull Request. |
fd5aae6 to
514a072
Compare
|
/ok-to-test |
2beef9d to
a8cceef
Compare
|
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. |
0b6b463 to
0ae618a
Compare
c7ab5ff to
0c32a4d
Compare
| // 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) { |
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.
We can change that name to initLocalCRIImageService.
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.
Updated, need to rebase anyway
| func init() { | ||
| config := criconfig.DefaultImageConfig() | ||
|
|
||
| registry.Register(&plugin.Registration{ |
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.
I think we should export config from image service because crictl info can dump CRI setting.
In this pull request, we lost this part.
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.
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.
fuweid
left a comment
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
The comments can be handled in that follow-up.
9be6d7d to
b4887b6
Compare
b4887b6 to
747fcc0
Compare
|
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>
747fcc0 to
5a3d3f6
Compare
|
Rebased again with latest go version. |
fuweid
left a comment
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
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.