Checkpoint recovery #209

Random-Liu · 2017-09-04T02:52:36Z

Fixes #120.

Add recovery logic during restart. I've manually tested this, it's working even better than I thought. :)

Based on #206 and #179.

@kubernetes-incubator/maintainers-cri-containerd Please review this carefully, especially pkg/server/restart.go. This is really important to the production quality.

I'll send another PR to add our own integration test framework, and will add restart test.

abhi

Initial comments.

abhi · 2017-09-19T05:24:11Z

pkg/server/restart.go

+	}
+
+	// Load network namespace.
+	netNS, err := sandboxstore.LoadNetNS(meta.NetNSPath)


NetNSPath is not set if its in host network namespace. Should we be checking for the network namespace before loading it ?

Good catch. We should.

abhi · 2017-09-19T05:32:31Z

pkg/server/restart.go

+		}
+		id := d.Name()
+		found := false
+		for _, c := range cntrs {


can we use a map to track the loaded containers/sandboxes from the caller. Just thinking, looping over containers for every directory may not be a faster method.

or may be keep a track of containerDirs in a map once loadContainer succeeds. So we can just loop around the dirs here and check if its in the containerDirs map , if not then clean it up.

abhi · 2017-09-19T05:32:50Z

pkg/server/restart.go

+		}
+		id := d.Name()
+		found := false
+		for _, c := range cntrs {


same as mentioned for sandboxes

Random-Liu · 2017-09-19T23:51:48Z

@abhinandanpb Addressed comments.

abhi

Did a first pass now. Looks good overall. Minor comments.

abhi · 2017-09-20T05:05:47Z

pkg/store/sandbox/netns.go

 }

+// LoadNetNS loads existing network namespace. It returns ErrClosedNetNS
+// is the network namespace has already been closed.


nit: if the network namespace

abhi · 2017-09-20T05:07:40Z

pkg/store/sandbox/netns.go

+// LoadNetNS loads existing network namespace. It returns ErrClosedNetNS
+// is the network namespace has already been closed.
+func LoadNetNS(path string) (*NetNS, error) {
+	if err := cnins.IsNSorErr(path); err != nil {


This check is already done by GetNS(path) and would return an error if it is closed.

We need to distinguish whether it's ErrClosedNetNS or not in restart.go.

GetNS would return cnins.NSPathNotExistErr so you can always check for that error if the nspath doesnt exist. Unless I am missing something. This is fine. If we can avoid an if condition even better.

abhi · 2017-09-20T05:10:37Z

pkg/store/sandbox/netns.go

+			return fmt.Errorf("failed to stat netns: %v", err)
+		}
+		// Follow possible /var/run -> /run symlink.
+		path, err := symlink.FollowSymlinkInScope(path, "/")


we dont create symlink currently right ? Should we explicitly follow the symlink ? kernel takes care of delinking the implicit ones ?

Copied from cri-o. Yeah, it seems that we don't need this even /var/run -> /run is a symlink.

abhi · 2017-09-20T06:11:59Z

pkg/server/restart.go

+		return container, fmt.Errorf("failed to load task: %v", err)
+	}
+	var s containerd.Status
+	notFound := false


var notFound bool

abhi · 2017-09-20T06:17:37Z

pkg/server/restart.go

+	if err != nil {
+		return fmt.Errorf("failed to list sandbox containers: %v", err)
+	}
+	for _, sandbox := range sandboxes {


I think you can create a sandboxmap[string]bool here and pass it to cleanuporphansandboxdir instead of creating one there.

Doesn't make too much difference to me, because we won't use the map here. Given so I prefer move the code into cleanuporphansandboxdir which is the place where the map will be used. :)

I meant instead of passing sandbox list and container list , pass a map for comparison. Not a blocker though

abhi · 2017-09-20T06:17:54Z

pkg/server/restart.go

+	if err != nil {
+		return fmt.Errorf("failed to list containers: %v", err)
+	}
+	for _, container := range containers {


same as sandbox

abhi

LGTM otherwise

abhi · 2017-09-21T17:48:47Z

pkg/store/sandbox/netns.go

+// LoadNetNS loads existing network namespace. It returns ErrClosedNetNS
+// is the network namespace has already been closed.
+func LoadNetNS(path string) (*NetNS, error) {
+	if err := cnins.IsNSorErr(path); err != nil {


GetNS would return cnins.NSPathNotExistErr so you can always check for that error if the nspath doesnt exist. Unless I am missing something. This is fine. If we can avoid an if condition even better.

Random-Liu · 2017-09-21T17:55:20Z

GetNS would return cnins.NSPathNotExistErr so you can always check for that error if the nspath doesnt exist. Unless I am missing something. This is fine. If we can avoid an if condition even better.

@abhinandanpb I read their code. You are right. We should use cnins.NSPathNotExistErr instead. We may not want to ignore the error if it's some other transient stat error.

Good catch!

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

Random-Liu · 2017-09-21T18:01:10Z

@abhinandanpb Addressed comments. I still keeps the ErrClosedNetNS, so that caller of LoadNetNS doesn't need to make assumption on the underlying implementation. However, I does change LoadNetNS to look at cnins.NSPathNotExistErr.

Random-Liu · 2017-09-21T18:25:15Z

@abhinandanpb Thanks for reviewing! Will merge after test passes.

mikebrow · 2017-09-21T18:34:46Z

/LGTM

k8s-ci-robot added cncf-cla: yes size/XXL labels Sep 4, 2017

Random-Liu assigned abhi and mikebrow Sep 4, 2017

Random-Liu force-pushed the checkpoint-recovery branch from 63b2d67 to 761131c Compare September 4, 2017 03:02

Random-Liu force-pushed the checkpoint-recovery branch 2 times, most recently from 48c960d to d6fa504 Compare September 12, 2017 05:46

Random-Liu added this to the v1.0.0-alpha.0 milestone Sep 12, 2017

Random-Liu force-pushed the checkpoint-recovery branch from d6fa504 to 18baa35 Compare September 18, 2017 17:36

Random-Liu mentioned this pull request Sep 19, 2017

Switch to containerd extension #251

Closed

abhi reviewed Sep 19, 2017

View reviewed changes

yanxuean mentioned this pull request Sep 19, 2017

Switch to containerd extension #260

Merged

Random-Liu force-pushed the checkpoint-recovery branch from 18baa35 to ed6d11d Compare September 19, 2017 23:51

abhi reviewed Sep 20, 2017

View reviewed changes

Random-Liu force-pushed the checkpoint-recovery branch from ed6d11d to b28a549 Compare September 20, 2017 21:01

k8s-ci-robot added size/XL and removed size/XXL labels Sep 20, 2017

Random-Liu force-pushed the checkpoint-recovery branch from b28a549 to 8e11861 Compare September 20, 2017 21:02

abhi reviewed Sep 21, 2017

View reviewed changes

Add restart recovery logic.

cc1b0b6

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

Random-Liu force-pushed the checkpoint-recovery branch from 8e11861 to cc1b0b6 Compare September 21, 2017 17:59

abhi added the lgtm label Sep 21, 2017

Random-Liu merged commit 9015b6e into containerd:master Sep 21, 2017

Random-Liu deleted the checkpoint-recovery branch September 21, 2017 18:32

Checkpoint recovery #209

Checkpoint recovery #209

Uh oh!

Conversation

Random-Liu commented Sep 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Random-Liu commented Sep 19, 2017

Uh oh!

abhi 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 Sep 20, 2017 • 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!

abhi Sep 20, 2017 • 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

Uh oh!

abhi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Random-Liu commented Sep 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Random-Liu commented Sep 21, 2017

Uh oh!

Random-Liu commented Sep 21, 2017

Uh oh!

mikebrow commented Sep 21, 2017

Uh oh!

Reviewers

Assignees

Labels

Random-Liu commented Sep 4, 2017 •

edited

Loading

Random-Liu Sep 20, 2017 •

edited

Loading

abhi Sep 20, 2017 •

edited

Loading

Random-Liu commented Sep 21, 2017 •

edited

Loading