Repository: moby/moby
File: daemon/containerd/image_manifest.go (Go)

Code before:
			size += desc.Size
			return nil, nil
		})),
		m.Target())
	return size, err
}

// SnapshotUsage returns the disk usage of the image's snapshots.
func (m *ImageManifest) SnapshotUsage(ctx context.Context, snapshotter snapshots.Snapshotter) (snapshots.Usage, error) {
	diffIDs, err := m.RootFS(ctx)
	if err != nil {
		return snapshots.Usage{}, errors.Wrapf(err, "failed to get rootfs of image %s", m.Name())
	}

	imageSnapshotID := identity.ChainID(diffIDs).String()
	unpackedUsage, err := calculateSnapshotTotalUsage(ctx, snapshotter, imageSnapshotID)
	if err != nil {
		if !cerrdefs.IsNotFound(err) {
			log.G(ctx).WithError(err).WithFields(log.Fields{
				"image":      m.Name(),
				"snapshotID": imageSnapshotID,
			}).Warn("failed to calculate unpacked size of image")
		}
		unpackedUsage = snapshots.Usage{Size: 0}
	}
	return unpackedUsage, nil
}


Review comment (bug):
Looking if our error-handling looks OK;

- The start of the function returns an error on failure;
- But here we do a "log and ignore"

Looking at call-sites of this function;

In `ImageService.multiPlatformSummary` we log the error and return `nil` (does that terminate the Walk?), so can probably just return the error (if it's not a "not found"?)

```go
snapshotUsage, err := img.SnapshotUsage(ctx, i.snapshotterService(i.snapshotter))
if err != nil {
    logger.WithError(err).Warn("failed to determine platform specific size")
    return nil
}
```

Interestingly, in `ImageService.singlePlatformImage` we return the error;

```go
snapshotUsage, err := imageManifest.SnapshotUsage(ctx, i.snapshotterService(i.snapshotter))
if err != nil {
	return nil, errors.Wrapf(err, "failed to calculate size of image %s", imageManifest.Name())
}
```

:point_up: wondering if that's _correct_ - i.e., should we fail everything when failing to calculate the size, or should we use an empty `snapshotUsage`, but return the rest of the information?

I somewhat think that in either case, the error could be returned here, if it's _not_ a "not found" error; "not found" means "no size" (so not a failure), and other errors returned to the caller, which can decide what to do.


Considering something like;

```suggestion
	if err != nil {
		if cerrdefs.IsNotFound(err) {
			// No content found, which is not an error; empty size.
			return snapshots.Usage{}, nil
		}

		log.G(ctx).WithError(err).WithFields(log.Fields{
			"image":      m.Name(),
			"snapshotID": imageSnapshotID,
		}).Warn("failed to calculate unpacked size of image")

		return snapshots.Usage{}, errors.Wrapf(err, "failed to calculate unpacked size of image %s", m.Name())
	}
```



Code after:
		return nil, nil
	}), m.Target())
	return size, err
}

// SnapshotUsage returns the disk usage of the image's snapshots.
func (m *ImageManifest) SnapshotUsage(ctx context.Context, snapshotter snapshots.Snapshotter) (snapshots.Usage, error) {
	diffIDs, err := m.RootFS(ctx)
	if err != nil {
		return snapshots.Usage{}, errors.Wrapf(err, "failed to get rootfs of image %s", m.Name())
	}

	imageSnapshotID := identity.ChainID(diffIDs).String()
	unpackedUsage, err := calculateSnapshotTotalUsage(ctx, snapshotter, imageSnapshotID)
	if err != nil {
		if cerrdefs.IsNotFound(err) {
			return snapshots.Usage{Size: 0}, nil
		}
		log.G(ctx).WithError(err).WithFields(log.Fields{
			"image":      m.Name(),
			"target":     m.Target(),
			"snapshotID": imageSnapshotID,
		}).Warn("failed to calculate snapshot usage of image")

		return snapshots.Usage{}, errors.Wrapf(err, "failed to calculate snapshot usage of image %s", m.Name())
	}
	return unpackedUsage, nil
}