c8d: Implement Children by comparing diff ids#45265
Conversation
daemon/containerd/image_children.go
Outdated
| // rootfs of the given image ID, excluding images with exactly the same rootfs. | ||
| // Called from list.go to filter containers. | ||
| func (i *ImageService) Children(id image.ID) []image.ID { | ||
| ctx := context.TODO() |
There was a problem hiding this comment.
Should we add the context as a parameter?
There was a problem hiding this comment.
It would be good to be able to cancel this operation because it can be heavy if there are a lot of images.
However, the interface function is used in other places in the graphdriver image store implementation which would also require context passing, which are not related to this particular implementation.
So I'd leave it for a follow up (handling context in bunch of other graphdriver image store code or refactoring the interface to make it possible to pass context only to this one).
There was a problem hiding this comment.
Hmm actually it might be simpler. Let me try.
There was a problem hiding this comment.
I know for (some of) the graph driver implementations we discussed that we are ok with only adding the context, but "ignoring it" / "not using it" if we can't (so only changing the interface, and add a _ context.Context where applicable)
There was a problem hiding this comment.
Updated - I initially misread Children method from image.Store as the one from ImageService 😄
Lines 18 to 27 in 58c027a
daemon/containerd/image_children.go
Outdated
| return false | ||
| } | ||
|
|
||
| for i := 0; i < len(parent.DiffIDs); i++ { |
There was a problem hiding this comment.
| for i := 0; i < len(parent.DiffIDs); i++ { | |
| for i := 0; i < parentLen; i++ { |
Implement Children method for containerd image store which makes the `ancestor` filter work for `docker ps`. Checking if image is a children of other image is implemented by comparing their rootfs diffids because containerd image store doesn't have a concept of image parentship like the graphdriver store. The child is expected to have more layers than the parent and should start with all parent layers. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
This only makes the containerd ImageService implementation respect context cancellation though. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
1321504 to
e0f36f9
Compare
| func platformRootfs(ctx context.Context, store content.Store, desc ocispec.Descriptor, platform ocispec.Platform) (ocispec.RootFS, error) { | ||
| empty := ocispec.RootFS{} | ||
|
|
||
| log := logrus.WithField("desc", desc.Digest).WithField("platform", platforms.Format(platform)) |
There was a problem hiding this comment.
Probably okay for a follow-up, but I recall Cory had some changes elsewhere for things like this (as multiple WithField() could cause quite some work to be done, even if the logger isn't called?) as this is called for every image, we may want to look if it should be optimised.
At least could use WithFields;
log := logrus.WithFields(logrus.Fields{"desc": desc.Digest, "platform": platforms.Format(platform)})`But maybe we need to consider duplicating the code, and only constructing this when we need it.
| return []image.ID{} | ||
| } | ||
|
|
||
| is := i.client.ImageService() |
There was a problem hiding this comment.
nit; is looks to be only used at line 46, so perhaps we don't need the variable (or if we want to have a variable, move it closer to where it's used)
| logrus.WithError(err).Error("failed to get parent image") | ||
| return []image.ID{} |
There was a problem hiding this comment.
Starting to wonder if we should just (wrap, and) return these errors, and let the caller either log, or handle them.
(can be a follow-up)
Implement Children method for containerd image store which makes the
ancestorfilter work fordocker ps.Checking if image is a children of other image is implemented by comparing their rootfs diffids because containerd image store doesn't have a concept of image parentship like the graphdriver store. The child is expected to have more layers than the parent and should start with all parent layers.
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)