Skip to content

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.

Details

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.

@abel-von abel-von force-pushed the make-shim-independent branch from c5b85d5 to bd6caa5 Compare February 22, 2024 09:01
@mxpv
Copy link
Member

mxpv commented Feb 22, 2024

/ok-to-test

@abel-von abel-von force-pushed the make-shim-independent branch from bd6caa5 to a89fe4d Compare February 23, 2024 02:41
@abel-von abel-von changed the title sandbox: make a independent shim plugin sandbox: make an independent shim plugin Feb 23, 2024
@abel-von abel-von force-pushed the make-shim-independent branch from a89fe4d to 5439913 Compare February 23, 2024 03:14
@fuweid fuweid self-requested a review February 25, 2024 09:11
@abel-von abel-von force-pushed the make-shim-independent branch from 5439913 to 960a06b Compare March 13, 2024 03:55
@abel-von
Copy link
Contributor Author

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

@abel-von
Copy link
Contributor Author

/cc @cpuguy83 @Iceber

@k8s-ci-robot k8s-ci-robot requested review from Iceber and cpuguy83 April 16, 2024 06:30
// ID of the shim manager
func (m *ShimManager) ID() string {
return plugins.RuntimePluginV2.String() + ".shim"
}
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 abel-von force-pushed the make-shim-independent branch from 960a06b to ad0f706 Compare April 19, 2024 08:37
@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.

Signed-off-by: Abel Feng <fshb1988@gmail.com>
@abel-von abel-von force-pushed the make-shim-independent branch from ad0f706 to a12aced Compare April 24, 2024 06:27
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
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.

6 participants