check image readiness when recover #345

yanxuean · 2017-10-12T03:26:44Z

fix #303

Signed-off-by: yanxuean yan.xuean@zte.com.cn

Random-Liu · 2017-10-13T03:45:15Z

/test all

mikebrow · 2017-10-17T20:51:20Z

/test all

mikebrow

Question / nit...

mikebrow · 2017-10-18T21:34:26Z

pkg/server/restart.go

@@ -101,6 +103,11 @@ func (c *criContainerdService) recover(ctx context.Context) error {
 		return fmt.Errorf("failed to load images: %v", err)
 	}
 	for _, image := range images {
+		if _, err := c.client.SnapshotService(c.config.ContainerdConfig.Snapshotter).Stat(ctx, image.ChainID); err != nil {
+			glog.Warningf("Failed to check top snapshot %+v: %v", image, err)


top snapshot?

Thanks. What do you suggest?

"Failed to stat the top-level snapshot for image %+v: %v"
or
"Failed to locate information for snapshot using chainid for image %+v: %v"

We want to check readiness of top snapshot. We do it by calling Stat() function now.
Is the error message better to indicating our purpose?

I understand your meaning and the issue description but most won't. Top snapshot could have many meanings. Images have layers.. the image.chainid points to the top-level layer for an image snapshot and contains references to the rest of the layers. IMO it's better to just say what failed vs. using the issue description of what needed to be done here.

For example: "Failed to locate information for snapshot using chainid for image %+v: %v"
As another example: "Failed to stat the top-level snapshot for image %+v: %v"
I think those are more explanatory to the reader of the warnings than: "Failed to check top snapshot %+v: %v"

What do you think?

You could add a comment above the if test stating the purpose of the check:
// Checking existence of top-level snapshot for each image being recovered.

You are right. You have a lot of experience,we need to learn from you.:)
Thank for your review and guidance.

mikebrow · 2017-10-20T14:23:44Z

/test all

mikebrow

/LGTM

Random-Liu · 2017-10-23T09:17:36Z

pkg/server/restart.go

@@ -26,6 +26,8 @@ import (
 	"github.com/containerd/containerd"
 	"github.com/containerd/containerd/content"
 	"github.com/containerd/containerd/errdefs"
+	imagemanage "github.com/containerd/containerd/images"


This name is confusing. I prefer containerdimages if we really need another name for the package.

Random-Liu · 2017-10-23T09:18:19Z

pkg/server/restart.go

@@ -101,6 +103,12 @@ func (c *criContainerdService) recover(ctx context.Context) error {
 		return fmt.Errorf("failed to load images: %v", err)
 	}
 	for _, image := range images {
+		// Checking existence of top-level snapshot for each image being recovered.


Why not add this logic into loadImages function?

Because we need call c.client.SnapshotService.

Just pass it in. :)

I feel like logically this should be part of loadImages, we load it only when it is ready.
In the future, we may need to recover the snapshot or something, which all should be handled in loadImages instead of here.

Random-Liu · 2017-10-23T09:19:44Z

pkg/server/restart.go

@@ -342,6 +350,10 @@ func loadImages(ctx context.Context, cImages []containerd.Image, provider conten
 		// imgs len must be > 0, or else the entry will not be created in
 		// previous loop.
 		i := imgs[0]
+		if ok, _, _, _, err := imagemanage.Check(ctx, provider, i.Target(), platforms.Default()); !ok || err != nil {
+			glog.Warningf("Failed to check image readiness for %q: %v", i.Name(), err)


Let's log different for !ok and err != nil.

Failed to check image content readiness for %q: %v

fix containerd#303 Signed-off-by: yanxuean <yan.xuean@zte.com.cn>

Random-Liu · 2017-10-23T14:09:15Z

LGTM

k8s-ci-robot added cncf-cla: yes size/S labels Oct 12, 2017

Random-Liu self-assigned this Oct 12, 2017

Random-Liu added this to the v1.0.0-alpha.1 milestone Oct 12, 2017

yanxuean force-pushed the imagereadiness branch from 748ec2c to 9546321 Compare October 12, 2017 05:45

yanxuean force-pushed the imagereadiness branch from 9546321 to c78347a Compare October 17, 2017 09:32

k8s-ci-robot added size/L and removed size/S labels Oct 17, 2017

yanxuean force-pushed the imagereadiness branch from c78347a to 21496bc Compare October 17, 2017 10:30

k8s-ci-robot added size/S and removed size/L labels Oct 17, 2017

mikebrow reviewed Oct 18, 2017

View reviewed changes

yanxuean force-pushed the imagereadiness branch 2 times, most recently from 3ba1186 to 6772474 Compare October 20, 2017 02:57

mikebrow approved these changes Oct 20, 2017

View reviewed changes

k8s-ci-robot assigned mikebrow Oct 20, 2017

k8s-ci-robot added the lgtm label Oct 20, 2017

Random-Liu reviewed Oct 23, 2017

View reviewed changes

yanxuean force-pushed the imagereadiness branch from 6772474 to 9d06ac0 Compare October 23, 2017 11:18

check image readiness when recover

9d06ac0

fix containerd#303 Signed-off-by: yanxuean <yan.xuean@zte.com.cn>

Random-Liu merged commit 698f0ea into containerd:master Oct 23, 2017

yanxuean deleted the imagereadiness branch October 24, 2017 00:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

check image readiness when recover #345

check image readiness when recover #345

yanxuean commented Oct 12, 2017

Random-Liu commented Oct 13, 2017

mikebrow commented Oct 17, 2017

mikebrow left a comment

mikebrow Oct 18, 2017

yanxuean Oct 19, 2017

mikebrow Oct 19, 2017 •

edited

Loading

yanxuean Oct 19, 2017

mikebrow Oct 19, 2017 •

edited

Loading

yanxuean Oct 20, 2017

yanxuean Oct 20, 2017

mikebrow commented Oct 20, 2017

mikebrow left a comment

Random-Liu Oct 23, 2017

yanxuean Oct 23, 2017

Random-Liu Oct 23, 2017

yanxuean Oct 23, 2017

Random-Liu Oct 23, 2017

Random-Liu Oct 23, 2017 •

edited

Loading

yanxuean Oct 23, 2017

Random-Liu Oct 23, 2017

Random-Liu Oct 23, 2017

yanxuean Oct 23, 2017

Random-Liu commented Oct 23, 2017

check image readiness when recover #345

check image readiness when recover #345

Conversation

yanxuean commented Oct 12, 2017

Random-Liu commented Oct 13, 2017

mikebrow commented Oct 17, 2017

mikebrow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikebrow Oct 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikebrow Oct 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikebrow commented Oct 20, 2017

mikebrow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Random-Liu Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Random-Liu commented Oct 23, 2017

mikebrow Oct 19, 2017 •

edited

Loading

mikebrow Oct 19, 2017 •

edited

Loading

Random-Liu Oct 23, 2017 •

edited

Loading