Operator: Improve removing stale CEPs from CESs entries on start. #24596

alan-kut · 2023-03-28T09:48:37Z

Handle situation when the CEP is duplicated in CESs.
Keep only one CEP if the CEP still exists or
delete all the instnaces if it doesn't exist anymore.
Prefer keeping CEP with the correct identity if such exists.

It's related to #24581 but it doesn't fully fix it.
It only fixes the reconciliation at the startup.

The operator now reconciles duplicate entries in a CiliumEndpointSlice on startup.

operator/pkg/ciliumendpointslice/endpointslice.go

operator/pkg/ciliumendpointslice/endpointslice_test.go

AwesomePatrol · 2023-03-28T10:00:11Z

operator/pkg/ciliumendpointslice/manager.go

+			ces.removedCEPs[GetCEPNameFromCCEP(&ep, ces.ces.Namespace)] = struct{}{}
+			ces.ces.Endpoints =
+				append(ces.ces.Endpoints[:i],
+					ces.ces.Endpoints[i+1:]...)


do we expect more than a few endpoints to pass this check and be removed? if yes, this is a good candidate for optimization

This section is moved from a RemoveCEPFromCache function.

Before this PR it would always remove 1 endpoint.
With this PR it is possible (and tested) that it can remove more than one in case something really wrong happened but I don't think it is possible now except for some bugs.

So - the point of this PR is to make sure the state is OK after operator restart regardless of anything.
In practice I don't think in the current code it is possible to have duplicated CEP in the same CES.
However it is possible that different CEPs would be removed from the same CES.

Performance-wise, it should be insignificant, unless the duplicate list is big, but then we have another big problem.

AwesomePatrol · 2023-03-28T10:01:51Z

operator/pkg/ciliumendpointslice/manager.go

+
+func (c *cesMgr) getAllCESs() []cesOperations {
+	var cess []cesOperations
+	for _, ces := range c.desiredCESs.getAllCESs() {


Please consider preallocating cess or using copy (I am not sure of the type)

Preallocated the slice

dlapcevic · 2023-03-28T10:18:43Z

operator/pkg/ciliumendpointslice/endpointslice.go

+			found := false
+			cep := storeCep.(*cilium_api_v2.CiliumEndpoint)
+			// Skip first element for now
+			for _, mapping := range mappings[1:] {


Is it maybe simpler if it goes through the mappings twice? First to find the CEP with the matching ID, and then to remove all but selected (the first CEP or with matching ID).

There may be multiple CEPs with matching ID or no CEPs with matching ID.
The code would be something like:
matchingIdIndex := 0
for loop {
if matches id {
matchingIdIndex = currentIndex
}
for loop {
if currentIndex == matchingIdIndex {
keep
} else {
remove
}
}

I don't think it would be much simpler

dlapcevic · 2023-03-28T10:22:24Z

operator/pkg/ciliumendpointslice/manager.go

+			ces.removedCEPs[GetCEPNameFromCCEP(&ep, ces.ces.Namespace)] = struct{}{}
+			ces.ces.Endpoints =
+				append(ces.ces.Endpoints[:i],
+					ces.ces.Endpoints[i+1:]...)


Performance-wise, it should be insignificant, unless the duplicate list is big, but then we have another big problem.

operator/pkg/ciliumendpointslice/endpointslice_test.go

alan-kut · 2023-03-31T08:20:15Z

What are the next steps?

squeed · 2023-03-31T08:36:57Z

@alan-kut I wrote a brief release note, please take a look. I'll trigger the last CI jobs now, and if those go green, it will be picked up for merge.

squeed · 2023-03-31T08:37:03Z

/test

Job 'Cilium-PR-K8s-1.16-kernel-4.19' failed:

Click to show.

Test Name

K8sDatapathConfig Host firewall With VXLAN

Failure Output

FAIL: Error deleting resource /home/jenkins/workspace/Cilium-PR-K8s-1.16-kernel-4.19/src/github.com/cilium/cilium/test/k8s/manifests/host-policies.yaml: Cannot retrieve "cilium-l7rgf"'s policy revision: cannot get policy revision: ""

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.16-kernel-4.19 so I can create one.

alan-kut · 2023-03-31T08:39:38Z

Looks good.

I didn't know how to spot realease notes from other PRs, I'll try to include them for the future.

squeed · 2023-03-31T11:00:09Z

Yeah, it's a judgement call whether or not to write a release note. I default towards yes; it's always easier to filter them at release time. If a PR doesn't need a release note, set the label release-note/misc.

alan-kut · 2023-04-05T07:03:26Z

I tried to analyze the failures but failed.

squeed · 2023-04-05T08:12:55Z

The GKE failure seems like the annoying case where it gets stuck on the metrics finalizer. Jenkins failure #24697

squeed · 2023-04-05T08:13:24Z

Those are known issues, marking as ready-to-merge.

christarazi

Thanks for the PR.

From the title / commit, I did not get the impression that this was resolving a bug with CES, but rather with CEPs in general. For the latter, we have logic inside the Agent itself to cleanup CEPs, but not in the context of CESs. Could you clarify that in the commit please?

alan-kut · 2023-04-06T07:08:44Z

You are right, I didn't notice it. Fixed.

christarazi

Thanks! There's a small typo "instnaces" in the commit, but otherwise LGTM

alan-kut · 2023-04-11T07:11:24Z

Fixed the typo

Handle situation when the CEP is duplicated in CESs. Keep only one CEP if the CEP still exists or delete all the instances if it doesn't exist anymore. Prefer keeping CEP with the correct identity if such exists. Signed-off-by: Alan Kutniewski <kutniewski@google.com>

christarazi · 2023-04-11T07:12:43Z

/test

Job 'Cilium-PR-K8s-1.16-kernel-4.19' failed:

Click to show.

Test Name

K8sDatapathConfig MonitorAggregation Checks that monitor aggregation restricts notifications

Failure Output

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.16-kernel-4.19/865/

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.16-kernel-4.19 so I can create one.

alan-kut · 2023-04-11T08:50:56Z

/mlh new-flake Cilium-PR-K8s-1.16-kernel-4.19

I think it is flake and I didn't find it reported on the issues.

squeed · 2023-04-11T09:17:43Z

Agreed, failure looks like a flake; marking as ready-to-merge.

dylandreimerink · 2023-04-11T15:37:50Z

/mlh new-flake Cilium-PR-K8s-1.16-kernel-4.19

dylandreimerink · 2023-04-11T15:40:35Z

The ci-eks hits #24774 (already fixed in master). ci-datapath should be fixed in master by (#24809).

maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Mar 28, 2023

github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Mar 28, 2023

alan-kut force-pushed the fix-stale-ceps branch from a175f56 to 28ae7d4 Compare March 28, 2023 09:53

AwesomePatrol reviewed Mar 28, 2023

View reviewed changes

alan-kut force-pushed the fix-stale-ceps branch from 28ae7d4 to c5b8f47 Compare March 28, 2023 10:18

alan-kut requested a review from AwesomePatrol March 28, 2023 10:18

dlapcevic reviewed Mar 28, 2023

View reviewed changes

alan-kut force-pushed the fix-stale-ceps branch 2 times, most recently from 12c955e to 378989d Compare March 28, 2023 10:37

alan-kut requested review from dlapcevic and removed request for AwesomePatrol March 28, 2023 10:37

alan-kut marked this pull request as ready for review March 28, 2023 10:37

alan-kut requested a review from a team as a code owner March 28, 2023 10:37

alan-kut requested review from squeed and removed request for dlapcevic March 28, 2023 10:37

dlapcevic approved these changes Mar 28, 2023

View reviewed changes

alan-kut force-pushed the fix-stale-ceps branch 2 times, most recently from de3d1ae to 29cc185 Compare March 29, 2023 08:57

squeed approved these changes Mar 29, 2023

View reviewed changes

alan-kut force-pushed the fix-stale-ceps branch from 29cc185 to 2edde46 Compare March 29, 2023 12:02

squeed added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Mar 31, 2023

maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Mar 31, 2023

squeed added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 5, 2023

christarazi requested changes Apr 5, 2023

View reviewed changes

christarazi removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 5, 2023

alan-kut force-pushed the fix-stale-ceps branch from 2edde46 to 1845605 Compare April 6, 2023 07:05

alan-kut changed the title ~~Operator: Improve removing stale CEP entries on start.~~ Operator: Improve removing stale CEPs from CESs entries on start. Apr 6, 2023

christarazi approved these changes Apr 6, 2023

View reviewed changes

alan-kut force-pushed the fix-stale-ceps branch from 1845605 to 0cb2473 Compare April 6, 2023 19:04

christarazi force-pushed the fix-stale-ceps branch from 0cb2473 to ac0ce66 Compare April 11, 2023 07:12

squeed added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 11, 2023

dylandreimerink merged commit 80c6ebd into cilium:master Apr 11, 2023
54 of 57 checks passed

nbusseneau mentioned this pull request Jul 24, 2023

operator: Handle conflicts in CES update. #26455

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Operator: Improve removing stale CEPs from CESs entries on start. #24596

Operator: Improve removing stale CEPs from CESs entries on start. #24596

alan-kut commented Mar 28, 2023 •

edited by christarazi

AwesomePatrol Mar 28, 2023

alan-kut Mar 28, 2023

dlapcevic Mar 28, 2023

AwesomePatrol Mar 28, 2023

alan-kut Mar 28, 2023

dlapcevic Mar 28, 2023

alan-kut Mar 28, 2023

dlapcevic Mar 28, 2023

alan-kut commented Mar 31, 2023

squeed commented Mar 31, 2023

squeed commented Mar 31, 2023 •

edited by maintainer-s-little-helper bot

Test Name

Failure Output

alan-kut commented Mar 31, 2023

squeed commented Mar 31, 2023

alan-kut commented Apr 5, 2023

squeed commented Apr 5, 2023

squeed commented Apr 5, 2023

christarazi left a comment •

edited

alan-kut commented Apr 6, 2023

christarazi left a comment

alan-kut commented Apr 11, 2023

christarazi commented Apr 11, 2023 •

edited by maintainer-s-little-helper bot

Test Name

Failure Output

alan-kut commented Apr 11, 2023

squeed commented Apr 11, 2023

dylandreimerink commented Apr 11, 2023

dylandreimerink commented Apr 11, 2023

Operator: Improve removing stale CEPs from CESs entries on start. #24596

Operator: Improve removing stale CEPs from CESs entries on start. #24596

Conversation

alan-kut commented Mar 28, 2023 • edited by christarazi

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alan-kut commented Mar 31, 2023

squeed commented Mar 31, 2023

squeed commented Mar 31, 2023 • edited by maintainer-s-little-helper bot

Test Name

Failure Output

alan-kut commented Mar 31, 2023

squeed commented Mar 31, 2023

alan-kut commented Apr 5, 2023

squeed commented Apr 5, 2023

squeed commented Apr 5, 2023

christarazi left a comment • edited

Choose a reason for hiding this comment

alan-kut commented Apr 6, 2023

christarazi left a comment

Choose a reason for hiding this comment

alan-kut commented Apr 11, 2023

christarazi commented Apr 11, 2023 • edited by maintainer-s-little-helper bot

Test Name

Failure Output

alan-kut commented Apr 11, 2023

squeed commented Apr 11, 2023

dylandreimerink commented Apr 11, 2023

dylandreimerink commented Apr 11, 2023

alan-kut commented Mar 28, 2023 •

edited by christarazi

squeed commented Mar 31, 2023 •

edited by maintainer-s-little-helper bot

christarazi left a comment •

edited

christarazi commented Apr 11, 2023 •

edited by maintainer-s-little-helper bot