Operator gc incluster identities only #17589

ArthurChiao · 2021-10-13T02:26:35Z

A tentative change as a follow-up of #16825 #16825 (comment).

To make the changes clear, I added a new commit on top of the previous PR.
If all things are done, I can squash them. @jrajahalme @aanm Thanks!

ArthurChiao · 2021-10-13T07:28:43Z

level=error msg="Command execution failed" cmd="[ipset create cilium_node_set_v4 iphash family inet -exist]" error="exec: \"ipset\": executable file not found in $PATH" subsys=iptables
level=error msg="Command execution failed" cmd="[ipset add cilium_node_set_v4 10.30.0.19 -exist]" error="exec: \"ipset\": executable file not found in $PATH" subsys=iptables
level=error msg="Failed to add IP to ipset cilium_node_set_v4" error="exec: \"ipset\": executable file not found in $PATH" ipAddr=10.30.0.19 subsys=iptables
level=info msg="Envoy: Starting xDS gRPC server listening on /tmp/cilium-test-run2675004348/xds.sock" subsys=envoy-manager
level=error msg="Command execution failed" cmd="[ipset create cilium_node_set_v6 iphash family inet6 -exist]" error="exec: \"ipset\": executable file not found in $PATH" subsys=iptables
level=info msg="Adding local node to cluster" node="{travis-job-dc0ed932-82b3-4d32-8c61-8cac45737807 default [{InternalIP 10.30.0.19} {InternalIP fc00::10ca:1} {CiliumInternalIP 1.1.1.2} {CiliumInternalIP cafe::2}] 10.19.0.0/16 f00d::a13:0:0:0/96 1.1.1.192 cafe::58c2 0 local 0 map[] 6 }" subsys=nodediscovery
level=error msg="Command execution failed" cmd="[ipset add cilium_node_set_v6 fc00::10ca:1 -exist]" error="exec: \"ipset\": executable file not found in $PATH" subsys=iptables
level=error msg="Failed to add IP to ipset cilium_node_set_v6" error="exec: \"ipset\": executable file not found in $PATH" ipAddr="fc00::10ca:1" subsys=iptables
level=info msg="Initializing identity allocator" subsys=identity-cache
level=fatal msg="Unable to initialize Identity Allocator with backend kvstore" error="maximum ID must be greater than minimum ID" subsys=identity-cache

Seems Cilium agent failed with the ipset binary not found?

tklauser · 2021-10-13T12:34:30Z

@ArthurChiao could you please rebase the PR on top of latest master? The newest runtime Docker images and VM images should contain ipset.

aanm · 2021-10-13T12:52:12Z

@ArthurChiao I think CI had a legitimate failure:

ipAddr="fc00::10ca:1" subsys=iptables
level=info msg="Initializing identity allocator" subsys=identity-cache
level=fatal msg="Unable to initialize Identity Allocator with backend kvstore" error="maximum ID must be greater than minimum ID" subsys=identity-cache
level=info msg="Starting IP identity watcher" subsys=ipcache
level=info msg="Got lease ID 49ec7c778584d805" subsys=kvstore
level=info msg="Got lock lease ID 49ec7c778584d808" subsys=kvstore
level=info msg="Initial etcd session established" config= endpoints="[http://127.0.0.1:4002]" subsys=kvstore
level=info msg="Successfully verified version of etcd endpoint" config= endpoints="[http://127.0.0.1:4002]" etcdEndpoint="http://127.0.0.1:4002" subsys=kvstore version=3.3.20
FAIL	github.com/cilium/cilium/daemon/cmd	2.058s
FAIL
make: *** [Makefile:205: integration-tests] Error 1

ArthurChiao · 2021-10-13T13:14:56Z

Just rebased, I'll look at the CI failure, thanks! @aanm @tklauser

aanm · 2021-10-18T22:11:14Z

/test

kkourt

Thank you for the patch. LGTM me overall.

Would it make sense to have a struct with the max and min IDs, instead of passing them around as arguments? The initialization and checking if an id is within range could be encoded in this structure.

kkourt · 2021-10-22T14:34:56Z

pkg/allocator/allocator.go

@@ -265,11 +266,15 @@ type Backend interface {
 // After creation, IDs can be allocated with Allocate() and released with
 // Release()
 func NewAllocator(typ AllocatorKey, backend Backend, opts ...AllocatorOption) (*Allocator, error) {
+	// Calculate initial ID range with cluster ID
+	minID := option.Config.ClusterID<<identity.ClusterIDShift + 1
+	maxID := minID + (1<<identity.ClusterIDShift - 1)


Not very familiar with this code, but do ids start at 1 (not 0)?

Thanks for your comments!

Would it make sense to have a struct with the max and min IDs, instead of passing them around as arguments? The initialization and checking if an id is within range could be encoded in this structure.

I'll have a look.

Not very familiar with this code, but do ids start at 1 (not 0)?

I saw the underlying allocator has a minID of 1: https://github.com/cilium/cilium/blob/master/pkg/allocator/allocator.go#L271.

Thanks for your comments!

Would it make sense to have a struct with the max and min IDs, instead of passing them around as arguments? The initialization and checking if an id is within range could be encoded in this structure.

I'll have a look.

To be clear, this is a suggestion, not intended to block the PR. Feel free to ignore it or leave it as a followup.

Not very familiar with this code, but do ids start at 1 (not 0)?

I saw the underlying allocator has a minID of 1: https://github.com/cilium/cilium/blob/master/pkg/allocator/allocator.go#L271.

Ah, makes sense, thanks!

kkourt

From a janitor's perspective: LGTM.

jrajahalme

LGTM

pkg/kvstore/allocator/allocator.go

Fix: cilium#16805 Signed-off-by: ArthurChiao <arthurchiao@hotmail.com>

1. Calculate identity range after well-known identities are initialized 2. Calculate identity range more accurately 3. Include the invalid min/max identity ID in error log Ref: 7048088 Signed-off-by: ArthurChiao <arthurchiao@hotmail.com>

pchaigno · 2021-11-09T15:05:04Z

/test

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

Click to show.

Test Name

RuntimePrivilegedUnitTests Run Tests

Failure Output

FAIL: Failed to run privileged unit tests

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

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

Click to show.

Test Name

K8sDatapathConfig Host firewall With VXLAN

Failure Output

FAIL: Failed to reach 10.0.0.32:80 from testclient-host-7qjzm

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-1.20-kernel-5.4 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 Host firewall With VXLAN

Failure Output

FAIL: Managed to reach 192.168.36.11:69 from testserver-host-hgmh9

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.

aanm · 2021-11-11T21:27:22Z

/test-1.21-4.19

aanm · 2021-11-11T21:27:32Z

/test-1.20-5.4

aanm · 2021-11-11T21:27:42Z

/test-runtime

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

K8sServicesTest Checks service across nodes Tests NodePort BPF Tests with vxlan Tests with secondary NodePort device

Failure Output

FAIL: Request from k8s1 to service http://192.168.37.12:30915 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.

ArthurChiao requested a review from a team as a code owner October 13, 2021 02:26

ArthurChiao requested a review from a team October 13, 2021 02:26

ArthurChiao requested a review from a team as a code owner October 13, 2021 02:26

ArthurChiao requested review from a team, nathanjsweet and tklauser October 13, 2021 02:26

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 Oct 13, 2021

ArthurChiao requested a review from jrajahalme October 13, 2021 02:26

maintainer-s-little-helper bot assigned nathanjsweet, tklauser and jrajahalme Oct 13, 2021

ArthurChiao force-pushed the operator_gc_incluster_identities_only branch from b3a332c to a0dfaf8 Compare October 13, 2021 13:13

ArthurChiao force-pushed the operator_gc_incluster_identities_only branch 2 times, most recently from 4b9ff36 to c551747 Compare October 14, 2021 07:23

kkourt reviewed Oct 22, 2021

View reviewed changes

kkourt approved these changes Oct 25, 2021

View reviewed changes

kkourt requested a review from a team October 25, 2021 13:12

jrajahalme approved these changes Oct 28, 2021

View reviewed changes

maintainer-s-little-helper bot unassigned jrajahalme Oct 28, 2021

tklauser requested a review from aanm October 29, 2021 06:14

maintainer-s-little-helper bot assigned aanm Oct 29, 2021

tklauser added area/clustermesh Relates to multi-cluster routing functionality in Cilium. release-note/bug This PR fixes an issue in a previous release of Cilium. labels Oct 29, 2021

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 Oct 29, 2021

tklauser approved these changes Oct 29, 2021

View reviewed changes

maintainer-s-little-helper bot unassigned tklauser Oct 29, 2021

aanm requested changes Nov 4, 2021

View reviewed changes

pkg/kvstore/allocator/allocator.go Outdated Show resolved Hide resolved

nathanjsweet removed their request for review November 4, 2021 21:28

pchaigno unassigned nathanjsweet Nov 5, 2021

pchaigno added the kind/bug This is a bug in the Cilium logic. label Nov 5, 2021

pchaigno requested a review from aanm November 8, 2021 10:35

aanm requested changes Nov 8, 2021

View reviewed changes

pkg/kvstore/allocator/allocator.go Outdated Show resolved Hide resolved

ArthurChiao force-pushed the operator_gc_incluster_identities_only branch from c551747 to 123654b Compare November 9, 2021 02:02

ArthurChiao added 2 commits November 9, 2021 10:05

operator: only GC identity keys of its own cluster

520025f

Fix: cilium#16805 Signed-off-by: ArthurChiao <arthurchiao@hotmail.com>

ArthurChiao force-pushed the operator_gc_incluster_identities_only branch from 123654b to e3bc9c6 Compare November 9, 2021 02:05

pchaigno requested a review from aanm November 9, 2021 15:04

aanm approved these changes Nov 9, 2021

View reviewed changes

maintainer-s-little-helper bot unassigned aanm Nov 9, 2021

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

aanm mentioned this pull request Nov 23, 2021

Prepare for release v1.11.0-rc3 #17960

Merged

pchaigno mentioned this pull request Jan 11, 2022

Enabling wellKnownIdentities on v1.11.0 causes the agent to crash #18436

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Operator gc incluster identities only #17589

Operator gc incluster identities only #17589

ArthurChiao commented Oct 13, 2021

ArthurChiao commented Oct 13, 2021

tklauser commented Oct 13, 2021

aanm commented Oct 13, 2021

ArthurChiao commented Oct 13, 2021

aanm commented Oct 18, 2021

kkourt left a comment

kkourt Oct 22, 2021

ArthurChiao Oct 25, 2021

kkourt Oct 25, 2021

kkourt left a comment

jrajahalme left a comment

pchaigno commented Nov 9, 2021 •

edited by maintainer-s-little-helper bot

Test Name

Failure Output

Test Name

Failure Output

Test Name

Failure Output

aanm commented Nov 11, 2021

aanm commented Nov 11, 2021

aanm commented Nov 11, 2021 •

edited by maintainer-s-little-helper bot

Test Name

Failure Output

Operator gc incluster identities only #17589

Operator gc incluster identities only #17589

Conversation

ArthurChiao commented Oct 13, 2021

ArthurChiao commented Oct 13, 2021

tklauser commented Oct 13, 2021

aanm commented Oct 13, 2021

ArthurChiao commented Oct 13, 2021

aanm commented Oct 18, 2021

kkourt left a comment

Choose a reason for hiding this comment

kkourt Oct 22, 2021

Choose a reason for hiding this comment

ArthurChiao Oct 25, 2021

Choose a reason for hiding this comment

kkourt Oct 25, 2021

Choose a reason for hiding this comment

kkourt left a comment

Choose a reason for hiding this comment

jrajahalme left a comment

Choose a reason for hiding this comment

pchaigno commented Nov 9, 2021 • edited by maintainer-s-little-helper bot

Test Name

Failure Output

Test Name

Failure Output

Test Name

Failure Output

aanm commented Nov 11, 2021

aanm commented Nov 11, 2021

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

Test Name

Failure Output

pchaigno commented Nov 9, 2021 •

edited by maintainer-s-little-helper bot

aanm commented Nov 11, 2021 •

edited by maintainer-s-little-helper bot