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

fix inconsistent behavior of running kube-proxy on 1.12 #66830

Closed

Conversation

Huang-Wei
Copy link
Member

@Huang-Wei Huang-Wei commented Jul 31, 2018

What this PR does / why we need it:

As feature ScheduleDaemonSetPods will be promoted to beta in 1.12, it will result in a "Pending" kube-proxy pod for a fresh kubeadm installation. PS: upgrade case should not be impacted.

This PR is to make sure kubeadm behaviors the same in 1.12 on kube-proxy daemonset.

Which issue(s) this PR fixes:
Fixes part of #66831

Special notes for your reviewer:

Release note:

kube-proxy is now deployed using nodeAffinity with explicit matchExpressions.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 31, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Huang-Wei
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: fabriziopandini

If they are not already assigned, you can assign the PR to them by writing /assign @fabriziopandini in a comment when ready.

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

@Huang-Wei
Copy link
Member Author

/sig cluster-lifecycle

@MHBauer
Copy link
Contributor

MHBauer commented Jul 31, 2018

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 31, 2018
@neolit123
Copy link
Member

As feature ScheduleDaemonSetPods will be promoted to beta in 1.12

but it becomes a question if we want to enable it by default in kubeadm in 1.12.

/assign @luxas
/assign @timothysc

@Huang-Wei
Copy link
Member Author

/hold

@neolit123 we're evaluating if it's better to give a fix so as to keep the behavior consistent

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 3, 2018
@@ -108,7 +108,16 @@ spec:
- key: CriticalAddonsOnly
operator: Exists
- operator: Exists
nodeSelector:
beta.kubernetes.io/arch: {{ .Arch }}
affinity:
Copy link
Member

Choose a reason for hiding this comment

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

... Why is the previous node-selector inconsistent?

Affinity is super expensive.

/cc @bsalamat

Copy link
Member Author

Choose a reason for hiding this comment

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

@timothysc sorry for the late /hold label - we're internally evaluating changes to keep the behavior consistent. Most probably we will go that way, and then we don't need this change and will close this pr.

Copy link
Member

Choose a reason for hiding this comment

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

@timothysc as @Huang-Wei said already, we probably won't need this change. We will fix the code on the DS controller side so that the old configuration keeps working.
BTW, inter-pod affinity is expensive. Node affinity which is used here is not that expensive.

Copy link
Member

Choose a reason for hiding this comment

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

And long term we're hoping we can completely ditch this so we can just use manifest lists.
cc @dims @neolit123 @mkumatag

@Huang-Wei
Copy link
Member Author

Current kube-proxy manifest will continue to work due to #66953.

/close

@Huang-Wei Huang-Wei deleted the update-kube-proxy-manifest branch August 10, 2018 05:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/kubeadm cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants