manifest_test: safer registry setup and teardown

[edsantiago]

manifest_test:authenticated_push() is the final test left to
fix before merging #14397. The reason it's failing seems to be
that podman is running with a mix of netavark and CNI, and
that seems to be because this test invokes hack/podman-registry
which invokes plain podman without whatever options used in e2e.

Starting a registry directly from the test is insane: there is
no reusable code for doing that (see login_logout_test.go and
push_test.go. Yeesh.)

Solution: set $PODMAN, by inspecting the podmanTest object
which includes both a path and a list of options. podman-registry
will invoke that. (It will also override --root and --runroot.
This is the desired behavior).

Also: add cleanup. If auth-push test fails, stop the registry.

Also: add a sanity check to podman-registry script, have it
wait for the registry port to activate. Die if it doesn't.
That could've saved us a nice bit of debugging time.

Signed-off-by: Ed Santiago [email protected]

None

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago

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 [edsantiago]

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

[Luap99]

Do we have to worry about options containing a white space, should we escape the options?

[edsantiago]

Good point. I don't know, because I've never spent much time in the e2e tests and don't have a sense for what the test options are. Let's see what CI has to say.

[edsantiago]

Marking as draft because this is getting way beyond the original scope -- I'm playing with new images due to the missing-runc bug, and that is proving to be a mess.

[edsantiago]

Update: criu seems to be broken in f35 and ubuntu. 1minutetip is down, so I can't investigate.

[edsantiago]

It is currently (temporarily) impossible to upgrade VMs, so I've rolled back that part of my changeset. It's back to just addressing the registry problem.

All int tests have passed, so this is ready for review. (The rest are running, but the only way they'll fail is if flakes).

@lsm5 once this merges, please rebase your #14397. I think this will address your remaining test failures.

[vrothberg]

LGTM
@Luap99 PTAL

[Luap99]

/lgtm

[mtrmac]

If the registry is state related to a single test, I’d naively expect only that test to be dealing with that, without the callers having to care:

		os.Setenv("PODMAN", podmanTest.PodmanBinary+" "+opts)
		registry, err = podmanRegistry.StartWithOptions(registryOptions)
		Expect(err).To(BeNil())
		defer func() {
			err := registry.Stop()
			Expect(err).To(BeNil())
		}()

Same for the environment variable. Is there some non-obvious reason why that won’t work, e.g. is there a risk of the test process crashing and not having a chance to run the defer? If so, it would be nice to add a comment. (Or is this a general practice of the e2e tests not to rely on defer, which everyone understands and I’m just unaware of the convention?)

[edsantiago]

I see plenty of defer()s under test/e2e. The reason I didn't use it is that I'm not a seasoned Go programmer, and completely forgot about it. Thank you, I will submit a new PR (Monday) to address this.

[mtrmac]

In that case, it’s just a small low-priority cleanup — the code as is works just fine.