e2e networking test: better way to get host IP #18270
Conversation
uber/jaeger-client-go library is deprecated. Remove it. Only place it's used is in one e2e test, a test that is flaking in a way that suggests that the HostIP() weighting heuristic from that module was not actually getting the best outgoing IP address. So, switch to using what seems to be the current best practice. No need to make it reusable, since it's only used in one place. Oh, also remove undesired "-dt" from two "podman run"s. In one it's harmless, in the other it would cause a test failure under some circumstances. Closes: containers#18269 (optimistic, aren't I?) Signed-off-by: Ed Santiago <[email protected]>
openshift-ci
bot
added
release-note-none
approved
|
Justification: jaeger is deprecated. |
|
Woot! On the very first CI run, on debian, the flaky test ran with a |
There was a problem hiding this comment.
The flake likely depends on the ordering of interfaces. The real reason why you see the wrong IP is because the podman bridge interfaces are NAT-ed to allow external communication for all containers without complex routing rules. So ncat sees the IP after NAT was applied which is the external ip and not the bridge ip thus the missmatch.
It is trivial to reproduce locally. Just set the outbound-addr to the podman bridge ip.
So yes this patch should definitely fix the flake is it always uses the external ip and not the bridge ip.
I also doubled check that the output-addr is not a NOP in this scenario, without it ncat shows the connection coming from 127.0.0.1 so it definitely tests what it should.
LGTM
| @@ -576,15 +579,15 @@ EXPOSE 2004-2005/tcp`, ALPINE) | |||
|
|
|||
| if strings.Contains(slirp4netnsHelp.OutputToString(), "outbound-addr") { | |||
There was a problem hiding this comment.
I wonder if we should remove this branch logic?
outbound-addr was added in June 2020, https://github.com/rootless-containers/slirp4netns/releases/tag/v1.1.0
I doubt that we care to support versions that do not support it. RHEL 8 ships v1.2.
I know this isn't the purpose of your change and I definitely don't expect you to do it here.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: edsantiago, giuseppe, Luap99 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 |
github-actions
bot
added
the
locked - please file new issue/PR
uber/jaeger-client-go library is deprecated. Remove it.
Only place it's used is in one e2e test, a test that is flaking
in a way that suggests that the HostIP() weighting heuristic from
that module was not actually getting the best outgoing IP address.
So, switch to using what seems to be the current best practice.
No need to make it reusable, since it's only used in one place.
Oh, also remove undesired "-dt" from two "podman run"s. In one
it's harmless, in the other it would cause a test failure under
some circumstances.
Closes: #18269 (optimistic, aren't I?)
Signed-off-by: Ed Santiago [email protected]