Integration tests: run git daemon on a random-but-bind()able port by nalind · Pull Request #5783 · containers/buildah

nalind · 2024-10-16T20:21:21Z

What type of PR is this?

/kind flake

What this PR does / why we need it:

Use a listener helper to bind to an available-according-to-the-kernel listening port and run a command with its stdio more or less tied to the connection instead of trying to launch a git daemon directly using a port number that we can only guess is available.

How to verify it

Our build-using-a-git-context tests should continue to pass.

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

Does this PR introduce a user-facing change?

None

Luap99

This looks like a bit overkill, in podman we have this long solved by using random_free_port() which seem to hold up fine there.
https://github.com/containers/podman/blob/740f1d1fc710d4560d2463b11333cc9ad68e22bd/test/system/helpers.network.bash#L306-L329

I think this is simpler for test issues to debug then dealing with custom written binary that can fail in a lot of unexpected ways. It will also means we would have to ship this binary in the test package for gating tests

nalind · 2024-10-18T15:56:50Z

Now that I know about it, it's tempting, but it's not appreciably shorter, and debugging Go code is easier than attempting to copy and keep up-to-date shell functions that live in a different repository.
Taking tests that were written here, incorporating and reusing them elsewhere, and then expecting them to not require any upkeep, is an unrealistic expectation to place on any project. I can add the helper binary to the .spec file, though, that's simple enough.

Luap99 · 2024-10-18T16:15:53Z

Well debugging this custom go code may be easier for you but I doubt it is easier for the person running these tests on gating env that have to look at any failures and make sense of them such as @edsantiago.

In any case you are the main maintainer here and I am certainly not going to block this if it helps to solve a flake.

Luap99 · 2024-10-18T16:18:06Z

tests/inet/inet.go

+	args := flag.Args()
+	if len(args) < 1 {
+		fmt.Printf("Usage: %s [-port-file filename] [-pid-file filename] command ...\n", filepath.Base(os.Args[0]))
+		return


should this exit(1)?

Yeah, probably. Changing it.

Luap99 · 2024-10-18T16:18:10Z

tests/inet/inet.go

+		return
+	}
+	// Start listening without specifying a port number.
+	ln, err := net.ListenTCP("tcp", &net.TCPAddr{})


would it make sense to at least bind 127.0.0.1, there seems little reason to expose this externally

Hmm, leaving it empty means we don't have to care about ipv4/ipv6 here, though.

I mean fair enough, it should not be a problem though as all test connect to localhost which generally should try both ::1 and 127.0.0.1 so I doubt it causes issues.

Luap99 · 2024-10-18T16:18:26Z

tests/inet/inet.go

+	colon := strings.LastIndex(addrString, ":")
+	if colon == -1 {
+		logrus.Fatalf("finding the start of the port number in %q", addrString)
+	}
+	// Write the port number part to the specified file, if one was specified.
+	portString := addrString[colon+1:]


you can use net.SplitHostPort()

Oh, nice. Switching to it.

edsantiago · 2024-10-21T14:50:09Z

Makefile

 bin/tutorial: $(SOURCES) tests/tutorial/tutorial.go
 	$(GO_BUILD) $(BUILDAH_LDFLAGS) -o $@ $(BUILDFLAGS) ./tests/tutorial/tutorial.go

+bin/inet: tests/inet/inet.go


This will need to be shipped in the buildah-tests package, presumably in /usr/bin. Could I encourage you to name it buildah-inet-helper or something less generic?

cc @lsm5, @jnovy, because you will need to deal with this in gating.yaml on all affected branches

The scripts will need to be told where it is regardless, since it won't be in ../bin relative to where the scripts end up packaged. At that point it's the packager's decision, as it is with the other test helpers.

Luap99

go code LGTM

Luap99 · 2024-10-21T15:19:03Z

rpm/buildah.spec

 %gobuild -o bin/imgtype ./tests/imgtype
 %gobuild -o bin/copy ./tests/copy
 %gobuild -o bin/tutorial ./tests/tutorial
+%gobuild -o bin/inet ./tests/inet


this also needs to be added to the install section like bin/copy and then listed under %files tests

And since we had this discussion last week about podman-tests do we ship buildah-tests in RHEL as well? I really doubt we want to support these test helpers?

Ah, right. Adding.

Luap99 · 2024-10-21T15:21:43Z

tests/inet/inet.go

+		return
+	}
+	// Start listening without specifying a port number.
+	ln, err := net.ListenTCP("tcp", &net.TCPAddr{})


I mean fair enough, it should not be a problem though as all test connect to localhost which generally should try both ::1 and 127.0.0.1 so I doubt it causes issues.

Use a listener helper to bind to an available-according-to-the-kernel listening port and run a command with its stdio more or less tied to the connection instead of trying to launch a git daemon directly using a port number that we can only guess is available. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

This wrapper doesn't need to load anything from helpers.bash, because the various .bats files already do so on their own. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

Luap99

LGTM

openshift-ci · 2024-10-22T09:05:47Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [nalind]

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

rhatdan · 2024-10-22T10:18:34Z

/lgtm

Added in containers/buildah#5783 Signed-off-by: Paul Holzinger <pholzing@redhat.com>

Added in containers/buildah#5783 Signed-off-by: Paul Holzinger <pholzing@redhat.com> (cherry picked from commit 22152a2) Signed-off-by: Paul Holzinger <pholzing@redhat.com>

openshift-ci bot added kind/flake Categorizes issue or PR as related to a flaky test. approved labels Oct 16, 2024

nalind force-pushed the inet branch from c4a9cee to b904d3b Compare October 17, 2024 13:38

Luap99 reviewed Oct 18, 2024

View reviewed changes

nalind force-pushed the inet branch from b904d3b to 2f978d2 Compare October 18, 2024 15:56

Luap99 reviewed Oct 18, 2024

View reviewed changes

nalind force-pushed the inet branch from 2f978d2 to 566ec23 Compare October 18, 2024 20:04

edsantiago reviewed Oct 21, 2024

View reviewed changes

Luap99 reviewed Oct 21, 2024

View reviewed changes

nalind force-pushed the inet branch from 566ec23 to 1d73965 Compare October 21, 2024 17:43

nalind added 2 commits October 21, 2024 14:39

tests/test_runner.sh: remove some redundancies

855ec0f

This wrapper doesn't need to load anything from helpers.bash, because the various .bats files already do so on their own. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

nalind force-pushed the inet branch from 1d73965 to 855ec0f Compare October 21, 2024 18:40

Luap99 approved these changes Oct 22, 2024

View reviewed changes

openshift-ci bot assigned rhatdan Oct 22, 2024

openshift-ci bot added the lgtm label Oct 22, 2024

openshift-merge-bot bot merged commit aeec2a1 into containers:main Oct 22, 2024

nalind deleted the inet branch October 23, 2024 21:07

Luap99 added a commit to Luap99/libpod that referenced this pull request Nov 1, 2024

test/buildah-bud: build new inet helper

0e65544

Added in containers/buildah#5783 Signed-off-by: Paul Holzinger <pholzing@redhat.com>

Luap99 added a commit to Luap99/libpod that referenced this pull request Nov 4, 2024

test/buildah-bud: build new inet helper

57493aa

Added in containers/buildah#5783 Signed-off-by: Paul Holzinger <pholzing@redhat.com>

Luap99 added a commit to Luap99/libpod that referenced this pull request Nov 5, 2024

test/buildah-bud: build new inet helper

d8df8cb

Added in containers/buildah#5783 Signed-off-by: Paul Holzinger <pholzing@redhat.com>

Luap99 added a commit to Luap99/libpod that referenced this pull request Nov 6, 2024

test/buildah-bud: build new inet helper

d130ccc

Added in containers/buildah#5783 Signed-off-by: Paul Holzinger <pholzing@redhat.com>

Luap99 added a commit to Luap99/libpod that referenced this pull request Nov 6, 2024

test/buildah-bud: build new inet helper

0d9f6dc

Added in containers/buildah#5783 Signed-off-by: Paul Holzinger <pholzing@redhat.com>

Luap99 added a commit to Luap99/libpod that referenced this pull request Nov 7, 2024

test/buildah-bud: build new inet helper

22152a2

Added in containers/buildah#5783 Signed-off-by: Paul Holzinger <pholzing@redhat.com>

stale-locking-app bot added the locked - please file new issue/PR label Jan 22, 2025

stale-locking-app bot locked as resolved and limited conversation to collaborators Jan 22, 2025

Conversation

nalind commented Oct 16, 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?

Uh oh!

Luap99 left a comment

Choose a reason for hiding this comment

Uh oh!

nalind commented Oct 18, 2024

Uh oh!

Luap99 commented Oct 18, 2024

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!

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!

Luap99 left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Oct 22, 2024

Uh oh!

rhatdan commented Oct 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants