storage icon indicating copy to clipboard operation
storage copied to clipboard

RFC: Move the tar-split digest into the TOC

Open mtrmac opened this issue 1 year ago • 1 comments

A minimal prototype for a fix of #1888 .

Note that this would immediately break pulling all currently-existing layers.

Also this probably increases memory requirements because the TOC exists as individual Go values longer.

Cc: @giuseppe

mtrmac avatar Apr 22 '24 22:04 mtrmac

@giuseppe @mtrmac What should we do with this one?

rhatdan avatar May 13 '24 15:05 rhatdan

#1888 must have a solution. This is one possible solution; it also breaks all currently-existing zstd:chunked images, which might be fine or very bad, I don’t know which one it is.

mtrmac avatar May 13 '24 15:05 mtrmac

#1888 must have a solution. This is one possible solution; it also breaks all currently-existing zstd:chunked images, which might be fine or very bad, I don’t know which one it is.

It is fine to break these images now.

The tarsplit data must not be mandatory. If the tarsplit data is missing, we should just skip fetching it (and deal with errors to recreate the original image later)

giuseppe avatar May 14 '24 08:05 giuseppe

With this PR:

  • If there is no tar-split offset, the image is accepted
  • If there is a tar-split offset and the new tar-split digest in the TOC, the image is accepted
  • If there is a tar-split offset and the TOC does not have the new digest, the image is rejected (most recently-created images are like this)

Alternatively, in the last case, we could accept the layer and ignore the tar-split data. If we do that, the images will continue to pull fine; but previously they also pushed fine and that would now break.

Marking ready for review now.

mtrmac avatar May 14 '24 08:05 mtrmac

  • If there is a tar-split offset and the TOC does not have the new digest, the image is rejected (most recently-created images are like this)

could we just drop it from the image generation and always ignore it? So we don't have to worry about it in future

giuseppe avatar May 14 '24 09:05 giuseppe

could we just drop it from the image generation and always ignore it? So we don't have to worry about it in future

I’m not sure which part you mean should be dropped. We need:

  • the offset to be provided somehow. Currently it is an annotation. (It could be in the TOC instead.)
  • a digest, in the TOC. (That’s being added in this PR. The previous digest, in the annotation, is still being added, but it is now ignored).

We can’t drop the tar-split offset. Do you mean to stop adding the TarSplitChecksumKey annotation? Something else?

mtrmac avatar May 14 '24 09:05 mtrmac

We can’t drop the tar-split offset. Do you mean to stop adding the TarSplitChecksumKey annotation? Something else?

yes, to stop doing that and use only the information in the TOC

giuseppe avatar May 14 '24 10:05 giuseppe

Done.

mtrmac avatar May 14 '24 15:05 mtrmac

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, mtrmac

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • ~~OWNERS~~ [giuseppe,mtrmac]

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci[bot] avatar May 15 '24 12:05 openshift-ci[bot]

/lgtm

rhatdan avatar May 16 '24 21:05 rhatdan