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

sandbox: make an independent shim plugin #9853

Merged
merged 1 commit into from
Apr 29, 2024

Conversation

abel-von
Copy link
Contributor

Currently, The Shim and Task plugin is mixed into a core/runtime/v2/manager.go, In this PR, we split the Shim plugin to make it an independent plugin.

This is the second PR of the sandboxed Task demonstrated in draft #9574

@k8s-ci-robot
Copy link

Hi @abel-von. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@mxpv
Copy link
Member

mxpv commented Feb 22, 2024

/ok-to-test

core/runtime/v2/manager.go Outdated Show resolved Hide resolved
plugins/sandbox/controller.go Outdated Show resolved Hide resolved
@abel-von
Copy link
Contributor Author

Could you please take a look, again? @mxpv @fuweid

@abel-von
Copy link
Contributor Author

/cc @cpuguy83 @Iceber

@@ -186,17 +158,7 @@ func (m *ShimManager) ID() string {
}
Copy link
Member

Choose a reason for hiding this comment

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

ID should return plugins.ShimPlugin.String() + .shim?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

func init() {
registry.Register(&plugin.Registration{
Type: plugins.RuntimePluginV2,
ID: "task",
Copy link
Member

Choose a reason for hiding this comment

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

The shim plugin will migrate for this type in shim_manager.go. Is it conflicted?

const originalPluginName = string(plugins.RuntimePluginV2) + ".task"
			original, ok := pluginConfigs[originalPluginName]
			if !ok {
				return nil
			}
			const newPluginName = string(plugins.ShimPlugin) + ".shim"
			pluginConfigs[originalPluginName] = nil
			pluginConfigs[newPluginName] = original
			return nil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do migration only for the config file of configVersion < 3, and the in the newest config file, shim configs should be defined under io.containerd.shim.v1.shim and we won't migrate them from io.containerd.runtime.v2.task anymore. some comments are added to illustrate that.

@abel-von
Copy link
Contributor Author

/retest

1 similar comment
@abel-von
Copy link
Contributor Author

/retest

@Burning1020
Copy link
Member

Shell we add this in 2.0 milestone list? @mxpv @fuweid

@fuweid fuweid added this to the 2.0 milestone Apr 20, 2024
@abel-von
Copy link
Contributor Author

/retest

Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

LGTM just minor naming / code doc nits..
The naming of parameters alternates from state/root with no description ... to stateDir/rootDir and then is described and used as a path name with join/extension for specific files. Maybe some normalization to the form stateDir and a couple lines of doc.

core/runtime/v2/task_manager.go Show resolved Hide resolved
core/runtime/v2/shim_load.go Show resolved Hide resolved
Signed-off-by: Abel Feng <fshb1988@gmail.com>
Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

LGTM

@mikebrow
Copy link
Member

/test pull-containerd-node-e2e

1 similar comment
@abel-von
Copy link
Contributor Author

/test pull-containerd-node-e2e

@mxpv mxpv added this pull request to the merge queue Apr 29, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 29, 2024
@mxpv mxpv added this pull request to the merge queue Apr 29, 2024
Merged via the queue into containerd:main with commit 7feb1f3 Apr 29, 2024
47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants