pull,load: use `*Image` instead of re-resolving via `name` #2339

flouthoc · 2025-03-01T17:29:26Z

Following commit fixes a race condition in libimage because in Pull( after performing copy from remote sources it agains attempts to resolve image via LookupImage, any operation between copy and LookupImage can remove name from the recently pulled image. Causing race in builds.

Removing unnecessary image -> name -> image round-trip from Pull(

This issue was discoverd while working on PR containers/buildah#5971

buildah build -t test --jobs=2 --skip-unused-stages=false .

Containerfile

FROM quay.io/jitesoft/alpine
RUN arch
FROM --platform=linux/arm64 quay.io/jitesoft/alpine AS foreign

Following commit also addresses the commit e2324dd by performing the neccessary refactor.

No functional change in public exposed API, exisiting tests should pass as-is. [NO NEW TESTS NEEDED]

flouthoc · 2025-03-03T05:56:26Z

I don't know why after image.reload( here https://github.com/containers/common/blob/main/libimage/image_test.go#L53 the digest gets changed although I never changed anything in storage. @mtrmac any hints ?

mtrmac · 2025-03-03T17:28:10Z

I didn’t trace it, for now a quick guess: https://github.com/containers/image/blob/c3293eccad1afb55fb9b45d674c8cd06f605eac9/storage/storage_reference.go#L173-L188 means that when using a digested c/storage reference, the reference refers to the manifest with that digest.

So, for a digested pull, the resolved reference is a name@digest@ID reference, and always uses the manifest with referenced digest. But Image.reload replaces the storage reference with an @ID-only reference, in which case the “default” manifest selection applies, and that might differ.

Let me know if this does not help and I should look into this in more detail.

flouthoc · 2025-03-03T17:36:26Z

I didn’t trace it, for now a quick guess: https://github.com/containers/image/blob/c3293eccad1afb55fb9b45d674c8cd06f605eac9/storage/storage_reference.go#L173-L188 means that when using a digested c/storage reference, the reference refers to the manifest with that digest.

So, for a digested pull, the resolved reference is a name@digest@ID reference, and always uses the manifest with referenced digest. But Image.reload replaces the storage reference with an @ID-only reference, in which case the “default” manifest selection applies, and that might differ.

Let me know if this does not help and I should look into this in more detail.

This is very helpful, I will investigate with above notes and will report back 😄 , thanks for the tip.

mtrmac · 2025-03-04T20:12:10Z

But Image.reload replaces the storage reference with an @ID-only reference,

That’s imprecise I’m afraid; Image.storageReference is not modified — but Image.storageImage is, to an image coming directly from c/storage and not from storageReference.resolveImage. But I think the core of the idea, that resolveImage modifies Image.Digest to match the user input, remains.

libimage/pull.go

flouthoc · 2025-03-04T21:22:52Z

@mtrmac Tests passes now, this is ready for review. Although I must created vendor PR in podman and buildah to verify everything else.

mtrmac

Just an extremely brief skim for now, without reading any of the context; mostly just a list of topics to discuss / for me to research.

I very much like minimizing repeated image lookups. The overall direction is great.
I’m not at all sure about the .reload() part now. Do we know what is exactly happening?
I don’t know about the non-registry copies, e.g. loads from archives. I would be fine with not fixing those cases in this PR and deferring that for later… OTOH it might be easier to do all now than to re-establish the context later.

libimage/load.go

libimage/pull.go

libimage/load.go

flouthoc · 2025-03-06T22:24:43Z

Okay buildah tests all pass containers/buildah#6039, podman test is having an issue once I fix the podman tests I will address comments on this PR

packit-as-a-service · 2025-03-14T03:17:08Z

Ephemeral COPR build failed. @containers/packit-build please check.

flouthoc · 2025-03-17T18:29:11Z

@mtrmac Could you review the PR again ? I have modfied tests a little bit and also removed reload( from my code.

Luap99 · 2025-03-17T18:32:05Z

libimage/image_test.go

+	digestFound := false
+	for _, d := range digests {
+		if d.String() == origDigest.String() {
+			digestFound = true
+			break
+		}
+	}
+	require.True(t, digestFound, "original digest string should be present in the digests array of new pulled image")


That results in a bad error message, the proper matcher for this is require.Contains()

I want to match .String() part of the object. I wonder if require.Contains would allow to me to match only .String() parts of it.

digest is defined as type Digest string so there doesn't not seem to be a reason why you would need to match .String() as this does just a type cast to string

func (d Digest) String() string { return string(d) }

(I’m academically interested in learning a good answer, but, pragmatically, I’m not worried. If the test ever fails, someone will add a few debugging prints.)

(I’m academically interested in learning a good answer, but, pragmatically, I’m not worried. If the test ever fails, someone will add a few debugging prints.)

The first time this fails you would be greeted with a rather useless "true should equal false" message. Then in order to debug you would have to to extra work adding these prints which seems unnecessary. If the failure doesn't reproduce locally and/or is a flake then you must commit first all the debug info to CI so we can gather more useful logs which again is extra work.

One thing we should always assume is that the person who wrote the test is not the same person who will see this fail so they don't know what the test is doing, or even the same person will not remember right away and then you have to dig in the sources, etc... In the best case the error is good enough for another person to see what they broke without having to fully read and understand the entire test code. Or consider a QE person like Ed looking at flakes. They will not necessarily dig up the sources or understand them enough to then file a meaningful issue. If they just copy paste the test error it would be much more meaningful if it includes the real digests.

Logically for this this test there seem to be a few rather different cases I could see right away with the better error. The digest list is empty so the element cannot be part of it, list is not empty but only has different digests and similar maybe the digest algorithm was changed but the test still expects a sha256 one.

Sure here in c/common with these unit tests this is less of a concern compared to podman integration tests but still we should apply the same good habits everywhere.

mtrmac

The logic LGTM, just some cleanups.

libimage/pull.go

libimage/copier.go

libimage/pull.go

mtrmac

Non-blocking: As discussed elsewhere, consider removing the report… parameter from NewCopier.

Great work!

libimage/copier.go

mtrmac

(This doesn’t build, but otherwise) LGTM.

Following commit fixes a `race` condition in `libimage` because in `Pull(` after performing `copy` from remote sources it agains attempts to resolve image via `LookupImage`, any operation between `copy` and `LookupImage` can remove `name` from the recently pulled image. Causing race in builds. This issue was discoverd while working on PR containers/buildah#5971 ``` buildah build -t test --jobs=2 --skip-unused-stages=false . ``` Containerfile ``` FROM quay.io/jitesoft/alpine RUN arch FROM --platform=linux/arm64 quay.io/jitesoft/alpine AS foreign ``` Following commit also addresses the commit containers@e2324dd by performing the neccessary refactor. No functional change in public exposed API, exisiting tests should pass as-is. [NO NEW TESTS NEEDED] Signed-off-by: flouthoc <[email protected]>

Fix linter error ``` Error: libimage/copier.go:180:51: `(*Runtime).newCopier` - `reportResolvedReference` always receives `nil` (unparam) ``` Signed-off-by: flouthoc <[email protected]>

flouthoc · 2025-03-21T14:17:35Z

@containers/podman-maintainers @nalind PTAL

flouthoc · 2025-03-21T15:42:33Z

Both buildah and podman CI passes with this patch.

Luap99

/lgtm

openshift-ci · 2025-03-21T17:13:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc, Luap99

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~~ [Luap99]

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

flouthoc force-pushed the refactor-pull branch 2 times, most recently from 4f50746 to 663a29b Compare March 1, 2025 17:35

flouthoc force-pushed the refactor-pull branch from 663a29b to 4152934 Compare March 4, 2025 18:11

flouthoc force-pushed the refactor-pull branch from 4152934 to 9bbe085 Compare March 4, 2025 20:22

mtrmac reviewed Mar 4, 2025

View reviewed changes

libimage/pull.go Outdated Show resolved Hide resolved

flouthoc force-pushed the refactor-pull branch 2 times, most recently from 37b8f64 to d585e3d Compare March 4, 2025 20:38

flouthoc requested a review from mtrmac March 4, 2025 21:22

mtrmac reviewed Mar 4, 2025

View reviewed changes

flouthoc mentioned this pull request Mar 4, 2025

vendor: bump c/common to dbeb17e40c80 containers/podman#25468

Merged

flouthoc force-pushed the refactor-pull branch from d585e3d to c5198a2 Compare March 14, 2025 03:15

flouthoc force-pushed the refactor-pull branch 5 times, most recently from da19354 to f1f8f60 Compare March 17, 2025 18:05

flouthoc requested a review from mtrmac March 17, 2025 18:28

Luap99 reviewed Mar 17, 2025

View reviewed changes

flouthoc requested a review from Luap99 March 17, 2025 19:15

flouthoc force-pushed the refactor-pull branch 2 times, most recently from 82c2b5e to 706bcf2 Compare March 18, 2025 14:02

mtrmac reviewed Mar 19, 2025

View reviewed changes

flouthoc force-pushed the refactor-pull branch 2 times, most recently from 280ee10 to 20e2caa Compare March 20, 2025 19:05

mtrmac reviewed Mar 20, 2025

View reviewed changes

libimage/copier.go Outdated Show resolved Hide resolved

libimage/copier.go Outdated Show resolved Hide resolved

libimage/pull.go Outdated Show resolved Hide resolved

libimage/pull.go Outdated Show resolved Hide resolved

libimage/pull.go Show resolved Hide resolved

flouthoc force-pushed the refactor-pull branch from 20e2caa to 77aba90 Compare March 20, 2025 19:46

mtrmac reviewed Mar 20, 2025

View reviewed changes

libimage/copier.go Outdated Show resolved Hide resolved

flouthoc force-pushed the refactor-pull branch from 646e404 to 21344a8 Compare March 20, 2025 20:30

mtrmac reviewed Mar 20, 2025

View reviewed changes

flouthoc added 2 commits March 20, 2025 13:37

copier: modify newCopier for golangci-lint

569f6de

Fix linter error ``` Error: libimage/copier.go:180:51: `(*Runtime).newCopier` - `reportResolvedReference` always receives `nil` (unparam) ``` Signed-off-by: flouthoc <[email protected]>

flouthoc force-pushed the refactor-pull branch from 21344a8 to 569f6de Compare March 20, 2025 20:38

Luap99 approved these changes Mar 21, 2025

View reviewed changes

openshift-ci bot assigned Luap99 Mar 21, 2025

openshift-ci bot added the lgtm label Mar 21, 2025

openshift-ci bot added the approved label Mar 21, 2025

openshift-merge-bot bot merged commit dbeb17e into containers:main Mar 21, 2025
15 checks passed

This was referenced Mar 21, 2025

vendor: bump c/common to dbeb17e40c80 containers/buildah#6039

Merged

stage_executor: reset platform in systemcontext for every stage. containers/buildah#5971

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pull,load: use `*Image` instead of re-resolving via `name` #2339

pull,load: use `*Image` instead of re-resolving via `name` #2339

flouthoc commented Mar 1, 2025 •

edited

Loading

flouthoc commented Mar 3, 2025

mtrmac commented Mar 3, 2025

flouthoc commented Mar 3, 2025

mtrmac commented Mar 4, 2025

flouthoc commented Mar 4, 2025

mtrmac left a comment

flouthoc commented Mar 6, 2025

packit-as-a-service bot commented Mar 14, 2025

flouthoc commented Mar 17, 2025

Luap99 Mar 17, 2025

flouthoc Mar 17, 2025

Luap99 Mar 17, 2025

mtrmac Mar 17, 2025

flouthoc Mar 17, 2025

Luap99 Mar 18, 2025

mtrmac left a comment

mtrmac left a comment

mtrmac left a comment

flouthoc commented Mar 21, 2025

flouthoc commented Mar 21, 2025

Luap99 left a comment

openshift-ci bot commented Mar 21, 2025

pull,load: use *Image instead of re-resolving via name #2339

pull,load: use *Image instead of re-resolving via name #2339

Conversation

flouthoc commented Mar 1, 2025 • edited Loading

flouthoc commented Mar 3, 2025

mtrmac commented Mar 3, 2025

flouthoc commented Mar 3, 2025

mtrmac commented Mar 4, 2025

flouthoc commented Mar 4, 2025

mtrmac left a comment

Choose a reason for hiding this comment

flouthoc commented Mar 6, 2025

packit-as-a-service bot commented Mar 14, 2025

flouthoc commented Mar 17, 2025

Luap99 Mar 17, 2025

Choose a reason for hiding this comment

flouthoc Mar 17, 2025

Choose a reason for hiding this comment

Luap99 Mar 17, 2025

Choose a reason for hiding this comment

mtrmac Mar 17, 2025

Choose a reason for hiding this comment

flouthoc Mar 17, 2025

Choose a reason for hiding this comment

Luap99 Mar 18, 2025

Choose a reason for hiding this comment

mtrmac left a comment

Choose a reason for hiding this comment

mtrmac left a comment

Choose a reason for hiding this comment

mtrmac left a comment

Choose a reason for hiding this comment

flouthoc commented Mar 21, 2025

flouthoc commented Mar 21, 2025

Luap99 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Mar 21, 2025

pull,load: use `*Image` instead of re-resolving via `name` #2339

pull,load: use `*Image` instead of re-resolving via `name` #2339

flouthoc commented Mar 1, 2025 •

edited

Loading