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
Priorities use SharedLister interface instead of NodeInfo Map #84449
Conversation
08ba856
to
29120e6
Compare
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahg-g The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This is ready, but depends on #84389 |
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.
LGTM. Just a nit.
@@ -704,7 +704,7 @@ func (g *genericScheduler) podFitsOnNode( | |||
func PrioritizeNodes( | |||
ctx context.Context, | |||
pod *v1.Pod, | |||
nodeNameToInfo map[string]*schedulernodeinfo.NodeInfo, | |||
snapshot *nodeinfosnapshot.Snapshot, |
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.
We can make the parameter as sharedLister schedulerlisters.SharedLister
.
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.
genericScheduler
is aware of Snapshot
, it is actually the one that updates it, so I think it is a little confusing to use Snapshot
in some part of the code here and SharedLister
in others. I think a potential useful refactoring is to make PrioritizeNodes
a member function of genericScheduler
, I can do that in a followup PR.
29120e6
to
c6baa26
Compare
Rebased. |
/retest |
/lgtm |
/retest |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Updates Priorities, including PriorityMetadataProducer and related plugins to use SharedLister instead of NodeInfo Map. This is part of cleaning up the dependency of plugins on framework.NodeInfoSnapshot and replace it with SharedLister.
Which issue(s) this PR fixes:
Fixes #84448