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.

Sign up for GitHub

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

Jump to bottom

Remove unused parameter from NewCachingIdentityAllocator #25594

Merged
merged 1 commit into from May 24, 2023
Merged

Remove unused parameter from NewCachingIdentityAllocator #25594

merged 1 commit into from May 24, 2023

Conversation

giorio94
Copy link
Member

@giorio94 giorio94 commented May 22, 2023 •
edited

The NewCachingIdentityAllocator used to take a cache.Store as parameter. Yet, it is ignored since 78cbaf0 ("Optimize identity allocation with CRD backend."). Hence, let's drop it and remove the remaining occurrences.

Suggested by @tklauser: #25049 (comment)

Remove unused parameter from NewCachingIdentityAllocator

@giorio94 giorio94 added kind/cleanup This includes no functional changes. area/misc Impacts miscellaneous areas of the code not otherwise owned by another area. labels May 22, 2023
@giorio94 giorio94 requested review from a team as code owners May 22, 2023 16:34
@maintainer-s-little-helper 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 May 22, 2023
@giorio94 giorio94 added sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. release-note/misc This PR makes changes that have no direct user impact. and removed area/misc Impacts miscellaneous areas of the code not otherwise owned by another area. labels May 22, 2023
@maintainer-s-little-helper 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 May 22, 2023
@giorio94 giorio94 mentioned this pull request May 22, 2023
Merged
@giorio94 giorio94 force-pushed the mio/init-identity-allocator-cleanup branch from ef4ac56 to 4a0d114 Compare May 22, 2023 16:37
@christarazi
Copy link
Member

/test

tklauser
tklauser approved these changes May 22, 2023
View reviewed changes
Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

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

Thanks!

@giorio94
Copy link
Member Author

/ci-e2e

@giorio94
Copy link
Member Author

giorio94 commented May 23, 2023 •
edited by maintainer-s-little-helper bot

/test-1.26-net-next

Hit known flake #25605

Job 'Cilium-PR-K8s-1.26-kernel-net-next' failed:

Click to show.

Test Name

K8sDatapathConfig High-scale IPcache Test ingress policy enforcement

Failure Output

FAIL: Expected command: kubectl exec -n kube-system cilium-2gsrw -- bpftool map update pinned /sys/fs/bpf/tc/globals/cilium_world_cidrs4 key 0 0 0 0 0 0 0 0 value 1

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.26-kernel-net-next/65/

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.26-kernel-net-next so I can create one.

Then please upload the Jenkins artifacts to that issue.

@giorio94
Copy link
Member Author

One of the Cilium Conformance matrix entries failed while creating the kind cluster, with the same error as reported in #19350. Rerunning, as most likely a temporary error unrelated from Cilium.

@giorio94
Copy link
Member Author

giorio94 commented May 23, 2023 •
edited by maintainer-s-little-helper bot

/test-1.26-net-next

Failed again due to known flake: #25605

Job 'Cilium-PR-K8s-1.26-kernel-net-next' failed:

Click to show.

Test Name

K8sDatapathConfig High-scale IPcache Test ingress policy enforcement

Failure Output

FAIL: Expected command: kubectl exec -n kube-system cilium-bbbgm -- bpftool map update pinned /sys/fs/bpf/tc/globals/cilium_world_cidrs4 key 0 0 0 0 0 0 0 0 value 1

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.26-kernel-net-next/88/

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.26-kernel-net-next so I can create one.

Then please upload the Jenkins artifacts to that issue.

@giorio94
allocator: drop unused store parameter
8c7e9c8 
The `NewCachingIdentityAllocator` used to take a `cache.Store` as
parameter. Yet, it is ignored since 78cbaf0 ("Optimize
identity allocation with CRD backend."). Hence, let's drop it and
remove the remaining occurrences.

Suggested-by: Tobias Klauser <tobias@cilium.io>
Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
@giorio94 giorio94 force-pushed the mio/init-identity-allocator-cleanup branch from 4a0d114 to 8c7e9c8 Compare May 24, 2023 06:48
@giorio94
Copy link
Member Author

/test

@giorio94
Copy link
Member Author

K8s-1.26-kernel-net-next seemed to hit #25605 quite consistently. I'm attempting a rebase to see if it reproduces again (it cannot be possibly due to this PR, since it only removes an already unused parameter).

@giorio94 giorio94 mentioned this pull request May 24, 2023
Closed
@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 May 24, 2023
@squeed squeed merged commit 48f4b93 into cilium:main May 24, 2023
58 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tklauser tklauser tklauser approved these changes

@christarazi christarazi christarazi approved these changes

@YutaroHayakawa YutaroHayakawa YutaroHayakawa approved these changes

Assignees
No one assigned
Labels
kind/cleanup This includes no functional changes. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@giorio94 @christarazi @tklauser @YutaroHayakawa @squeed