Networking test: fix silent breakage #11027
Conversation
Wow did I screw up. containers#10982 introduced (at my suggestion) a new wait_for_port() helper, with the goal of eliminating a race condition. It didn't work. First: wait_for_port() tests by connecting to the port, which is a Bad Idea when you have a one-shot server that exits upon the first connection closing. We should've caught that, but: Second: I wrote wait_for_port() for a non-BATS test framework, and used the conventional file descriptor 3. BATS uses fd3 for internal control. Overriding that made the test silently just disappear, no "not ok" message, no warnings, nothing except vanishing into the ether. Third: this was caught by my log-colorizer script, which loudly yelled "WARNING: expected 234" (tests) at the bottom of the log. Unfortunately, since this wasn't my PR, I didn't actually look at the test logs. Solution: we can't use wait_for_port() in the network port test. Use wait_for_output() instead, triggering on the 'listening' message emitted by netcat in the container. Also: fix wait_for_port() to use fd5 instead of 3. Although no code currently uses wait_for_port() as of this PR, it's a useful helper that we may want to keep. Signed-off-by: Ed Santiago <[email protected]>
openshift-ci
bot
added
the
approved
|
OK, this worked. sample log shows no angry red "Expected" message, and all networking tests are listed. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: edsantiago, vrothberg 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 |
| @@ -288,7 +288,7 @@ function wait_for_port() { | |||
|
|
|||
| # Wait | |||
| while [ $_timeout -gt 0 ]; do | |||
| { exec 3<> /dev/tcp/$host/$port; } &>/dev/null && return | |||
| { exec 5<> /dev/tcp/$host/$port; } &>/dev/null && return | |||
There was a problem hiding this comment.
@edsantiago you can avoid hardcoded file descriptors in bash entirely by using {name} feature, e.g.
exec {conn_fd} <> /dev/tcp/$host/$portand then you can use $conn_fd variable which is a numeric FD that bash chose.
See https://github.com/opencontainers/runc/blob/master/tests/integration/checkpoint.bats for some more examples of this style (which I personally call "brackets and braces programming" 😄)
There was a problem hiding this comment.
@kolyshkin that is wonderful, I'm so glad to learn that! Thank you! (For future reference, should anyone find this thread: this feature is documented in bash(1) under REDIRECTION).
github-actions
bot
added
the
locked - please file new issue/PR
Wow did I screw up. #10982 introduced (at my suggestion) a
new wait_for_port() helper, with the goal of eliminating a
race condition. It didn't work.
First: wait_for_port() tests by connecting to the port, which
is a Bad Idea when you have a one-shot server that exits upon
the first connection closing. We should've caught that, but:
Second: I wrote wait_for_port() for a non-BATS test framework,
and used the conventional file descriptor 3. BATS uses fd3
for internal control. Overriding that made the test silently
just disappear, no "not ok" message, no warnings, nothing
except vanishing into the ether.
Third: this was caught by my log-colorizer script, which
loudly yelled "WARNING: expected 234" (tests) at the
bottom of the log. Unfortunately, since this wasn't
my PR, I didn't actually look at the test logs.
Solution: we can't use wait_for_port() in the network port
test. Use wait_for_output() instead, triggering on the
'listening' message emitted by netcat in the container.
Also: fix wait_for_port() to use fd5 instead of 3. Although
no code currently uses wait_for_port() as of this PR, it's
a useful helper that we may want to keep.
Signed-off-by: Ed Santiago [email protected]