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

ElasticQuota of Capacity Scheduling Api define #50

Merged
merged 1 commit into from
Sep 29, 2020

Conversation

denkensk
Copy link
Member

@denkensk denkensk commented Sep 7, 2020

Add Capacity Scheduling plugin for the new scheduler framework.
This plugin will be put in kubernetes-sigs/scheduler-plugins firstly.

Kep : #10

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 7, 2020
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 7, 2020
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 8, 2020
@denkensk denkensk force-pushed the capacity-scheduling branch 2 times, most recently from df1762a to ed186dc Compare September 8, 2020 03:36
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 8, 2020
- name: CapacityScheduling
postFilter:
enabled:
- name: CapacityScheduling
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to disable the default preemption 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.

Done thanks ~

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 18, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 22, 2020
@denkensk denkensk changed the title [WIP] Capacity Scheduling Base on Framework Capacity Scheduling Base on Framework Sep 22, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 22, 2020
@denkensk denkensk force-pushed the capacity-scheduling branch 3 times, most recently from dc7d63d to 418b521 Compare September 24, 2020 12:36
@denkensk
Copy link
Member Author

/cc @Huang-Wei @yuanchen8911

@denkensk
Copy link
Member Author

/cc @xq2005

@k8s-ci-robot
Copy link
Contributor

@denkensk: GitHub didn't allow me to request PR reviews from the following users: xq2005.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @xq2005

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.

@Huang-Wei
Copy link
Contributor

@denkensk can you split this PR into two sub-PRs? One is the API and codegen, and the second one to cover core logic and tests.

@denkensk denkensk changed the title Capacity Scheduling Base on Framework ElasticQuota of Capacity Scheduling Api define Sep 25, 2020
@denkensk
Copy link
Member Author

@Huang-Wei

can you split this PR into two sub-PRs? One is the API and codegen, and the second one to cover core logic and tests.

Thanks. I update the pr. It's easy to review.

Copy link
Contributor

@Huang-Wei Huang-Wei left a comment

Choose a reason for hiding this comment

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

Thanks @denkensk ! Comments below.

@@ -33,5 +33,5 @@ bash "${CODEGEN_PKG}"/generate-groups.sh \
all \
sigs.k8s.io/scheduler-plugins/pkg/generated \
sigs.k8s.io/scheduler-plugins/pkg/apis \
podgroup:v1alpha1 \
--go-header-file "${SCRIPT_ROOT}"/hack/boilerplate/boilerplate.generatego.txt
"podgroup:v1alpha1 capacityscheduling:v1alpha1" \
Copy link
Contributor

Choose a reason for hiding this comment

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

resourcequota:v1alpha1? In general introduction, we can say we're doing capacity scheduling, however, to make it a concrete concept/API, let's name it resourcequota.

Copy link
Member

Choose a reason for hiding this comment

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

elasticquota?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. elasticquota

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, typo...

manifests/capacityscheduling/scheduler-config.yaml Outdated Show resolved Hide resolved
pkg/apis/capacityscheduling/register.go Outdated Show resolved Hide resolved
@@ -0,0 +1,4 @@
// +k8s:deepcopy-gen=package
// +groupName=capacityscheduling.sigs.k8s.io
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// +groupName=capacityscheduling.sigs.k8s.io
// +groupName=scheduling.sigs.k8s.io

pkg/apis/capacityscheduling/v1alpha1/register.go Outdated Show resolved Hide resolved
pkg/apis/capacityscheduling/v1alpha1/types.go Outdated Show resolved Hide resolved
metav1.TypeMeta `json:",inline"`

// KubeConfigPath is the path of kubeconfig.
KubeConfigPath string `json:"kubeconfigpath,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: make it a *string in versioned pkg, so we can easily set a default value if it's nil; if it's a concrete string, we cannot tell if the zero value ("") is set by users.

Copy link
Member

@yuanchen8911 yuanchen8911 Sep 27, 2020

Choose a reason for hiding this comment

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

In general, we can set the default parameters in default.go for all plugins: CoScheduling, NodeResouresAllocatable, CapacityScheduling.

Something like

func (csa *CapacitySchedulingArgs) SetDefaults() {
   defaultPath := "/var/lib/kubernetes/kubeconfig"
    if  csa.KubeConfigPath == nil {
       csa.KubeConfigPath = &defaultPath
    }
   ...

We can then validate the parameters in validation.go.

// Validate checks and verifies the configuration parameters.
func (conf *CapacitySchedulingArgs) Validate() Error
     ...
}

Copy link
Member

Choose a reason for hiding this comment

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

line 55: // An example resource set might includes "cpu" (millicores) and "memory" (bytes)
includes -> include

Copy link
Member Author

Choose a reason for hiding this comment

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

line 55: // An example resource set might includes "cpu" (millicores) and "memory" (bytes)


thanks. I address it together in this pr.

In general, we can set the default parameters default.go for all plugins: CoScheduling, NodeResouresAllocatable, CapacityScheduling.

Done

@yuanchen8911

@denkensk denkensk closed this Sep 27, 2020
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 27, 2020
@k8s-ci-robot k8s-ci-robot reopened this Sep 27, 2020
@k8s-ci-robot
Copy link
Contributor

@denkensk: Reopened this PR.

In response to this:

/reopen

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.

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 27, 2020
@denkensk
Copy link
Member Author

@Huang-Wei @yuanchen8911
Please review it again.
Thanks

hack/update-codegen.sh Show resolved Hide resolved
podgroup:v1alpha1 \
--go-header-file "${SCRIPT_ROOT}"/hack/boilerplate/boilerplate.generatego.txt
"scheduling:v1alpha1" \
--go-header-file "${SCRIPT_ROOT}"/hack/boilerplate/boilerplate.generatego.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

Keep the ending character.

pkg/apis/scheduling/register.go Show resolved Hide resolved
@denkensk
Copy link
Member Author

/cc @Huang-Wei
Updated Please review it again.

@denkensk denkensk force-pushed the capacity-scheduling branch 2 times, most recently from 09a84c7 to fd0cc56 Compare September 29, 2020 05:42
hack/update-codegen.sh Outdated Show resolved Hide resolved
metav1.TypeMeta

// KubeConfigPath is the path of kubeconfig.
KubeConfigPath *string
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make it (internal version) string.

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

pgclientset "sigs.k8s.io/scheduler-plugins/pkg/generated/clientset/versioned"
pginformer "sigs.k8s.io/scheduler-plugins/pkg/generated/informers/externalversions/podgroup/v1alpha1"
pglister "sigs.k8s.io/scheduler-plugins/pkg/generated/listers/podgroup/v1alpha1"
schedulingv1alpha1 "sigs.k8s.io/scheduler-plugins/pkg/apis/scheduling/v1alpha1"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: the aliases can be shortened to "schedXYZ" (save us 5 characters :))

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

@denkensk
Copy link
Member Author

@Huang-Wei
Please review it again.
Thanks~

@Huang-Wei
Copy link
Contributor

Thanks!

/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 Sep 29, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: denkensk, Huang-Wei

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 Sep 29, 2020
@k8s-ci-robot k8s-ci-robot merged commit fac4fef into kubernetes-sigs:master Sep 29, 2020
swatisehgal pushed a commit to swatisehgal/scheduler-plugins that referenced this pull request Oct 11, 2022
doc: resync.md: Document release branch upstream resync strategy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants