Remove duplicate CiliumNode watcher #17873

aanm · 2021-11-13T04:19:01Z

In certain cases the Cilium Operator could end up with two CiliumNode
k8s watchers. This PR re-writes the code in a way that a single
watcher will be used for all cases.

aanm · 2021-11-13T04:19:07Z

/test

aanm · 2021-11-13T21:23:28Z

/test

Job 'Cilium-PR-K8s-1.22-kernel-4.9' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sServicesTest Checks ClusterIP Connectivity Checks service on same node

Failure Output

FAIL: cannot curl to service IP from host: time-> DNS: '0.000017()', Connect: '0.000000',Transfer '0.000000', total '5.001987'command terminated with exit code 28

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-1.22-kernel-4.9 so I can create a new GitHub issue to track it.

jrajahalme

Looks OK on the top level, but I was not happy with 3 sets of handler funcs which all repeat the same conversion and error handling.

operator/cilium_node.go

jrajahalme · 2021-11-15T15:07:23Z

operator/cilium_node.go

+			}
+		} else {
+			// nodeManager not nil thus the events will be processed by
+			// ciliumNodeKVStore and the nodeManager


There is a lot of code duplication here, would it be better to only have one set of resource event handler funcs that would work both with nil and non-nil nodeManager?

There is, I was worried on optimizing the code over readbility. Please see the commit operator: simplify duplicated code in the event handling after this one where I've simplified the code a little bit.

operator/cilium_node.go

joestringer

LGTM from docs perspective. Looks like Jarno already reviewed the core functionality here, so I skipped that for now. Let me know if you would like additional review.

aanm · 2021-11-16T02:52:57Z

@joestringer @ldelossa since this code changed so much in this PR I decided to go ahead and added in the last commit to have the workqueue pattern that we have discussed a couple weeks ago. Let me know what you think. /cc @christarazi FYI

aanm · 2021-11-16T02:53:04Z

/test

aanm · 2021-11-18T02:33:09Z

/test

Job 'Cilium-PR-K8s-GKE' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sDatapathConfig Host firewall With native routing

Failure Output

FAIL: Failed to reach 10.128.0.61:80 from testclient-qx6jt

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-GKE so I can create a new GitHub issue to track it.

Job 'Cilium-PR-K8s-1.21-kernel-4.19' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sDatapathConfig Transparent encryption DirectRouting Check connectivity with transparent encryption and direct routing

Failure Output

FAIL: Connectivity test between nodes failed

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-1.21-kernel-4.19 so I can create a new GitHub issue to track it.

ldelossa

My feedback was handled. Approving contingent on other's reviews.

joestringer

LGTM. I'm sure the more I look, the more I could find ways we can incrementally tidy up the code but I think it'll just become bikeshedding at that point. 🚢

operator/cilium_node.go

aanm · 2021-11-19T16:05:45Z

/test

Job 'Cilium-PR-K8s-1.22-kernel-4.9' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sServicesTest Checks service across nodes IPv6 masquerading across K8s nodes

Failure Output

FAIL: Can not connect to url "\"http://[fd03::310]:80/\"" from pod(test-k8s2-7f96d84c65-tn2w6)

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-1.22-kernel-4.9 so I can create a new GitHub issue to track it.

aanm · 2021-11-20T00:10:45Z

/mlh new-flake Cilium-PR-K8s-1.22-kernel-4.9

👍 created #17942

aanm · 2021-11-20T00:10:53Z

/test-1.22-4.9

jrajahalme

LGTM

christarazi

Changes LGTM, one question below regarding workqueue handling.

operator/cilium_node.go

In certain cases the Cilium Operator could end up with two CiliumNode k8s watchers. This commit re-writes the code in a way that a single watcher will be used for all cases. Signed-off-by: André Martins <andre@cilium.io>

The code present in k8s_node was not specific to a k8s node. Thus, this commit moved the code into a better appropriate file, cilium_node.go Signed-off-by: André Martins <andre@cilium.io>

This flag is no longer used in the code so we can safely remove it. Fixes: 3aacf41 ("add capability to disable CNP NodeStatus updates") Signed-off-by: André Martins <andre@cilium.io>

Signed-off-by: André Martins <andre@cilium.io>

As this code suffers a major rewrite, this commit introduces the right way of handling Kubernetes events, with a workqueue. Signed-off-by: André Martins <andre@cilium.io>

Signed-off-by: André Martins <andre@cilium.io>

aanm · 2021-11-22T22:33:48Z

/test

aanm · 2021-11-22T23:34:10Z

I'll merge this PR since there was only one line added and the previous CI run was green IIRC.:

--- a/operator/cilium_node.go
+++ b/operator/cilium_node.go
@@ -232,9 +232,6 @@ func processNextWorkItem(queue workqueue.RateLimitingInterface, syncHandler func
                return true
        }
 
+       log.WithError(err).Errorf("sync %q failed with %v", key, err)
+       queue.AddRateLimited(key)
+
        return true
 }

This line will only be executed on error cases which is unlikely to be caught by our CI.

This flag is deprecated in 1.11, so remove it for 1.12 now. Relates to cilium#17873 Signed-off-by: Tam Mach <tam.mach@isovalent.com>

aanm added kind/bug This is a bug in the Cilium logic. release-note/misc This PR makes changes that have no direct user impact. labels Nov 13, 2021

aanm force-pushed the pr/fix-cilium-node-watchers branch from 2368af8 to c3eaf9d Compare November 13, 2021 21:23

aanm requested a review from jrajahalme November 14, 2021 00:33

maintainer-s-little-helper bot assigned jrajahalme Nov 14, 2021

aanm marked this pull request as ready for review November 14, 2021 00:33

aanm requested review from a team as code owners November 14, 2021 00:33

aanm requested review from joestringer and errordeveloper November 14, 2021 00:33

maintainer-s-little-helper bot assigned joestringer and errordeveloper and unassigned joestringer Nov 14, 2021

jrajahalme approved these changes Nov 15, 2021

View reviewed changes

maintainer-s-little-helper bot unassigned jrajahalme Nov 15, 2021

joestringer approved these changes Nov 15, 2021

View reviewed changes

maintainer-s-little-helper bot unassigned joestringer Nov 15, 2021

aanm force-pushed the pr/fix-cilium-node-watchers branch from c3eaf9d to 0001118 Compare November 16, 2021 02:48

aanm requested a review from ldelossa November 16, 2021 02:51

maintainer-s-little-helper bot assigned ldelossa Nov 16, 2021

aanm requested review from jrajahalme and joestringer November 16, 2021 02:51

maintainer-s-little-helper bot assigned jrajahalme and joestringer Nov 16, 2021

aanm added the area/operator Impacts the cilium-operator component label Nov 17, 2021

ldelossa approved these changes Nov 18, 2021

View reviewed changes

maintainer-s-little-helper bot unassigned ldelossa Nov 18, 2021

joestringer approved these changes Nov 18, 2021

View reviewed changes

operator/cilium_node.go Outdated Show resolved Hide resolved

maintainer-s-little-helper bot unassigned joestringer Nov 18, 2021

aanm force-pushed the pr/fix-cilium-node-watchers branch from fee2750 to 25377ca Compare November 19, 2021 16:05

jrajahalme approved these changes Nov 22, 2021

View reviewed changes

maintainer-s-little-helper bot unassigned jrajahalme Nov 22, 2021

christarazi reviewed Nov 22, 2021

View reviewed changes

operator/cilium_node.go Outdated Show resolved Hide resolved

aanm added 6 commits November 22, 2021 23:30

operator: merge duplicate k8s watchers

6f8a4e4

In certain cases the Cilium Operator could end up with two CiliumNode k8s watchers. This commit re-writes the code in a way that a single watcher will be used for all cases. Signed-off-by: André Martins <andre@cilium.io>

operator: refator code from k8s_node.go to cilium_node.go

855b271

The code present in k8s_node was not specific to a k8s node. Thus, this commit moved the code into a better appropriate file, cilium_node.go Signed-off-by: André Martins <andre@cilium.io>

operator: remove unused 'nodes-gc-interval' flag

df3623a

This flag is no longer used in the code so we can safely remove it. Fixes: 3aacf41 ("add capability to disable CNP NodeStatus updates") Signed-off-by: André Martins <andre@cilium.io>

operator: simplify duplicated code in the event handling

173c078

Signed-off-by: André Martins <andre@cilium.io>

operator: use workqueue for event handling of CiliumNodes

b7fd485

As this code suffers a major rewrite, this commit introduces the right way of handling Kubernetes events, with a workqueue. Signed-off-by: André Martins <andre@cilium.io>

operator: use context of leader election for background operations

ab6ec99

Signed-off-by: André Martins <andre@cilium.io>

aanm force-pushed the pr/fix-cilium-node-watchers branch from 25377ca to ab6ec99 Compare November 22, 2021 22:30

aanm added the release-blocker/1.11 This issue will prevent the release of the next version of Cilium. label Nov 22, 2021

aanm merged commit f313f7d into cilium:master Nov 22, 2021

aanm deleted the pr/fix-cilium-node-watchers branch November 22, 2021 23:34

aanm mentioned this pull request Nov 23, 2021

Prepare for release v1.11.0-rc3 #17960

Merged

sayboras mentioned this pull request Apr 20, 2022

operator: Remove deprecated nodes-gc-interval flag #19500

Closed

sayboras added a commit to sayboras/cilium that referenced this pull request Apr 20, 2022

operator: Remove deprecated nodes-gc-interval flag

93ffba5

This flag is deprecated in 1.11, so remove it for 1.12 now. Relates to cilium#17873 Signed-off-by: Tam Mach <tam.mach@isovalent.com>

dylandreimerink mentioned this pull request Oct 4, 2022

ipam: Fix overlapping/duplicate PodCIDR allocation when nodes are added while operator is down #21526

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove duplicate CiliumNode watcher #17873

Remove duplicate CiliumNode watcher #17873

aanm commented Nov 13, 2021

aanm commented Nov 13, 2021

aanm commented Nov 13, 2021 •

edited by maintainer-s-little-helper bot

Test Name

Failure Output

jrajahalme left a comment

jrajahalme Nov 15, 2021

aanm Nov 16, 2021

joestringer left a comment

aanm commented Nov 16, 2021

aanm commented Nov 16, 2021

aanm commented Nov 18, 2021 •

edited by maintainer-s-little-helper bot

Test Name

Failure Output

Test Name

Failure Output

ldelossa left a comment •

edited

joestringer left a comment

aanm commented Nov 19, 2021 •

edited by maintainer-s-little-helper bot

Test Name

Failure Output

aanm commented Nov 20, 2021 •

edited by maintainer-s-little-helper bot

aanm commented Nov 20, 2021

jrajahalme left a comment

christarazi left a comment

aanm commented Nov 22, 2021

aanm commented Nov 22, 2021

Remove duplicate CiliumNode watcher #17873

Remove duplicate CiliumNode watcher #17873

Conversation

aanm commented Nov 13, 2021

aanm commented Nov 13, 2021

aanm commented Nov 13, 2021 • edited by maintainer-s-little-helper bot

Test Name

Failure Output

jrajahalme left a comment

Choose a reason for hiding this comment

jrajahalme Nov 15, 2021

Choose a reason for hiding this comment

aanm Nov 16, 2021

Choose a reason for hiding this comment

joestringer left a comment

Choose a reason for hiding this comment

aanm commented Nov 16, 2021

aanm commented Nov 16, 2021

aanm commented Nov 18, 2021 • edited by maintainer-s-little-helper bot

Test Name

Failure Output

Test Name

Failure Output

ldelossa left a comment • edited

Choose a reason for hiding this comment

joestringer left a comment

Choose a reason for hiding this comment

aanm commented Nov 19, 2021 • edited by maintainer-s-little-helper bot

Test Name

Failure Output

aanm commented Nov 20, 2021 • edited by maintainer-s-little-helper bot

aanm commented Nov 20, 2021

jrajahalme left a comment

Choose a reason for hiding this comment

christarazi left a comment

Choose a reason for hiding this comment

aanm commented Nov 22, 2021

aanm commented Nov 22, 2021

aanm commented Nov 13, 2021 •

edited by maintainer-s-little-helper bot

aanm commented Nov 18, 2021 •

edited by maintainer-s-little-helper bot

ldelossa left a comment •

edited

aanm commented Nov 19, 2021 •

edited by maintainer-s-little-helper bot

aanm commented Nov 20, 2021 •

edited by maintainer-s-little-helper bot