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

Add ipv6 load balancer tests #926

Merged
merged 5 commits into from
Jan 13, 2025
Merged

Add ipv6 load balancer tests #926

merged 5 commits into from
Jan 13, 2025

Conversation

petrutlucian94
Copy link
Contributor

@petrutlucian94 petrutlucian94 commented Dec 31, 2024

This PR adds ipv6-only and ipv6-dualstack load balancer tests. Note that the ipv6-only test is currently skipped due to a known cilium limitation:

@petrutlucian94 petrutlucian94 requested a review from a team as a code owner December 31, 2024 07:49
@petrutlucian94 petrutlucian94 marked this pull request as draft December 31, 2024 07:50
@petrutlucian94 petrutlucian94 changed the title wip: Add ipv6 load balancer tests Add ipv6 load balancer tests Jan 3, 2025
@petrutlucian94 petrutlucian94 marked this pull request as ready for review January 3, 2025 09:51
Copy link
Contributor

@bschimke95 bschimke95 left a comment

Choose a reason for hiding this comment

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

Great work @petrutlucian94
Added a couple of comments. Please also move the year change to a separate PR. Thanks!

tests/integration/tests/conftest.py Outdated Show resolved Hide resolved
tests/integration/tests/test_loadbalancer.py Outdated Show resolved Hide resolved
tests/integration/tests/test_loadbalancer.py Outdated Show resolved Hide resolved
tests/integration/tests/test_util/util.py Outdated Show resolved Hide resolved
tests/integration/tests/test_util/util.py Outdated Show resolved Hide resolved
@petrutlucian94
Copy link
Contributor Author

Please also move the year change to a separate PR. Thanks!

I had to run make format to avoid lint job failures but it bumped the headers. I'll open a separate PR.

@petrutlucian94 petrutlucian94 force-pushed the lpetrut/lb_ipv6_tests branch 2 times, most recently from ad7bf49 to ca4535b Compare January 8, 2025 10:30
@bschimke95
Copy link
Contributor

looks like the dualstack test is not yet working.

@petrutlucian94
Copy link
Contributor Author

looks like the dualstack test is not yet working.

I'll rebase the PR to get the metallb logs, fwiw it passed locally.

We're adjusting the load balancer tests and related utils to
also cover ipv6.

This is currently wip as we're hitting the following Cilium issue:

* cilium/cilium#15082
* cilium/cilium#17240
* add a "network type" param to the lb test
* move header bump to separate PR
* update ipv6 cidr
@petrutlucian94
Copy link
Contributor Author

Strange, the test passes locally and the metallb logs seem fine. The lb ip gets assigned properly, however it's not accessible, but only in case of a dualstack environments.

"status": {
                "loadBalancer": {
                    "ingress": [
                        {
                            "ip": "10.10.61.4",
                            "ipMode": "VIP"
                        }
                    ]
                }
            }

I think I'll have to use this to take a look inside the GH runner: https://github.com/mxschmitt/action-tmate

@petrutlucian94 petrutlucian94 force-pushed the lpetrut/lb_ipv6_tests branch 2 times, most recently from e2a7d32 to 6ebcec8 Compare January 10, 2025 13:12
@petrutlucian94
Copy link
Contributor Author

The dualstack test uses a separate bridge, so the Docker workaround had to be applied there as well. Thanks @berkayoz for the suggestion!

@petrutlucian94
Copy link
Contributor Author

Interesting, test_strict_interfaces is now failing:

____________________________ test_strict_interfaces ____________________________
Traceback (most recent call last):
  File "/home/ubuntu/actions-runner/_work/k8s-snap/k8s-snap/tests/integration/tests/test_strict_interfaces.py", line 33, in test_strict_interfaces
    util.setup_k8s_snap(cp, tmp_path, channel, connect_interfaces=False)
  File "/home/ubuntu/actions-runner/_work/k8s-snap/k8s-snap/tests/integration/tests/test_util/util.py", line 202, in setup_k8s_snap
    instance.exec(cmd)
  File "/home/ubuntu/actions-runner/_work/k8s-snap/k8s-snap/tests/integration/tests/test_util/harness/lxd.py", line 227, in exec
    return run(
  File "/home/ubuntu/actions-runner/_work/k8s-snap/k8s-snap/tests/integration/tests/test_util/util.py", line 44, in run
    return subprocess.run(command, **kwargs)
  File "/home/ubuntu/actions-runner/_work/_tool/Python/3.10.16/x64/lib/python3.10/subprocess.py", line 526, in run
    raise CalledProcessError(retcode, process.args,
subprocess.CalledProcessError: Command '['lxc', 'shell', 'k8s-integration-a370fb-53', '--', 'bash', '-c', 'snap install --classic k8s --channel latest/edge/strict']' returned non-zero exit status 1.

I see a permission error:

error: cannot perform the following tasks:
- Run configure hook of "k8s" snap if present (run hook "configure": 
-----
Warning: meta.orb is none, skipping reconcile actions
mkdir: cannot create directory ‘/etc/systemd/system/snap.k8s.containerd.service.d’: Permission denied
-----)

We should probably disable this test since the strict branch is known as broken and the strict jobs are disabled as well.

The "strict" test jobs are currently disabled, except for
"test_strict_interfaces", which is now failing.

We'll disable this test as well until we get to fix the "strict"
channel, which is currently low-prio.
@bschimke95
Copy link
Contributor

It is fine to disable the test for now but we should still be aware of such issues. I created a card to investigate this in the future.
We likely broke this test when we changed the containerd path.

Copy link
Contributor

@bschimke95 bschimke95 left a comment

Choose a reason for hiding this comment

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

LGTM

@petrutlucian94 petrutlucian94 merged commit 6b3136f into main Jan 13, 2025
19 checks passed
@petrutlucian94 petrutlucian94 deleted the lpetrut/lb_ipv6_tests branch January 13, 2025 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants