Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add podTopologySpread plugin #2487

Merged
merged 1 commit into from
Sep 26, 2022
Merged

Add podTopologySpread plugin #2487

merged 1 commit into from
Sep 26, 2022

Conversation

Monokaix
Copy link
Member

@Monokaix Monokaix commented Sep 2, 2022 •
edited
Loading

add podTopologySpread plugin, imported from kubernetes.

Signed-off-by: Xuzheng Chang [email protected]

  1. new added pod spec:
      topologySpreadConstraints:
        - maxSkew: 1
          topologyKey: topology.kubernetes.io/zone
          whenUnsatisfiable: DoNotSchedule
          labelSelector:
            matchLabels:
              app: pod-topology-spread-test
  1. when set not-exist topology key, all pods are pending.
    Warning FailedScheduling 19s volcano all nodes are unavailable: 3 plugin PodTopologySpread predicates failed node(s) didn't match pod topology spread constraints (missing required label).
  2. deploy pods with replica 3, pods spread even across topology zone.
$ kubectl get po -owide
NAME                                        READY   STATUS    RESTARTS   AGE     IP             NODE          NOMINATED NODE   READINESS GATES
pod-topology-spread-test-55bf749879-8pgbw   1/1     Running   0          5m59s   172.20.0.167   10.1.16.255   <none>           <none>
pod-topology-spread-test-55bf749879-gcjdz   1/1     Running   0          5m59s   172.20.0.113   10.1.17.3     <none>           <none>
pod-topology-spread-test-55bf749879-mt2mq   1/1     Running   0          5m59s   172.20.1.134   10.1.20.94    <none>           <none>
pod-topology-spread-test-55bf749879-pgs5g   1/1     Running   0          5m59s   172.20.0.112   10.1.17.3     <none>           <none>
pod-topology-spread-test-55bf749879-vvmc7   1/1     Running   0          5m59s   172.20.1.135   10.1.20.94    <none>           <none>
pod-topology-spread-test-55bf749879-zq2s2   1/1     Running   0          5m59s   172.20.0.166   10.1.16.255   <none>           <none>

@volcano-sh-bot volcano-sh-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 2, 2022
@Monokaix Monokaix force-pushed the master branch 2 times, most recently from 0d0d76c to ac9b4e6 Compare September 2, 2022 08:22
william-wang
william-wang reviewed Sep 22, 2022
View reviewed changes
pkg/scheduler/plugins/nodeorder/nodeorder.go Outdated
balancedResourceWeight: 1,
taintTolerationWeight: 1,
imageLocalityWeight: 1,
podTopologySpreadWeight: 1,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the default weight of podTopologySpread in kube-scheduler?

Copy link
Member Author

@Monokaix Monokaix Sep 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If not set, it will be 2.

william-wang
william-wang reviewed Sep 22, 2022
View reviewed changes
pkg/scheduler/plugins/predicates/predicates.go Outdated
@@ -319,6 +320,11 @@ func (pp *predicatesPlugin) OnSessionOpen(ssn *framework.Session) {
// 7. VolumeZone
plugin, _ = volumezone.New(nil, handle)
volumeZoneFilter := plugin.(*volumezone.VolumeZone)
// 8. PodTopologySpread
// TODO: make this configurable? ref: https://kubernetes.io/docs/concepts/scheduling-eviction/topology-spread-constraints/#internal-default-constraints
Copy link
Member

@william-wang william-wang Sep 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's ok not support cluster level default constraint in this pr. you can make a explicit comment here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done for that.

william-wang
william-wang reviewed Sep 22, 2022
View reviewed changes
pkg/scheduler/plugins/nodeorder/nodeorder.go
@@ -315,6 +323,12 @@ func (pp *nodeOrderPlugin) OnSessionOpen(ssn *framework.Session) {
p, _ = tainttoleration.New(nil, handle)
taintToleration := p.(*tainttoleration.TaintToleration)

ptsArgs := &config.PodTopologySpreadArgs{
DefaultingType: config.SystemDefaulting,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make clear whether the config.SystemDefaulting impact batch workload without configuring topologySpread.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This default policy has action ScheduleAnyway when unsatisfiable, so it will not affect filter process, and the default maxSkew value has a little affect on score process.

elinx
elinx reviewed Sep 23, 2022
View reviewed changes
vendor/k8s.io/kubernetes/pkg/scheduler/framework/plugins/podtopologyspread/plugin.go
Comment on lines +121 to +124
pl.services = factory.Core().V1().Services().Lister()
pl.replicationCtrls = factory.Core().V1().ReplicationControllers().Lister()
pl.replicaSets = factory.Apps().V1().ReplicaSets().Lister()
pl.statefulSets = factory.Apps().V1().StatefulSets().Lister()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please note that these listers will not work until it's informers are registered into the shared factory, and by now it's not; check out #2500 cache.go if you want to test this plugin.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's great! thanks for reminding.

@Monokaix
add podTopologySpread plugin
a102ce0 
Signed-off-by: Xuzheng Chang <[email protected]>
william-wang
william-wang approved these changes Sep 26, 2022
View reviewed changes
Copy link
Member

@william-wang william-wang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@volcano-sh-bot volcano-sh-bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 26, 2022
@jiangkaihua
Copy link
Contributor

/lgtm

@volcano-sh-bot
Copy link
Contributor

@jiangkaihua: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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.

hwdef
hwdef reviewed Sep 26, 2022
View reviewed changes
Copy link
Member

@hwdef hwdef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

shinytang6
shinytang6 approved these changes Sep 26, 2022
View reviewed changes
Copy link
Member

@shinytang6 shinytang6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@volcano-sh-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: shinytang6, william-wang

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:
  • OWNERS [shinytang6,william-wang]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@volcano-sh-bot volcano-sh-bot merged commit 5d60215 into volcano-sh:master Sep 26, 2022
@Monokaix Monokaix mentioned this pull request Sep 28, 2022
Merged
waiterQ pushed a commit to waiterQ/volcano that referenced this pull request Oct 19, 2022
@waiterQ
merge volcano-sh#2500(add selector spread plugin) and volcano-sh#2487(…
8adec94 
…add podTopologySpread plugin) conflict;

Signed-off-by: shaoqiu <[email protected]>
@william-wang william-wang changed the title add podTopologySpread plugin Add podTopologySpread plugin Jan 8, 2023
@Monokaix Monokaix mentioned this pull request Aug 1, 2023
Closed
@Monokaix Monokaix mentioned this pull request Nov 25, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hwdef hwdef hwdef left review comments

@elinx elinx elinx left review comments

@shinytang6 shinytang6 shinytang6 approved these changes

@william-wang william-wang william-wang approved these changes

Assignees

@william-wang william-wang

@hwdef hwdef

@shinytang6 shinytang6

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Monokaix @jiangkaihua @volcano-sh-bot @william-wang @elinx @hwdef @shinytang6