Remove exotic netkit e2e tests#44998
Conversation
|
/test |
1 similar comment
|
/test |
|
/ci-l3-l4 |
smagnani96
left a comment
There was a problem hiding this comment.
PR LGTM. Just to be clear, after this we're basically skipping the whole L7 tests with netkit, is that the intended behavior?
So to be clear, I did think about it, but I concluded that both workflows are usually executed, and the one iteration that was However the devil is in the detail. I clearly haven't looked hard enough in the workflow logic. I only saw the split of tests in the matrix, not the filters further down. I'll fix it so they're replicated on both, given the filters. Undo your approval... 😉 |
Now we have #45020, the scheduled runs on |
|
Closed in error 😳 |
smagnani96
left a comment
There was a problem hiding this comment.
Now we have #45020, the scheduled runs on test-e2e-upgrade should run L7 tests on netkit configs. So, once merged, this PR should be good, if you agree @smagnani96 ?
Yep, agree. This PR can go, is not blocked by that. If there are issues we'll see in scheduled runs anyway (not in PR runs) 👍🏼
|
@ajmmm Smoke Test failing at your change https://github.com/isovalent/cilium/blob/366fc3ad0e83ca2c75ffa5963ab22d5fa74bdb78/.github/workflows/tests-smoke.yaml#L276. Need to sync feature metrics. |
Hey @smagnani96, yeah I haven't checked that out yet. Strange that I need to, I haven't touched feature metrics! |
Maybe needs a rebase? It never occurred to me either. |
Rationalise netkit e2e testing down into simplest form that aligns with the Cilium tuning guide, which is namely netkit in native routing mode and KPR. Notes on some exotic combinations: - [endpoint-routes: true, ciliumendpointslice: true] This permutation was originally added following the issues related to per-endpoint routes and skb scrubbing [0], but following from commit [1] this permutation is now rejected by the connector if the host doesn't provide netkit scrub attributes. Hence, we remove it from the e2e testing for netkit. [0] c345a69 ("cilium, ci: Add netkit with per-endpoint-routes to e2e test") [1] 700a878 ("pkg/datapath: require scrub attributes with netkit and endpoint routes") - host-fw: true This permutation was originally added following the issues related to ARP passthrough to the host stack [2]. However, following commit [3] the ARP passthrough knob has just been collapsed into standard IPv4 support, meaning this is no longer tied to netkit being enabled. Hence, we remove it from e2e testing for netkit. [2] c60ea32 ("datapath, netkit: Allow ARP passthrough on host when using netkit") [3] efeca29 ("bpf: migrate runtime config (enable_arp_*)") Other misc notes: - BPF Masquerade is now enabled as a requirement for netkit. - VXLAN remains in place to verify tunnel-based routing. - IPv6 is default enabled, so removed as an explicit flag. - BigTCP is removed because it's not tied to netkit. Signed-off-by: Alasdair McWilliam <alasdair.mcwilliam@isovalent.com>
|
/test |
1 similar comment
|
/test |
| - name: 'netkit-4' | ||
| kernel: '6.12' | ||
| misc: 'bpf.datapathMode=netkit-l2' | ||
| misc: 'bpf.datapathMode=netkit-l2,bpf.masquerade=true' |
There was a problem hiding this comment.
Note that .github/actions/cilium-config/action.yml does this automatically for KPR=true configs. Debatable whether such hidden opt-ins are a good thing ;).
There was a problem hiding this comment.
I personally prefer them to be explicit, because assumptions can change!
I will split that out if you would like. I'm already splitting out bpf.masquerade=true from other CI workflows/actions so I can do the same for cilium-config :)
There was a problem hiding this comment.
I personally prefer them to be explicit, because assumptions can change!
Personally I agree. In particular since we are decoupling BPF-masq from BPF-nodeport (even though the ideal setup is all-out BPF of course).
I will split that out if you would like. I'm already splitting out
bpf.masquerade=truefrom other CI workflows/actions so I can do the same forcilium-config:)
👍
Rationalise netkit e2e testing down into simplest form that aligns with the Cilium tuning guide, which is namely netkit in native routing mode and KPR.
Notes on some exotic combinations:
[endpoint-routes: true, ciliumendpointslice: true] This permutation was originally added following the issues related to per-endpoint routes and skb scrubbing [0], but following from commit [1] this permutation is now rejected by the connector if the host doesn't provide netkit scrub attributes. Hence, we remove it from the e2e testing for netkit.
[0] c345a69 ("cilium, ci: Add netkit with per-endpoint-routes to e2e test")
[1] 700a878 ("pkg/datapath: require scrub attributes with netkit and endpoint routes")
host-fw: true This permutation was originally added following the issues related to ARP passthrough to the host stack [2]. However, following commit [3] the ARP passthrough knob has just been collapsed into standard IPv4 support, meaning this is no longer tied to netkit being enabled. Hence, we remove it from e2e testing for netkit.
[2] c60ea32 ("datapath, netkit: Allow ARP passthrough on host when using netkit")
[3] efeca29 ("bpf: migrate runtime config (enable_arp_*)")
Other misc notes: