storage icon indicating copy to clipboard operation
storage copied to clipboard

`pull_options.convert_images` does not guarantee chunked/composefs usage

Open mtrmac opened this issue 1 year ago • 1 comments

#2115 + #2118 worked to give the c/storage code the option, if convert_images is set and a partial pull (or creation of a composefs-based layer) is not possible, to fail GetDiffer in a way which terminates the c/image pull process entirely, disabling the fallback to non-chunked pulls.

https://github.com/containers/storage/pull/2118#issuecomment-2385165798 discussed that, but, to be explicit and to list the various conditions:

  • If some variant of the layer exists locally, PutBlobPartial/GetDiffer will not be called; instead, the existing layer will be directly reused (probably fine), or (if the parent or ID differs) exported to a tar file and reimported the traditional way, without invoking GetDiffer
  • If the source of the pull is a transport which just doesn’t support partial pulls (i.e. anything but a registry), PutBlobPartial will not be called; the traditional path will be used
  • If the pull operation also includes a conversion from schema1 to a different format (not during ordinary podman pull but possible with Skopeo), PutBlobPartial will not be called; the traditional path will be used
  • If the pull operation is decrypting an encrypted image (or, uh, encrypting one, in theory), PutBlobPartial will not be called; the traditional path will be used
  • FUTURE: If the source image is schema1, PutBlobPartial will trigger a fallback without calling GetDiffer

So, enforcing the chunked-only code path would require one of:

  • Some kind of more explicit coordination with c/image, telling it to not attempt any of the other code paths (… and reimplementing the affected features some other way???)
  • Also enforcing convert_images around layerStore.applyDiffWithOptions, triggering a failure (… and losing the affected features entirely; unacceptable)
  • Refactoring the code so that layerStore.applyDiffWithOptions can also fully implement the convert_images code path (probably best overall)
  • (Independently), possibly special-casing the “reuse of a local layer with a different ID/parent” situation in c/storage, so that this happens in some more efficient way.

Again, #2115/#2118 were clear enough that fully enforcing the conversions had not yet been a goal at this time; I’m filing this issue

  • to be fairly explicit about that situation
  • to have a place to discuss the tradeoffs or implementation approaches
  • because the current zstd:chunked layer ID work means that we should not even attempt to use partial pulls for schema1 images (because they don’t commit to a DiffID value, the only way to avoid a layer view ambiguity is to only ever use one view); and I wanted to have an issue to point to for the design discussion of that.

Cc/RFC: @giuseppe

mtrmac avatar Nov 26 '24 19:11 mtrmac