Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pasta: Fix pasta tests to work on hosts with multiple interfaces #19021

Merged
merged 1 commit into from
Jun 28, 2023

Conversation

dgibson
Copy link
Collaborator

@dgibson dgibson commented Jun 28, 2023

At various points the pasta bats tests need to know the name of the interface that pasta will use by default, and the host addresses it will use by default. Currently we use the pre-existing helper functions ether_get_name and ipv[46]_get_addr_global to retreive that.

However, those just pick the first non-loopback interface or address, which may not be the one that pasta uses if there are multiple connected host interfaces.

Replace those helpers with local ones which examine the routing table to more closely match pasta's internal logic about which interface to select. This allows the tests to run successfully on a host with multiple interfaces.

Closes: #19007

Does this PR introduce a user-facing change?

None

@openshift-ci openshift-ci bot added the do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None label Jun 28, 2023
@dgibson
Copy link
Collaborator Author

dgibson commented Jun 28, 2023

/cc @Luap99 @sbrivio-rh

@openshift-ci openshift-ci bot requested a review from Luap99 June 28, 2023 03:11
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 28, 2023

@dgibson: GitHub didn't allow me to request PR reviews from the following users: sbrivio-rh.

Note that only containers members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @Luap99 @sbrivio-rh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

At various points the pasta bats tests need to know the name of the
interface that pasta will use by default, and the host addresses it will
use by default.  Currently we use the pre-existing helper functions
ether_get_name and ipv[46]_get_addr_global to retreive that.

However, those just pick the first non-loopback interface or address, which
may not be the one that pasta uses if there are multiple connected host
interfaces.

Replace those helpers with local ones which examine the routing table to
more closely match pasta's internal logic about which interface to select.
This allows the tests to run successfully on a host with multiple
interfaces.

Closes: containers#19007

Signed-off-by: David Gibson <[email protected]>
@openshift-ci openshift-ci bot added release-note-none and removed do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None labels Jun 28, 2023
@dgibson
Copy link
Collaborator Author

dgibson commented Jun 28, 2023

I don't think the test failures here are related to my patch.

@BlackHole1
Copy link
Contributor

I don't think the test failures here are related to my patch.

Hi @dgibson. You can open and log in to https://cirrus-ci.com/task/6441926329630720, then click the "Retry" button, and CI will run again.

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, @sbrivio-rh PTAL

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 28, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dgibson, 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-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 28, 2023
@sbrivio-rh
Copy link
Collaborator

sbrivio-rh commented Jun 28, 2023

Reviewed-by: Stefano Brivio <[email protected]>

LGTM

@rhatdan
Copy link
Member

rhatdan commented Jun 28, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 28, 2023
@openshift-merge-robot openshift-merge-robot merged commit be49741 into containers:main Jun 28, 2023
@dgibson dgibson deleted the bug19007 branch June 28, 2023 11:08
dgibson added a commit to dgibson/podman that referenced this pull request Jun 29, 2023
containers#19021 fixed bugs with the pasta
networking tests not working on hosts with multiple interfaces.  Alas, the
patch left in some stale code that generates spurious error messages for
the IPv6 case.  This is sort of harmless - later code overrides what's done
here and the tests can pass anyway.  However if a test fails for some other
reason it means we get a misleading irrelevant error message.

Signed-off-by: David Gibson <[email protected]>
ashley-cui pushed a commit to ashley-cui/podman that referenced this pull request Jun 30, 2023
containers#19021 fixed bugs with the pasta
networking tests not working on hosts with multiple interfaces.  Alas, the
patch left in some stale code that generates spurious error messages for
the IPv6 case.  This is sort of harmless - later code overrides what's done
here and the tests can pass anyway.  However if a test fails for some other
reason it means we get a misleading irrelevant error message.

Signed-off-by: David Gibson <[email protected]>
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 26, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 26, 2023
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.

Pasta networking tests don't work on a host with multiple interfaces
6 participants