Allow running kube-proxy as a DaemonSet when using kube-up.sh on GCE #50705

MrHohn · 2017-08-15T17:56:07Z

What this PR does / why we need it:
From #23225, this PR adds an option for user to run kube-proxy as a DaemonSet instead of static pods using GCE startup scripts. By default, kube-proxy will run as static pods.

This is the first step for moving kube-proxy into a DaemonSet in GCE, remaining tasks will be tracked on #23225.

Special notes for your reviewer:
The last commit are purely for testing out kube-proxy as daemonset via CIs.

cc @kubernetes/sig-network-misc @kubernetes/sig-cluster-lifecycle-misc

Release note (Edited):

[Experiment Only] When using kube-up.sh on GCE, user could set env `KUBE_PROXY_DAEMONSET=true` to run kube-proxy as a DaemonSet. kube-proxy is run as static pods by default.

MrHohn · 2017-08-15T20:27:26Z

Seems like I missed the debian bits, will reassign when ready.

/assign
/unassign @gmarek @zmerlynn

MrHohn · 2017-08-16T03:53:51Z

CIs passed, reassign...

/assign @gmarek @zmerlynn
/unassign

MrHohn · 2017-08-18T01:35:10Z

cluster/gce/gci/configure-helper.sh

@@ -25,6 +25,8 @@ set -o errexit
 set -o nounset
 set -o pipefail

+LOCAL_NODE_LABELS="${NODE_LABELS:-}"


Setting kube-proxy-ds-ready label only on nodes so that kube-proxy won't be scheduled on master. It is due to the fact that daemonset controller does not respect node.spec.unschedulable field and we don't set taint on master by default.

I'm a bit concerned about defining this LOCAL_NODE_LABELS env as it differs from NODE_LABELS set in kube-env and may introduce inconsistency and confusion.

Another possible option is set NoSchedule taint on master (as defined by #39112). From #43537 it seems that requires a comprehensive plan but don't have a clear timeline and owner yet. So may not want to block this on that.

cc @mikedanese @thockin

MrHohn · 2017-08-21T23:44:19Z

cluster/addons/kube-proxy/kube-proxy-ds.yaml

+# Please keep kube-proxy configuration in-sync with:
+# cluster/saltbase/salt/kube-proxy/kube-proxy.manifest
+
+apiVersion: extensions/v1beta1


Just realized we have removed support for debian masters in GCE: #41666. So I removed some unnecessary salt configuration from this addon file. cc @bowei

MrHohn · 2017-08-23T00:24:34Z

Forgot to cc sig-node as well. @kubernetes/sig-node-pr-reviews

yujuhong · 2017-08-25T20:05:25Z

kube-proxy as a DaemonSet would only be an alpha feature in 1.8. The rollback mechanism suggested by @thockin #50705 (comment) seems sufficient to me.

I agree with @dchen1107 that we should probably wait for 1.9 before making this change. This change works best when pod priority and scheduler preemption work reliably. They are both alpha features in 1.8. So, I would wait for 1.9 when they get some mileage.

Would having this as an alpha feature help test and stabilize pod priority and scheduler preemption?

dchen1107 · 2017-08-25T21:02:09Z

kube-proxy as a DaemonSet would only be an alpha feature in 1.8. The rollback mechanism suggested by @thockin #50705 (comment) seems sufficient to me.

First, no one told me that is an alpha feature. According to @MrHohn, in his original proposal, this should be alpha feature, but decided not later. Using the new logic introduced by pod priority feature is gated by an alpha. Also even we plan to make this alpha here, it will be very error-prone. Asking @bowei @gmarek how painful & messy to introduce an alpha feature gate through kube-up scripts.

To achieve @thockin's suggest, @MrHohn has to add extra complexity to his logic on already-messy-kube-up. He has to introduce two vars:

one for master: want to run kube-proxy as the daemonset, so that addon manager will create those kube-proxy daemonest spec
one for node to remove that node label, instead of populating the static pod manifest.

If sig-scalability and sig-scheduling have the confidence on the rescheduler (#50705 (comment)), and we have low chance to run into case 3), yes, let's take the risk to do this. I want to get rid of those static pods from worker nodes long time ago.

But if no such confidence, why in a rush? We don't have vertical scaling yet; we don't have GKE deployment & addon manager can integrate that feature yet. No gain in a short term, instead of risk.

bsalamat · 2017-08-25T21:20:37Z

kube-proxy as a DaemonSet would only be an alpha feature in 1.8. The rollback mechanism suggested by @thockin #50705 (comment) seems sufficient to me.
Would having this as an alpha feature help test and stabilize pod priority and scheduler preemption?

Sure, this sounds good to me from the scheduling point of view. You should make sure that sig-network is fine with this.

yujuhong · 2017-08-25T21:27:08Z

First, no one told me that is an alpha feature. According to @MrHohn, in his original proposal, this should be alpha feature, but decided not later. Using the new logic introduced by pod priority feature is gated by an alpha. Also even we plan to make this alpha here, it will be very error-prone. Asking @bowei @gmarek how painful & messy to introduce an alpha feature gate through kube-up scripts.

I believe this was intended to be an Alpha feature, but I could be wrong (of course).

I don't doubt that it's a pain to change the kube-up scripts, but isn't this necessary unless we are so confident that we can ship this as a beta/GA feature directly? If that's the preferred route (i.e., direct to beta), I agree that it's premature to introduce this feature in 1.8.

To achieve @thockin's suggest, @MrHohn has to add extra complexity to his logic on already-messy-kube-up. He has to introduce two vars:

one for master: want to run kube-proxy as the daemonset, so that addon manager will create those kube-proxy daemonest spec
one for node to remove that node label, instead of populating the static pod manifest.

Even if this goes directly to Beta, we still need to have a rollback mechanism. I don't see how we can avoid that....

Regardless whether this makes 1.8 or not, I think it might be good to add the code (as a pre-alpha experimental feature?) and start a test cluster to run kube-proxy as a DaemonSet with pod priority and scheduler preemption.

mikedanese · 2017-08-25T22:00:09Z

If this is going directly to beta, we shouldn't include a configuration option to turn it off.

mikedanese · 2017-08-25T22:11:27Z

cluster/gce/gci/configure-helper.sh

+fi
+if [[ -n "${node_labels:-}" ]]; then
+  flags+=" --node-labels=${node_labels}"
+fi


Put this in start-kubelet

Sorry, fixed now.

dchen1107 · 2017-08-26T00:19:46Z

Ok, quickly chat with @MrHohn and @bowei, so that we can move forward. We agreed upon that there is no way to make this an alpha feature in OSS k8s. But we could only enable this feature on GKE's alpha clusters if we want to.

This particular change along with other pending changes including the pod priority and preemption, the vertical scaling, etc. can give us a much stable and easy managing kube-proxy and cluster in a long run, but not in 1.8 for sure. The current state of this change doesn't bring any value to the users directly at this moment, except to introduce more complexity to GKE alpha clusters. At the end, all of us agreed that there is no value to enable this for GKE alpha clusters in 1.8 since there is no opt-in option for the user to choose which alpha features to run.

But enabling the test for the future movement is a good idea. We decided to disable KUBE_PROXY_DAEMONSET for test by default, instead, creating a separate e2e job to enable this for testing.

MrHohn · 2017-08-28T20:11:18Z

Test passed. PR ready for another round.

mikedanese · 2017-08-28T20:23:35Z

LGTM, please squash fixup commits.

…d CoreOS).

MrHohn · 2017-08-28T20:33:59Z

@mikedanese Thanks, squashed commits and made KUBE_PROXY_DAEMONSET default to false.

mikedanese · 2017-08-28T20:36:51Z

/lgtm

k8s-github-robot · 2017-08-28T20:37:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MrHohn, mikedanese

Associated issue: 23225

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~cluster/OWNERS~~ [mikedanese]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

fejta-bot · 2017-08-28T23:19:46Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

fejta-bot · 2017-08-29T03:31:46Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

k8s-github-robot · 2017-08-29T08:17:43Z

Automatic merge from submit-queue (batch tested with PRs 50932, 49610, 51312, 51415, 50705)

@roberthbailey

…-tests Automatic merge from submit-queue (batch tested with PRs 51228, 50185, 50940, 51544, 51543) Add upgrades tests for kube-proxy daemonset migration path **What this PR does / why we need it**: From kubernetes#23225, this is a part of setting up CIs to validate the kube-proxy migration path (static pods -> daemonset and reverse). The other part of the works (adding real CIs that run these tests) will be in a separate PR against [kubernetes/test-infra](https://github.com/kubernetes/test-infra). Though this is currently blocked by kubernetes#50705. **Special notes for your reviewer**: cc @roberthbailey @pwittrock **Release note**: ```release-note NONE ```

@bsalamat

Automatic merge from submit-queue (batch tested with PRs 51553, 51538, 51663, 51069, 51737) Allow enable pod priority feature gate for GCE and configure priority for kube-proxy **What this PR does / why we need it**: From #23225, this PR adds an option for user to enable pod priority feature gate using GCE startup scripts, and configure pod priority for kube-proxy when enabled. The setup `priorityClassName: system` derives from: https://github.com/kubernetes/kubernetes/blob/ce1485c626d8cddc9dbffe24f91f7d62480148cd/staging/src/k8s.io/api/core/v1/types.go#L2536-L2542 The plan is to configure pod priority for kube-proxy daemonset (#50705) in the same way. **Special notes for your reviewer**: cc @bsalamat @davidopp @thockin **Release note**: ```release-note When using kube-up.sh on GCE, user could set env `ENABLE_POD_PRIORITY=true` to enable pod priority feature gate. ```

k8s-ci-robot added sig/network Categorizes an issue or PR as relevant to SIG Network. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 15, 2017

k8s-github-robot assigned gmarek and zmerlynn Aug 15, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Aug 15, 2017

MrHohn mentioned this pull request Aug 15, 2017

move kube-proxy into a daemonset #23225

Closed

k8s-ci-robot assigned MrHohn and unassigned gmarek and zmerlynn Aug 15, 2017

MrHohn force-pushed the kube-proxy-ds branch from 91c7579 to 591a2e1 Compare August 16, 2017 01:20

k8s-ci-robot assigned gmarek and zmerlynn and unassigned MrHohn Aug 16, 2017

MrHohn mentioned this pull request Aug 17, 2017

Run kube-proxy as a daemonset in alpha feature CIs and include networking tests. kubernetes/test-infra#4104

Merged

MrHohn commented Aug 18, 2017

View reviewed changes

This was referenced Aug 18, 2017

Add upgrades tests for kube-proxy daemonset migration path #50940

Merged

Add upgrade CIs for kube-proxy daemonset migration path kubernetes/test-infra#4127

Merged

MrHohn force-pushed the kube-proxy-ds branch from 591a2e1 to 1f57b0a Compare August 21, 2017 23:27

MrHohn commented Aug 21, 2017

View reviewed changes

MrHohn force-pushed the kube-proxy-ds branch from 1f57b0a to 6337925 Compare August 22, 2017 00:15

MrHohn mentioned this pull request Aug 22, 2017

Allow enable pod priority feature gate for GCE and configure priority for kube-proxy #51069

Merged

k8s-ci-robot added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Aug 23, 2017

MrHohn force-pushed the kube-proxy-ds branch from 8f31a28 to 7890a6b Compare August 25, 2017 22:06

mikedanese reviewed Aug 25, 2017

View reviewed changes

MrHohn force-pushed the kube-proxy-ds branch from 7890a6b to 4cb3b73 Compare August 25, 2017 22:14

MrHohn force-pushed the kube-proxy-ds branch 3 times, most recently from 277bdf2 to f4b5cdc Compare August 28, 2017 18:17

MrHohn added 2 commits August 28, 2017 13:31

Set up KUBE_PROXY_DAEMONSET env for GCE and common.sh.

1102656

Add kube-proxy daemonset track to GCE startup scripts (GCI, Debian an…

6d35b94

…d CoreOS).

MrHohn force-pushed the kube-proxy-ds branch from f4b5cdc to 6d35b94 Compare August 28, 2017 20:31

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 28, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 28, 2017

k8s-github-robot merged commit 04b3ab9 into kubernetes:master Aug 29, 2017

MrHohn deleted the kube-proxy-ds branch August 30, 2017 23:13

MrHohn mentioned this pull request Feb 2, 2018

add a gce version of istio job kubernetes/test-infra#6612

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow running kube-proxy as a DaemonSet when using kube-up.sh on GCE #50705

Allow running kube-proxy as a DaemonSet when using kube-up.sh on GCE #50705

MrHohn commented Aug 15, 2017 •

edited

Loading

MrHohn commented Aug 15, 2017 •

edited

Loading

MrHohn commented Aug 16, 2017

MrHohn Aug 18, 2017

MrHohn Aug 21, 2017

MrHohn commented Aug 23, 2017

yujuhong commented Aug 25, 2017

dchen1107 commented Aug 25, 2017 •

edited

Loading

bsalamat commented Aug 25, 2017

yujuhong commented Aug 25, 2017 •

edited by dchen1107

Loading

mikedanese commented Aug 25, 2017

mikedanese Aug 25, 2017

MrHohn Aug 25, 2017

dchen1107 commented Aug 26, 2017

MrHohn commented Aug 28, 2017

mikedanese commented Aug 28, 2017

MrHohn commented Aug 28, 2017

mikedanese commented Aug 28, 2017

k8s-github-robot commented Aug 28, 2017

fejta-bot commented Aug 28, 2017

fejta-bot commented Aug 29, 2017

k8s-github-robot commented Aug 29, 2017

Allow running kube-proxy as a DaemonSet when using kube-up.sh on GCE #50705

Allow running kube-proxy as a DaemonSet when using kube-up.sh on GCE #50705

Conversation

MrHohn commented Aug 15, 2017 • edited Loading

MrHohn commented Aug 15, 2017 • edited Loading

MrHohn commented Aug 16, 2017

MrHohn Aug 18, 2017

Choose a reason for hiding this comment

MrHohn Aug 21, 2017

Choose a reason for hiding this comment

MrHohn commented Aug 23, 2017

yujuhong commented Aug 25, 2017

dchen1107 commented Aug 25, 2017 • edited Loading

bsalamat commented Aug 25, 2017

yujuhong commented Aug 25, 2017 • edited by dchen1107 Loading

mikedanese commented Aug 25, 2017

mikedanese Aug 25, 2017

Choose a reason for hiding this comment

MrHohn Aug 25, 2017

Choose a reason for hiding this comment

dchen1107 commented Aug 26, 2017

MrHohn commented Aug 28, 2017

mikedanese commented Aug 28, 2017

MrHohn commented Aug 28, 2017

mikedanese commented Aug 28, 2017

k8s-github-robot commented Aug 28, 2017

fejta-bot commented Aug 28, 2017

fejta-bot commented Aug 29, 2017

k8s-github-robot commented Aug 29, 2017

MrHohn commented Aug 15, 2017 •

edited

Loading

MrHohn commented Aug 15, 2017 •

edited

Loading

dchen1107 commented Aug 25, 2017 •

edited

Loading

yujuhong commented Aug 25, 2017 •

edited by dchen1107

Loading