sandbox: remove sandboxStore from podsandbox controller #9463

abel-von · 2023-12-04T17:14:35Z

For decoupling of podsandbox controller to cri service, podsandbox controller should create its own cache of PodSandbox object.

k8s-ci-robot · 2023-12-04T17:14:45Z

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.

akhilerm · 2023-12-04T17:28:10Z

/ok-to-test

fuweid · 2023-12-05T14:34:58Z

sandbox/controller.go

 	Stop(ctx context.Context, sandboxID string, opts ...StopOpt) error
 	// Wait blocks until sandbox process exits.
-	Wait(ctx context.Context, sandboxID string) (ExitStatus, error)
+	Wait(ctx context.Context, sandboxID string) (<-chan ExitStatus, error)


I think you change the definition here. You should change the comment as well.
Would you please add more comment about why it becomes async?

fuweid · 2023-12-05T14:53:43Z

pkg/cri/server/podsandbox/store.go

+	return nil
+}
+
+func (s *Store) Get(id string) (*types.PodSandbox, bool) {


bool seems duplicate information here, since that Save ensures the p is not nil.

done, the existing is based on whether the return value is nil

fuweid · 2023-12-05T14:57:42Z

pkg/cri/server/podsandbox/controller.go

+	ch := make(chan sandbox.ExitStatus, 1)
+	go func() {
+		// because we don't set timeout of Wait, so it will not return err
+		status, _ := podSandbox.Wait(ctrdutil.NamespacedContext())


I think we should use ctx here. The caller should ensure that context should not share the same scope with RunPodSandbox one.

abel-von · 2023-12-11T01:17:11Z

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

fuweid

That change is too large. It seems it mixed bugfix and refactor.
If possible, please separate changes into several pull requests.

fuweid · 2024-01-04T03:54:19Z

sandbox/controller.go

 	// Stop will stop sandbox instance
 	Stop(ctx context.Context, sandboxID string, opts ...StopOpt) error
-	// Wait blocks until sandbox process exits.
+	// Wait asynchronously waits for the sandbox to exit, and sends the exit code to the returned channel


The comment should be updated.

reverted the change.

fuweid · 2024-01-04T03:55:40Z

plugins/sandbox/controller.go

 	})
-
 	if err != nil {
-		return sandbox.ExitStatus{}, fmt.Errorf("failed to wait sandbox %s: %w", sandboxID, errdefs.FromGRPC(err))


Is there any reason to change this line?

fuweid · 2024-01-04T04:04:57Z

pkg/cri/server/sandbox_status.go

+// In most cases, controller.Status() with verbose=true should have SandboxInfo in the return,
+// but if controller.Status() returns a NotFound error,
+// we should fallback to get SandboxInfo from cached sandbox itself.
+func toCRISandboxInfo(sandbox sandboxstore.Sandbox) (map[string]string, error) {


Would you please change this name to, like toDeletedCRISandboxInfo? toCRISandboxInfo looks weird because it's just used when sandbox is not found.

fuweid · 2024-01-04T04:10:11Z

pkg/cri/server/sandbox_run.go

+	exitCh, err := sandboxClient.Wait(util.NamespacedContext())
+	if err != nil {
+		log.G(ctx).WithError(err).Error("failed to wait for sandbox exit")
+		return


it's not correct. Please return err here. The named return error is retErr instead of err.

Will change this the next PR as I reverted commit of sandboxClient.

fuweid · 2024-01-04T05:20:50Z

pkg/cri/server/restart.go

 			}
+
+			// Convert the old pause Container to the new Sandbox
+			info, _ := sb.Container.Info(ctx)


What if err is not nil?

as the commit is reverted, will consider this in the next PR

fuweid · 2024-01-04T05:22:22Z

pkg/cri/server/restart.go


+	for _, sb := range c.sandboxStore.List() {
+		sb := sb
+		go func() {


Is it bugfix?

I think it can be considered as a bugfix, before this PR, the recover.go in podsandbox will handle the task exit and update the sandbox in the cri cache. Now as in this pr we removed the sandboxStore in podsandbox controller, it can not update the sandbox in cri cache anymore. so we have to call Wait of sandbox and startSandboxExitMonitor here.
I think it is also neccessary if we have more sandbox controllers other than podsandbox. we have to call Wait of sandbox and monitor it by ourselves.

fuweid · 2024-01-04T05:23:26Z

pkg/cri/server/podsandbox/sandbox_stop.go

 	if err != nil {
 		if !errdefs.IsNotFound(err) {
-			return fmt.Errorf("failed to get sandbox container: %w", err)
+			return fmt.Errorf("failed to get podSandbox container: %w", err)


s/podSandbox/pod sandbox/g

fuweid · 2024-01-04T05:25:04Z

pkg/cri/server/podsandbox/sandbox_stop.go

-			sandboxID, err)
+	podSandbox := c.store.Get(sandboxID)
+	if podSandbox == nil {
+		return nil


Could you consider return ErrNotFound here?

done, and let cri server log and ignore the ErrNotFound.

fuweid · 2024-01-04T05:27:02Z

client/sandbox.go

+func (s *sandboxClient) Wait(ctx context.Context) (<-chan api.ExitStatus, error) {
+	ch := make(chan api.ExitStatus, 1)
+	go func() {
+		status, err := s.client.SandboxController(s.metadata.Sandboxer).Wait(ctx, s.ID())


suggest to close ch in defer and remain that UnknownExitStatus if Wait returns error.

abel-von · 2024-01-04T16:52:51Z

If possible, please separate changes into several pull requests.

ok, I revert the pr of refator to use Sandbox Client. and will submit another PR with that refactor.

fuweid · 2024-01-05T02:17:00Z

pkg/cri/server/sandbox_status.go

 	}
 }
+
+// toCRISandboxInfo converts cached sandbox to CRI sandbox status response info map.


Align name with function name

fuweid · 2024-01-05T02:24:49Z

pkg/cri/server/podsandbox/sandbox_stop.go

-	case <-sandbox.Stopped():
-		return nil
-	}
+func (c *Controller) waitSandboxStop(ctx context.Context, sandbox *types.PodSandbox) error {


We can remove this function and call sandbox.Wait(ctx) directly.

fuweid · 2024-01-05T02:31:26Z

pkg/cri/server/podsandbox/sandbox_stop.go

 				// TODO: how to backoff
 				c.cri.BackOffEvent(id, e)
 			}
+			podSandbox.Exit(*containerd.NewExitStatus(exitStatus, exitedAt, err))


I don't think it's correct. Line 107 has backoff event. And then the code still goes into set Exit.
And the Exit has close the channel. I think it might cause leak issue.

I think we can remove the codes of backoff event here because the startSandboxExitMonitor is responsible to handle backoff when sandbox exit event handle failed. So I removed the backoff logic here.

changed to call podSandbox.Exit when the error is not a context error.

I think we have to wait #9598 is merged, and then the backoff event can be handled again by the podsandbox controller.

fuweid · 2024-01-05T02:35:46Z

pkg/cri/server/podsandbox/sandbox_run.go

+	podSandbox := types.NewPodSandbox(info.ID, sandboxstore.Status{State: sandboxstore.StateUnknown})
+	podSandbox.Metadata = metadata
+	podSandbox.Runtime = info.Runtime
+	_ = c.store.Save(podSandbox)


return c.store.Save(podSandbox)

fuweid · 2024-01-05T02:37:37Z

pkg/cri/server/podsandbox/recover.go

+		}()
+	}
+
+	_ = c.store.Save(podSandbox)


check error

it only return error when podSandbox is nil and as we know it is not nil, so it will not return error. anyway, I add the check error logic in case that Save function returns some other error in the future.

abel-von · 2024-01-08T06:13:10Z

/retest-required

fuweid · 2024-01-08T12:33:46Z

pkg/cri/server/podsandbox/controller.go

+		dctx, dcancel := context.WithTimeout(dctx, handleEventTimeout)
+		defer dcancel()
+		event := &eventtypes.TaskExit{ExitStatus: exitStatus, ExitedAt: protobuf.ToTimestamp(exitedAt)}
+		if err := handleSandboxTaskExit(dctx, p, event); err != nil {


is it going to shadow named return err?

I think this is for purpose because we don't want to return the handle task error here, anyway, I changed the name here from err to cleanErr, so to make this clear.

Signed-off-by: Abel Feng <fshb1988@gmail.com>

fuweid

LGTM

fuweid · 2024-01-09T01:38:22Z

pkg/cri/server/podsandbox/controller.go

+		dctx, dcancel := context.WithTimeout(dctx, handleEventTimeout)
+		defer dcancel()
+		event := &eventtypes.TaskExit{ExitStatus: exitStatus, ExitedAt: protobuf.ToTimestamp(exitedAt)}
+		if cleanErr := handleSandboxTaskExit(dctx, p, event); cleanErr != nil {


Let's add some test cases to see what if fail to cleanup task and more than 2 StopPodSandbox requests.

definitely decoupled now... still would like to see extra test cases, this is one of the touchiest areas of the code base

Yes I will add some more tests here

mikebrow · 2024-01-09T17:16:25Z

pkg/cri/server/podsandbox/recover.go

 			if taskStatus.Status == containerd.Running {
-				// Wait for the task for sandbox monitor.
-				// wait is a long running background request, no timeout needed.
+				status.State = sandboxstore.StateReady


why are we setting ready if there is still a window for failure to wait for the container task (see return below)

@abel-von ...

Sorry for the late reply, I think you are right, I have to set the status to NotReady before return, I will submit another PR to do it

@mikebrow #9644

k8s-ci-robot added size/XL needs-ok-to-test labels Dec 4, 2023

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Dec 4, 2023

abel-von force-pushed the sandbox-plugin-1205 branch from 0e49613 to 6208f4d Compare December 5, 2023 02:38

fuweid reviewed Dec 5, 2023

View reviewed changes

abel-von force-pushed the sandbox-plugin-1205 branch from 6208f4d to b7bd664 Compare December 9, 2023 15:07

abel-von mentioned this pull request Dec 11, 2023

Sandbox API work continued #9431

Open

19 tasks

abel-von requested a review from fuweid December 18, 2023 01:39

k8s-ci-robot added size/XXL and removed size/XL labels Dec 22, 2023

abel-von closed this Dec 25, 2023

abel-von reopened this Dec 25, 2023

abel-von mentioned this pull request Jan 3, 2024

sandbox: add event monitor for podsandbox controller #9598

Merged

mxpv approved these changes Jan 3, 2024

View reviewed changes

fuweid reviewed Jan 4, 2024

View reviewed changes

abel-von force-pushed the sandbox-plugin-1205 branch from 7f4589d to 0daa015 Compare January 4, 2024 16:09

k8s-ci-robot added size/XL and removed size/XXL labels Jan 4, 2024

abel-von force-pushed the sandbox-plugin-1205 branch 2 times, most recently from 8e06d18 to 2b18128 Compare January 4, 2024 16:48

abel-von requested a review from fuweid January 4, 2024 17:01

fuweid reviewed Jan 5, 2024

View reviewed changes

k8s-ci-robot added the needs-rebase label Jan 5, 2024

abel-von force-pushed the sandbox-plugin-1205 branch 2 times, most recently from a8a35f1 to 7daa93e Compare January 5, 2024 09:20

k8s-ci-robot removed the needs-rebase label Jan 5, 2024

abel-von requested a review from fuweid January 8, 2024 07:35

abel-von force-pushed the sandbox-plugin-1205 branch 2 times, most recently from 21741d6 to ec695af Compare January 8, 2024 11:33

fuweid reviewed Jan 8, 2024

View reviewed changes

sandbox: remove sandboxStore from podsandbox

7dadd5f

Signed-off-by: Abel Feng <fshb1988@gmail.com>

abel-von force-pushed the sandbox-plugin-1205 branch from ec695af to 7dadd5f Compare January 8, 2024 16:19

dmcgowan approved these changes Jan 8, 2024

View reviewed changes

abel-von requested a review from fuweid January 9, 2024 01:31

fuweid approved these changes Jan 9, 2024

View reviewed changes

fuweid added this pull request to the merge queue Jan 9, 2024

Merged via the queue into containerd:main with commit 9f8b845 Jan 9, 2024

abel-von mentioned this pull request Jan 9, 2024

sandbox: use sandboxService in CRI plugin instead of calling controller API directly #9617

Merged

mikebrow reviewed Jan 9, 2024

View reviewed changes

abel-von mentioned this pull request Jan 16, 2024

sandbox: fix podsandbox recover status issue #9644

Merged

sandbox: remove sandboxStore from podsandbox controller #9463

sandbox: remove sandboxStore from podsandbox controller #9463

Uh oh!

Conversation

abel-von commented Dec 4, 2023

Uh oh!

k8s-ci-robot commented Dec 4, 2023

Uh oh!

akhilerm commented Dec 4, 2023

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!

abel-von commented Dec 11, 2023

Uh oh!

fuweid left a comment

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!

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!

abel-von commented Jan 4, 2024

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