-
Notifications
You must be signed in to change notification settings - Fork 348
Conversation
748ec2c
to
9546321
Compare
/test all |
9546321
to
c78347a
Compare
c78347a
to
21496bc
Compare
/test all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question / nit...
pkg/server/restart.go
Outdated
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
top snapshot?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. What do you suggest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Failed to stat the top-level snapshot for image %+v: %v"
or
"Failed to locate information for snapshot using chainid for image %+v: %v"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. You have a lot of experience,we need to learn from you.:)
Thank for your review and guidance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
3ba1186
to
6772474
Compare
/test all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/LGTM
pkg/server/restart.go
Outdated
@@ -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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This name is confusing. I prefer containerdimages
if we really need another name for the package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
pkg/server/restart.go
Outdated
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not add this logic into loadImages
function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we need call c.client.SnapshotService.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just pass it in. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
pkg/server/restart.go
Outdated
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's log different for !ok
and err != nil
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Failed to check image content readiness for %q: %v
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
6772474
to
9d06ac0
Compare
fix containerd#303 Signed-off-by: yanxuean <yan.xuean@zte.com.cn>
LGTM |
fix #303
Signed-off-by: yanxuean yan.xuean@zte.com.cn