Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pkg/allocator: Improve 'Key allocation attempt failed' handling for C… #28810

Merged
merged 3 commits into from Oct 31, 2023

Conversation

aanm
Copy link
Member

@aanm aanm commented Oct 26, 2023

…RD mode

In CRD mode, the Cilium agent uses CRD to create identities. After an identity is created, the agent acquires a reference for that key. This involves fetching the CRD from the local Kubernetes cache and checking for an annotation applied by cilium-operator to mark the identity for deletion. However, there may be a delay before the Cilium Identity is cached locally, leading to the 'Key allocation attempt failed' error. This patch ensures that we fallback to the newly allocated Cilium Identity if it's not found in the Kubernetes cache.

Fix false positives of 'Key allocation attempt failed' in CRD mode

Fixes #11487

@aanm aanm added kind/bug This is a bug in the Cilium logic. sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. release-note/bug This PR fixes an issue in a previous release of Cilium. affects/v1.12 This issue affects v1.12 branch affects/v1.13 This issue affects v1.13 branch affects/v1.14 This issue affects v1.14 branch needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Oct 26, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.14.4 Oct 26, 2023
@aanm
Copy link
Member Author

aanm commented Oct 26, 2023

/test

@aanm aanm force-pushed the pr/fix-key-allocator-attempt-error branch from fb3a9f8 to c271b4b Compare October 26, 2023 13:56
@aanm
Copy link
Member Author

aanm commented Oct 26, 2023

/test

@aanm aanm marked this pull request as ready for review October 26, 2023 20:57
@aanm aanm requested review from a team as code owners October 26, 2023 20:57
pkg/k8s/identitybackend/identity.go Outdated Show resolved Hide resolved
pkg/allocator/allocator.go Outdated Show resolved Hide resolved
@aanm aanm force-pushed the pr/fix-key-allocator-attempt-error branch from c271b4b to 7a3f044 Compare October 30, 2023 12:57
@aanm
Copy link
Member Author

aanm commented Oct 30, 2023

/test

@aanm aanm requested a review from giorio94 October 30, 2023 12:57
Copy link
Member

@giorio94 giorio94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of nits inline.

About "Instead of storing the entire CiliumIdentities object, it should be sufficient to have a Created flag (or something similar) only.", do you think it could be make sense or not being worthy?

pkg/allocator/allocator.go Outdated Show resolved Hide resolved
pkg/allocator/allocator.go Outdated Show resolved Hide resolved
pkg/k8s/identitybackend/identity.go Outdated Show resolved Hide resolved
pkg/identity/key/global_identity.go Outdated Show resolved Hide resolved
…RD mode

In CRD mode, the Cilium agent uses CRD to create identities. After an
identity is created, the agent acquires a reference for that key. This
involves fetching the CRD from the local Kubernetes cache and checking
for an annotation applied by cilium-operator to mark the identity for
deletion. However, there may be a delay before the Cilium Identity is
cached locally, leading to the 'Key allocation attempt failed' error. This
patch ensures that we fallback to the newly allocated Cilium Identity if
it's not found in the Kubernetes cache.

Signed-off-by: André Martins <andre@cilium.io>
We don't need to always DeepCopy Cilium Identity. We just need to
perform that operation if we are going perform writes.

Signed-off-by: André Martins <andre@cilium.io>
Renamed 'slave' to 'secondary' in the error messages that are presented
to users.

Signed-off-by: André Martins <andre@cilium.io>
@aanm aanm force-pushed the pr/fix-key-allocator-attempt-error branch from 7a3f044 to ee9ce2c Compare October 30, 2023 13:56
@aanm
Copy link
Member Author

aanm commented Oct 30, 2023

About "Instead of storing the entire CiliumIdentities object, it should be sufficient to have a Created flag (or something similar) only.", do you think it could be make sense or not being worthy?

Idk TBH, the "created" flag doesn't provide the state which is also what I was going for. Have in mind that although it's storing the entire CI object, that Key will be GC since it's only used here.

@aanm aanm requested a review from giorio94 October 30, 2023 13:57
@aanm
Copy link
Member Author

aanm commented Oct 30, 2023

/test

@giorio94
Copy link
Member

Idk TBH, the "created" flag doesn't provide the state which is also what I was going for. Have in mind that although it's storing the entire CI object, that Key will be GC since it's only used here.

Yeah, I saw that it is currently not stored anywhere. I was just a bit concerned that at a certain point we start storing it for other reasons, and we end up with bloated objects. Mostly curiosity though, makes sense as approach to me given the current structure.

Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks ✅

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 31, 2023
@aditighag aditighag merged commit 3b2e1ad into cilium:main Oct 31, 2023
60 of 62 checks passed
@aanm aanm deleted the pr/fix-key-allocator-attempt-error branch October 31, 2023 19:27
@aanm aanm added backport/author The backport will be carried out by the author of the PR. and removed backport/author The backport will be carried out by the author of the PR. labels Nov 3, 2023
@aanm aanm mentioned this pull request Nov 8, 2023
2 tasks
@aanm aanm added backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. and removed needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Nov 8, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from main to Backport pending to v1.14 in 1.14.4 Nov 8, 2023
@aanm aanm added the backport/author The backport will be carried out by the author of the PR. label Nov 8, 2023
@github-actions github-actions bot added backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. and removed backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. labels Nov 9, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.14 to Backport done to v1.14 in 1.14.4 Nov 9, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed this from Backport done to v1.14 in 1.14.4 Nov 9, 2023
@julianwiedmann julianwiedmann added backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed affects/v1.13 This issue affects v1.13 branch affects/v1.14 This issue affects v1.14 branch labels Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects/v1.12 This issue affects v1.12 branch backport/author The backport will be carried out by the author of the PR. backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. kind/bug This is a bug in the Cilium logic. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium. sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Key allocation attempt failed" on first attempt counted as warning
7 participants