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

Convert daemon ipcache usages to new ipcache async API #25749

Merged
merged 3 commits into from Jun 8, 2023
Merged

Convert daemon ipcache usages to new ipcache async API #25749

merged 3 commits into from Jun 8, 2023

Conversation

christarazi
Copy link
Member

@christarazi christarazi commented May 29, 2023 •
edited

  • daemon: Convert host IP sync to async ipcache API
  • ip: Add IPNetToPrefix() helper
  • dameon: Convert ingress restoration code to async ipcache API

Related: #21142

@christarazi christarazi added kind/enhancement This would improve or streamline existing functionality. area/daemon Impacts operation of the Cilium daemon. 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. labels May 29, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. and removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels May 29, 2023
@christarazi christarazi changed the title pr/christarazi/ipcache new daemon ingress Convert daemon ipcache usages to new ipcache async API May 29, 2023
@christarazi
Copy link
Member Author

/test

christarazi
christarazi commented May 29, 2023
View reviewed changes
daemon/cmd/daemon.go Outdated Show resolved Hide resolved
@christarazi christarazi mentioned this pull request May 29, 2023
Open
29 tasks
@christarazi christarazi force-pushed the pr/christarazi/ipcache-new-daemon-ingress branch from 2074178 to efddbe6 Compare May 30, 2023 04:46
@christarazi christarazi marked this pull request as ready for review May 30, 2023 04:46
@christarazi christarazi requested review from a team as code owners May 30, 2023 04:46
@christarazi
Copy link
Member Author

/test

meyskens
meyskens approved these changes Jun 2, 2023
View reviewed changes
Copy link
Member

@meyskens meyskens left a comment

Choose a reason for hiding this comment

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

LGTM for me :)

julianwiedmann
julianwiedmann approved these changes Jun 2, 2023
View reviewed changes
Copy link
Member

@julianwiedmann julianwiedmann left a comment

Choose a reason for hiding this comment

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

Just one non-blocking note.

daemon/cmd/datapath.go Outdated Show resolved Hide resolved
@christarazi christarazi force-pushed the pr/christarazi/ipcache-new-daemon-ingress branch 2 times, most recently from 0232093 to 1699b2b Compare June 7, 2023 00:04
@christarazi
daemon: Convert host IP sync to async ipcache API
ec2c1f9 
Switch over from using direct ipcache insertion with raw identities to a
label-based model, see GH-21142.

Signed-off-by: Chris Tarazi <chris@isovalent.com>
@christarazi
ip: Add IPNetToPrefix() helper
eace112 
This will be useful going forward when wanting to convert net.IPNet
prefixes to netip.Prefix.

Signed-off-by: Chris Tarazi <chris@isovalent.com>
@christarazi
dameon: Convert ingress restoration code to async ipcache API
f868917 
Move to the new async ipcache API, see GH-21142.

Signed-off-by: Chris Tarazi <chris@isovalent.com>
@christarazi christarazi force-pushed the pr/christarazi/ipcache-new-daemon-ingress branch from 1699b2b to f868917 Compare June 7, 2023 00:05
@christarazi
Copy link
Member Author

/test

jrajahalme
jrajahalme approved these changes Jun 8, 2023
View reviewed changes
daemon/cmd/daemon.go
} else {
log.WithError(err).Warning("could not restore Ingress IP, a new one will be allocated")
}
d.ipcache.UpsertLabels(
Copy link
Member

Choose a reason for hiding this comment

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

Nice that this can not fail any more :-)

@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 8, 2023
@dylandreimerink dylandreimerink merged commit 745cd32 into main Jun 8, 2023
61 of 62 checks passed
@dylandreimerink dylandreimerink deleted the pr/christarazi/ipcache-new-daemon-ingress branch June 8, 2023 10:50
tklauser
tklauser reviewed Jun 8, 2023
View reviewed changes
pkg/ip/cidr.go
// IPNetToPrefix is a convenience helper for migrating from the older 'net'
// standard library types to the newer 'netip' types. Use this to plug the
// new types in newer code into older types in older code during the migration.
func IPNetToPrefix(prefix *net.IPNet) netip.Prefix {
Copy link
Member

Choose a reason for hiding this comment

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

I think this equivalent to netipx.FromStdIPNet (sans the additional bool return). Given that we already vendor that library I wonder whether we could use that conversion function instead of adding our own conversion helper?

Also ref. #24246 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I forgot about this. Sending a PR to revert and fixup the use of net pkg. Thanks for catching this!

Copy link
Member Author

Choose a reason for hiding this comment

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

#26045

@tklauser tklauser mentioned this pull request Oct 25, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tklauser tklauser tklauser left review comments

@meyskens meyskens meyskens approved these changes

@dylandreimerink dylandreimerink dylandreimerink approved these changes

@jrajahalme jrajahalme jrajahalme approved these changes

@julianwiedmann julianwiedmann julianwiedmann approved these changes

Assignees
No one assigned
Labels
area/daemon Impacts operation of the Cilium daemon. kind/enhancement This would improve or streamline existing 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/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
6 participants
@christarazi @tklauser @meyskens @dylandreimerink @jrajahalme @julianwiedmann