Avoid containerd access as much as possible. by Random-Liu · Pull Request #571 · containerd/cri

Random-Liu · 2018-01-25T01:19:40Z

Related to containerd/containerd#2020.

This PR:

Cache sandbox status in sandbox store.
Update sandbox status based on containerd event.
Recover sandbox cache during restart.

This should reduce the containerd/containerd-shim cpu/memory usage.

Signed-off-by: Lantao Liu [email protected]

miaoyq · 2018-01-25T01:32:00Z

golint failed.

I0125 01:26:35.928] pkg/store/sandbox/status.go:33:2:warning: don't use underscores in Go names; const PodSandboxState_UNKNOWN should be PodSandboxStateUNKNOWN (golint)
I0125 01:26:35.929] pkg/store/sandbox/status.go:36:2:warning: don't use underscores in Go names; const PodSandboxState_READY should be PodSandboxStateREADY (golint)
I0125 01:26:35.930] pkg/store/sandbox/status.go:42:2:warning: don't use underscores in Go names; const PodSandboxState_NOTREADY should be PodSandboxStateNOTREADY (golint)

Random-Liu · 2018-01-25T01:37:32Z

@miaoyq Fixing it. :)

Random-Liu · 2018-01-25T01:44:42Z

 	// NetNSPath is the network namespace used by the sandbox.
 	NetNSPath string
+	// IP of Pod if it is attached to non host network
+	IP string


@abhi This moves IP from Sandbox into Metadata. This doesn't make any logical difference, just a minor cleanup.

Random-Liu · 2018-01-25T01:45:27Z

+	defer timeoutTimer.Stop()
+	for {
+		// Poll once before waiting for stopCheckPollInterval.
+		// TODO(random-liu): Use channel with event handler instead of polling.


@miaoyq We should also change this to channel.

OK, will change this after this pr merged.

Random-Liu · 2018-01-25T01:50:11Z

-	pid uint32, processStatus containerd.ProcessStatus, verbose bool) (map[string]string, error) {
-	if !verbose {
-		return nil, nil
+func toCRISandboxInfo(ctx context.Context, sandbox sandboxstore.Sandbox) (map[string]string, error) {


@mikebrow I change this function to return error instead of logging. We are going to merge cri-containerd and containerd, this won't be a problem then.

Random-Liu · 2018-01-25T02:15:00Z

@mikebrow @yanxuean @miaoyq PR ready for review.

yanxuean · 2018-01-25T01:35:45Z

-		return nil, fmt.Errorf("failed to list sandbox containers: %v", err)
-	}
-
 	var sandboxes []*runtime.PodSandbox


make slice with len?

Let's do this kind of optimization when we see this is a bottleneck. :)

I don't think it will make too much difference.

yanxuean · 2018-01-25T02:29:31Z

 func (s *Store) Get(id string) (Sandbox, error) {
+	sb, err := s.GetAll(id)
+	if err != nil {
+		return sb, err


Sandbox{},err

It's fine here. I think. :)

it's fine..

yanxuean · 2018-01-25T02:52:36Z

@@ -106,53 +107,20 @@ func (em *eventMonitor) handleEvent(evt *events.Envelope) {
 		e := any.(*eventtypes.TaskExit)


Do we not handle sandbox with TaskOOM?

Yeah, we don't need to do that. Even we handle, what can we do? :)

yanxuean · 2018-01-25T06:00:04Z

+		defer func() {
+			if retErr != nil {
+				// Cleanup the sandbox container if an error is returned.
+				if _, err := task.Delete(ctx, containerd.WithProcessKill); err != nil {


Now we also do task.Delete with unknown state in event.handleSandboxExit, so we should check that if the err is NotFound. The NotFound is a normal case.

Changed both sandbox_run.go and container_start.go to check IsNotFound. Good catch!

yanxuean · 2018-01-25T06:05:43Z

+func handleSandboxExit(e *eventtypes.TaskExit, sb sandboxstore.Sandbox) {
+	// Don't need to check pid because sandbox container should only have one process.
+	// No stream attached to sandbox container.
+	task, err := sb.Container.Task(context.Background(), nil)


I think we should not do this on unknown state. Let RunPodSandbox do it.
Or else we maybe do task.Delete both in handleSandboxExit and RunPodSandbox.

On a second thought, I still prefer put task.Delete out of `Update.

Update will hold a lock of the container/sandbox, and List (which is a critical operation for kubelet) will be blocked until the lock is released.

We should put as little logic as possible into Update. The issue you mentioned can be handled by #571 (comment).

miaoyq · 2018-01-25T07:32:32Z

+		}
+
+		pid = task.Pid()
+		processStatus = taskStatus.Status


Could we also store the pid and processStatus to sandboxstore?

processState keeps changing, it's hard to maintain it in our store. I'd prefer not to do that until we have to.

I'll do pid.

Random-Liu · 2018-01-25T19:43:50Z

Test failure seems like a GCE flake.

/test pull-cri-containerd-node-e2e

mikebrow

See comments & questions.

mikebrow · 2018-01-25T18:54:55Z

+			return false, err
+		}
+		return status.GetState() == runtime.PodSandboxState_SANDBOX_NOTREADY, nil
+	}, time.Second, 30*time.Second), "sandbox state should become NOTREADY")


todo for configuring timeout for becoming ready.. possibly receive direction over cri api

I think fixed 30 second for test is fine. This should happen very soon.

I think it's fine to, but people not wanting to wait for a new pod might not like it..

Hm, what do you mean?

Here, we kill the corresponding task from containerd, and wait for the pod sandbox status to be updated NOTREADY by cri-containerd. It usually only takes several microseconds I guess?

NVM I was distracted. Ignore :-)

mikebrow · 2018-01-25T19:30:25Z

 	// List all sandboxes from store.
 	sandboxesInStore := c.sandboxStore.List()
-
-	response, err := c.client.TaskService().List(ctx, &tasks.ListTasksRequest{})


Consider adding a todo for running a validate routine to make sure we have stayed in sync and didn't just miss an event.. I like this pattern of working off the push of events, but without some periodic validation there is a risk of an event failing to reach us, low but risk nonetheless? Or are all the events returning successful delivery to containerd?

Good point. Will add a TODO for that.

We sometimes see Docker stay in weird state because it missed an event and not in sync with containerd.

mikebrow · 2018-01-25T19:42:54Z

-	pid uint32, processStatus containerd.ProcessStatus, verbose bool) (map[string]string, error) {
-	if !verbose {
-		return nil, nil
+func toCRISandboxInfo(ctx context.Context, sandbox sandboxstore.Sandbox) (map[string]string, error) {


mikebrow · 2018-01-25T19:45:52Z

-	info, err := toCRISandboxInfo(ctx, sandbox, pid, processStatus, r.GetVerbose())
+
+	// Generate verbose information.
+	info, err := toCRISandboxInfo(ctx, sandbox)


if r.GetVerbose() there is no need to get info here..

If !r.GetVerbose() it will return directly above.

oh there it is :-)

mikebrow · 2018-01-25T19:55:12Z


+// stopCheckPollInterval is the the interval to check whether a sandbox
+// is stopped successfully.
+const stopCheckPollInterval = 100 * time.Millisecond


todo make configurable

We'll remove this very soon by adding a stop channel. @yanxuean @miaoyq are going to do that.
See #570

mikebrow · 2018-01-25T19:57:51Z


-	if err := c.stopSandboxContainer(ctx, sandbox.Container); err != nil {
-		return nil, fmt.Errorf("failed to stop sandbox container %q: %v", id, err)
+	// Only stop sandbox container when it's running.


is no error the the expected CRI behavior if it's not running, seems like we should tell them they can't stop a non running podsandbox ...
At least log a warning?

Actually we expect StopPodSandbox to be idempotent. Caller should be able to call this many times.

And kubelet does call this function multiple times for one pod sandbox.

See https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/apis/cri/v1alpha1/runtime/api.proto#L28.

ok. just checking, odd behavior IMO Maybe they call stop multiple times cause no one threw an already stopped error that they could easily ignore if they wanted to treat it as idempotent for their use case.

mikebrow · 2018-01-25T20:03:40Z

 func (s *Store) Get(id string) (Sandbox, error) {
+	sb, err := s.GetAll(id)
+	if err != nil {
+		return sb, err


it's fine..

Random-Liu · 2018-01-25T21:29:42Z

/test pull-cri-containerd-node-e2e

mikebrow

/LGTM

mikebrow · 2018-01-25T23:28:04Z


-	if err := c.stopSandboxContainer(ctx, sandbox.Container); err != nil {
-		return nil, fmt.Errorf("failed to stop sandbox container %q: %v", id, err)
+	// Only stop sandbox container when it's running.


ok. just checking, odd behavior IMO Maybe they call stop multiple times cause no one threw an already stopped error that they could easily ignore if they wanted to treat it as idempotent for their use case.

mikebrow · 2018-01-25T23:31:43Z

+			return false, err
+		}
+		return status.GetState() == runtime.PodSandboxState_SANDBOX_NOTREADY, nil
+	}, time.Second, 30*time.Second), "sandbox state should become NOTREADY")


NVM I was distracted. Ignore :-)

mikebrow · 2018-01-25T23:32:45Z

 // eventMonitor monitors containerd event and updates internal state correspondingly.
-// TODO(random-liu): [P1] Is it possible to drop event during containerd is running?
+// TODO(random-liu): [P1] Figure out is it possible to drop event during containerd
+// is running. If it is, we should do periodically list to sync state with containerd.


looks good!

Signed-off-by: Lantao Liu <[email protected]>

Random-Liu · 2018-01-25T23:36:30Z

Rebased.
@mikebrow Thanks for reviewing!
@miaoyq @yanxuean Are you 2 OK with this PR now? :)

yanxuean · 2018-01-25T23:56:38Z

/lgtm

miaoyq · 2018-01-26T00:12:19Z

/lgtm

Random-Liu added this to the v1.0.0-rc.0 milestone Jan 25, 2018

Random-Liu assigned mikebrow, miaoyq and yanxuean Jan 25, 2018

k8s-ci-robot added the size/XXL label Jan 25, 2018

Random-Liu force-pushed the do-not-list-task branch from 9e1012d to 718d1cc Compare January 25, 2018 01:22

Random-Liu added the kind/performance label Jan 25, 2018

Random-Liu force-pushed the do-not-list-task branch 3 times, most recently from 5121ea6 to 099c13d Compare January 25, 2018 02:09

Random-Liu commented Jan 25, 2018

View reviewed changes

Random-Liu force-pushed the do-not-list-task branch from 099c13d to 7d9512f Compare January 25, 2018 02:43

yanxuean reviewed Jan 25, 2018

View reviewed changes

miaoyq reviewed Jan 25, 2018

View reviewed changes

Random-Liu force-pushed the do-not-list-task branch from 7d9512f to 5b934ef Compare January 25, 2018 18:53

Random-Liu mentioned this pull request Jan 25, 2018

[release/1.0] cmd/containerd-shim: aggressive memory reclamation containerd/containerd#2058

Merged

mikebrow reviewed Jan 25, 2018

View reviewed changes

mikebrow approved these changes Jan 25, 2018

View reviewed changes

k8s-ci-robot added the lgtm label Jan 25, 2018

Avoid containerd access as much as possible.

df58d68

Signed-off-by: Lantao Liu <[email protected]>

Random-Liu force-pushed the do-not-list-task branch from 8799774 to df58d68 Compare January 25, 2018 23:36

Random-Liu merged commit f401662 into containerd:master Jan 26, 2018

Random-Liu deleted the do-not-list-task branch January 26, 2018 00:13

miaoyq mentioned this pull request Jan 26, 2018

Use channel to propagate the stop info of sandbox #575

Merged

		@@ -106,53 +107,20 @@ func (em eventMonitor) handleEvent(evt events.Envelope) {
		e := any.(*eventtypes.TaskExit)

Conversation

Random-Liu commented Jan 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miaoyq commented Jan 25, 2018

Uh oh!

Random-Liu commented Jan 25, 2018

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!

Random-Liu commented Jan 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Random-Liu Jan 25, 2018 • 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!

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!

yanxuean Jan 25, 2018 • 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!

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!

Random-Liu commented Jan 25, 2018

Uh oh!

mikebrow 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!

Random-Liu Jan 25, 2018 • 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!

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

Random-Liu commented Jan 25, 2018 •

edited

Loading

Random-Liu Jan 25, 2018 •

edited

Loading

yanxuean Jan 25, 2018 •

edited

Loading

Random-Liu Jan 25, 2018 •

edited

Loading

Random-Liu Jan 25, 2018 •

edited

Loading

Random-Liu Jan 25, 2018 •

edited

Loading