[v1.18] Backport: k8s: include namespace in EndpointSliceName#44021
[v1.18] Backport: k8s: include namespace in EndpointSliceName#44021tklauser merged 2 commits intocilium:v1.18from
Conversation
|
These CI tests are strugglin'. |
eaf9f0f to
86175c2
Compare
|
/test |
|
Looks like we some legitimate failures here: https://github.com/cilium/cilium/actions/runs/21623311418/job/62317229107 Can you take a look and see if you suspect they are attributed to this pull request? |
I don't think these failures are related. This PR is about the cilium load balancer. The test failures are to do with permissions on envoy ingress resources and some random dns failures. The field changed in this PR isn't consumed by anything other than |
|
@EmilyShepherd The Conformance Cluster Mesh failures persist so could you try to rebase on latest v1.18 and we'll see if that fixes it? I don't see how it could be related to your changes. |
Yeah, a rebase is required to fix it. |
[ upstream commit 74c1bde ] This commit adds a test that when we have two EndpointSlices in different namespaces with the same name, that these do not cause any collisions or problems. Specifically, we test the scenario where two EndpointSlices are setup, then one is modified by removing an address. This should be correctly picked up and the backend removed from the BPF maps. This test is a regression test for cilium#43999. Signed-off-by: Emily Shepherd <emily@redcoat.dev>
[ upstream commit 5c146c3 ] We previously did not include the Namespace in EndpointSliceNames, relying on the fact that EndpointSlices that use generateName- are unlikely to have name collisions, even across namespaces. While this is usually the case, there is no requirement for EndpointSlice managers to use generateName, and there are examples of controllers that do not (for example the master kubernetes service's EndpointSlice is always called "kubernetes"). Including the namespace in EndpointSliceNames guarantees collisions cannot occur. See cilium#43999 for further discussion of this bug. Signed-off-by: Emily Shepherd <emily@redcoat.dev>
Head branch was pushed to by a user without write access
86175c2 to
ec5d2c0
Compare
|
/test |
|
@EmilyShepherd I see this is now closed. What happened? |
|
@pchaigno After re-basing a different test failed. As before I am not sure how those failures could have been related to these changes, however I attempted to investigate as far as I could. Unfortunately, however, as I am not a Member of the project I cannot trigger retests without persistently tagging members which feels like bad form to just do over and over for however long it will take to either get to the bottom of the issue with this PR, if there is one, or get the tests to pass, if they are just flaky - I am not sure which is the culprit. As a result, I do not see a way for this PR to productively be resolved. |
|
@EmilyShepherd If you've triaged the test failures and don't see any relation to your changes, I'd say it's up to the reviews/maintainers to retrigger until it passes. That's usually what I do on lots of PRs from external contributors. |
Backport of #43999:
5c146c3
74c1bde
I have provided an explicit backport PR because v1.18 does not include d003678, which removed the older
EndpointSlicesV1BetaandEndpointsAPIs. As v1.18 still includes these APIs, simply copy-pasting the patchset from #43999 would cause the tests that covered those APIs to fail.cc @joamaki & @borkmann as you reviewed the original PR.
Once this PR is merged, a GitHub action will update the labels of these PRs: