-
Notifications
You must be signed in to change notification settings - Fork 7
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
feat: refactor to maintain policy state in-memory #49
base: main
Are you sure you want to change the base?
Conversation
…notations. unit tests for key functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way you implemented it is great. Thank you for your efforts!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
I've reviewed core.go
and policy.go
mostly and added comments. I'll review the other files next and add comments if any.
p.qualifiedPods = p.qualifiedPods.Delete(key) | ||
|
||
if p.PodIsManagedByPolicy(key) { | ||
p.managedPods = p.managedPods.Delete(key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know if Delete
from the set is thread safe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it is since it is implemented via map[string]struct{}
and delete(map, key)
is not thread safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you like a thread safe implementation or leave it as-is?
Co-authored-by: Anish Ramasekar <[email protected]>
Co-authored-by: Anish Ramasekar <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added few more comments!
One thing I noticed is that PreScore
now depends on cycle state set by the PreFilter
phase. If a user wants to to use the scheduler plugin only for the preScore
and score
extension points and not configure it for preFilter
and filter
the plugin will now not work. IMO we should not depend on state being set by a different extension point.
Co-authored-by: Anish Ramasekar <[email protected]>
Co-authored-by: Anish Ramasekar <[email protected]>
Co-authored-by: Anish Ramasekar <[email protected]>
Co-authored-by: Anish Ramasekar <[email protected]>
Co-authored-by: Anish Ramasekar <[email protected]>
Co-authored-by: Anish Ramasekar <[email protected]>
Co-authored-by: Anish Ramasekar <[email protected]>
Co-authored-by: Anish Ramasekar <[email protected]>
Co-authored-by: Anish Ramasekar <[email protected]>
Co-authored-by: Anish Ramasekar <[email protected]>
I've removed that dependency now. For context, I had been looking at the Volume Binding plugin which sets state data in |
@aramase what is the status of the PR? Can we merge it soon? |
Re: #25
The capacity scheduling plugin was used for frame of reference. A collection of policy information is kept in-memory on the PlacementPolicyManager. Instead of annotations, the policy info in the collection includes 2 sets:
An event handler was attached to the pod informer so pod deletion can trigger updating the in-memory version of the policy accordingly. Additionally, the custom info object is included in the state object added to cycle state as a part of
PreFilter
andPreScore
stages so thatFilter
andScore
stages have access to it as well.