Fix: `farm build` can't handle `registry/path:tag` #27088

kcrocombe · 2025-09-16T15:50:17Z

Contrary to its specified behaviour (https://docs.podman.io/en/stable/markdown/podman-farm-build.1.html), podman farm build cannot handle the more extended form of --tag that might be passed to it. Whilst --tags of the form registry/repository are handled ok, instances of the form registry/repository:tag cause the farm to fail when attempting to push the images it has built to the registry.

This commit addresses that issue.

The primary cause (in list_builder.go) is the indiscriminate appending of the UnknownDigestSuffix to ALL forms of the supplied --tag prior to pushing its associated images to the registry. This suffix exists specifically to allow images to be pushed without either a digest or :tag, so attaching it to an image actually bearing a :tag is a logical contradiction, and the construct subsequently gets caught and kicked out by docker_transport.go.

This solution:

* amends list_builder.go as follows:

     - the listLocal struct is refactored so that the provided `--tag` is held as a valid
       `v5/docker/reference.Named` rather than as a plain string. The  `--tag` string is then
       made available to the program via two new member functions. It is made available in two
       forms: a longer reference format (`registry/repository[:tag]`) and a shorter name format
       (`registry/repository`). Some parts of the existing processing require the long `reference` form,
       and others the short `name` form. The remainder of code has thus been amended to use the
       contextually appropriate form;

     - newManifestListBuilder() is amended so that it throws an errors if the `--tag` it is
        to be build upon can't be transformed to a valid Reference.Named.

* amends farm.go to handle any new error thrown by the above;

* amends farm_build_test.go to provide additional test cases to exercise the above.

Signed-off-by: kevin kevin.crocombe@pegortech.co.uk

Does this PR introduce a user-facing change?

None

kcrocombe · 2025-09-16T16:10:14Z

/hold

Existing integration test cases for `farm` functionality is limited to the operations that create/update/delete farm objects. There is no test coverage of any actual farm builds. This submission seeks to address this via the addition of a new integration test module: `farm_build_test.go` This module: - prepares a simulated, multi-farm, multi-node test environment; - performs testing of the podman farm build function across that environment; - confirms that the expected number of build are performed; that they are of the correct architecture; and that the builds occur on the expected node, given the build parameters that were supplied. The commit also includes two minor changes: - A very minor change the farm documention to clarify one of of the `default` setting. - The farm system-test script 001-farm.bats now uses an explicitly set connection name rather than relying on the default in the users environment being set in a supportive manner. Signed-off-by: Kevin Crocombe <kevin.crocombe@pegortech.co.uk>

Fixes: containers#25039 Contrary to its specified behaviour (https://docs.podman.io/en/stable/markdown/podman-farm-build.1.html), `podman farm build` *cannot* handle the more extended form of `--tag` that might be passed to it. Whilst `--tag`s of the form `registry/repository` are handled ok, instances of the form `registry/repository:tag` cause the farm to fail when attempting to push the images it has built to the registry. This commit addresses that issue. The primary cause (in list_builder.go) is the indiscriminate appending of the `UnknownDigestSuffix` to ALL forms of the supplied `--tag` prior to pushing its associated images to the registry. This suffix exists specifically to allow images to be pushed without either a digest or :tag, so attaching it to an image actually bearing a :tag is a logical contradiction, and the construct subsequently gets caught and kicked out by `docker_transport.go`. This solution: * amends list_builder.go as follows: - the listLocal struct is refactored so that the provided `--tag` is held as a valid `v5/docker/reference.Named` rather than as a plain string. The `--tag` string is then made available to the program via two new member functions. It is made available in two forms: a longer reference format (`registry/repository[:tag]`) and a shorter name format (`registry/repository`). Some parts of the existing processing require the long `reference` form, and others the short `name` form. The remainder of code has thus been amended to use the contextually appropriate form; - newManifestListBuilder() is amended so that it throws an errors if the `--tag` it is to be build upon can't be transformed to a valid Reference.Named. * amends farm.go to handle any new error thrown by the above; * amends farm_build_test.go to provide additional test cases to exercise the above. Signed-off-by: Kevin Crocombe <kevin.crocombe@pegortech.co.uk>

kcrocombe · 2025-09-16T17:49:16Z

@containers/podman-maintainers: Is it possible to re-run the above failing checks please? Can't draw a line between the test failures and anything in my PR, so want to rule out flakiness.

kcrocombe · 2025-09-16T17:50:13Z

/unhold

Honny1 · 2025-09-17T09:33:00Z

@containers/podman-maintainers: Is it possible to re-run the above failing checks please? Can't draw a line between the test failures and anything in my PR, so want to rule out flakiness.

Yes, as the creator of a PR, you can retrigger Cirrus CI jobs. Just open the details of a failed task (for example, at its URL https://cirrus-ci.com/task/6481659157020672). If you are authenticated to Cirrus CI with your GitHub account, you should see a "Rerun" button. For Packit jobs, you can rerun them from a comment. See the documentation on retriggering.

kcrocombe · 2025-09-17T10:32:13Z

/packit testing-farm:fedora-41-x86_64:podman-fedora

Honny1

The code looks good to me, but I'm not confident about the Go tests. It seems like some parts could be moved to common_test.go and simplified, for instance, by using PodmanExitCleanly.

test/e2e/farm_build_test.go

Honny1 · 2025-09-17T10:45:20Z

/packit testing-farm:fedora-41-x86_64:podman-fedora

This will rerun packit tests.

/packit retest-failed

packit-as-a-service · 2025-09-17T10:45:27Z

Account Honny1 has no write access nor is author of PR!

kcrocombe · 2025-09-17T10:50:55Z

/packit retest-failed

Luap99 · 2025-09-17T12:12:06Z

Don't bother about packit tests, they are non blocking and currently hard failing due selinux issues discussed in containers/container-selinux#405 (comment)

mheon · 2025-09-17T12:44:11Z

/approve

flouthoc · 2025-09-17T19:22:07Z

pkg/farm/farm.go

 					emulated[e] = name
 				}
 			}
+


Nit: whiteline.

TomSweeneyRedHat · 2025-09-18T00:02:37Z

test/e2e/farm_build_test.go

+	const OFFLINE_URL = "OFFLINE_URL"
+	const GOOD_SHORT_TAG = "GOOD_SHORT_TAG"
+	const GOOD_LONG_TAG = "GOOD_LONG_TAG"
+	const GOOD_VERY_LONG_TAG = "GOOD_VERY_LONG_TAG"


I might have missed it, but do you test for a badly formatted tag somewhere?

I see at least one down below. It might be good to add a BAD_TAG const here too and use that

There is definitely a couple of bad ones in there, and happy to const them up.

I have a further PR to submit to fix another farm issue (#26822) . This is ready to submit immediately after this one clears. I am under some pressure to move on now, so my preference would be to keep things moving for now, and tidy these issues up in that PR. If you can live with that it would help me out a bit.

TomSweeneyRedHat · 2025-09-18T00:04:50Z

test/e2e/farm_build_test.go

+			/* #############################################################################################################*/
+			/* #############################################################################################################*/
+			/*                                                                                                              */
+			/*  Farm Build Test Scearios : scenarios expected to succeed                                                    */


Suggested change

/* Farm Build Test Scearios : scenarios expected to succeed */

/* Farm Build Test Scenarios : scenarios expected to succeed */

Happy to tidy up as per the above.

TomSweeneyRedHat · 2025-09-18T00:06:05Z

test/e2e/farm_build_test.go

+			//
+			/* #############################################################################################################*/
+			/*                                                                                                              */
+			/*  Farm Build Test Scearios : scenarios expected to fail.                                                      */


Suggested change

/* Farm Build Test Scearios : scenarios expected to fail. */

/* Farm Build Test Scenarios : scenarios expected to fail. */

and anywhere else I might have missed.

Happy to tidy up as per the above.

TomSweeneyRedHat · 2025-09-18T00:09:28Z

@kcrocombe wowsa! TYVM for this! I've just a few nitty things which could be touched up later.
LGTM
@nalind thoughts?

Honny1

LGTM

openshift-ci · 2025-09-19T08:33:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Honny1, kcrocombe, mheon

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

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

Luap99

Thanks for the PR I agree farm builds could need some better tests though like this I think they will be hard to maintain as all the new test complexity with the several nested BeforeAll/Each in nested context and so on is really had to make sense of with all the global vars so maybe there is a way to simplify this further.

Please have a look at the test logs, i.e. https://api.cirrus-ci.com/v1/artifact/task/4792809296756736/html/int-remote-fedora-42-root-host-sqlite.log.html#t--Testing-farm-build-functionality-Performing-farm-build-tests-Basic-builds-Empty-Farm-but-with-a-local-builder-to-fall-back-to--using-podman-binary--1

The names like this are very hard to parse and understand and don't seem to add much value. Look especially at the bottom with the test timings.
The timings also seem quite slow with the amount of tests cases that adds up quickly. it would be good if you could have a look if you can speed them up a bit.

Luap99 · 2025-09-19T10:27:46Z

pkg/farm/list_builder.go

+// Bug #25039: listLocal now holds the reference of the manifest to be built
+// as a referenceNamed, rather than as a plain string. It is now made available in two
+// string formats via two member functions (listName() and listReference())
+//
+//	reference  := name [ ":" tag ] [ "@" digest ]
+//	name       := [domain '/'] path-component ['/' path-component]*
+//
+// Prior to this fix, the program was incorrectly using the fuller `reference` format
+// in places that actually required the shorter `name` format, and failing as a consequence.
+// The program only worked when the reference was supplied untagged (i.e some.domain/name).
+// In such situations, `name` and `reference` formats would be identical.
+//
+// In the body of the code, listName has been refactored to listReference() where that is
+// appropriate form to use in that context.


A comment like this belongs in the commit message not the code, for a normal code reader it doesn't matter what the previous code did. In particular I don't think the whole comment adds much value.
It could be a simple as

Store a proper named reference so we can handle references with tags correctly: https://github.com/containers/podman/issues/25039

Luap99 · 2025-09-19T10:28:53Z

pkg/farm/list_builder.go

+// Bug #25039: manifestListBuilder amended to address problems arising from the
+// provision of an incorrectly specified reference. If the provided reference
+// will not parse correctly, the function now throws an error.
+func newManifestListBuilder(listName string, localEngine entities.ImageEngine, options listBuilderOptions) (*listLocal, error) {


This comment should can just be If the provided reference will not parse correctly, the function now throws an error.

Luap99 · 2025-09-19T10:29:48Z

pkg/farm/list_builder.go

 	}
 	// Create list if it doesn't exist
+	//
+	// Bug #25039: we can safely use the longer form, (i.e. potentially tagged), listReference() here. (previously listName)


same here there is no value in linking the same bug over and over and describing what the old field was

Luap99 · 2025-09-19T10:31:01Z

pkg/farm/list_builder.go

+			// Bug #25039: prior to fix, would crash here owing to the UnknownDigestSuffix being inappropriately applied to
+			// the longer `reference` format . We can only use the shorter `name` format here.


This does indeed have value here

Luap99 · 2025-09-19T10:31:52Z

pkg/farm/list_builder.go

+func (l *listLocal) listName() string {
+	// i.e. domain/path-component
+	return l.listRef.Name()
+}
+
+func (l *listLocal) listReference() string {
+	// i.e. domain/path-component:tag
+	return l.listRef.String()


note the inline comment should be in the function so there are visable in the function docs for callers

(A drive-by comment:) Writing function documentation is always good; I think even more of an improvement would be to also name the functions for what they return / how they are used, rather than how they are internally implemented. “list name” is not intuitively self-explanatory.

destinationRepo, destinationListReference?

Luap99 · 2025-09-19T11:00:37Z

test/e2e/farm_build_test.go

+			// :"application/vnd.oci.image.manifest.v1+json","digest":"sha256:6afa31388822c02a0452e190617affa6257d3c28ac5\
+			// 4681ab0ab35193ab86df3","size":680,"platform":{"architecture":"amd64","os":"linux"}}]}
+
+			var SkopeoBinary = "skopeo"


there is no point in define it as var if you use it one below

Luap99 · 2025-09-19T11:02:12Z

test/e2e/farm_build_test.go

+
+			var SkopeoBinary = "skopeo"
+
+			var dockerImage = fmt.Sprintf("docker:%s%s", `//`, image)


why the double %s%s? just write it as
fmt.Sprintf("docker://%s", image)

and also the var name = value syntax is generally not very go style. please use
name := value if you assign a value right away

Luap99 · 2025-09-19T11:04:10Z

test/e2e/farm_build_test.go

+			// will be no garbage!)
+
+			remainder := bldSpec.arch
+			opSys, remainder, _ := strings.Cut(remainder, "/")


opSys should be names os I guess? kinda hard to figure out what opSys means otherwise

Luap99 · 2025-09-19T11:06:15Z

test/farm/001-farm.bats

+    #assert "$output" =~ "test-node"
+assert "$output" =~ ${CONNECTION_NAME}


indention is off and comment and don't leave the old one commented out

Luap99 · 2025-09-19T11:06:59Z

test/farm/setup_suite.bash

    stop_registry
+
+#..and the connection
+run_podman system connection rm ${CONNECTION_NAME}


indentation is off here as well

kcrocombe · 2025-09-22T16:47:17Z

@Luap99 : Thanks for reviewing this, although I did spend part of the weekend pondering some of my life choices as a result :-) .

Most of the issues seem to fall into the category of benign niggle rather than anything particularly substantive, and don't really merit too much in the way of conversation. I'm happy to tidy them up in my final PR, but please don't let the 'perfect' become the enemy of the 'perfectly serviceable' here.

Below, I'll just respond to your more general points.

though like this I think they will be hard to maintain as all the new test complexity with the several nested BeforeAll/Each in nested context and so on is really hard to make sense of with all the global vars so maybe there is a way to simplify this further.

On the complexity point: I'm not sure I accept this, tbh. The code is a ginkgo test module, and, so is structured like one, uses its idioms, and appropriately takes advantage of the facilities provided by the ginkgo framework. Contexts (sometimes nested) are used to give a logical structure and, in my view, simplify rather than add complexity. That might not be immediately obvious from eyeballing the code, but the appropriate plugin makes it pretty clear.

On the 'global' variables point: I'm guessing you are referring to the handful of variables in the outer Context that are referenced by the inner 'setup' and 'test execution' contexts? Those are there to facilitate sharing of data between the two, and there use is a pretty standard idiom. I'm not aware of other ways to do it, but happy to listen if you know of one.

On the 'hard-to-maintain' point: I would imagine most future maintenance are likely to be adding new test cases, or maybe changing parameters on existing ones. The design specifically tries to make that very straightforward, and I thought it had succeeded.

The timings also seem quite slow with the amount of tests cases that adds up quickly. it would be good if you could have a look if you can speed them up a bit.

Yes, they do take a little while to run. However, I'm not sure you can draw too many conclusions based on that report. If you look at each of the tests labelled 'Basic builds', each of those is largely the same operation, so they should have run times within 15-20% of each other (as they do on my platform). Instead, there is a 500% variation. So my guess is external factors (contention? hardware stress?) are exerting an undue influence here. Is that likely?

Based on measurements from a more stable platform, I would estimate the testing harness is adding approx 20% overhead on top of the runtime of the raw operation under test. Not surprisingly, pretty much all of this is consumed in setting up/tearing down the test environment between tests. This is already pretty much at a minimum; most of the test environment is set up just once and re-used for each test. The sole exception is the registry container, which probably does need to be pristine for each test.

If timings are be a problem, maybe, at some point in the future, some of the testing could (and should?) be offloaded into unit-testing and mocks deployed instead.

The names like this are very hard to parse and understand and don't seem to add much value. Look especially at the bottom with the test timings.

Well, slightly facetiously, in the above, you will notice I have just used those names to diagnose a possible issue with the tests. That is some value, surely. Less facetiously, this report is not the principle context within which those names are used, and in those other contexts, I think they do have more value.

I agree, it messes up the aesthetics of this section of this report a little, though this program is not the only offender in that regard. Though, to say it make it hard to parse/understand might be overstating things a little. However, I can add some brevity and strip out the white space if that would help; ironically that is only there to improve the aesthetics of a different report.

As mentioned several times before, I am under pressure to bale, so your support on keeping things moving would be appreciated.

baude · 2025-09-25T14:58:18Z

I did a quick lookover and didn't find much that was not already pointed out. @kcrocombe thank you so much for the submission and tackling this topical area. First, I hope you are willing to see this PR through and address the review comments.

The way I read @Luap99 comments, and I've been in your spot as well, is that he is reviewing code in the way he, as a core maintainer, would like his code reviewed. Straight and to the point. That said, you, as the contributor, certainly have leeway in the interpretation and acceptance of the review comments (unless otherwise noted). My advice to you would be to fix what you are willing to fix. If you are under pressure for a hit and run contribution, we certainly understand that.

Again thank you for the contribution and I hope we see more from you in the future.

kcrocombe · 2025-09-30T17:20:37Z

My apologies, but I have to give this one up now. Effort expended on this and its predecessor PR has not really yielded anything tangible, and, without obvious progress to point to, I've not been able to make the argument for more.

That said, I am still of the belief that the work performed to date represents a small but worthwhile contribution. Accordingly, I will push this, plus some other as yet un-submitted work, to a public fork somewhere, and update the relevant issues with the details. Hopefully, it can be still be of use further down the line. My apologies once more.

openshift-ci bot added the release-note-none label Sep 16, 2025

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 16, 2025

kcrocombe added 2 commits September 16, 2025 16:51

kcrocombe force-pushed the main branch from 99cccc2 to 106eb1c Compare September 16, 2025 16:54

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 16, 2025

Honny1 reviewed Sep 17, 2025

View reviewed changes

test/e2e/farm_build_test.go Show resolved Hide resolved

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 17, 2025

flouthoc reviewed Sep 17, 2025

View reviewed changes

pkg/farm/farm.go

emulated[e] = name

}

}

Copy link

Collaborator

flouthoc Sep 17, 2025

Choose a reason for hiding this comment

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

Nit: whiteline.

TomSweeneyRedHat reviewed Sep 18, 2025

View reviewed changes

Honny1 approved these changes Sep 19, 2025

View reviewed changes

Luap99 reviewed Sep 19, 2025

View reviewed changes

kcrocombe closed this Sep 30, 2025

	/* Farm Build Test Scearios : scenarios expected to succeed */
	/* Farm Build Test Scenarios : scenarios expected to succeed */

		// Bug #25039: prior to fix, would crash here owing to the UnknownDigestSuffix being inappropriately applied to
		// the longer `reference` format . We can only use the shorter `name` format here.


		var SkopeoBinary = "skopeo"

		var dockerImage = fmt.Sprintf("docker:%s%s", `//`, image)

		#assert "$output" =~ "test-node"
		assert "$output" =~ ${CONNECTION_NAME}

Fix: farm build can't handle registry/path:tag #27088

Fix: farm build can't handle registry/path:tag #27088

Conversation

kcrocombe commented Sep 16, 2025

Does this PR introduce a user-facing change?

Uh oh!

kcrocombe commented Sep 16, 2025

Uh oh!

kcrocombe commented Sep 16, 2025

Uh oh!

kcrocombe commented Sep 16, 2025

Uh oh!

Honny1 commented Sep 17, 2025

Uh oh!

kcrocombe commented Sep 17, 2025

Uh oh!

Honny1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Honny1 commented Sep 17, 2025

Uh oh!

packit-as-a-service bot commented Sep 17, 2025

Uh oh!

kcrocombe commented Sep 17, 2025

Uh oh!

Luap99 commented Sep 17, 2025

Uh oh!

mheon commented Sep 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomSweeneyRedHat commented Sep 18, 2025

Uh oh!

Honny1 left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Sep 19, 2025

Uh oh!

Luap99 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kcrocombe commented Sep 22, 2025

Uh oh!

Fix: `farm build` can't handle `registry/path:tag` #27088

Fix: `farm build` can't handle `registry/path:tag` #27088