Refactor NewImageSource to add a manifest type abstraction #5743

aaronlehmann · 2024-09-17T23:28:58Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Currently, NewImageSource creates a Docker schema2 manifest and an OCI manifest at the same time. This precludes functionality that isn't supported by both manifest types, for example zstd compression. Refactoring this to create only the desired manifest type solves this and also cleans up the code by separating manifest-type-specific code into distinct implementations of a "manifest builder".

How to verify it

Existing tests should suffice since this does not change any semantics, it only cleans up the implementation.

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

See discussion in #5452.

Does this PR introduce a user-facing change?

None

aaronlehmann · 2024-09-19T14:39:20Z

@mtrmac: PTAL - thanks!

aaronlehmann · 2024-09-24T17:02:57Z

@rhatdan @mtrmac: Would it be possible to get some eyes on this when you have a moment? I was asked to do this to unblock #5452, so I would really appreciate if we could avoid letting it sit too long.

aaronlehmann · 2024-10-08T04:12:31Z

Any possibility of moving this forward? It's a bit frustrating that I was asked to come back and do this refactor, but the PR is not getting reviewed.

TomSweeneyRedHat · 2024-11-01T22:11:16Z

@mtrmac have you had a chance to look at this?

TomSweeneyRedHat · 2024-11-01T22:12:20Z

@aaronlehmann sorry for the delay. Many of the maintainers are Red Hat based, and we've all been under a big crunch the past few weeks.

mtrmac · 2024-11-04T16:19:49Z

@Honny1 Do you happen to have the time to take a look?

github-actions · 2024-12-05T00:14:26Z

A friendly reminder that this PR had no activity for 30 days.

aaronlehmann · 2025-02-27T13:37:54Z

@TomSweeneyRedHat @mtrmac @Honny1: Would any of you be able to look at this? I've been trying to get this through for a very long time.

mtrmac · 2025-02-27T16:20:36Z

@flouthoc do you happen to have time for this?

flouthoc · 2025-02-28T18:13:26Z

I will review this thanks.

flouthoc · 2025-02-28T18:16:41Z

@aaronlehmann Could you please rebase this first, just to be sure if every thing is good.

aaronlehmann · 2025-02-28T21:24:45Z

Rebased.

aaronlehmann · 2025-03-10T17:20:22Z

Hi @flouthoc, have you had a chance to look at this yet?

flouthoc

There is some duplication but PR LGTM. Could you please rebase again.

aaronlehmann · 2025-03-18T17:47:40Z

Rebased again.

aaronlehmann · 2025-03-19T16:03:06Z

@flouthoc: Hoping I can get this merged before it needs yet another rebase 😄

flouthoc

LGTM @nalind any comments ?

openshift-ci · 2025-03-19T16:47:33Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: aaronlehmann, flouthoc
Once this PR has been reviewed and has the lgtm label, please assign rhatdan for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

nalind · 2025-03-19T18:30:52Z

image.go

+
+type dockerSchema2ManifestBuilder struct {
+	i         *containerImageRef
+	mediaType string


layerMediaType, please.

nalind · 2025-03-19T18:30:53Z

image.go

-// without making unintended changes to the original Builder.
-func (i *containerImageRef) createConfigsAndManifests() (v1.Image, v1.Manifest, docker.V2Image, docker.V2S2Manifest, error) {
+// without making unintended changes to the original Builder (Docker schema 2).
+func newDockerSchema2ManifestBuilder(i *containerImageRef) (manifestBuilder, error) {


This could just as well be a method of containerImageRef.

nalind · 2025-03-19T18:30:57Z

image.go

+}
+
+func (mb *dockerSchema2ManifestBuilder) buildHistory(extraImageContentDiff string, extraImageContentDiffDigest digest.Digest) error {
+	// Build history notes in the image configurations.


Only one configuration, this comment needs an update.

nalind · 2025-03-19T18:31:01Z

image.go

+	// Build history notes in the image configurations.
+	appendHistory := func(history []v1.History, empty bool) {
+		for i := range history {
+			var created *time.Time


This would be slightly shorter if created wasn't a pointer here.

nalind · 2025-03-19T18:31:06Z

image.go

+
+type ociManifestBuilder struct {
+	i         *containerImageRef
+	mediaType string


layerMediaType, please.

nalind · 2025-03-19T18:31:09Z

image.go

+
+// Build fresh copies of the container configuration structures so that we can edit them
+// without making unintended changes to the original Builder (OCI manifest).
+func newOCIManifestBuilder(i *containerImageRef) (manifestBuilder, error) {


This could just as well be a method of containerImageRef.

nalind · 2025-03-19T18:31:15Z

image.go

+		oimage.Config.ExposedPorts = nil
+	}
+
+	// Build empty manifest.  The Layers lists will be populated later.


This comment needs an update.

nalind · 2025-03-19T18:31:23Z

image.go

+		case archive.Zstd:
+			// Until the image specs define a media type for zstd-compressed layers, even if we know
+			// how to decompress them, we can't try to compress layers with zstd.
+			return errors.New("media type for zstd-compressed layers is not defined")


This could set omediaType to v1.MediaTypeImageLayerZstd instead of returning an error.

In #5452 I was asked not to support zstd in Docker manifests because the spec doesn't officially support it. That was the impetus for this refactor.

Oh, sorry, this is for the OCI manifest. The idea was to make this change in #5452 once this PR is merged, but I could unify the two PRs if you prefer.

nalind · 2025-03-19T18:31:26Z

image.go

-		return nil, fmt.Errorf("no supported manifest types (attempted to use %q, only know %q and %q)",
-			manifestType, v1.MediaTypeImageManifest, manifest.DockerV2Schema2MediaType)
+func (mb *ociManifestBuilder) buildHistory(extraImageContentDiff string, extraImageContentDiffDigest digest.Digest) error {
+	// Build history notes in the image configurations.


Only one configuration, this comment needs an update.

nalind · 2025-03-19T18:31:50Z

image.go

@@ -720,187 +1027,19 @@ func (i *containerImageRef) NewImageSource(_ context.Context, _ *types.SystemCon
 		}
 		// Add a note in the manifest about the layer.  The blobs are identified by their possibly-
 		// compressed blob digests.


This looks like a comment that addLayer() should have in it, or as part of its godoc.

nalind · 2025-03-19T18:31:59Z

image.go

-		MediaType: v1.MediaTypeImageManifest,
-		Config: v1.Descriptor{
-			MediaType: v1.MediaTypeImageConfig,
+	// Build empty manifest.  The Layers lists will be populated later.


This comment needs an update.

aaronlehmann · 2025-03-19T19:16:58Z

@nalind: Thanks for the review - updated based on your feedback.

aaronlehmann · 2025-03-21T21:40:19Z

@nalind @flouthoc: Any further comments?

aaronlehmann · 2025-03-25T17:13:33Z

@nalind @flouthoc: Can we please move ahead with merging this? Thanks!

flouthoc · 2025-03-25T21:00:11Z

PR LGTM I am waiting for @nalind to ack on his last remarks.

aaronlehmann · 2025-03-31T15:34:57Z

@nalind: Sorry to be so persistent here, but I'd really like to get this merged, since it has been dragging on for so long, and it's hard to keep coming back to this after long pauses.

Currently, NewImageSource creates a Docker schema2 manifest and an OCI manifest at the same time. This precludes functionality that isn't supported by both manifest types, for example zstd compression. Refactoring this to create only the desired manifest type solves this and also cleans up the code by separating manifest-type-specific code into distinct implementations of a "manifest builder". See discussion in containers#5452. Signed-off-by: Aaron Lehmann <[email protected]>

Signed-off-by: Aaron Lehmann <[email protected]>

openshift-ci bot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Sep 17, 2024

aaronlehmann mentioned this pull request Sep 17, 2024

Support zstd compression in image commit #5452

Open

aaronlehmann force-pushed the refactor-manifest-types branch 2 times, most recently from 9f1c553 to 81fea1d Compare September 17, 2024 23:39

github-actions bot added the stale-pr label Dec 5, 2024

github-actions bot removed the stale-pr label Feb 28, 2025

flouthoc self-assigned this Feb 28, 2025

aaronlehmann force-pushed the refactor-manifest-types branch from 81fea1d to b962939 Compare February 28, 2025 21:24

flouthoc added the No New Tests Allow PR to proceed without adding regression tests label Feb 28, 2025

flouthoc reviewed Mar 18, 2025

View reviewed changes

aaronlehmann force-pushed the refactor-manifest-types branch from b962939 to 72ea5a8 Compare March 18, 2025 17:47

flouthoc approved these changes Mar 19, 2025

View reviewed changes

nalind reviewed Mar 19, 2025

View reviewed changes

image.go Outdated

type dockerSchema2ManifestBuilder struct {

i *containerImageRef

mediaType string

Copy link

Member

nalind Mar 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

layerMediaType, please.

nalind reviewed Mar 19, 2025

View reviewed changes

image.go Outdated

type ociManifestBuilder struct {

i *containerImageRef

mediaType string

Copy link

Member

nalind Mar 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

layerMediaType, please.

nalind reviewed Mar 19, 2025

View reviewed changes

aaronlehmann force-pushed the refactor-manifest-types branch from 5686f79 to 7fbdf26 Compare March 19, 2025 19:48

aaronlehmann added 2 commits March 31, 2025 15:37

Review feedback

8008e9c

Signed-off-by: Aaron Lehmann <[email protected]>

aaronlehmann force-pushed the refactor-manifest-types branch from 7fbdf26 to 8008e9c Compare March 31, 2025 15:37

Refactor NewImageSource to add a manifest type abstraction #5743

Are you sure you want to change the base?

Refactor NewImageSource to add a manifest type abstraction #5743

Conversation

aaronlehmann commented Sep 17, 2024

What type of PR is this?

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

aaronlehmann commented Sep 19, 2024

aaronlehmann commented Sep 24, 2024

aaronlehmann commented Oct 8, 2024

TomSweeneyRedHat commented Nov 1, 2024

TomSweeneyRedHat commented Nov 1, 2024

mtrmac commented Nov 4, 2024

github-actions bot commented Dec 5, 2024

aaronlehmann commented Feb 27, 2025

mtrmac commented Feb 27, 2025

flouthoc commented Feb 28, 2025

flouthoc commented Feb 28, 2025

aaronlehmann commented Feb 28, 2025

aaronlehmann commented Mar 10, 2025

flouthoc left a comment

Choose a reason for hiding this comment

aaronlehmann commented Mar 18, 2025

aaronlehmann commented Mar 19, 2025

flouthoc left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Mar 19, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronlehmann commented Mar 19, 2025

aaronlehmann commented Mar 21, 2025

aaronlehmann commented Mar 25, 2025

flouthoc commented Mar 25, 2025

aaronlehmann commented Mar 31, 2025