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

support PriorityClass on frontend #53

Merged
merged 3 commits into from
Dec 26, 2021

Conversation

khalilswdp
Copy link
Contributor

@khalilswdp khalilswdp commented Dec 9, 2021

What type of PR is this?

/kind feature

What this PR does / why we need it:

Add frontend to consume api for priorityclass from backend.

Which issue(s) this PR fixes:

Related #27
Fixes #27

Special notes for your reviewer:

Just a question, was I supposed to separate PRs by making two separate branches (with separate commits only relevant for PR, like here) or make two branches that might share some code and commits: for example, I leave same previous branch and create a branch on commit where work on backend ended and branch on commit where work on frontend ended and make PRs for both? What are the best practices to follow next time?
This is the front end commits after separating them from branch previous branch (front end won't work since it requires back end changes to be present).
(Answered)

/label tide/merge-method-squash

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 9, 2021
@k8s-ci-robot k8s-ci-robot added sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 9, 2021
@khalilswdp
Copy link
Contributor Author

/assign @sanposhiho

@sanposhiho
Copy link
Member

Just a question, ....

In most cases, I think the best way is to wait to create frontend PR until backend PR is merged.
Since the specification of a new backend API is not approved until backend PR is merged, it may need to be changed even if the front-end is developed before backend PR is merged.

But, for this feature, the API for PriorityClass should be exactly the same as the API of other resources, so I think it is okay to develop web frontend before backend PR is merged.
However, as you said, this frontend changes don't work and won't be reviewed until backend PR is merged.

@sanposhiho
Copy link
Member

/label tide/merge-method-squash

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Dec 13, 2021
@sanposhiho
Copy link
Member

@khalilswdp The PR for backend API was merged.
If you rebase this branch with the latest master, you can check the behavior of this frontend change with backend.

@sanposhiho
Copy link
Member

sanposhiho commented Dec 19, 2021

@khalilswdp
We created the pull request description template.
https://raw.githubusercontent.com/kubernetes-sigs/kube-scheduler-simulator/master/.github/PULL_REQUEST_TEMPLATE.md

Could you please update the pull request description in the format of this? (like #54)

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 19, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 19, 2021
@khalilswdp
Copy link
Contributor Author

/kind feature
/label tide/merge-method-squash

@khalilswdp
Copy link
Contributor Author

Fixes #27

@sanposhiho
Copy link
Member

sanposhiho commented Dec 20, 2021

@khalilswdp Thanks for updating! 👍 I'll review it later.

Copy link
Member

@sanposhiho sanposhiho left a comment

Choose a reason for hiding this comment

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

Nice contribution. 👍
I think almost all the changes are okay.

web/components/PriorityClassList.vue Outdated Show resolved Hide resolved
Copy link
Member

@sanposhiho sanposhiho left a comment

Choose a reason for hiding this comment

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

added small comments :)

@@ -0,0 +1,5 @@
metadata:
name: default-priority-class
Copy link
Member

Choose a reason for hiding this comment

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

nits:

Suggested change
name: default-priority-class
name: priority-class

name: default-priority-class
value: 1000
globalDefault: true
description: "Default priority class for all pods"
Copy link
Member

Choose a reason for hiding this comment

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

nits: If my English is strange, please change it to a proper English sentence. (I'm not a native speaker.)

Suggested change
description: "Default priority class for all pods"
description: "This is a template priority class for all pods"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not a native english speaker myself, but it look good to me

Comment on lines 129 to 130
// if store.count = 0, name suffix is 1.
targetTemplate = priorityclassTemplate((store.count + 1).toString());
Copy link
Member

Choose a reason for hiding this comment

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

There are two Priority classes from the beginning. ("system-cluster-critical" and "system-node-critical")

Kubernetes already ships with two PriorityClasses: system-cluster-critical and system-node-critical. These are common classes and are used to ensure that critical components are always scheduled first.
https://kubernetes.io/docs/concepts/scheduling-eviction/pod-priority-preemption/

Suggested change
// if store.count = 0, name suffix is 1.
targetTemplate = priorityclassTemplate((store.count + 1).toString());
// if store.count = 2, name suffix is 1.
targetTemplate = priorityclassTemplate((store.count - 1).toString());

@khalilswdp
Copy link
Contributor Author

Hi @sanposhiho I followed the proposed suggestions

Copy link
Member

@sanposhiho sanposhiho left a comment

Choose a reason for hiding this comment

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

I approve this change. Thanks @khalilswdp!

/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 Dec 26, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: khalilswdp, sanposhiho

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 Dec 26, 2021
@k8s-ci-robot k8s-ci-robot merged commit 4abe7f1 into kubernetes-sigs:master Dec 26, 2021
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. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support PriorityClass
3 participants