operator: only GC identity keys of its own cluster #16825

ArthurChiao · 2021-07-08T01:24:09Z

Ref: #16805

Signed-off-by: ArthurChiao arthurchiao@hotmail.com

pkg/kvstore/allocator/allocator_test.go

pkg/option/config.go

christarazi

Thanks for the PR! A few minor comments

pkg/allocator/allocator.go

pkg/kvstore/allocator/allocator.go

tklauser · 2021-08-24T06:22:25Z

test-me-please

tklauser

LGTM

joestringer · 2021-09-13T18:33:11Z

I note that all of the Jenkins-based CI runs were successful (except test-gke), while the GHA-based CI runs were not successful. @ArthurChiao would you mind rebasing once more so we can evaluate whether the GHA-based actions were just flaky at the time or whether they were potentially revealing a real issue with the PR?

pchaigno · 2021-09-13T19:25:39Z

Rebasing should fix the current AKS error. Not sure for the rest. We do still have lots of flakes in GitHub workflows.

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

aanm · 2021-09-15T08:53:59Z

test-me-please

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

Click to show.

Test Name

K8sKafkaPolicyTest Kafka Policy Tests KafkaPolicies

Failure Output

FAIL: Kafka Pods are not ready after timeout

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.16-net-next' 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 direct routing Tests NodePort

Failure Output

FAIL: Can not connect to service "tftp://192.168.36.12:30669/hello" from outside cluster (1/10)

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

jrajahalme · 2021-10-06T11:14:14Z

@aanm Unfortunately this breaks multi-cluster with a fatal warning in the logs:

level=fatal msg="Unable to initialize Identity Allocator with backend crd" error="maximum ID must be greater than minimum ID" subsys=identity-cache

This happens whenever --cluster-id is set to non-default value (I tested with --cluster-id=1).

On the first order this is due to the new WithMin() and WithMax() functions allowing max to be set to less than the min and min to be set greater than min.

But more fundamentally, NewAllocator() should not limit the range of the IDs by itself, as its callers are passing in options to set the min, max and mask as they see fit. It appears that the fix in this PR should have been made one level up in the call stack.

As this is now this PR needs to be reverted from master.

This reverts commit e646d3e. Original PR: cilium#16825 Rationale: breaks multicluster functionality at install time with ``` level=fatal msg="Unable to initialize Identity Allocator with backend crd" error="maximum ID must be greater than minimum ID" subsys=identity-cache ``` when `--cluster-id` is set to a non-default value (e.g. `1`, as in our CI). Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>

This reverts commit e646d3e. Original PR: #16825 Rationale: breaks multicluster functionality at install time with ``` level=fatal msg="Unable to initialize Identity Allocator with backend crd" error="maximum ID must be greater than minimum ID" subsys=identity-cache ``` when `--cluster-id` is set to a non-default value (e.g. `1`, as in our CI). Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>

ArthurChiao · 2021-10-07T01:25:56Z

Thanks, I'll look at this tomorrow @jrajahalme

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 Jul 8, 2021

ArthurChiao mentioned this pull request Jul 8, 2021

cilium-operator: only GC identities of its own cluster #16805

Closed

pchaigno added area/clustermesh Relates to multi-cluster routing functionality in Cilium. kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium. labels Jul 8, 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 Jul 8, 2021

ArthurChiao force-pushed the gc_incluster_identities_only branch 3 times, most recently from 3e03023 to 93a4162 Compare July 15, 2021 08:36

ArthurChiao marked this pull request as ready for review July 15, 2021 14:40

ArthurChiao requested a review from a team July 15, 2021 14:40

ArthurChiao requested review from a team as code owners July 15, 2021 14:40

ArthurChiao requested review from jibi, twpayne and tklauser July 15, 2021 14:40

maintainer-s-little-helper bot assigned jibi, twpayne and tklauser Jul 15, 2021

pchaigno added the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Jul 27, 2021

twpayne reviewed Jul 27, 2021

View reviewed changes

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

pkg/option/config.go Outdated Show resolved Hide resolved

ArthurChiao requested a review from twpayne July 28, 2021 07:45

ArthurChiao commented Jul 28, 2021

View reviewed changes

pkg/option/config.go Outdated Show resolved Hide resolved

ArthurChiao force-pushed the gc_incluster_identities_only branch from 93a4162 to 53d27de Compare August 4, 2021 10:38

ArthurChiao requested a review from a team as a code owner August 4, 2021 10:38

ArthurChiao requested review from a team and nathanjsweet August 4, 2021 10:38

maintainer-s-little-helper bot assigned nathanjsweet Aug 4, 2021

ArthurChiao force-pushed the gc_incluster_identities_only branch from 53d27de to 8cf5c69 Compare August 5, 2021 02:43

christarazi requested changes Aug 13, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Aug 17, 2021

This comment has been minimized.

Sign in to view

ArthurChiao force-pushed the gc_incluster_identities_only branch from f7fd4e8 to f46b0ff Compare August 17, 2021 00:54

maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Aug 17, 2021

pchaigno removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Aug 17, 2021

tklauser approved these changes Aug 24, 2021

View reviewed changes

maintainer-s-little-helper bot unassigned tklauser Aug 24, 2021

twpayne approved these changes Aug 30, 2021

View reviewed changes

maintainer-s-little-helper bot unassigned twpayne Aug 30, 2021

jibi approved these changes Aug 30, 2021

View reviewed changes

maintainer-s-little-helper bot unassigned jibi Aug 30, 2021

nathanjsweet approved these changes Aug 30, 2021

View reviewed changes

maintainer-s-little-helper bot unassigned nathanjsweet Aug 30, 2021

operator: only GC identity keys of its own cluster

a78502e

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

ArthurChiao force-pushed the gc_incluster_identities_only branch from f46b0ff to a78502e Compare September 13, 2021 21:48

aanm merged commit e646d3e into cilium:master Oct 2, 2021

jrajahalme mentioned this pull request Oct 6, 2021

Cilium envoy crd #17191

Closed

9 tasks

nbusseneau mentioned this pull request Oct 6, 2021

Revert "operator: only GC identity keys of its own cluster" #17549

Merged

ArthurChiao deleted the gc_incluster_identities_only branch October 12, 2021 08:58

ArthurChiao mentioned this pull request Oct 13, 2021

Operator gc incluster identities only #17589

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

operator: only GC identity keys of its own cluster #16825

operator: only GC identity keys of its own cluster #16825

ArthurChiao commented Jul 8, 2021

christarazi left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

tklauser commented Aug 24, 2021

tklauser left a comment

joestringer commented Sep 13, 2021

pchaigno commented Sep 13, 2021

aanm commented Sep 15, 2021 •

edited by maintainer-s-little-helper bot

Test Name

Failure Output

Test Name

Failure Output

jrajahalme commented Oct 6, 2021 •

edited

ArthurChiao commented Oct 7, 2021

operator: only GC identity keys of its own cluster #16825

operator: only GC identity keys of its own cluster #16825

Conversation

ArthurChiao commented Jul 8, 2021

christarazi left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

tklauser commented Aug 24, 2021

tklauser left a comment

Choose a reason for hiding this comment

joestringer commented Sep 13, 2021

pchaigno commented Sep 13, 2021

aanm commented Sep 15, 2021 • edited by maintainer-s-little-helper bot

Test Name

Failure Output

Test Name

Failure Output

jrajahalme commented Oct 6, 2021 • edited

ArthurChiao commented Oct 7, 2021

aanm commented Sep 15, 2021 •

edited by maintainer-s-little-helper bot

jrajahalme commented Oct 6, 2021 •

edited