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

Added Configuration to enable/disable predicates #854

Merged
merged 1 commit into from May 18, 2019
Merged

Added Configuration to enable/disable predicates #854

merged 1 commit into from May 18, 2019

Conversation

thandayuthapani
Copy link
Contributor

@thandayuthapani thandayuthapani commented May 16, 2019 •
edited

What this PR does / why we need it:
Add Configuration for predicate plugin to enable/disable predicates algorithm
Which issue(s) this PR fixes
Fixes #802

Special notes for your reviewer:
Have added enabled or disabled option only for MemoryPressurePredicate, DiskPressurePredicate, PIDPressurePredicate. If necessary it can be added to other predicates as well.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 16, 2019
@k8s-ci-robot
Copy link
Contributor

Hi @thandayuthapani. Thanks for your PR.

I'm waiting for a kubernetes-sigs or kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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 needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 16, 2019
@k82cn
Copy link
Contributor

k82cn commented May 16, 2019

/cc @hex108

hex108
hex108 reviewed May 16, 2019
View reviewed changes
Copy link
Contributor

@hex108 hex108 left a comment

Choose a reason for hiding this comment

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

Thanks!

Nit: The first commit seems not related, it will be better to send another PR for it. :)

pkg/scheduler/plugins/predicates/predicates.go Outdated

/*
User Should give predicatesEnable in this format(predicate.NodeConditionEnable, predicate.NodeUnschedulableEnable, predicate.PodTolerationEnable,
predicate.MemoryPressureEnable, predicate.DiskPressureEnable, predicate.PIDPressureEnable, predicate.PodAffinityEnable).
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no predicate.PodAffinityEnable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Add it might be better to move them to the doc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

First I was thinking whether PodAffinity Predicate should be default? Then I changed it to be default predicates, so in Doc I would have missed, will edit that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doc has also been added for users to give configuration accordingly

pkg/scheduler/plugins/predicates/predicates.go Outdated
pidPressureEnable bool
}

func initPredicateEnable() predicateEnable {
Copy link
Contributor

Choose a reason for hiding this comment

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

If all values are false, perhaps we do not need this function?

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 thought it is better to initialize those values. WIll remove function block.

@k82cn
Copy link
Contributor

k82cn commented May 17, 2019

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 17, 2019
k82cn
k82cn reviewed May 17, 2019
View reviewed changes
hack/run-e2e-kind.sh Outdated
export KIND_OPT=${KIND_OPT:=" --config ${ROOT_DIR}/hack/e2e-kind-config.yaml"}
export KA_BIN=_output/bin
export WAIT_TIME="--wait 20s"
Copy link
Contributor

Choose a reason for hiding this comment

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

only 20s there, move --wait to kind command.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed that commit from this PR and raised new PR for that commit

@thandayuthapani
Copy link
Contributor Author

Thanks!

Nit: The first commit seems not related, it will be better to send another PR for it. :)

Yes will raise new PR for that commit

hex108
hex108 reviewed May 17, 2019
View reviewed changes
Copy link
Contributor

@hex108 hex108 left a comment

Choose a reason for hiding this comment

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

LGTM

pkg/scheduler/plugins/predicates/predicates.go
*/

predicate := predicateEnable{
memoryPressureEnable: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Their default values are false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, But Initializing those values is better, that is why have initialized it. What do you suggest?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think var predicate predicateEnable is enough :) It is OK if you think we'd better initialize them explicitly.

@hex108
Copy link
Contributor

hex108 commented May 17, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 17, 2019
@k82cn
Copy link
Contributor

k82cn commented May 18, 2019

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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 May 18, 2019
@k82cn k82cn mentioned this pull request May 18, 2019
Closed
@k8s-ci-robot k8s-ci-robot merged commit 6bb37fb into kubernetes-retired:master May 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hex108 hex108 hex108 left review comments

@k82cn k82cn k82cn left review comments

@hzxuzhonghu hzxuzhonghu Awaiting requested review from hzxuzhonghu

Assignees

@hex108 hex108

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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add Configuration for predicate plugin to enable/disable predicates algorithm
4 participants
@thandayuthapani @k8s-ci-robot @k82cn @hex108