-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Fix network test helpers #26798
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
base: main
Are you sure you want to change the base?
Fix network test helpers #26798
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ricardobranco777 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
6c360f6
to
1547945
Compare
Fixes ipv4_get_addr_global & ipv6_get_addr_global by skipping link-local IPv4/IPv6 addresses as they are not globally routable. Uses a trick from Richard W. Stevens's "Unix Network Programming" book to get the default global IPv4 & IPv6 address that doesn't need a target IP host to be connected at all to the Internet. Signed-off-by: Ricardo Branco <[email protected]>
1547945
to
44e8ff1
Compare
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While that is an option it is not really correct. It doesn't matter to be the most precise or correct here. What matters is to match what pasta will do and AFAIK they are not doing this, pasta actually list routes and and addresses to get the info.
Also pasta has since learned a "local mode" when no suitable ip is found so I think most test assumptions/skips here are simply incorrect now and need to be reviewed again.
It seems pasta has issues with TAP interfaces: |
"tap-style" (not tap, nor TAP) in that context means interfaces without an address, which is not your case. I still have to go through the changes and the rest of the comments. |
@ricardobranco777 first off, thanks for your contribution! Having these tests running on openSUSE will certainly save us some headaches and tickets later on, and at the same time most of the contributions to this project come from Red Hat folks so I'm really glad to see other names, especially in the testing area. I have a concern about the general approach, though, which is an aspect of what @Luap99 mentioned: pasta(1) enables IPv6 (by default) if there's a default IPv6 route (similar to those checks we currently have in tests), or if there's no route at all (for any protocol family) using a so-called "local mode". And this is something we should actually test with Podman, even in isolated environments. It doesn't (or shouldn't!) matter so much if we have real IPv6 connectivity or not. But I guess you're coming here with tests that are otherwise failing. What is failing exactly without these changes? I'd rather fix those to work in isolated IPv6 environments. Otherwise we risk not testing IPv6 at all on openSUSE (except for s390x, but we would be doing tests on big-endian only). |
You're welcome. For a general overview of the tests we're running:
Until yesterday we were ignoring the pasta tests because of the failures, which can be seen in the openQA jobs for openSUSE Tumbleweed Build20250808 and earlier dates like: https://openqa.opensuse.org/tests/5230632/file/podman-bats-user-local.tap The output of The most recent will always be:
I raised the topic about IPv6 with our QA team. Feel free to ping me if you want to try another approach as we can easily try PR's with openQA. |
The issue wasn't only IPv6 as we can see here
|
Thanks for sharing the old results, from a quick look I'd say most of these failures are actually due to the fact that we missed to update tests as we introduced local mode, and things just happened to work in CIs from Podman and passt itself so we didn't touch them, but I need a bit more time to go through the list of previous failures and comment on them. |
I think trying to reimplement how pasta decided what address to pick seems pointless? I understand why we did it originally when the logic was nice and simple but now I am not sure if they add any value. I don't think podman must guess and verify what address pasta picked. That seems something you can verify in the pasta tests if you really want, for podman we care that options work like -4/-6 and that connection/forwarding works and that our default options we depend on such as --dns-forward/--map-guest-addr work and are correctly used by podman to populate resolv.conf/hosts file. So if we do |
|
True, we should probably drop some checks. When I added them my urge was to check that nothing unexpected would happen because of a flaw in the Podman integration, but that came at a lower cost. Note, though, that the logic is more complicated mostly because of specific requests from Podman users, which might indicate that we care about not breaking some of those subtleties particularly when started by Podman and in typical (test) environments used by Podman tests.
That is, I agree that there's no need, but:
...it's quite convenient (for the moment, as long as we're not done with a test framework that can be automated more easily) to have those running as part of Podman's CI as it runs more frequently on more diverse environments. On top of that, we're not investing effort in passt's current test suite right now as we plan to replace the framework eventually, so those checks would logically belong there, they're not Podman's CI job, but I'd keep what can be reasonably kept if it's not too much effort to maintain.
Right, definitely (I broke this by the way, and this only came up in the OpenQA environment, so that's the kind of test I'd keep).
@ricardobranco777 I'm generally trying to avoid hardcoding references to whatever assigned address or name (see also #23336 (comment), even though the specific problem I mentioned there doesn't apply here as you wouldn't actually use the resolvers). But my main question at the moment is whether we need to revisit this part at all. List of failing tests with comments (from the results you shared):
Ouch, I guess I broke this in https://passt.top/passt/commit/?id=14b84a7f077ecb734bb0e724f70bafeaa6d35a61, added to my to-do list. It's good that this broke, no changes needed for this one.
I would simply drop this test going in the direction @Luap99 suggested. There can be way too many combinations leading to many different outcomes in pasta for this one. We'll eventually have to write more detailed tests in pasta's own suite.
But wait, there's a default route on the host according to https://openqa.opensuse.org/tests/5234566/logfile?filename=podman-ip6-route.txt (am I reading it wrong?). Even if IPv6 is not globally routable on the host, it should work at least between container and host (see #22959 (comment), the ticket is still open but at least that part should be fixed). If we fail to copy the default route, that will breaks things. And we do copy one address in fe80::/64, so copying the route shouldn't fail because of this. This is something else to investigate (please feel free, or it's going to my to-do list as well), but not a good reason to consider IPv6 disabled for the purposes of this test. If it's due to timing, though (IPv6 not configured yet when tests are started), that's not fixed yet, and we need to find a workaround for the moment being, if that's the case.
...and all the forwarding / transfer tests: oops, why are we failing to bind to that? We don't need to add the zone / interface specifier (say, @ricardobranco777 some other changes from your commit, such as preferring permanent addresses (thanks, I had no idea how to do that with |
We should at least revisit the comments and function names to see if the code matches the indended behaviour. It seems to me that we're trying to guess what the kernel routing code would do instead of using available information. We can use I'm not yet familiar with pasta local mode so I don't know how much more I can help here other than helping running tests and making suggestions.
Maybe it's fixed in latest code? We're only testing released versions:
The full package list: https://openqa.opensuse.org/tests/5234566/logfile?filename=podman-rpm-qa.txt
I'll try in another branch. I want to test with the current setup we have and then with the new setup when IPv6 is fixed on all openQA workers. |
Well, that wasn't really the purpose. The idea behind those implementations of Now pasta does something more complicated than that, in that it actually copies multiple addresses and routes (if present) to the container, by default. I think it's the function names that are misleading, yes.
That doesn't match pasta's logic, though. In any case, should we need something like that, I would consider an address in TEST-NET-1 (say, 192.0.2.1), which should be something nobody will ever want to add specific routes to.
No, no relevant fixes since then. :( |
Fix network test helpers used by the pasta tests.
Right now the code doesn't work for us in openQA where we use TAP networking for IPv4, and IPv6 is fully functional only on the s390x worker. The current code incorrectly detects IPv6 as routable on hosts without IPv6 connectivity.
This PR:
ipv4_get_addr_global
&ipv6_get_addr_global
by skipping link-local IPv4/IPv6 addresses as they are not globally routable as defined by RFC 3927 (IPv4169.254.0.0/16
) and RFC 4291 (IPv6fe80::/10
).ip addr
&ip route
.Notes:
This PR was tested on:
Verification runs available at os-autoinst/os-autoinst-distri-opensuse#22933
Does this PR introduce a user-facing change?