Revendor CA against tip of k8s.io/kubernetes #2515
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
/cc @alculquicondor See new scheduler invocation code: https://github.com/kubernetes/autoscaler/pull/2515/files#diff-aa62a4a46f434572b43666b621f733efR116 |
| if err != nil { | ||
| return nil, fmt.Errorf("couldn't create scheduler using provider %q: %v", algorithmProvider, err) | ||
| defaultProviderName := schedulerconfig.SchedulerDefaultProviderName | ||
| algorithmSource := schedulerconfig.SchedulerAlgorithmSource{ |
There was a problem hiding this comment.
@ahg-g maybe we should move this to an option as well. WDYT?
There was a problem hiding this comment.
Created kubernetes/kubernetes#85140
| @@ -261,7 +228,16 @@ func (p *PredicateChecker) GetPredicateMetadata(pod *apiv1.Pod, nodeInfos map[st | |||
| if !p.enableAffinityPredicate { | |||
| return nil | |||
| } | |||
| return p.predicateMetadataProducer(pod, nodeInfos) | |||
| nodeInfoList := make([]*schedulernodeinfo.NodeInfo, 0, len(nodeInfos)) | |||
There was a problem hiding this comment.
how is nodeInfos created? Maybe you should try to use NewSnapshot or provide your own implementation of listers.SharedLister
This usage of snapshot seems to be relying too much on internals.
There was a problem hiding this comment.
Multiple versions of nodeInfos are created as part of scale-down simulation here:
It does not seem feasible to migrate the code to NewSnapshot. If possible at all major refactor of the method would be needed.
Do you think I should create our own implementation of SharedLister? It would be a copy of what you have. Yet I agree it will improve the separation of CA and scheduler.
There was a problem hiding this comment.
middle ground: if CA wants to maintain the NodeInfoMap, then we can have a NewSnapshotFromNodeInfoMap func to create the snapshot.
Note that we do a little more than just create the NodeInfoList, but also a HavePodsWithAffinityNodeInfoList: https://github.com/kubernetes/kubernetes/blob/bcb171b375df0dd9326bf5d3a23428beea1aeb89/pkg/scheduler/nodeinfo/snapshot/snapshot.go#L52
There was a problem hiding this comment.
Once kubernetes/kubernetes#85139 is merged, you should be able to use NewSnapshot.
There was a problem hiding this comment.
Thanks! Updated the PR.
There was a problem hiding this comment.
wait for this one as well kubernetes/kubernetes#85151 , should allow you to not specify algorithm source explicitly
losipiuk
force-pushed
the
lo/ca-update-vendor-master
branch
from
9bdf89a to
6a7f3da
Compare
|
/reopen |
|
@alculquicondor: Reopened this PR. In response to this:
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/test-infra repository. |
losipiuk
force-pushed
the
lo/ca-update-vendor-master
branch
from
6a7f3da to
b5ed259
Compare
…er (d87c921) Change-Id: If8a00e019477b78fc69fbbe0c498a37694d89094
losipiuk
force-pushed
the
lo/ca-update-vendor-master
branch
from
b5ed259 to
8161ca6
Compare
|
Updated |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aleksandra-malinowska 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 |
k8s-ci-robot
added
the
approved
Preparatory for 1.17 release
/assign @MaciekPytel
cc: @ahg-g (
Migrate scheduler creation to scheduler.New()commit)Fixes: #2449