OCI layout extensions

[vrothberg]

Taken over from #2567 but:

• Added tests for the three commits
• Use reflinks when adding a file

[vrothberg]

In draft for now.

[mtrmac]

Just a quick skim; I’m afraid I didn’t read the added tests yet.

[vrothberg]

I'd love to have some explicit test condition on Linux to make sure that the reflinking works. We're using the function from c/storage which is e2e tested. The unit tests run on tmp, so we have to create the test dirs in $HOME ... not sure we should though.

[mtrmac]

Actual detailed review still pending, I’m afraid.

[mtrmac]

A full review now.

[mtrmac]

Holding the line at “all of *_transport.go should have test coverage”, please port the two test cases added to TestGetManifestDescriptor from #1677 (and then see if more need to be added).

[vrothberg]

Isn't that already being tested now with fixtures/two_images_manifest ?

[mtrmac]

Tooling says that case ref.image != "" && ref.sourceIndex != -1: and case ref.sourceIndex != -1: is not covered by tests.

[vrothberg]

Ah. These cases cannot be covered by TestGetManifestDescriptor where sourceIndex is always -1 AFAICS.

[vrothberg]

Can case ref.image != "" && ref.sourceIndex != -1: ever be executed by the code at all?

// NewIndexReference returns an OCI reference for a path and a zero-based source manifest index.
func NewIndexReference(dir string, sourceIndex int) (types.ImageReference, error) {
	return newReference(dir, "", sourceIndex)
}

// NewReference returns an OCI reference for a directory and a image.
//
// We do not expose an API supplying the resolvedDir; we could, but recomputing it
// is generally cheap enough that we prefer being confident about the properties of resolvedDir.
func NewReference(dir, image string) (types.ImageReference, error) {
	return newReference(dir, image, -1)
}

[vrothberg]

TestNewIndexReference seems to cover those cases

[mtrmac]

• You’re right, case ref.image != "" && ref.sourceIndex != -1: should be unreachable. We could manually manufacture an invalid value, but that would be rather excessive. Please only add a comment documenting how this code is expected to be unreachable, similar to

  image/docker/daemon/daemon_transport.go

  Line 170 in 75e4f15

  default: // Coverage: Should never happen, NewReference above should refuse such values.

  .
• case ref.sourceIndex != -1: is legitimate and should be reachable. Yes, that does not fit the current loop in TestGetManifestDescriptor — adding code outside of that loop is an option. OCI-archive multi-manifest support POC #1677 contains a smoke test for the success case, please add also a test for an out-of-bounds index.

[vrothberg]

Done ✅

[mtrmac]

I'd love to have some explicit test condition on Linux to make sure that the reflinking works.

…

The unit tests run on tmp, so we have to create the test dirs in $HOME ... not sure we should though.

Tests writing to non-test directories , and possibly leaving them around on aborts/failures, seems rather unexpected to me, some might consider it (or, at least, be worried that it is) hostile. So I’d prefer to leave this to e2e tests, maybe the future tests of the Podman artifact CLI.

(Just because I was looking it up, Google says reflinks don’t work on ext4. That’s not really a blocker, we could gate this behind a filesystem type check.)

[mtrmac]

Thanks! ACK overall.

Ideally, I’d prefer sharing the reflink call with c/storage, but the logic (usage of c/storage/pkg/pools requires c/storage to explicitly special-case CopyFileRange, which is otherwise automatic) is just different enough to make that awkward, and anyway it’s ~10 lines, so not worth worrying about.

[mtrmac]

#2633 (comment) + #2633 (comment) .

[mtrmac]

Thanks!

One last thing, feel free to merge afterwards.

[mtrmac]

Please move that comment to the uncovered getManifestDescriptor (line 222).

We do actually test this line, so it does not need a comment justifying missing test coverage.