Conversation
| c, err := i.client.ContainerService().Get(ctx, containerID) | ||
| // Get() returns an error if the container is not found, we return 0, 0 in this case and | ||
| // not an error because this will happen when the container exited. | ||
| if err != nil { |
There was a problem hiding this comment.
Do we need to check if it's a errdefs.IsNotFound(err), to prevent we're not ignoring other errors ? https://github.com/containerd/containerd/blob/bbe46b8c43fc2febe316775bc2d4b9d697bbf05c/errdefs/errors.go#L56-L59
There was a problem hiding this comment.
I guess we could ignore the error; perhaps for errors that are not "not found", we could log them; not sure if either is best handled in this function or by the caller though (haven't looked closely where it's all used)
There was a problem hiding this comment.
Looks like the error is not a errdefs.IsNotFound :/
Output from the cli:
Error response from daemon: failed to retrieve container list: container "d4ee0fe826d4043c1b8eb619d959f43ec6bd01c2440bf6a0b1f8d39d12233f34" in namespace "moby": not found
Daemon logs:
DEBU[2022-08-08T14:33:29.885930020+02:00] FIXME: Got an API for which error does not match any expected type!!!: failed to retrieve container list: container "d4ee0fe826d4043c1b8eb619d959f43ec6bd01c2440bf6a0b1f8d39d12233f34" in namespace "moby": not found error_type="*errors.errorString" module=api
The documentation of the Get method says:
// Container object is returned on success. If the id is not known to the
// store, an error will be returned.
There was a problem hiding this comment.
Did you use the containerd errdefs? (not by accident the moby errdefs?)
There was a problem hiding this comment.
OK nevermind, I imported the wrong package...
There was a problem hiding this comment.
But yeah, if it's the only error it can return, it's probably fine (for now); it's a bit scary though to ignore "any" error (in case things change in containerd)
There was a problem hiding this comment.
Did you use the containerd errdefs? (not by accident the moby errdefs?)
This is exactly what happened :D
There was a problem hiding this comment.
I got bitten by the errdefs a few times too. Maybe we could we have a soft convention to import the containerd errdefs as containerderrdefs/cerrdefs/c8derrdefs?
| c, err := i.client.ContainerService().Get(ctx, containerID) | ||
| // Get() returns an error if the container is not found, we return 0, 0 in this case and | ||
| // not an error because this will happen when the container exited. | ||
| if err != nil { |
There was a problem hiding this comment.
I got bitten by the errdefs a few times too. Maybe we could we have a soft convention to import the containerd errdefs as containerderrdefs/cerrdefs/c8derrdefs?
This change fixes multiple things: * pass ctx instead of nil because we need it while computing the layer size * don't return an error if the container is not found during the layer size calculation, if a container is not found it means that it exited * create the container with the image it was created from, the image is used when computing the layer size Signed-off-by: Djordje Lukic <[email protected]>
daemon/containerd/service.go
Outdated
| "fmt" | ||
|
|
||
| "github.com/containerd/containerd" | ||
| "github.com/containerd/containerd/errdefs" |
There was a problem hiding this comment.
Perhaps use an alias for this one to prevent that. Looks like we've not been consistent on "what" alias to use in upstream (something we should fix); here's the existing list of aliases "pick one" and we should then update upstream to use the same everywhere :)
And in BuildKit;
There was a problem hiding this comment.
What a mess, I'll update to cerrdefs
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
LGTM
(left two comments for consideration)
| containertypes "github.com/docker/docker/api/types/container" | ||
| "github.com/docker/docker/container" | ||
| "github.com/docker/docker/errdefs" | ||
| v1 "github.com/opencontainers/image-spec/specs-go/v1" |
There was a problem hiding this comment.
I think in most places, this is currently aliased as "specs"
| if daemon.UsesSnapshotter() { | ||
| newContainerOpts = append(newContainerOpts, containerd.WithSnapshotter(containerd.DefaultSnapshotter)) | ||
| newContainerOpts = append(newContainerOpts, containerd.WithSnapshot(container.ID)) | ||
| c8dImge, err := daemon.imageService.(containerdImage).GetContainerdImage(ctx, container.Config.Image, &v1.Platform{}) |
There was a problem hiding this comment.
Arf, I should've looked at the whole diff instead of leaving one comment at a time 🙈❤️
I'd use either "c8dImage" or "c8dImg" (the "e" at the end now looks a bit odd)
|
This one should be upstreamed together with #19 |
4 participants
This change fixes multiple things:
size
size calculation, if a container is not found it means that it exited
used when computing the layer size