Integration tests: run git daemon on a random-but-bind()able port #5783
Conversation
openshift-ci
bot
added
kind/flake
There was a problem hiding this comment.
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
|
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. |
|
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. |
tests/inet/inet.go
Outdated
| 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 |
There was a problem hiding this comment.
Yeah, probably. Changing it.
| return | ||
| } | ||
| // Start listening without specifying a port number. | ||
| ln, err := net.ListenTCP("tcp", &net.TCPAddr{}) |
There was a problem hiding this comment.
would it make sense to at least bind 127.0.0.1, there seems little reason to expose this externally
There was a problem hiding this comment.
Hmm, leaving it empty means we don't have to care about ipv4/ipv6 here, though.
There was a problem hiding this comment.
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.
tests/inet/inet.go
Outdated
| 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:] |
There was a problem hiding this comment.
Oh, nice. Switching to it.
| @@ -110,6 +110,9 @@ bin/copy: $(SOURCES) tests/copy/copy.go | |||
| bin/tutorial: $(SOURCES) tests/tutorial/tutorial.go | |||
| $(GO_BUILD) $(BUILDAH_LDFLAGS) -o $@ $(BUILDFLAGS) ./tests/tutorial/tutorial.go | |||
|
|
|||
| bin/inet: tests/inet/inet.go | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
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.
| @@ -133,6 +133,7 @@ export BUILDTAGS+=" btrfs_noversion exclude_graphdriver_btrfs" | |||
| %gobuild -o bin/imgtype ./tests/imgtype | |||
| %gobuild -o bin/copy ./tests/copy | |||
| %gobuild -o bin/tutorial ./tests/tutorial | |||
| %gobuild -o bin/inet ./tests/inet | |||
There was a problem hiding this comment.
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?
| return | ||
| } | ||
| // Start listening without specifying a port number. | ||
| ln, err := net.ListenTCP("tcp", &net.TCPAddr{}) |
There was a problem hiding this comment.
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 <[email protected]>
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 <[email protected]>
|
[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
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
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?