Moving Scheduler interfaces to staging: Move PodInfo and NodeInfo interfaces (together with related types) to staging repo, leaving internal implementation in kubernetes/kubernetes/pkg/scheduler #132457
Conversation
|
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Hi @ania-borowiec. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ania-borowiec The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/ok-to-test |
| // We have it as a cache purpose to avoid re-computing which event(s) might ungate the Pod. | ||
| GetGatingPluginEvents() []ClusterEvent | ||
|
|
||
| DeepCopy() QueuedPodInfo |
There was a problem hiding this comment.
Maybe DeepCopy() is not needed to be in the interface
| // the pod's status in the scheduling queue, such as the timestamp when | ||
| // it's added to the queue. | ||
| type QueuedPodInfo interface { | ||
| GetPodInfo() PodInfo |
| // AddPodInfo adds pod information to this NodeInfo. | ||
| // Consider using this instead of AddPod if a PodInfo is already computed. | ||
| AddPodInfo(podInfo PodInfo) | ||
| // AddPod is a wrapper around AddPodInfo. | ||
| AddPod(pod *v1.Pod) | ||
| // RemovePod subtracts pod information from this NodeInfo. | ||
| RemovePod(logger klog.Logger, pod *v1.Pod) error | ||
| // SetNode sets the overall node information. | ||
| SetNode(node *v1.Node) | ||
| // RemoveNode removes the node object, leaving all other tracking information. | ||
| RemoveNode() |
There was a problem hiding this comment.
Do we need to have the Add/Set/Remove methods in the interface? Are they used by plugins?
| // GetPreferredAntiAffinityTerms returns the precomputed anti-affinity terms with weights. | ||
| GetPreferredAntiAffinityTerms() []WeightedAffinityTerm | ||
| // DeepCopy returns a deep copy of the PodInfo object. | ||
| DeepCopy() PodInfo |
There was a problem hiding this comment.
Maybe DeepCopy() is not needed to be in the interface
| // Update creates a full new PodInfo by default. And only updates the pod when the PodInfo | ||
| // has been instantiated and the passed pod is the exact same one as the original pod. | ||
| Update(pod *v1.Pod) error | ||
| // CalculateResource is only intended to be used by NodeInfo. | ||
| CalculateResource() PodResource |
There was a problem hiding this comment.
Do we need to expose these methods?
There was a problem hiding this comment.
Do we need changes in this file? It uses *schedulerframework.NodeInfo, so we don't need to use any interfaces and their methods?
There was a problem hiding this comment.
Similarly with any kubelet change
| @@ -854,6 +889,14 @@ type Resource struct { | |||
| ScalarResources map[v1.ResourceName]int64 | |||
| } | |||
|
|
|||
| func (r *Resource) GetMilliCPU() int64 { return r.MilliCPU } | |||
There was a problem hiding this comment.
Don't make single-line functions:
| func (r *Resource) GetMilliCPU() int64 { return r.MilliCPU } | |
| func (r *Resource) GetMilliCPU() int64 { | |
| return r.MilliCPU | |
| } |
| var node *v1.Node | ||
| if nodeInfo != nil { | ||
| node = nodeInfo.GetNode() | ||
| } |
| nodeInfoMap map[string]fwk.NodeInfo | ||
| // nodeInfoList is the list of nodes as ordered in the cache's nodeTree. | ||
| nodeInfoList []*framework.NodeInfo | ||
| nodeInfoList []fwk.NodeInfo | ||
| // havePodsWithAffinityNodeInfoList is the list of nodes with at least one pod declaring affinity terms. | ||
| havePodsWithAffinityNodeInfoList []*framework.NodeInfo | ||
| havePodsWithAffinityNodeInfoList []fwk.NodeInfo | ||
| // havePodsWithRequiredAntiAffinityNodeInfoList is the list of nodes with at least one pod declaring | ||
| // required anti-affinity terms. | ||
| havePodsWithRequiredAntiAffinityNodeInfoList []*framework.NodeInfo | ||
| havePodsWithRequiredAntiAffinityNodeInfoList []fwk.NodeInfo |
| @@ -986,7 +994,7 @@ func (f *frameworkImpl) runPostFilterPlugin(ctx context.Context, pl framework.Po | |||
| // and add the nominated pods. Removal of the victims is done by | |||
| // SelectVictimsOnNode(). Preempt removes victims from PreFilter state and | |||
| // NodeInfo before calling this function. | |||
| func (f *frameworkImpl) RunFilterPluginsWithNominatedPods(ctx context.Context, state fwk.CycleState, pod *v1.Pod, info *framework.NodeInfo) *framework.Status { | |||
| func (f *frameworkImpl) RunFilterPluginsWithNominatedPods(ctx context.Context, state fwk.CycleState, pod *v1.Pod, info fwk.NodeInfo) *framework.Status { | |||
There was a problem hiding this comment.
I'm wondering if we could make a type conversion here not to populate fwk.NodeInfo to internal methods? It won't be that clear, but I think, still better than the actual huge changes.
…erfaces (together with related types) to staging repo, leaving internal implementation in kubernetes/kubernetes/pkg/scheduler
a27c77b to
740a809
Compare
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
This PR depends on #132190 - I will rebase after that one is merged.
This is part of a larger change that moves interfaces and type and func defiinitions to the staging repo "k8s.io/kube-scheduler", to allow users for importing scheduler framework interfaces without importing k/k repo.
This PR moves types NodeInfo, PodInfo, QueuedPodInfo, PodResource, AffinityTerm, WeightedAffinityTerm, Resource, ImageStateSummary, ProtocolPort and HostPortInfo from k/k to the staging repo. Some types are moved as interfaces instead of concrete implementation, to keep internal implementation in k/k.
Which issue(s) this PR is related to:
Part of #89930
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: