Support PID NamespaceMode_TARGET by tghartland · Pull Request #5203 · containerd/containerd

tghartland · 2021-03-15T11:52:40Z

As mentioned in the issue, support for the namespace mode TARGET is needed for debugging with kubernetes ephemeral containers. Kubernetes is already passing a TargetID, so containerd just needs to set up the new container with the existing PID namespace of the target container.

I'll need some guidance on these things:

Add unit tests.
Enforce that the new and target containers exist in the same pod?
Confirm the behaviour when the target container is not running (error?).
Anything else I might have missed.

And @verb I'd like you to take a look to ensure that this implementation matches what is expected by kubernetes.

k8s-ci-robot · 2021-03-15T11:52:49Z

Hi @tghartland. 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.

theopenlab-ci · 2021-03-15T12:00:07Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 5m 48s (non-voting)

theopenlab-ci · 2021-03-16T13:48:41Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 5m 50s (non-voting)

theopenlab-ci · 2021-03-16T14:51:34Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 5m 46s (non-voting)

tghartland · 2021-03-25T16:31:54Z

@AkihiroSuda or @estesp, can either of you review this when you have time?

verb · 2021-04-13T13:29:13Z

@tghartland This looks to me like the behavior Kubernetes expects. Thanks for implementing this!

dims · 2021-04-18T20:27:27Z

cc @mikebrow

@dmcgowan could we get this into 1.5.0 please? will help with kubectl debug with --target scenario ( see kubernetes/kubernetes#101074 )

dims · 2021-04-18T20:27:38Z

/ok-to-test

dims · 2021-04-18T20:29:50Z

@mikebrow @dmcgowan please feel free to move the milestone if 1.5.0 is not appropriate.

mikebrow

See comments.. Cheers!

mikebrow · 2021-04-20T19:41:07Z

Oh hey, I see this is your first contribution. Welcome aboard!

fuweid · 2021-04-21T04:50:31Z

Confirm the behaviour when the target container is not running (error?)

Would you mind to add the checker before start like what normal container does? I am worry about pid resue here :p

The checker doesn't prevent anything but it can lower the possibility.

REF: https://github.com/containerd/containerd/blob/master/pkg/cri/server/container_start.go#L83

theopenlab-ci · 2021-04-21T08:30:43Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 5m 57s (non-voting)

mikebrow · 2021-04-21T15:04:09Z

Confirm the behaviour when the target container is not running (error?)

Would you mind to add the checker before start like what normal container does? I am worry about pid resue here :p
The checker doesn't prevent anything but it can lower the possibility.
REF: https://github.com/containerd/containerd/blob/master/pkg/cri/server/container_start.go#L83

And keep the same check where it is now?
Or should it be valid to create a container, then create a container targeting that container, and finally start both containers? (As long as the target container is started first).

After a quick check of the code for the feature... I see that it needs to be checked/verified at least on create. And I agree with @fuweid that it should also be checked/verified before doing the start for the created debug container.

tghartland · 2021-04-21T15:07:56Z

Pushed changes:

Moved the validation checks for the target container into a function in pkg/cri/server/helpers.go
- Check that the target container exists, that it is in the same sandbox as the new container, and that it is running.
Check the target validity when creating the container, and recheck when starting the container.
Moved the unit test to helpers_test.go and adapted it slightly to test the validation function.

theopenlab-ci · 2021-04-21T15:09:33Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 5m 48s (non-voting)

theopenlab-ci · 2021-04-21T15:27:08Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 5m 53s (non-voting)

theopenlab-ci · 2021-04-21T15:44:18Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 5m 54s (non-voting)

This commit adds support for the PID namespace mode TARGET when generating a container spec. The container that is created will be sharing its PID namespace with the target container that was specified by ID in the namespace options. Signed-off-by: Thomas Hartland <[email protected]>

theopenlab-ci · 2021-04-21T16:02:55Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 5m 49s (non-voting)

mikebrow

LGTM

mikebrow · 2021-04-21T16:13:11Z

I didn't see any label or annotation marking ephemeral container in a pod. Maybe I miss something. If the caller (kubelet) can add label on container, it will be helpful.

+1 for the caller identifying the container.. can just see some admin opening an issue about a random container left running in their pod :-)

verb · 2021-04-21T16:58:21Z

I didn't see any label or annotation marking ephemeral container in a pod. Maybe I miss something. If the caller (kubelet) can add label on container, it will be helpful.

+1 for the caller identifying the container.. can just see some admin opening an issue about a random container left running in their pod :-)

I set a label in an earlier implementation, but it wasn't necessary for the kubelet so I dropped it. Happy to add it back if it would be helpful.

Not sure if someone here intends to implement the change in the kubelet, but if not it would make a good new contributor issue. If you open an enhancements tracking issue and tag me I'll guide it.

theopenlab-ci · 2021-04-21T17:52:03Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 5m 53s (non-voting)

Signed-off-by: Thomas Hartland <[email protected]>

theopenlab-ci · 2021-04-21T18:07:16Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 5m 56s (non-voting)

mikebrow · 2021-04-21T18:59:08Z

@dims good to merge?

dims

LGTM

tghartland · 2021-04-22T08:07:58Z

Great, thanks for the reviews.

Is it still possible for this to be in 1.5.0 or does it need to wait for a later release now?

fuweid · 2021-04-22T08:28:38Z

@tghartland this patch is already in the release 1.5 and just wait for release. Thanks for contribution!

k8s-ci-robot added the needs-ok-to-test label Mar 15, 2021

tghartland force-pushed the 5008-target-namespace branch from ea9a6b8 to 95beac7 Compare March 16, 2021 13:41

tghartland changed the title ~~WIP: Support PID NamespaceMode_TARGET~~ Support PID NamespaceMode_TARGET Mar 25, 2021

verb mentioned this pull request Apr 13, 2021

Cannot enter the process namespace using kubectl alpha debug with target parameter kubernetes/kubernetes#98362

Closed

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Apr 18, 2021

dims assigned Random-Liu and mikebrow Apr 18, 2021

dims requested review from Random-Liu and mxpv and removed request for Random-Liu April 18, 2021 20:28

dims added this to the 1.5 milestone Apr 18, 2021

mxpv approved these changes Apr 18, 2021

View reviewed changes

AkihiroSuda requested a review from mikebrow April 20, 2021 16:03

AkihiroSuda approved these changes Apr 20, 2021

View reviewed changes

AkihiroSuda requested a review from dmcgowan April 20, 2021 16:04

mikebrow reviewed Apr 20, 2021

View reviewed changes

Comment thread pkg/cri/server/container_create_linux.go Outdated

tghartland force-pushed the 5008-target-namespace branch from f4668bd to 6880528 Compare April 21, 2021 08:22

tghartland force-pushed the 5008-target-namespace branch from 6880528 to 101d1bf Compare April 21, 2021 15:02

dims reviewed Apr 21, 2021

View reviewed changes

Comment thread pkg/cri/server/helpers.go Outdated

dims reviewed Apr 21, 2021

View reviewed changes

Comment thread pkg/cri/server/helpers.go Outdated

dims reviewed Apr 21, 2021

View reviewed changes

Comment thread pkg/cri/server/helpers.go Outdated

tghartland force-pushed the 5008-target-namespace branch from 101d1bf to 2fc5a50 Compare April 21, 2021 15:19

tghartland force-pushed the 5008-target-namespace branch from 2fc5a50 to f5d43ad Compare April 21, 2021 15:36

tghartland force-pushed the 5008-target-namespace branch from f5d43ad to 7f3f632 Compare April 21, 2021 15:55

mikebrow approved these changes Apr 21, 2021

View reviewed changes

tghartland force-pushed the 5008-target-namespace branch from 7f3f632 to d63e9c6 Compare April 21, 2021 17:44

Add unit tests for PID NamespaceMode_TARGET validation

efcb187

Signed-off-by: Thomas Hartland <[email protected]>

tghartland force-pushed the 5008-target-namespace branch from d63e9c6 to efcb187 Compare April 21, 2021 17:59

dims approved these changes Apr 21, 2021

View reviewed changes

mikebrow merged commit 3dad67e into containerd:master Apr 22, 2021

fuweid mentioned this pull request Apr 23, 2021

Prepare v1.5.0-rc.3 release notes #5400

Merged

ejose19 mentioned this pull request May 31, 2021

--target doesn't work for kubectl debug k3s-io/k3s#3381

Closed

DawnGuoDev mentioned this pull request Aug 18, 2021

Support for ephemeral containers .. NamespaceMode=Target containerd/cri#1554

Closed

Conversation

tghartland commented Mar 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Mar 15, 2021

Uh oh!

theopenlab-ci Bot commented Mar 15, 2021

Uh oh!

theopenlab-ci Bot commented Mar 16, 2021

Uh oh!

theopenlab-ci Bot commented Mar 16, 2021

Uh oh!

tghartland commented Mar 25, 2021

Uh oh!

verb commented Apr 13, 2021

Uh oh!

dims commented Apr 18, 2021

Uh oh!

dims commented Apr 18, 2021

Uh oh!

dims commented Apr 18, 2021

Uh oh!

mikebrow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mikebrow commented Apr 20, 2021

Uh oh!

fuweid commented Apr 21, 2021

Uh oh!

theopenlab-ci Bot commented Apr 21, 2021

Uh oh!

mikebrow commented Apr 21, 2021

Uh oh!

tghartland commented Apr 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theopenlab-ci Bot commented Apr 21, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

theopenlab-ci Bot commented Apr 21, 2021

Uh oh!

theopenlab-ci Bot commented Apr 21, 2021

Uh oh!

theopenlab-ci Bot commented Apr 21, 2021

Uh oh!

mikebrow left a comment

Choose a reason for hiding this comment

Uh oh!

mikebrow commented Apr 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

verb commented Apr 21, 2021

Uh oh!

theopenlab-ci Bot commented Apr 21, 2021

Uh oh!

theopenlab-ci Bot commented Apr 21, 2021

Uh oh!

mikebrow commented Apr 21, 2021

Uh oh!

dims left a comment

Choose a reason for hiding this comment

Uh oh!

tghartland commented Apr 22, 2021

Uh oh!

fuweid commented Apr 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

tghartland commented Mar 15, 2021 •

edited

Loading

tghartland commented Apr 21, 2021 •

edited

Loading

mikebrow commented Apr 21, 2021 •

edited

Loading