operator: fix identity GC collection #19649
Conversation
As the operator uses a different constructor for the GC of the allocator, this constructor was not being initialized with the min and max values of the identities that it should be GC. This commit adds those options making it possible for the Operator to GC those identities. Fixes: 0c0f965 ("operator: only GC identity keys of its own cluster") Signed-off-by: André Martins <andre@cilium.io>
aanm
added
release-note/bug
|
/test |
There was a problem hiding this comment.
The fix seems straightforward so LGTM. But looking at the linked "Fixed" PR, we seem to have different identity allocators, and an operator is required to GC identities only from their cluster. While ideally we should have a unified way of constructing the allocators to avoid such misses, but that's probably out of the scope of your PR. But at the very least, can we add some comments to the code (how min and max are set for the different allocators)?
The linked PR is very light on details in the commit description - aanm@0c0f965.
There was a problem hiding this comment.
Synced up offline with André. Looks like the linked PR that introduced this regression relied mainly on unit tests to test the fix? 😕 The unit test passes a different allocator to RunGC.
Edit : Since the regression went unnoticed until now, it's likely that we don't have any e2e tests. :(
| // The allocator can be configured by passing in additional options: | ||
| // - WithMin(id) - minimum ID to allocate (default: 1) | ||
| // - WithMax(id) - maximum ID to allocate (default max(uint64)) | ||
| func NewAllocatorForGC(backend Backend, opts ...AllocatorOption) *Allocator { |
There was a problem hiding this comment.
I guess we can prevent this from ever happening again by making the constructors take the min and max, but maybe that's not useful anymore now. Good catch.
There was a problem hiding this comment.
Aditi above slightly touched on this.
maintainer-s-little-helper
bot
moved this from Needs backport from master
to Backport pending to v1.10
in 1.11.5
aditighag
added
backport-done/1.11
maintainer-s-little-helper
bot
moved this from Backport pending to v1.10
to Backport done to v1.11
in 1.11.5
As the operator uses a different constructor for the GC of the
allocator, this constructor was not being initialized with the min and
max values of the identities that it should be GC. This commit adds
those options making it possible for the Operator to GC those
identities.
Fixes: 0c0f965 ("operator: only GC identity keys of its own cluster")
Signed-off-by: André Martins andre@cilium.io
Fixes #19648