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

Proposal fast ipam #460

Merged
merged 3 commits into from
May 23, 2024

Conversation

ivelichkovich
Copy link
Contributor

@ivelichkovich ivelichkovich commented Apr 17, 2024

What this PR does / why we need it:

proposal for fast IPAM through node slicing. Code PR: #458

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer (optional):

Copy link
Collaborator

@maiqueb maiqueb left a comment

Choose a reason for hiding this comment

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

some initial questions that popped up.

doc/proposals/fast_ipam_by_node.md Outdated Show resolved Hide resolved
doc/proposals/fast_ipam_by_node.md Outdated Show resolved Hide resolved
doc/proposals/fast_ipam_by_node.md Show resolved Hide resolved
### Non-Goals

- Migrate all users to this new mode

Copy link
Member

Choose a reason for hiding this comment

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

Potentially we can add here "integrate reconciliation controller and the new controller" as a non-goal -- that can be left as a follow on if it adds too much overhead. I'm ok with starting with a brand new controller for this process.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I was actually thinking about adding this as a non-goal but we may want to do it in the future (with changes to the existing controller) so I left it as is for now but I can add it


Another design is that the whereabouts daemonset that runs the install-cni script could be used to have a startup and
shutdown hook which would handle the assignment of nodes to a node slice. This would require locking for the `NodeSlicePools`
on Node join. The reason to use the controller over this design is because the reconciliation pattern reduces the likelyhood for bugs (like leaked IPs)
Copy link
Member

Choose a reason for hiding this comment

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

+1

@dougbtv
Copy link
Member

dougbtv commented Apr 18, 2024

One thing I'm kind of thinking about is that the reconciler has this "audit style reconciliation" that runs on a timed fashion. I'm wondering if we might introduce resource contention with it with the fast style allocation since it won't be using the leases. Just thinking out loud

@ivelichkovich
Copy link
Contributor Author

One thing I'm kind of thinking about is that the reconciler has this "audit style reconciliation" that runs on a timed fashion. I'm wondering if we might introduce resource contention with it with the fast style allocation since it won't be using the leases. Just thinking out loud

Are you referring to the gocron running within the existing reconciler controller? That one doesn't use leases at all as far as I can tell, it calls allocation.IterateForDeallocation directly.

@dougbtv
Copy link
Member

dougbtv commented Apr 23, 2024

One thing I'm kind of thinking about is that the reconciler has this "audit style reconciliation" that runs on a timed fashion. I'm wondering if we might introduce resource contention with it with the fast style allocation since it won't be using the leases. Just thinking out loud

Are you referring to the gocron running within the existing reconciler controller? That one doesn't use leases at all as far as I can tell, it calls allocation.IterateForDeallocation directly.

So! That being the case... we don't need to worry about it. I think that might just be an existing issue, so, I will look into that, thanks for taking a peek.


- Migrate all users to this new mode

<hr>
Copy link
Member

@dougbtv dougbtv May 2, 2024

Choose a reason for hiding this comment

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

I think we're good to go on this design! Let's just add a section for next steps, may I suggest

## Phases

Initial phase:
[x] Fast ranges base functionality

Feature parity phase:
[ ] Range start, range-end function
[ ] Live changes to range (from regular to fast)
[ ] Multiple ranges

Optimization phase:
[ ] Dynamic range slice rebalancing

Definitely feel free to add / remove / edit these, just to get a placeholdre, thanks Igor!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

awesome thanks for the help! Let me just add that nice diagram as well from the maintainers call and then we should be good to go!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

woops I thought you committed this in already, just pushed the suggestions

Copy link
Member

@dougbtv dougbtv left a comment

Choose a reason for hiding this comment

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

Thanks for detailing this out!

@dougbtv dougbtv merged commit e4d9741 into k8snetworkplumbingwg:master May 23, 2024
8 of 9 checks passed
Copy link

Pull Request Test Coverage Report for Build 9023285561

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 11 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.4%) to 71.99%

Files with Coverage Reduction New Missed Lines %
cmd/whereabouts.go 11 46.99%
Totals Coverage Status
Change from base Build 8689276647: -0.4%
Covered Lines: 1136
Relevant Lines: 1578

💛 - Coveralls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants