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
Rename scheduler/nodeinfo pkg to scheduler/types #89703
Conversation
/assign @msau42 Michelle can you please help approve the test related changes? this is just renaming a scheduler package. |
@@ -31,7 +31,7 @@ import ( | |||
extenderv1 "k8s.io/kube-scheduler/extender/v1" | |||
schedulerapi "k8s.io/kubernetes/pkg/scheduler/apis/config" | |||
"k8s.io/kubernetes/pkg/scheduler/listers" | |||
schedulernodeinfo "k8s.io/kubernetes/pkg/scheduler/nodeinfo" | |||
schedulertypes "k8s.io/kubernetes/pkg/scheduler/types" |
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 don't need this alias in a pkg/scheduler
package. Same for the rest of the imports that don't have collisions and are within pkg/scheduler
.
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.
This is a significant refactor, which I tried to automate, it will be too much work to select the places where this conflicts and where it doesn't, and the return is not obvious. The alias type is mostly used to refer to the apimachinary one. We also used schedulerlisters, so this is not diverging too much.
for _, c := range pod.Spec.Containers { | ||
resPtr.Add(c.Resources.Requests) | ||
|
||
non0CPUReq, non0MemReq := schedutil.GetNonzeroRequests(&c.Resources.Requests) |
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.
For a followup: This package shouldn't depend in other pkg/scheduler
packages.
It looks like this function could be moved to this package https://sourcegraph.com/github.com/kubernetes/kubernetes@master/-/blob/pkg/scheduler/util/non_zero.go#L41:6&tab=references except for some import done in test/e2e/scheduling
, which should go away according to #74352
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.
yes, that will certainly make sense especially when we have the pod_info.go file in this package.
/lgtm |
/retest |
@@ -190,7 +190,7 @@ | |||
"k8s.io/kubernetes/pkg/scheduler/internal/parallelize", | |||
"k8s.io/kubernetes/pkg/scheduler/listers", | |||
"k8s.io/kubernetes/pkg/scheduler/metrics", | |||
"k8s.io/kubernetes/pkg/scheduler/nodeinfo", | |||
"k8s.io/kubernetes/pkg/scheduler/types", |
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.
It looks like you can't simply remove nodeinfo
due to the dependency chain: e2e/framework
-> kubemark
-> kubelet
-> scheduler
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.
yes, I just added it back.
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahg-g, msau42 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 |
/retest |
/retest |
/lgtm |
/retest Review the full test history for this PR. Silence the bot with an |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
renames pkg/scheduler/nodeinfo to pkg/scheduler/types. This is in preparation to introduce a new type to the scheduler named PodInfo which will be placed in this new package.
Which issue(s) this PR fixes:
Part of #89528
Does this PR introduce a user-facing change?:
/assign @alculquicondor