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.

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

Add a scheduler profile level parameter percentageOfNodesToScore #112521

Merged
merged 1 commit into from
Oct 17, 2022

Conversation

yuanchen8911
Copy link
Member

@yuanchen8911 yuanchen8911 commented Sep 16, 2022

What type of PR is this?

/kind feature
/kind api-change

What this PR does / why we need it:

Add percentageOfNodesToScore as a per scheduler profile parameter in API version v1.

The PR makes the following changes:

  • Add profile level PercentageOfNodesToScore to API Version v1. v1beta2and v1beta3 will still use the old way (global value). In v1, if a profile level percentageOfNodesToScore is set, it will override the global (scheduler level) value. If the profile percentageOfNodesToScore is not set and the global percentageOfNodesToScore is set, the global value will be used. Otherwise, the default percentageOfNodesToScore is used.

  • Change both scheduler and profile level PercentageOfNodesToScore to pointers.

The goal is to support fine-grained per profile/workload scheduling customization and better balance scheduling performance and quality in multi-tenant Kubernetes clusters.

Which issue(s) this PR fixes:

Fixes # #93270

Reference:#95709 #95823
Previous PR: #97263 (incomplete)

Special notes for your reviewer:

We'll review and evaluate if the current global percentageOfNodesToScore should be deprecated and removed in the future versions.

TODO (future PRs)

  • Replace deprecated pointer.Int32Ptr/Int64Ptr with pointer.Int32/Int64 in v1.
  • Replace errorString by field.ErrorList so the coverage is more comprehensive.
  • Replace PluginConfig variable with defaults.PluginConfigs when it applies in apis/config/scheme/scheme_test.go.

Does this PR introduce a user-facing change?

Add percentageOfNodesToScore as a scheduler profile level parameter to API version v1. If a profile percentageOfNodesToScore is set, it will override global percentageOfNodesToScore. 

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 16, 2022
@k8s-ci-robot k8s-ci-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Sep 16, 2022
@k8s-ci-robot k8s-ci-robot added sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Sep 16, 2022
@yuanchen8911
Copy link
Member Author

/cc @Huang-Wei @ahg-g

@yuanchen8911
Copy link
Member Author

yuanchen8911 commented Sep 16, 2022

@Huang-Wei got a bunch of errors. Should I make changes to config/v1beta2 and v1beta3 too? Local testing and build worked fine. Thanks.

E0916 20:35:29.981766    6344 conversion.go:756] Warning: could not find nor generate a final Conversion function for k8s.io/kubernetes/pkg/scheduler/apis/config.KubeSchedulerProfile -> k8s.io/kube-scheduler/config/v1beta2.KubeSchedulerProfile
[33](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/112521/pull-kubernetes-verify-govet-levee/1570872862435184640#1:build-log.txt%3A33)
E0916 20:35:29.981863    6344 conversion.go:757]   the following fields need manual conversion:
[34](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/112521/pull-kubernetes-verify-govet-levee/1570872862435184640#1:build-log.txt%3A34)
E0916 20:35:29.981868    6344 conversion.go:759]       - PercentageOfNodesToScore
[35](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/112521/pull-kubernetes-verify-govet-levee/1570872862435184640#1:build-log.txt%3A35)
E0916 20:35:30.022813    6344 conversion.go:756] Warning: could not find nor generate a final Conversion function for k8s.io/kubernetes/pkg/scheduler/apis/config.KubeSchedulerProfile -> k8s.io/kube-scheduler/config/v1beta3.KubeSchedulerProfile
[36](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/112521/pull-kubernetes-verify-govet-levee/1570872862435184640#1:build-log.txt%3A36)
E0916 20:35:30.022849    6344 conversion.go:757]   the following fields need manual conversion:
[37](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/112521/pull-kubernetes-verify-govet-levee/1570872862435184640#1:build-log.txt%3A37)
E0916 20:35:30.022855    6344 conversion.go:759]       - PercentageOfNodesToScore

@yuanchen8911 yuanchen8911 changed the title Change percentageOfNodesToScore to a scheduler profile config parameter Change percentageOfNodesToScore to a profile config parameter Sep 16, 2022
@yuanchen8911 yuanchen8911 changed the title Change percentageOfNodesToScore to a profile config parameter Change percentageOfNodesToScore to a profile config parameter (WIP) Sep 16, 2022
@k8s-triage-robot
Copy link

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

@Huang-Wei
Copy link
Member

@yuanchen8911 you need to update the spec in versioned types.go as well: staging/src/k8s.io/kube-scheduler/config/v1/types.go. Then rerun codegen script, and then optionally, fill the conversion logic if the auto-generated is not smart enough to cover it for you.

@k8s-ci-robot k8s-ci-robot added area/code-generation sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Sep 16, 2022
@yuanchen8911 yuanchen8911 changed the title Change percentageOfNodesToScore to a profile config parameter (WIP) Make percentageOfNodesToScore a profile config parameter (WIP) Sep 16, 2022
@yuanchen8911 yuanchen8911 force-pushed the profile-config branch 2 times, most recently from 4d791f4 to fb58e03 Compare September 17, 2022 02:13
@yuanchen8911 yuanchen8911 changed the title Make percentageOfNodesToScore a profile config parameter (WIP) Make percentageOfNodesToScore a profile configuration parameter Sep 17, 2022
@yuanchen8911
Copy link
Member Author

@yuanchen8911 you need to update the spec in versioned types.go as well: staging/src/k8s.io/kube-scheduler/config/v1/types.go. Then rerun codegen script, and then optionally, fill the conversion logic if the auto-generated is not smart enough to cover it for you.

Fixed manual conversions.

@k8s-ci-robot
Copy link
Contributor

@yuanchen8911: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

  • /test pull-kubernetes-conformance-kind-ga-only-parallel
  • /test pull-kubernetes-coverage-unit
  • /test pull-kubernetes-dependencies
  • /test pull-kubernetes-dependencies-go-canary
  • /test pull-kubernetes-e2e-gce
  • /test pull-kubernetes-e2e-gce-100-performance
  • /test pull-kubernetes-e2e-gce-big-performance
  • /test pull-kubernetes-e2e-gce-canary
  • /test pull-kubernetes-e2e-gce-network-proxy-http-connect
  • /test pull-kubernetes-e2e-gce-no-stage
  • /test pull-kubernetes-e2e-gce-scale-performance-manual
  • /test pull-kubernetes-e2e-gce-ubuntu-containerd
  • /test pull-kubernetes-e2e-gce-ubuntu-containerd-canary
  • /test pull-kubernetes-e2e-kind
  • /test pull-kubernetes-e2e-kind-ipv6
  • /test pull-kubernetes-integration
  • /test pull-kubernetes-integration-go-canary
  • /test pull-kubernetes-kubemark-e2e-gce-scale
  • /test pull-kubernetes-node-e2e-containerd
  • /test pull-kubernetes-typecheck
  • /test pull-kubernetes-unit
  • /test pull-kubernetes-unit-go-canary
  • /test pull-kubernetes-update
  • /test pull-kubernetes-verify
  • /test pull-kubernetes-verify-go-canary
  • /test pull-kubernetes-verify-govet-levee

The following commands are available to trigger optional jobs:

  • /test check-dependency-stats
  • /test pull-ci-kubernetes-unit-windows
  • /test pull-e2e-gce-cloud-provider-disabled
  • /test pull-kubernetes-conformance-image-test
  • /test pull-kubernetes-conformance-kind-ga-only
  • /test pull-kubernetes-conformance-kind-ipv6-parallel
  • /test pull-kubernetes-cross
  • /test pull-kubernetes-e2e-capz-azure-disk
  • /test pull-kubernetes-e2e-capz-azure-disk-vmss
  • /test pull-kubernetes-e2e-capz-azure-file
  • /test pull-kubernetes-e2e-capz-azure-file-vmss
  • /test pull-kubernetes-e2e-capz-conformance
  • /test pull-kubernetes-e2e-capz-ha-control-plane
  • /test pull-kubernetes-e2e-capz-windows-containerd
  • /test pull-kubernetes-e2e-containerd-gce
  • /test pull-kubernetes-e2e-gce-alpha-features
  • /test pull-kubernetes-e2e-gce-correctness
  • /test pull-kubernetes-e2e-gce-csi-serial
  • /test pull-kubernetes-e2e-gce-device-plugin-gpu
  • /test pull-kubernetes-e2e-gce-kubetest2
  • /test pull-kubernetes-e2e-gce-network-proxy-grpc
  • /test pull-kubernetes-e2e-gce-storage-disruptive
  • /test pull-kubernetes-e2e-gce-storage-slow
  • /test pull-kubernetes-e2e-gce-storage-snapshot
  • /test pull-kubernetes-e2e-gce-ubuntu-containerd-serial
  • /test pull-kubernetes-e2e-gci-gce-autoscaling
  • /test pull-kubernetes-e2e-gci-gce-ingress
  • /test pull-kubernetes-e2e-gci-gce-ipvs
  • /test pull-kubernetes-e2e-kind-canary
  • /test pull-kubernetes-e2e-kind-dual-canary
  • /test pull-kubernetes-e2e-kind-ipv6-canary
  • /test pull-kubernetes-e2e-kind-ipvs-dual-canary
  • /test pull-kubernetes-e2e-kind-multizone
  • /test pull-kubernetes-e2e-kops-aws
  • /test pull-kubernetes-e2e-ubuntu-gce-network-policies
  • /test pull-kubernetes-kind-dra
  • /test pull-kubernetes-kubemark-e2e-gce-big
  • /test pull-kubernetes-local-e2e
  • /test pull-kubernetes-node-crio-cgrpv2-e2e
  • /test pull-kubernetes-node-crio-cgrpv2-e2e-kubetest2
  • /test pull-kubernetes-node-crio-e2e
  • /test pull-kubernetes-node-crio-e2e-kubetest2
  • /test pull-kubernetes-node-e2e-containerd-alpha-features
  • /test pull-kubernetes-node-e2e-containerd-features
  • /test pull-kubernetes-node-e2e-containerd-features-kubetest2
  • /test pull-kubernetes-node-e2e-containerd-kubetest2
  • /test pull-kubernetes-node-kubelet-credential-provider
  • /test pull-kubernetes-node-kubelet-serial-containerd
  • /test pull-kubernetes-node-kubelet-serial-containerd-kubetest2
  • /test pull-kubernetes-node-kubelet-serial-cpu-manager
  • /test pull-kubernetes-node-kubelet-serial-cpu-manager-kubetest2
  • /test pull-kubernetes-node-kubelet-serial-crio-cgroupv1
  • /test pull-kubernetes-node-kubelet-serial-crio-cgroupv2
  • /test pull-kubernetes-node-kubelet-serial-hugepages
  • /test pull-kubernetes-node-kubelet-serial-memory-manager
  • /test pull-kubernetes-node-kubelet-serial-topology-manager
  • /test pull-kubernetes-node-kubelet-serial-topology-manager-kubetest2
  • /test pull-kubernetes-node-memoryqos-cgrpv2
  • /test pull-kubernetes-node-swap-fedora
  • /test pull-kubernetes-node-swap-fedora-serial
  • /test pull-kubernetes-node-swap-ubuntu-serial
  • /test pull-kubernetes-unit-experimental
  • /test pull-publishing-bot-validate

Use /test all to run the following jobs that were automatically triggered:

  • pull-kubernetes-conformance-kind-ga-only-parallel
  • pull-kubernetes-dependencies
  • pull-kubernetes-e2e-gce-100-performance
  • pull-kubernetes-e2e-gce-ubuntu-containerd
  • pull-kubernetes-e2e-kind
  • pull-kubernetes-e2e-kind-ipv6
  • pull-kubernetes-integration
  • pull-kubernetes-node-e2e-containerd
  • pull-kubernetes-typecheck
  • pull-kubernetes-unit
  • pull-kubernetes-verify
  • pull-kubernetes-verify-govet-levee

In response to this:

/retest failed

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.

@yuanchen8911
Copy link
Member Author

/test pull-kubernetes-integration

// nodes will be scored.
PercentageOfNodesToScore int32
// nodes will be scored. It is overridden by profile level PercentageOfNodesToScore.
PercentageOfNodesToScore *int32
Copy link
Member

Choose a reason for hiding this comment

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

If we want to keep the API stable, why can we change this? Because no difference to end-users?
So the conclusion is if we can keep the consistent experience to users, no breaking change, then we can make some modifications, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Keep it consistent with the profile percentage. We need pointer to tell the difference between 0 and not set (nil) for profile percentage. If it's the latter, the global percentage will be used.

Copy link
Member

@alculquicondor alculquicondor left a comment

Choose a reason for hiding this comment

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

just nits.

pkg/scheduler/apis/config/scheme/scheme_test.go Outdated Show resolved Hide resolved
SchedulerName: "default-scheduler",
PercentageOfNodesToScore: nil,
Plugins: defaults.PluginsV1beta3,
PluginConfig: []config.PluginConfig{
Copy link
Member

Choose a reason for hiding this comment

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

defaults.PluginConfigV1beta3

Copy link
Member Author

@yuanchen8911 yuanchen8911 Oct 14, 2022

Choose a reason for hiding this comment

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

It's PluginsV1beta3.

var PluginsV1beta3 = &config.Plugins{

Copy link
Member

Choose a reason for hiding this comment

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

I'm referring to this one

var PluginConfigsV1beta3 = []config.PluginConfig{

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, replaced PluginConfig: ... with PluginConfig: defaults.PluginConfigsV1beta3

tests := []struct {
name string
percentageOfNodesToScore *int32
option *schedulerOptions
Copy link
Member

Choose a reason for hiding this comment

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

create this object inside the loop

Copy link
Member Author

Choose a reason for hiding this comment

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

How?

Copy link
Member

Choose a reason for hiding this comment

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

Actually, forget about this suggestion.
Can you instead call New(..., WithPercentageOfNodesToScore) and then verify that the scheduler is created with the appropriate value?

schedulerOptions is an internal object, so it's not much worth testing at that level.

Copy link
Member Author

@yuanchen8911 yuanchen8911 Oct 14, 2022

Choose a reason for hiding this comment

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

Rewrote the test function to create a scheduler using WithPercentageOfNodesToScore and to verify its percentageOfNodesToScore is set correctly.

@alculquicondor
Copy link
Member

There are a couple of suggestions that are still not addressed

@yuanchen8911
Copy link
Member Author

yuanchen8911 commented Oct 14, 2022

There are a couple of suggestions that are still not addressed

Made the suggested changes. Please take another look. Thanks.

SchedulerName: "default-scheduler",
PercentageOfNodesToScore: nil,
Plugins: defaults.PluginsV1beta2,
PluginConfig: []config.PluginConfig{
Copy link
Member

@alculquicondor alculquicondor Oct 14, 2022

Choose a reason for hiding this comment

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

defaults.PluginConfigsV1beta2 here

Copy link
Member Author

@yuanchen8911 yuanchen8911 Oct 14, 2022

Choose a reason for hiding this comment

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

Replaced it here. Other PluginConfigs cannot use defaults.DefaultPluginConfigs unless we modify minCandidateNodesPercentage: 50 to 10 (default value) and/or bindTimeoutSeconds: 300 to 600 (default value).

There are a few places that can be improved. How about a separate PR for a complete cleanup?

Copy link
Member

Choose a reason for hiding this comment

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

yes, those sound tangential

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, added it to TODO list.

Copy link
Member

@alculquicondor alculquicondor left a comment

Choose a reason for hiding this comment

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

missed one comment :)

Fix conversion errors

Changed the order

update

update

fix manaul coversions

keep the global parameter for backward compatibility

Address Wei's comments

Fix an error

Fix issues

Add unit tests for validation

Fix a comment

Address comments

Update comments

fix verifiation errors

Add tests for scheme_test.go

Convert percentageOfNodesToScore to pointer

Fix errors

Resolve conflicts

Fix testing errors

Address Wei's comments

Revert IntPtr to Int changes

Address comments

Not overrite percentageOfNodesToScore

Fix a bug

Fix a bug

change errs to err

Fix a nit

Remove duplication

Address comments

Fix lint warning

Fix an issue

Update comments

Clean up

Address comments

Revert changes to defaults

fix unit test error

Update

Fix tests

Use default PluginConfigs
@alculquicondor
Copy link
Member

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 14, 2022
@yuanchen8911
Copy link
Member Author

/assign @liggitt

@liggitt
Copy link
Member

liggitt commented Oct 17, 2022

/approve
for config API changes

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, Huang-Wei, liggitt, yuanchen8911

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 17, 2022
@Huang-Wei
Copy link
Member

/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 17, 2022
@k8s-ci-robot k8s-ci-robot merged commit 521fbd7 into kubernetes:master Oct 17, 2022
@yuanchen8911
Copy link
Member Author

yuanchen8911 commented Oct 17, 2022

Hi @alculquicondor, the PR was merged, but there's a red X with it. Is it related to pending/failed pull-kubernetes-e2e-gce-100-performance test. Just wanted to make sure everything is fine.

Screen Shot 2022-10-17 at 1 16 06 PM

Screen Shot 2022-10-17 at 4 06 45 PM

@yuanchen8911
Copy link
Member Author

/test pull-kubernetes-e2e-gce-100-performance

@liggitt
Copy link
Member

liggitt commented Oct 18, 2022

That means the CI job passed on a batch merge that included this commit, and failed on this individual PR. That happens occasionally with flaky jobs

@yuanchen8911
Copy link
Member Author

That means the CI job passed on a batch merge that included this commit, and failed on this individual PR. That happens occasionally with flaky jobs

Thanks! So we don't need to rerun the test since it passed all tests as part of the batch merge.

@liggitt
Copy link
Member

liggitt commented Oct 18, 2022

Right

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-review Categorizes an issue or PR as actively needing an API review. approved Indicates a PR has been approved by an approver from all required OWNERS files. area/code-generation area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
Status: API review completed, 1.26
Archived in project
Development

Successfully merging this pull request may close these issues.

10 participants