manifest: prepare internal EditInstances

[flouthoc]

Introduces a new private API which is modification of UpdateInstances so it can allow and support more that one operation on exisiting manifest list.

[flouthoc]

@mtrmac I was thinking of introducing a new ListEdit type which can contain instanceOperation. WDYT ?

[mtrmac]

Thanks! We do need a new API, and it’s better to add it private first.

---

This doesn’t deal in any way with the design questions in #1883 (review) ; the API in this PR right now requires the caller to handle ordering (which I think this subpackage should be handling), and eventually we need to think about deletes.

(It’s perfectly fine to not implement deletes now when we don’t need them, not even the enum value needs to be defined now — but it seems worthwhile to make it possible to add that without another large refactoring; and anyway the ordering point suggests a delete-friendly design.

---

Compare #1883 (comment) .)

So, maybe structurally something like

enum ListEdit {
// By default, all existing entries are preserved unmodified, unless .update or .delete instructs otherwise
// .update never changes the relative priority (i.e. position) of an entry.
case .update(oldDigest: Digest, newDigest: Digest?, newSize: Int64?, newMIMEType: String?, …)
// The `List` instance is responsible for adding the new entry with the right priority (position), compared to other entries. The caller just specifies what to add.
case .add(digest: Digest, size: Int64, mimeType: String, …)
// case .delete(digestToDelete: Digest) // unimplemented
}

---

(The comments on OCI apply symmetrically to schema2.)

[mtrmac]

I think we need something like #1896 (review) :

type ListEdit struct {
  Op ListEditOperation

  // if Op == ListEditUpdate (basically the previous UpdateInstances). All fields must be set.
  UpdateOldDigest digest.Digest
  UpdateDigest digest.Digest
  UpdateSize int64
  UpdateMediaType string

  // If Op = ListEditAdd. All fields must be set.
  AddDigest digest.Digest
  AddSize int64
  AddMediaType string
  AddPlatform imgspecv1.Platform
  AddAnnotations map[string]string
}

func (index *…) EditInstaces(edits []ListEdit) error {
  addedEntries := []Descriptor{}
  for _, edit := range edits {
    switch edit.op {
    case ListEditUpdate:
      i := /*index of entry matching UpdateOldDigest in index.Manifests */
      // Existing UpdateInstances code on index.Manifests[i]
    case ListEditAdd:
      addedEntries = append(addedEntries, /* data from edit */)
    }
  }
  // SOMEHOW sort addedEntries, and index.Manifests, to a reasonable resulting list of instances, and update index.Manifests.
}

with the “somehow sort” part being really the hard problem. When adding a Zstd instance to an existing gzip instance, sort the new one after; when adding a gzip instance to an existing Zstd one, sort the new one before. Hypothetically, if we get 3 different compression formats, the pre-existing manifest could already contain 2 of them in the “wrong” order, and I don’t know what we would do when adding the third one.

Maybe the only long-term sustainable thing is to just ignore the pre-existing order of index.Manifests, and whenever we are adding an entry, to re-sort them all (first by platform, just for human readability; then for platform variants - least compatible first?, then for compression variants).

[flouthoc]

@mtrmac One small question ( since I am not sure 😁 ) why is sorting important here ? We are anyways doing the comparison in ChooseInstance while instance is selected from the manifest list https://github.com/containers/image/blob/main/internal/manifest/oci_index.go#L153

[mtrmac]

why is sorting important here

To allow other implementations unaware of Zstd to choose the right instance. (And potentially to similarly deal with other “dimensions” of priority that might emerge in the future.)

[mtrmac]

Thanks, that’s some progress.

There is still some fundamental disconnect about the “update” operation — the caller should only need to edit just the wanted updates, not replacements for everything.

Note also the part about not sorting when no updates happen.

---

The schema2 comments apply to OCI similarly.

[mtrmac]

Thanks, that’s some progress.

There is still some fundamental disconnect about the “update” operation — the caller should only need to edit just the wanted updates, not replacements for everything.

Note also the part about not sorting when no updates happen.

---

The schema2 comments apply to OCI similarly.

[mtrmac]

(nice work on the recent changes.)

[flouthoc]

@mtrmac PTAL I made some changes, once it looks good I'll start working on adding some tests.

[mtrmac]

There are several earlier unaddressed review comments.

[mtrmac]

Collecting the earlier review comments:

• ListEdit with modifications irrelevant to matching should not sort
• Then make UpdateInstances a wrapper over EditInstances
• (x/exp/slices.SortStableFunc would be a tiny bit more concise than sort.SliceStable)

[flouthoc]

Updated

(x/exp/slices.SortStableFunc would be a tiny bit more concise than sort.SliceStable)

I just avoided this part cause x/exp mentioned that all functions are experimental/deprecated and they can be removed anytime.

[mtrmac]

Thanks, looks pretty good!

Some tests — basic smoke tests, and something for the “no changes do not sort” aspect and for Zstd prioritization, would be nice.

[flouthoc]

Added a test with zstdpriority check for OCI1Index.

[mtrmac]

The schema2 part is incorrect now.

[mtrmac]

Just assert.Equal(t, …, list.Instances(), …), checking all entries?
And potentially make a full comparison of each list.Instance() to make sure the update breaks nothing else, though that might be too verbose.

[mtrmac]

(Non-blocking, throughout: the parameters to assert.Equal are ordered (expected, actual), not (actual, expected). That only really matters if the test fails…)

[mtrmac]

Test that all of list.Instances() has the expected number, and order, of entries, please.

Possibly even that the data is all preserved/set as expected — that might be a bit too verbose.

[mtrmac]

(x/exp/slices.SortStableFunc would be a tiny bit more concise than sort.SliceStable)

I just avoided this part cause x/exp mentioned that all functions are experimental/deprecated and they can be removed anytime.

See https://tip.golang.org/doc/go1.21 / https://pkg.go.dev/slices@master — it is coming in Go 1.21.

*shrug*, I can update that afterwards.

[mtrmac]

Thanks!

Feature implementation LGTM; one last round of test cleanups, please. (Keeping the two tests similar, at least where they check for similar things, would be nice.)

[mtrmac]

(Absolutely non-blocking: Naming this old is a bit imprecise.)

[flouthoc]

Done.

[mtrmac]

I meant that “old” was getting updated with the after-edit state; at that point it becomes imprecise. The new “original” name does not change that part.

[flouthoc]

@mtrmac PTAL again

[mtrmac]

Thanks!

[flouthoc]

Yay !!!!