[port #5904 to sbserver] Setup pod network after creating the sandbox container by samuelkarp · Pull Request #7426 · containerd/containerd

samuelkarp · 2022-09-23T22:50:09Z

This pull request ports #5904 and #7481 to the sbserver implementation in the CRI plugin. Since sbserver is still incomplete, this PR is currently working around the incomplete parts. However, it does pass all the integration tests and is working.

I'd like to get this merged as we continue to work on the Sandbox API since the integration tests enabled by this PR will help catch behavior regressions.

(cc @qiutongs)

k8s-ci-robot · 2022-09-23T22:50:11Z

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

samuelkarp · 2022-09-23T22:55:42Z

/test all
/test pull-containerd-sandboxed-node-e2e

samuelkarp · 2022-09-25T07:07:54Z

/test all
/test pull-containerd-sandboxed-node-e2e

samuelkarp · 2022-09-26T22:52:34Z

samuelkarp force-pushed the port-pr-5904-to-sbserver branch from 1ddd92b to 4367e31

Updated per review comments from @qiutongs.

samuelkarp · 2022-09-29T21:06:42Z

samuelkarp force-pushed the port-pr-5904-to-sbserver branch from 4367e31 to 99e08fe

Rebased on #5904

samuelkarp · 2022-09-30T01:17:16Z

/test pull-containerd-sandboxed-node-e2e

sbuckfelder · 2022-10-12T14:30:25Z

Now that #5904 is merged could you rebase on main to make it easier to review your changes?

samuelkarp · 2022-10-13T07:01:03Z

Now that #5904 is merged could you rebase on main to make it easier to review your changes?

@sbuckfelder Done, though I may also need to port #7481 over to sbserver as well.

samuelkarp · 2022-10-13T07:20:53Z

/test pull-containerd-sandboxed-node-e2e

samuelkarp · 2022-10-14T00:05:59Z

I've updated this with additional commits porting #7481 to sbserver as well (and slightly rewriting the integration test, as some of the assumptions it makes don't apply to sbserver).

/test pull-containerd-sandboxed-node-e2e

samuelkarp · 2022-10-14T01:05:25Z

~~Marked as draft while I resolve the test failures.~~

No longer a draft.

samuelkarp · 2022-10-14T07:08:27Z

/retest

samuelkarp · 2022-10-14T07:08:50Z

/test pull-containerd-sandboxed-node-e2e

kzys · 2022-10-14T16:15:22Z

+		if resp != nil && resp.SandboxID == "" && resp.Pid == 0 && resp.CreatedAt == nil && len(resp.Labels) == 0 {
+			// if resp is a non-nil zero-value, an error occurred during cleanup
+			cleanupErr = fmt.Errorf("failed to cleanup sandbox")
+		}


Should Start() return an error instead of asking the caller to check whether resp is its zero value or not?

I struggled with this question too. Start() does already return an error in the case when the sandbox couldn't be started, and Start() attempts to roll-back the changes made in order to setup and start the sandbox. The challenge is that there's an additional indication needed for the case where Start() fails in its cleanup. Rather than add another error and change the sandbox.Controller interface, I opted for doing this zero-value approach instead. There's a note at the top of Start() now to check whether this is reasonable once Delete() is implemented, since an alternative would be to remove all cleanup actions from Start() and instead expect that the caller invoke Delete() explicitly if Start() returns an error.

I'm open to other suggestions here though if you've got some.

If Start() fails, Start() should internally roll-back what it created itself. If Start() succeeds, then Delete() needs to be called to clean up what created by Start().

If Start() fails, Start() should internally roll-back what it created itself.

This is discussing the case where Start() fails to roll-back what it created.

Ok, I mean, The rollback should eventually be handled within the SandboxController. includes handling Sandbox 's TaskEvents with EventMonitor in the SandboxController. The CRI should not be aware of the SandboxContainer, it only uses the SandboxController.

Of course, as an intermediate process, the current implementation is possible. I just imagine the final state.

Yes, I agree with you. Kubelet will call RemovePodSandbox for Pods in NotReady state.
The current implementation of SandboxController I feel that there are still some unfinished, such as handleSandboxExit processing in sbserver/events.go.

Yep. This PR is meant to port over the bugfix behavior of #5904 and enable the new tests as we continue to work on the overall Sandbox API support. I expect that as more of the SandboxController and sbserver are implemented, some of this code will need to change (though the tests shouldn't as those should help prevent behavior regression).

yes, I appreciate your work. It takes a lot of time.
If possible we can communicate together. I am very interested in this. :)

And I hope not to trouble you because of the pit I wrote.

No trouble, thanks for taking the time to look at the PR and share your feedback!

thaJeztah · 2022-10-14T16:26:55Z

Would it be possible to fix the first commit to revert the imports that moved to reduce code-churn a bit? (I'm guessing it git got confused when cherry-picking the changes)

samuelkarp · 2022-10-14T17:28:00Z

Would it be possible to fix the first commit to revert the imports that moved to reduce code-churn a bit? (I'm guessing it git got confused when cherry-picking the changes)

@thaJeztah Yes, I'll take a look at that today. Was focusing on getting the tests to pass first yesterday. 😂

Port of 4f4aad0 to sbserver Signed-off-by: Samuel Karp <[email protected]>

Signed-off-by: Samuel Karp <[email protected]>

Port of b41d6f4 to sbserver Signed-off-by: Samuel Karp <[email protected]>

Signed-off-by: Samuel Karp <[email protected]>

mxpv · 2022-11-22T20:49:41Z

/test pull-containerd-sandboxed-node-e2e

mxpv

LGTM on green

(I have a couple of follow ups, but don't want to block this PR as it painful to rebase and fix conflicts each time).

samuelkarp requested review from fuweid and mxpv September 23, 2022 22:50

samuelkarp added the area/cri Container Runtime Interface (CRI) label Sep 23, 2022

samuelkarp added this to the 1.7 milestone Sep 23, 2022

samuelkarp mentioned this pull request Sep 23, 2022

Setup pod network after creating the sandbox container #5904

Merged

samuelkarp force-pushed the port-pr-5904-to-sbserver branch 2 times, most recently from 4172c46 to 1ddd92b Compare September 23, 2022 23:45

qiutongs reviewed Sep 26, 2022

View reviewed changes

Comment thread pkg/cri/sbserver/podsandbox/sandbox_run.go Outdated

qiutongs reviewed Sep 26, 2022

View reviewed changes

Comment thread pkg/cri/sbserver/podsandbox/sandbox_stop.go

samuelkarp force-pushed the port-pr-5904-to-sbserver branch from 1ddd92b to 4367e31 Compare September 26, 2022 22:52

samuelkarp force-pushed the port-pr-5904-to-sbserver branch from 4367e31 to 99e08fe Compare September 29, 2022 21:04

samuelkarp marked this pull request as ready for review September 30, 2022 01:16

samuelkarp mentioned this pull request Sep 30, 2022

WIP: CRI: Sandbox support user namespace. part I #6909

Closed

sparr reviewed Oct 12, 2022

View reviewed changes

Comment thread pkg/cri/sbserver/podsandbox/sandbox_run.go

sparr reviewed Oct 12, 2022

View reviewed changes

Comment thread pkg/cri/sbserver/restart.go

sparr reviewed Oct 12, 2022

View reviewed changes

Comment thread pkg/cri/sbserver/sandbox_run.go

sparr reviewed Oct 12, 2022

View reviewed changes

Comment thread pkg/cri/sbserver/sandbox_run.go

mxpv reviewed Oct 12, 2022

View reviewed changes

Comment thread pkg/cri/sbserver/podsandbox/sandbox_run.go

samuelkarp force-pushed the port-pr-5904-to-sbserver branch from 99e08fe to ba43164 Compare October 13, 2022 06:58

thaJeztah reviewed Oct 14, 2022

View reviewed changes

Comment thread integration/sandbox_run_rollback_test.go Outdated

samuelkarp force-pushed the port-pr-5904-to-sbserver branch 2 times, most recently from 3c33ddb to fb884ef Compare October 14, 2022 06:20

samuelkarp marked this pull request as ready for review October 14, 2022 07:37

mxpv approved these changes Oct 14, 2022

View reviewed changes

kzys reviewed Oct 14, 2022

View reviewed changes

Comment thread integration/sandbox_run_rollback_test.go Outdated

kzys reviewed Oct 14, 2022

View reviewed changes

samuelkarp force-pushed the port-pr-5904-to-sbserver branch 3 times, most recently from 51538ac to cd4338f Compare October 14, 2022 18:05

qiutongs reviewed Oct 17, 2022

View reviewed changes

samuelkarp force-pushed the port-pr-5904-to-sbserver branch 2 times, most recently from 620c032 to aff235f Compare October 18, 2022 00:52

qiutongs mentioned this pull request Oct 19, 2022

Follow-ups on PR #5904 - Setup pod network after creating the sandbox container #7441

Closed

qiutongs approved these changes Oct 19, 2022

View reviewed changes

samuelkarp added 4 commits November 21, 2022 16:45

sbserver: persist sandbox during partial teardown

1deaedd

Port of 4f4aad0 to sbserver Signed-off-by: Samuel Karp <[email protected]>

integration: enable tests for sbserver

085d8e6

Signed-off-by: Samuel Karp <[email protected]>

sbserver: save netns in sandbox metadata on create

a74f7e9

Port of b41d6f4 to sbserver Signed-off-by: Samuel Karp <[email protected]>

integration: enable CNI slow test for sbserver

ac4af4d

Signed-off-by: Samuel Karp <[email protected]>

samuelkarp force-pushed the port-pr-5904-to-sbserver branch from aff235f to ac4af4d Compare November 22, 2022 00:47

github-advanced-security AI found potential problems Nov 22, 2022

View reviewed changes

Comment thread pkg/cri/sbserver/podsandbox/sandbox_run.go

mxpv approved these changes Nov 22, 2022

View reviewed changes

kzys approved these changes Nov 22, 2022

View reviewed changes

samuelkarp merged commit 7d3ca17 into containerd:main Nov 23, 2022

samuelkarp deleted the port-pr-5904-to-sbserver branch November 23, 2022 00:02

Conversation

samuelkarp commented Sep 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Sep 23, 2022

Uh oh!

samuelkarp commented Sep 23, 2022

Uh oh!

samuelkarp commented Sep 25, 2022

Uh oh!

Uh oh!

Uh oh!

samuelkarp commented Sep 26, 2022

Uh oh!

samuelkarp commented Sep 29, 2022

Uh oh!

samuelkarp commented Sep 30, 2022

Uh oh!

sbuckfelder commented Oct 12, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

samuelkarp commented Oct 13, 2022

Uh oh!

samuelkarp commented Oct 13, 2022

Uh oh!

samuelkarp commented Oct 14, 2022

Uh oh!

Uh oh!

samuelkarp commented Oct 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samuelkarp commented Oct 14, 2022

Uh oh!

samuelkarp commented Oct 14, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Oct 14, 2022

Uh oh!

samuelkarp commented Oct 14, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mxpv commented Nov 22, 2022

Uh oh!

mxpv left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

samuelkarp commented Sep 23, 2022 •

edited

Loading

samuelkarp commented Oct 14, 2022 •

edited

Loading

mxpv left a comment •

edited

Loading