Skip to content
This repository was archived by the owner on Mar 9, 2022. It is now read-only.

Conversation

@Random-Liu
Copy link
Member

@Random-Liu Random-Liu commented May 18, 2017

This PR finishes the containerd image management. There are still many TODOs to be addressed, I'll do them in some following PRs.

TODOs for this PR:

  • Add unit test as much as I can. Since containerd API is changing fast, I'll not add unit test for rootfs service related unit test now.
  • Add image list filters.

TODOs for future PRs:

  1. Some work for operation synchronization:
  • a) When pulling image, get digests of all resources associated to the image, wait for those pulling to be done by checking content status. This makes sure that concurrent pull which pulls the same resource won't return before the resource is fully pulled. (content store returns immediately for ongoing pulling). (Wait and check image pulling progress. #46)
    b) We'll not need checkpoint for image management. Write a cleaner and easier to use image metadata which could lock more operations together because all data is in memory.
    c) Add some synchronization between concurrent pulling/removing against the same image.
  1. Add authentication support

  2. Update containerd version to use the new snapshot service

This PR also depends on containerd/containerd#875.

@Random-Liu
Copy link
Member Author

The PR already passed all CRI image validation test:

Running Suite: E2ECRI Suite
===========================
Random Seed: 1495147524 - Will randomize all specs
Will run 6 of 36 specs

SSSS
------------------------------
[k8s.io] Image Manager 
  image status get image fields should not be empty [Conformance]
  /usr/local/google/home/lantaol/workspace/src/github.com/kubernetes-incubator/cri-tools/pkg/validate/image.go:74
[BeforeEach] [k8s.io] Image Manager
  /usr/local/google/home/lantaol/workspace/src/github.com/kubernetes-incubator/cri-tools/pkg/framework/framework.go:50
[BeforeEach] [k8s.io] Image Manager
  /usr/local/google/home/lantaol/workspace/src/github.com/kubernetes-incubator/cri-tools/pkg/validate/image.go:48
[It] image status get image fields should not be empty [Conformance]
  /usr/local/google/home/lantaol/workspace/src/github.com/kubernetes-incubator/cri-tools/pkg/validate/image.go:74
STEP: Pull image : busybox:1.26.2
STEP: Get image status
STEP: Get image status
STEP: Remove image : busybox:1.26.2
[AfterEach] [k8s.io] Image Manager
  /usr/local/google/home/lantaol/workspace/src/github.com/kubernetes-incubator/cri-tools/pkg/framework/framework.go:51
•SSSSS
------------------------------
[k8s.io] Image Manager 
  public image without tag should be pulled and removed [Conformance]
  /usr/local/google/home/lantaol/workspace/src/github.com/kubernetes-incubator/cri-tools/pkg/validate/image.go:56
[BeforeEach] [k8s.io] Image Manager
  /usr/local/google/home/lantaol/workspace/src/github.com/kubernetes-incubator/cri-tools/pkg/framework/framework.go:50
[BeforeEach] [k8s.io] Image Manager
  /usr/local/google/home/lantaol/workspace/src/github.com/kubernetes-incubator/cri-tools/pkg/validate/image.go:48
[It] public image without tag should be pulled and removed [Conformance]
  /usr/local/google/home/lantaol/workspace/src/github.com/kubernetes-incubator/cri-tools/pkg/validate/image.go:56
STEP: Pull image : busybox:latest
STEP: Check image list to make sure pulling image success : busybox:latest
STEP: Get image list for imageName : busybox:latest
STEP: Remove image : busybox:latest
STEP: Remove image : busybox:latest
STEP: Check image list empty
STEP: Get image list for imageName : busybox:latest
[AfterEach] [k8s.io] Image Manager
  /usr/local/google/home/lantaol/workspace/src/github.com/kubernetes-incubator/cri-tools/pkg/framework/framework.go:51
•SSSSS
------------------------------
[k8s.io] Image Manager 
  public image with tag should be pulled and removed [Conformance]
  /usr/local/google/home/lantaol/workspace/src/github.com/kubernetes-incubator/cri-tools/pkg/validate/image.go:52
[BeforeEach] [k8s.io] Image Manager
  /usr/local/google/home/lantaol/workspace/src/github.com/kubernetes-incubator/cri-tools/pkg/framework/framework.go:50
[BeforeEach] [k8s.io] Image Manager
  /usr/local/google/home/lantaol/workspace/src/github.com/kubernetes-incubator/cri-tools/pkg/validate/image.go:48
[It] public image with tag should be pulled and removed [Conformance]
  /usr/local/google/home/lantaol/workspace/src/github.com/kubernetes-incubator/cri-tools/pkg/validate/image.go:52
STEP: Pull image : busybox:1.26.2
STEP: Check image list to make sure pulling image success : busybox:1.26.2
STEP: Get image list for imageName : busybox:1.26.2
STEP: Remove image : busybox:1.26.2
STEP: Remove image : busybox:1.26.2
STEP: Check image list empty
STEP: Get image list for imageName : busybox:1.26.2
[AfterEach] [k8s.io] Image Manager
  /usr/local/google/home/lantaol/workspace/src/github.com/kubernetes-incubator/cri-tools/pkg/framework/framework.go:51
•SSSSS
------------------------------
[k8s.io] Image Manager 
  public image with digest should be pulled and removed [Conformance]
  /usr/local/google/home/lantaol/workspace/src/github.com/kubernetes-incubator/cri-tools/pkg/validate/image.go:60
[BeforeEach] [k8s.io] Image Manager
  /usr/local/google/home/lantaol/workspace/src/github.com/kubernetes-incubator/cri-tools/pkg/framework/framework.go:50
[BeforeEach] [k8s.io] Image Manager
  /usr/local/google/home/lantaol/workspace/src/github.com/kubernetes-incubator/cri-tools/pkg/validate/image.go:48
[It] public image with digest should be pulled and removed [Conformance]
  /usr/local/google/home/lantaol/workspace/src/github.com/kubernetes-incubator/cri-tools/pkg/validate/image.go:60
STEP: Pull image : busybox@sha256:817a12c32a39bbe394944ba49de563e085f1d3c5266eb8e9723256bc4448680e
STEP: Check image list to make sure pulling image success : busybox@sha256:817a12c32a39bbe394944ba49de563e085f1d3c5266eb8e9723256bc4448680e
STEP: Get image list for imageName : busybox@sha256:817a12c32a39bbe394944ba49de563e085f1d3c5266eb8e9723256bc4448680e
STEP: Remove image : busybox@sha256:817a12c32a39bbe394944ba49de563e085f1d3c5266eb8e9723256bc4448680e
STEP: Remove image : busybox@sha256:817a12c32a39bbe394944ba49de563e085f1d3c5266eb8e9723256bc4448680e
STEP: Check image list empty
STEP: Get image list for imageName : busybox@sha256:817a12c32a39bbe394944ba49de563e085f1d3c5266eb8e9723256bc4448680e
[AfterEach] [k8s.io] Image Manager
  /usr/local/google/home/lantaol/workspace/src/github.com/kubernetes-incubator/cri-tools/pkg/framework/framework.go:51
•
------------------------------
[k8s.io] Image Manager 
  listImage should get exactly 3 image in the result list [Conformance]
  /usr/local/google/home/lantaol/workspace/src/github.com/kubernetes-incubator/cri-tools/pkg/validate/image.go:109
[BeforeEach] [k8s.io] Image Manager
  /usr/local/google/home/lantaol/workspace/src/github.com/kubernetes-incubator/cri-tools/pkg/framework/framework.go:50
[BeforeEach] [k8s.io] Image Manager
  /usr/local/google/home/lantaol/workspace/src/github.com/kubernetes-incubator/cri-tools/pkg/validate/image.go:48
[It] listImage should get exactly 3 image in the result list [Conformance]
  /usr/local/google/home/lantaol/workspace/src/github.com/kubernetes-incubator/cri-tools/pkg/validate/image.go:109
STEP: Pull image : busybox:1-uclibc
STEP: Pull image : busybox:1-musl
STEP: Pull image : busybox:1-glibc
STEP: Remove image : busybox:1-uclibc
STEP: Remove image : busybox:1-musl
STEP: Remove image : busybox:1-glibc
[AfterEach] [k8s.io] Image Manager
  /usr/local/google/home/lantaol/workspace/src/github.com/kubernetes-incubator/cri-tools/pkg/framework/framework.go:51
•SSSSSSSSSS
------------------------------
[k8s.io] Image Manager 
  listImage should get exactly 3 repoTags in the result image [Conformance]
  /usr/local/google/home/lantaol/workspace/src/github.com/kubernetes-incubator/cri-tools/pkg/validate/image.go:146
[BeforeEach] [k8s.io] Image Manager
  /usr/local/google/home/lantaol/workspace/src/github.com/kubernetes-incubator/cri-tools/pkg/framework/framework.go:50
[BeforeEach] [k8s.io] Image Manager
  /usr/local/google/home/lantaol/workspace/src/github.com/kubernetes-incubator/cri-tools/pkg/validate/image.go:48
[It] listImage should get exactly 3 repoTags in the result image [Conformance]
  /usr/local/google/home/lantaol/workspace/src/github.com/kubernetes-incubator/cri-tools/pkg/validate/image.go:146
STEP: Pull image : busybox:1.26.2-uclibc
STEP: Pull image : busybox:1-uclibc
STEP: Pull image : busybox:1
STEP: Remove image : busybox:1.26.2-uclibc
STEP: Remove image : busybox:1-uclibc
STEP: Remove image : busybox:1
[AfterEach] [k8s.io] Image Manager
  /usr/local/google/home/lantaol/workspace/src/github.com/kubernetes-incubator/cri-tools/pkg/framework/framework.go:51
•S
Ran 6 of 36 Specs in 8.229 seconds
SUCCESS! -- 6 Passed | 0 Failed | 0 Pending | 30 Skipped PAS
</details>S

@Random-Liu Random-Liu force-pushed the finish-image-management branch 2 times, most recently from c157d02 to c2a21af Compare May 19, 2017 00:29
@mikebrow
Copy link
Member

Looking good so far!

@Random-Liu
Copy link
Member Author

Slashed out the image filter work. The reasons are:

  1. The api is not defined quite clear;
  2. It's a little bit complex to implement image name match;
  3. Kubelet is not using this feature.

@Random-Liu Random-Liu force-pushed the finish-image-management branch from c2a21af to 258e61d Compare May 22, 2017 03:44
@Random-Liu
Copy link
Member Author

Random-Liu commented May 22, 2017

@mikebrow @yujuhong The PR is ready for review.

@Random-Liu Random-Liu force-pushed the finish-image-management branch from 258e61d to c5d363f Compare May 22, 2017 21:03
@Random-Liu
Copy link
Member Author

/cc @stevvooe Could you help take a look when you have time? Just want to make sure we are using containerd image management api in a right way.

@yujuhong
Copy link
Member

@mikebrow @yujuhong The PR is ready for review.

Will review this tomorrow.

// Size of the image in bytes. Must be > 0.
Size uint64 `json:"size,omitempty"`
// Size is the compressed size of the image.
Size int64 `json:"size,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why changing this to int64?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Containerd returns int64, CRI uses uint64, so either is ok here. :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah that's why I picked uint..

// ImageMetadata is the unversioned image metadata.
type ImageMetadata struct {
// Id of the image. Normally the Digest
// Id of the image. Normally the repo digest.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If RepoDigest is exactly the same as what docker may report, it'd be <registry>/<repository>@<digest>. I think the ID would just be the manifest digest.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, the id is the config digest. I forgot to update the comment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

} {
t.Logf("TestCase %q", ref)
normalized, err := normalizeImageRef(ref)
assert.NoError(t, err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we verify that the normalized string is correct?

Copy link
Member Author

@Random-Liu Random-Liu May 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure whether we want to make assumption about what the image reference is like after normalization. We just want to make sure containerd understand it.

E.g. if in the future the docker package normalize busybox as docker/busbox, it's fine for us as long as containerd understand it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But you do own part of the normalization logic in normalizeImageRef()... The fact that containerd can accept it doesn't mean the reference was normarlized correctly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

assert.NoError(t, err)
normalizedD, err := normalizeImageRef(digested)
assert.NoError(t, err)
assert.Equal(t, normalizedTD.String(), normalizedD.String())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it enough that they are equal? We can also test that the the normalized string uses the digest.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same with above.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above. You don't just call a docker library function to normalize the string, you did a function yourself to perform the task.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

// Id of the image. Normally the repo digest.
ID string `json:"id,omitempty"`
// ChainID is the chainID of the image.
ChainID string `json:"chain_id,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just out of curiosity, what about the digest of the image config?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is the image id. Sorry for the wrong comment.


// getUserFromImageUser gets uid or user name of the image user.
// If user is numeric, it will be treated as uid; or else, it is treated as user name.
func getUserFromImageUser(user string) (*int64, string) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: getUserFromImage seems clear enough to me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copied the function from dockershim. Will do.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

// getUserFromImageUser gets uid or user name of the image user.
// If user is numeric, it will be treated as uid; or else, it is treated as user name.
func getUserFromImageUser(user string) (*int64, string) {
// return both nil if user is not specified in the image.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but you return nil and an empty string below.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

// TODO(mikebrow): remove the image via containerd
err := c.imageMetadataStore.Delete(r.GetImage().GetImage())
return &runtime.RemoveImageResponse{}, err
// TODO(random-liu): Update CRI to pass image reference instead of ImageSpec.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can link my issue if you want: kubernetes/kubernetes#46255

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

return nil, fmt.Errorf("can not resolve %q locally: %v", r.GetImage().GetImage(), err)
}
if imageID == "" {
// return empty without error when image not found.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not returning an error?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use codes.NotFound?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stevvooe We should. I'll file an issue in Kubernetes repo about this. Thanks

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image: &runtime.ImageSpec{Image: testID},
})
assert.NoError(t, err)
assert.NotNil(t, resp)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

require :-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

This was referenced May 24, 2017

// normalizeImageRef normalizes the image reference following the docker convention. This is added
// mainly for backward compatibility.
// The reference returned can only be either tagged or digested. For reference contains both tag
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reference returned can only be either tagged or digested

This statement is untrue: we support digested references with tags, as well.

Copy link
Member Author

@Random-Liu Random-Liu May 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stevvooe The problem is that I don't know where to fit it in our current RepoDigests and RepoTags model...

/cc @yujuhong

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Random-Liu I see. And hence the problem with that model. ;)

I suppose you can split them into a tagged and digested reference (as I think is being done here).

@Random-Liu
Copy link
Member Author

@stevvooe Some comments in the PR are out-of-date. I'll push a newer version first to help you review. And then address other comments.

@Random-Liu Random-Liu force-pushed the finish-image-management branch 5 times, most recently from 7adca5e to 441e356 Compare May 25, 2017 21:00
_, desc, fetcher, err := resolver.Resolve(ctx, ref)
if err != nil {
return desc, size, fmt.Errorf("failed to resolve ref %q: err: %v", ref, err)
return "", "", fmt.Errorf("failed to resolve ref %q: %v", ref, err)
Copy link
Member

@mikebrow mikebrow May 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why change all these is there some rule that all errors in kube must return invalid return nil values for all params?

Copy link
Member Author

@Random-Liu Random-Liu May 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we should return empty value on error usually, unless clearly defined that user could still use the return value on error.

desc)
if err != nil {
return desc, size, fmt.Errorf("failed to fetch %q: desc: %v err: %v", resolvedImageName, desc, err)
return "", "", fmt.Errorf("failed to fetch image %q desc %+v: %v", ref, desc, err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we be consistent and add field names to all such structures in error outputs? or was there a special purpose on this one?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just make the error output consistent. Usually, it's message: error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean using %+v vs %v

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually use %+v for structure, which will include field name. %v doesn't include field name.

Copy link
Member

@mikebrow mikebrow May 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes I understand the usage of + for field names my point was "should we be consistent and add said field names to all such structures in error outputs?" With obvious exceptions for standard structures like "error"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe an issue for error message consistency and add this to it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes I understand the usage of + for field names my point was "should we be consistent and add said field names to all such structures in error outputs?" With obvious exceptions for standard structures like "error"

Yeah, we should. Will file an issue.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. #55

return "", "", fmt.Errorf("unpack failed for manifest layers %v: %v", manifest.Layers, err)
}
size, err = image.Size(ctx, c.contentProvider)
// TODO(random-liu): Considering how to deal with the disk usage of content.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no err check?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which error are you talking about?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the pink line :-) nvm

if err != nil {
return desc, size,
fmt.Errorf("size failed for image:%q %v", image.Target.Digest, err)
return "", "", fmt.Errorf("failed to get config descriptor for image %q: %v", ref, err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed all the others to if var,err = exp; err !=nil { style but not this one.. Is there some rule for style here that we are adhering to?

Copy link
Member Author

@Random-Liu Random-Liu May 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we need configDesc later, so we could not assign it in if clause.

If we don't care about the return value later, we could put them in one line.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's what var's are for :-) ok just checking why / if I'm missing something on these edits.


// insertToStringSlice is a helper function to insert a string into the string slice
// if the string is not in the slice yet.
func insertToStringSlice(ss []string, s string) []string {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we have a place for string helpers?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, this is only used in image_pull. In the future, we can add a separate package for the string helpers, if we need more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah but who would know we have one here? just sayin :-)

}
}()
imageID, err := c.localResolve(ctx, r.GetImage().GetImage())
glog.V(0).Infof("Image ref to remove %q", imageID)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logging between error check and assign may cause confusion if error

Copy link
Member Author

@Random-Liu Random-Liu May 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was for testing. I'll remove it.. Sorry

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@Random-Liu Random-Liu force-pushed the finish-image-management branch from 441e356 to 7364a46 Compare May 26, 2017 06:27
// Resolve the image name; place that in the image store; then dispatch
// a handler for a sequence of handlers which: 1) fetch the object using a
// FetchHandler; and 3) recurse through any sub-layers via a ChildrenHandler
// pullImage pulls image and returns image id (config digest) and digest.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC
s/and digest/and manifest digest

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

// FetchHandler; and 3) recurse through any sub-layers via a ChildrenHandler
// pullImage pulls image and returns image id (config digest) and digest.
// TODO(random-liu): [P0] Wait for all downloadings to be done before return.
func (c *criContainerdService) pullImage(ctx context.Context, ref string) (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clarify what ref is in this context. I believe it should be normalized at the least.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK. It should be normalized image reference.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}
// Currently, the resolved image name is the same with ref in docker resolver,
// but they may be different in the future.
// TODO(random-liu): Always resolve image reference and use resolved image name in
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but you discarded the name returned by resolver.Resolve() and continue using ref in the rest of the code, why?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed containerd/containerd#924 for this

}

image, err := c.imageStoreService.Get(ctx, resolvedImageName)
// Read the image manifest from content store.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the store is sometimes referred as "content store" and sometimes just "image store". Would help to be consistent for all the comments.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The underlying logic does get image manifest from content store. I should probably move the comment down by one line.

err = c.imageStoreService.Put(ctx, resolvedImageName, desc)
if err != nil {
return desc, size, fmt.Errorf("failed to put %q: desc: %v err: %v", resolvedImageName, desc, err)
// Put the image information into containerd image store.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add comments to describe what this does, e.g.,

// In the future, containerd will rely on the information in the image store to perform image garbage collection.
// For now, we simply use it to store and retrieve information required for pulling an image.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


// Unpack the image layers into snapshots.
if _, err = c.rootfsUnpacker.Unpack(ctx, manifest.Layers); err != nil {
return "", "", fmt.Errorf("unpack failed for manifest layers %v: %v", manifest.Layers, err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: %+v for printing manifest.Layers

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

return "", "", fmt.Errorf("failed to get image %q from containerd image store: %v", ref, err)
}
p, err := content.ReadBlob(ctx, c.contentProvider, image.Target.Digest)
digest := image.Target.Digest
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we rename this to manifestDigest to help keeping my sanity? ;)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

named, err := reference.ParseNormalizedNamed(ref)
// TODO(random-liu): [P1] Schema 1 image is not supported in containerd now, we need to support
// it for backward compatiblity.
cfgDigest, digest, err := c.pullImage(ctx, image)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's have consistent naming for these two. Maybe cfgDigest and manifestDigest?
digest itself means very little...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

glog.V(4).Infof("Pulled image %q with image id %q, digest %q", image, imageID, digest)

var repoTag string
if _, ok := namedRef.(reference.NamedTagged); ok {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not make this a helper function too? or you can make getRepoDigestAndTags and return both.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

repoTag = namedRef.String()
}
repoDigest := getRepoDigest(namedRef, digest)
_, err = c.imageMetadataStore.Get(imageID)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option is to store namedRef in the metadata store, and only convert it to repoDigest/repoTags whenever needed. This way, you are not committed to persist RepoDigest and RepoTags for the foreseeable future.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Offline discussed, we won't persist the metadata in the future. And we do use repodigests and repotags in cri-containerd when delete image now.

@Random-Liu
Copy link
Member Author

@yujuhong Addressed comments.

Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Random-Liu Random-Liu force-pushed the finish-image-management branch from a01c86b to 5b23b99 Compare May 26, 2017 23:37
@yujuhong
Copy link
Member

LGTM. I look forward to using the new contanierd client ;)

@Random-Liu
Copy link
Member Author

Apply LGTM based on #41 (comment), #41 (review)

Signed-off-by: Random-Liu <[email protected]>
Signed-off-by: Random-Liu <[email protected]>
@Random-Liu Random-Liu force-pushed the finish-image-management branch from 5b23b99 to 8c1f267 Compare May 26, 2017 23:52
@Random-Liu Random-Liu merged commit bdc443a into containerd:master May 27, 2017
@Random-Liu Random-Liu deleted the finish-image-management branch May 27, 2017 00:04
lanchongyizu pushed a commit to lanchongyizu/cri-containerd that referenced this pull request Sep 3, 2017
adelina-t pushed a commit to adelina-t/cri that referenced this pull request Sep 24, 2019
…vendor

revendor containerd to include shim reconnect fix
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants