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

default use kube-system namespace as policyConfigmapNamespace #61388

Merged
merged 1 commit into from Apr 2, 2018

Conversation

@zjj2wry
Member

zjj2wry commented Mar 20, 2018

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #61368

Special notes for your reviewer:
cc @kubernetes/sig-scheduling-pr-reviews

Release note:

fix scheduling policy on ConfigMap breaks without the --policy-configmap-namespace flag set
@@ -155,6 +155,8 @@ func NewOptions() (*Options, error) {
// TODO: we should fix this up better (PR 59732)
o.config.LeaderElection.LeaderElect = true
o.policyConfigMapNamespace = metav1.NamespaceSystem

This comment has been minimized.

@dixudx

dixudx Mar 20, 2018

Member

Move to L142?

This comment has been minimized.

@k82cn

k82cn Mar 20, 2018

Member

prefer to give a default value of policy-configmap-namespace; so user knows which namespace will be used by default.

This comment has been minimized.

@zjj2wry

zjj2wry Mar 20, 2018

Member
kube-scheduler -h | grep policy-configmap-namespace       
      --policy-configmap-namespace string      The namespace where policy ConfigMap is located. The system namespace will be used if this is not provided or is empty. (default "kube-system")

@k82cn cobra did it~

This comment has been minimized.

@k82cn

k82cn Mar 20, 2018

Member

I mean set default value by cobra, update the following line.

fs.StringVar(&o.policyConfigMapNamespace, "policy-configmap-namespace", o.policyConfigMapNamespace, "The namespace where policy ConfigMap is located. The system namespace will be used if this is not provided or is empty.")

This comment has been minimized.

@liggitt

liggitt Mar 21, 2018

Member

Seems like it would be better to make the namespace explicit

This comment has been minimized.

@zjj2wry

zjj2wry Mar 21, 2018

Member

ref #60366
from @dixudx 's this issue, i think use o.policyConfigMapNamespace would be better.

@bsalamat

This comment has been minimized.

Contributor

bsalamat commented Mar 20, 2018

Please add a release note as this is fixing an issue. Otherwise, LGTM.

@zjj2wry

This comment has been minimized.

Member

zjj2wry commented Mar 21, 2018

updated, ptal

@zjj2wry

This comment has been minimized.

Member

zjj2wry commented Mar 23, 2018

@@ -115,7 +115,7 @@ func (o *Options) AddFlags(fs *pflag.FlagSet) {
fs.StringVar(&o.policyConfigFile, "policy-config-file", o.policyConfigFile, "File with scheduler policy configuration. This file is used if policy ConfigMap is not provided or --use-legacy-policy-config==true")
usage := fmt.Sprintf("Name of the ConfigMap object that contains scheduler's policy configuration. It must exist in the system namespace before scheduler initialization if --use-legacy-policy-config==false. The config must be provided as the value of an element in 'Data' map with the key='%v'", componentconfig.SchedulerPolicyConfigMapKey)
fs.StringVar(&o.policyConfigMapName, "policy-configmap", o.policyConfigMapName, usage)
fs.StringVar(&o.policyConfigMapNamespace, "policy-configmap-namespace", o.policyConfigMapNamespace, "The namespace where policy ConfigMap is located. The system namespace will be used if this is not provided or is empty.")
fs.StringVar(&o.policyConfigMapNamespace, "policy-configmap-namespace", o.policyConfigMapNamespace, "The namespace where policy ConfigMap is located. The kube-system namespace will be used if this is not provided or is empty.")

This comment has been minimized.

@k82cn

k82cn Mar 28, 2018

Member

how about this?

-       fs.StringVar(&o.policyConfigMapNamespace, "policy-configmap-namespace", o.policyConfigMapNamespace, "The namespace where policy ConfigMap is located. The system namespace will be used if this is not provided or is empty.")
+       fs.StringVar(&o.policyConfigMapNamespace, "policy-configmap-namespace", metav1.NamespaceSystem, "The namespace where policy ConfigMap is located. The system namespace will be used if this is not provided or is empty.")

This comment has been minimized.

@dixudx

dixudx Apr 2, 2018

Member

Quoted from @deads2k

Binding flags to structs allows the structs to be easily unit tested with predictable default behavior. Binding them to structs and using the current value as the default value allows the defaults to be manipulated in a NewFooOptions method for consistent use in those unit tests. It also allows for future embedding which is very useful for generic things.

I prefer o.policyConfigMapNamespace, instead of metav1.NamespaceSystem.

/cc @deads2k

This comment has been minimized.

@liggitt

liggitt Apr 2, 2018

Member

I prefer o.policyConfigMapNamespace, instead of metav1.NamespaceSystem.

agree. it is unintuitive that the act of binding the flag to the struct field would change its current value.

This comment has been minimized.

@k82cn

k82cn Apr 2, 2018

Member

easily unit tested

Reasonable to me :)

@zjj2wry , sorry for that, would you help to address the comments? I'd like to get this merged :)

This comment has been minimized.

@zjj2wry

zjj2wry Apr 2, 2018

Member

ok, I think it can be used as a specification in the future.

@k82cn addressed :)

@zjj2wry

This comment has been minimized.

Member

zjj2wry commented Apr 2, 2018

@k82cn updated, ptal

@k8s-ci-robot k8s-ci-robot requested a review from deads2k Apr 2, 2018

@dixudx

This comment has been minimized.

Member

dixudx commented Apr 2, 2018

/lgtm

@k82cn

k82cn approved these changes Apr 2, 2018

@k82cn

This comment has been minimized.

Member

k82cn commented Apr 2, 2018

/approve

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Apr 2, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dixudx, k82cn, zjj2wry

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-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Apr 2, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Apr 2, 2018

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit 47a1aac into kubernetes:master Apr 2, 2018

13 of 14 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-gce
Details
cla/linuxfoundation zjj2wry authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment