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

IPAM multipool followups #26138

Merged
merged 4 commits into from Jun 14, 2023

Conversation

tklauser
Copy link
Member

Address various review comments and possible cleanups from the IPAM multipool PRs #22762 #25824 and #25991.

Review per commit.

@tklauser tklauser added release-note/misc This PR makes changes that have no direct user impact. sig/ipam IP address management, including cloud IPAM area/ipam Impacts IP address management functionality. labels Jun 12, 2023
@tklauser tklauser force-pushed the pr/tklauser/ipam-allocator-create branch 2 times, most recently from 030c165 to f3dd593 Compare June 12, 2023 20:31
@tklauser tklauser changed the title IPAM multipool followups #2 IPAM multipool followups Jun 12, 2023
@tklauser tklauser force-pushed the pr/tklauser/ipam-allocator-create branch from f3dd593 to 3095ce1 Compare June 12, 2023 21:02
@tklauser
Copy link
Member Author

/test

@tklauser tklauser force-pushed the pr/tklauser/ipam-allocator-create branch from 3095ce1 to 2cb4d05 Compare June 13, 2023 12:58
@tklauser tklauser marked this pull request as ready for review June 13, 2023 19:31
@tklauser tklauser requested review from a team as code owners June 13, 2023 19:31
@tklauser
Copy link
Member Author

tklauser commented Jun 13, 2023

/test

Job 'Cilium-PR-K8s-1.26-kernel-net-next' hit: #25958 (92.28% similarity)

Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

🧹

Copy link
Contributor

@michi-covalent michi-covalent left a comment

Choose a reason for hiding this comment

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

🎩

@tklauser
Copy link
Member Author

/test-1.26-net-next

@tklauser tklauser force-pushed the pr/tklauser/ipam-allocator-create branch from 2cb4d05 to fcfa2a5 Compare June 14, 2023 08:29
@tklauser
Copy link
Member Author

Rebased to pull in fixes for the failing kafka tests (#26009)

@tklauser
Copy link
Member Author

/test

@tklauser tklauser removed the request for review from tommyp1ckles June 14, 2023 08:47
pkg/ipam/node_manager.go Outdated Show resolved Hide resolved
NodeEventHandler.Create already calls NodeEventHandler.Update in all
existing implementations. Thus, remove the Create method and rename
Update to Upsert which more clearly describes its purpose.

Also remove the unused (*Node).Update method instead of converting it to
call NodeEventHandler.Upsert. It's never called anywhere and not needed
to satisfy an interface.

Signed-off-by: Tobias Klauser <tobias@cilium.io>
@doniacld
Copy link
Contributor

Only nits! I like how you 🧹 and simplified the code! 👏🏽

Signed-off-by: Tobias Klauser <tobias@cilium.io>
ip.PrefixToIPNet implements the same functionality as the
netipx.PrefixIPNet function in the go4.org/netipx dependency that is
already vendored. Use it instead of maintaining our own copy.

Signed-off-by: Tobias Klauser <tobias@cilium.io>
In (*PoolAllocator).updateCIDRSets the CIDRs are currently parsed twice,
once using netip.ParsePrefix and once using net.ParseCIDR. Avoid this
and pass the CIDRs as []netip.Prefix and at the same time also convert
downstream functions in ipam/cidrset to take a netip.Prefix instead of
*net.IPNet.

Ref. cilium#25824 (comment)
Ref. cilium#25991 (comment)

Signed-off-by: Tobias Klauser <tobias@cilium.io>
@tklauser tklauser force-pushed the pr/tklauser/ipam-allocator-create branch from fcfa2a5 to 250e80c Compare June 14, 2023 10:26
@tklauser
Copy link
Member Author

/test

@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 Jun 14, 2023
@tklauser tklauser merged commit 68b3ae1 into cilium:main Jun 14, 2023
65 checks passed
@tklauser tklauser deleted the pr/tklauser/ipam-allocator-create branch June 14, 2023 13:27
@tklauser tklauser mentioned this pull request Jun 15, 2023
29 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ipam Impacts IP address management functionality. 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/ipam IP address management, including cloud IPAM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants