Skip to content
This repository has been archived by the owner on May 25, 2023. It is now read-only.

Check node spec for unschedulable as well #455

Merged
merged 1 commit into from
Oct 20, 2018
Merged

Check node spec for unschedulable as well #455

merged 1 commit into from
Oct 20, 2018

Conversation

jeefy
Copy link
Contributor

@jeefy jeefy commented Oct 19, 2018

Should address #453

Tested on minikube w/ v1.12.1

Let me know if I did something weird, first dive into this codebase. :)

Thanks!

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 19, 2018
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 19, 2018
k82cn
k82cn reviewed Oct 19, 2018
View reviewed changes
vendor/k8s.io/kubernetes/pkg/scheduler/algorithm/predicates/predicates.go Outdated
@@ -1549,6 +1549,14 @@ func isPodBestEffort(pod *v1.Pod) bool {
return v1qos.GetPodQOS(pod) == v1.PodQOSBestEffort
}

// Check to see if node spec is set to Schedulable or not
func CheckNodeUnschedulable(pod *v1.Pod, nodeInfo *schedulercache.NodeInfo) (bool, []algorithm.PredicateFailureReason, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

let move this func to pkg/scheduler/plugins/predicates/predicates.go; we do not modify files in vendor directory :)

k82cn
k82cn approved these changes Oct 19, 2018
View reviewed changes
Copy link
Contributor

@k82cn k82cn left a comment

Choose a reason for hiding this comment

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

LGTM overall, just nits :)

@k82cn
Copy link
Contributor

k82cn commented Oct 19, 2018

/approve

please help to address the above comments, and then I'll add lgtm label to get it merged.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jeefy, k82cn

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 19, 2018
@jeefy jeefy force-pushed the 453-check-node-spec branch from 59e2981 to e0b287f Compare October 20, 2018 04:09
@jeefy
Copy link
Contributor Author

jeefy commented Oct 20, 2018

My bad, I know better. :) Fixed/Re-tested!

Thanks!

@TravisBuddy
Copy link

Hey @jeefy,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

TravisBuddy Request Identifier: ffe65540-d41f-11e8-8287-c1a5f8036931

@k82cn
Copy link
Contributor

k82cn commented Oct 20, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 20, 2018
@k8s-ci-robot k8s-ci-robot merged commit a14807c into kubernetes-retired:master Oct 20, 2018
@jiaxuanzhou jiaxuanzhou mentioned this pull request Dec 6, 2018
Merged
kevin-wangzefeng pushed a commit to kevin-wangzefeng/scheduler that referenced this pull request Jun 28, 2019
@k8s-ci-robot
Merge pull request kubernetes-retired#455 from jeefy/453-check-node-spec
1006e74 
Check node spec for unschedulable as well
kevin-wangzefeng pushed a commit to kevin-wangzefeng/scheduler that referenced this pull request Jun 28, 2019
@k8s-ci-robot
Merge pull request kubernetes-retired#455 from jeefy/453-check-node-spec
e43b0af 
Check node spec for unschedulable as well
kevin-wangzefeng pushed a commit to kevin-wangzefeng/scheduler that referenced this pull request Jun 28, 2019
@k8s-ci-robot
Merge pull request kubernetes-retired#455 from jeefy/453-check-node-spec
194b616 
Check node spec for unschedulable as well
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@k82cn k82cn k82cn approved these changes

@jinzhejz jinzhejz Awaiting requested review from jinzhejz

Assignees

@k82cn k82cn

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 Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jeefy @k82cn @k8s-ci-robot @TravisBuddy