Skip to content

Conversation

@dgibson
Copy link
Collaborator

@dgibson dgibson commented May 22, 2024

The default_addr shell function in test/system/helpers.network is used to get the host's default address, which is used in a number of pasta networking tests. However, in certain circumstances it can incorrectly pick a deprecated address as the primary address. Correct it to exclude those.

Does this PR introduce a user-facing change?

No

The default_addr shell function in test/system/helpers.network is used to
get the host's default address, which is used in a number of pasta
networking tests.  However, in certain circumstances it can incorrectly
pick a deprecated address as the primary address.  Correct it to exclude
those.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
@openshift-ci openshift-ci bot added do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None release-note-none and removed do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None labels May 22, 2024
@dgibson dgibson self-assigned this May 22, 2024
Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 22, 2024
@sbrivio-rh
Copy link
Collaborator

LGTM

Reviewed-by: Stefano Brivio <sbrivio@redhat.com

Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 22, 2024
@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 22, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dgibson, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit c9241c9 into containers:main May 22, 2024
@edsantiago
Copy link
Member

This got merged before I had a chance to review. I think it's broken, and blowing up:

[+1968s] not ok 615 [505] pasta/bridge and host.containers.internal
...
<+032ms> # $ podman run --rm --network=bridge quay.io/libpod/testimage:20240123 grep host.containers.internal /etc/hosts
<+581ms> # [ rc=1 ]
         # #/vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
         # #|     FAIL: pasta ip must the only one one the host (bridge)
         # #| expected: 10.128.15.24010.88.0.1
         # #|   actual: 10.128.15.240
         # #\^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

See https://cirrus-ci.com/build/5129923445129216 but look ONLY at sys tests. This is a known-broken PR. Lots of failures are expected, but pasta failures are not.

@dgibson
Copy link
Collaborator Author

dgibson commented May 22, 2024

This got merged before I had a chance to review. I think it's broken, and blowing up:

[+1968s] not ok 615 [505] pasta/bridge and host.containers.internal
...
<+032ms> # $ podman run --rm --network=bridge quay.io/libpod/testimage:20240123 grep host.containers.internal /etc/hosts
<+581ms> # [ rc=1 ]
         # #/vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
         # #|     FAIL: pasta ip must the only one one the host (bridge)
         # #| expected: 10.128.15.24010.88.0.1

Huh. Looks like the host has 2 valid IPs that got concatenated here. .. but I'm not sure how that could happen with the jq script I had :/. Can you get the output of ip -j -4 addr show on the host in question?

     # #|   actual: 10.128.15.240
     # #\^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

See https://cirrus-ci.com/build/5129923445129216 but look _ONLY_ at `sys` tests. This is a known-broken PR. Lots of failures are expected, but pasta failures are not.

@dgibson dgibson deleted the deprecated-addr branch May 22, 2024 14:00
@edsantiago
Copy link
Member

Added to #22782, results will be ready in about 50 minutes (1500Z)

@edsantiago
Copy link
Member

Also, I have no idea if this has anything to do with your PR. I'm operating solely on post hoc ergo propter hoc logic.

@dgibson
Copy link
Collaborator Author

dgibson commented May 22, 2024

Huh. I think this is actually related to PR #22740 which added this new test.

@Luap99
Copy link
Member

Luap99 commented May 22, 2024

But this PR passed tests so and is rebased to contain the test from #22740 which passed looking at the logs, I think the error is introduced in your PR (although it is likely a issue with the test not handling the second ip correctly)

@dgibson
Copy link
Collaborator Author

dgibson commented May 22, 2024

But this PR passed tests so and is rebased to contain the test from #22740 which passed looking at the logs, I think the error is introduced in your PR (although it is likely a issue with the test not handling the second ip correctly)

I can't see how my patch could introduce this problem, though. The bogus looking expected value is not coming from the default_addr function I altered, but from hostname -I.

@Luap99
Copy link
Member

Luap99 commented May 22, 2024

But this PR passed tests so and is rebased to contain the test from #22740 which passed looking at the logs, I think the error is introduced in your PR (although it is likely a issue with the test not handling the second ip correctly)

I can't see how my patch could introduce this problem, though. The bogus looking expected value is not coming from the default_addr function I altered, but from hostname -I.

Sorry I meant you (@edsantiago) not you (@dgibson), so yes I agree this PR is fine and did not cause a regression.

@edsantiago
Copy link
Member

#22782 passed CI, so the bug must be in my registry PR. I am sorry for causing undue concern.

@stale-locking-app stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Aug 21, 2024
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators Aug 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note-none

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants