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

kubeadm: disable the kube-proxy DaemonSet on non-Linux nodes #76327

Merged
merged 1 commit into from
May 3, 2019

Conversation

neolit123
Copy link
Member

What this PR does / why we need it:

Windows worker nodes run kube-proxy as a Windows service.
In the future the kube-proxy DaemonSet might run on Windows nodes
too, but for now a temporary measure is needed to disable it.

Add a linux node selector in the kube-proxy manifest spec.
Similar was done already for CoreDNS.

Which issue(s) this PR fixes:

xref kubernetes/kubeadm#1393

Special notes for your reviewer:
NONE

Does this PR introduce a user-facing change?:

kubeadm: disable the kube-proxy DaemonSet on non-Linux nodes. This step is required to support Windows worker nodes.

/kind cleanup
/priority important-longterm
@kubernetes/sig-cluster-lifecycle-pr-reviews
/assign @timothysc @fabriziopandini
cc @PatrickLang

Windows worker nodes run kube-proxy as a Windows service.
In the future the kube-proxy DaemonSet might run on Windows nodes
too, but for now a temporary measure is needed to disable it.

Add a linux node selector in the kube-proxy manifest spec.
@k8s-ci-robot k8s-ci-robot added 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. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 9, 2019
@neolit123
Copy link
Member Author

/sig windows

@k8s-ci-robot k8s-ci-robot added sig/windows Categorizes an issue or PR as relevant to SIG Windows. area/kubeadm approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 9, 2019
@k8s-ci-robot k8s-ci-robot requested review from kad and luxas April 9, 2019 08:58
@SataQiu
Copy link
Member

SataQiu commented Apr 9, 2019

LGTM

@rosti
Copy link
Contributor

rosti commented Apr 9, 2019

My question is how we are going to replace the DaemonSet? I know that the answer is a Windows service ATM. However, do we expect, that users do their own kube-proxy provisioning or should kubeadm do it instead?

It looks good to me, but I want to know if we should be providing some replacement.

@neolit123
Copy link
Member Author

neolit123 commented Apr 9, 2019

My question is how we are going to replace the DaemonSet? I know that the answer is a Windows service ATM. However, do we expect, that users do their own kube-proxy provisioning or should kubeadm do it instead?

the plan is for kubeadm to not manage kube-proxy on Windows workers until the DaemonSet works.
we are going to have a wrapper around kubeadm that starts kube-proxy as a Windows service.

@rosti
Copy link
Contributor

rosti commented Apr 9, 2019

we are going to have a wrapper around kubeadm that starts kube-proxy as a Windows service.

Now this sounds horrible, unfortunately the alternative is to have an optional add-on per-node that deploys kube-proxy as a service on Windows nodes and this is even worse.
So I guess, that leaving it alone until DaemonSet kube-proxy works on Windows is a good choice for now. It does not imply the introduction of some technical debt for us (other than making sure to remove the node selector later on).

/lgtm
/hold
For @timothysc & @fabriziopandini 's opinion.

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Apr 9, 2019
@detiber
Copy link
Member

detiber commented Apr 9, 2019

Can't say I'm a fan of the approach, but at the same time I don't have a better alternative, so a hesitant +1 from me.

@fabriziopandini
Copy link
Member

I'm ok with this PR, but I'm not sure about

we are going to have a wrapper around kubeadm that starts kube-proxy as a Windows service.

On Linux, systemd service creation is not a kubeadm task, but it is something delegated to OS specific packages (deb or rpm); kubeadm task usually is limited to service configuration/start.

My first reaction is that Iwe should follow the same approach also on windows/for kube-proxy.
An OS specific package should install kube-proxy and create the service, then Kubeadm should check the service exists in preflight, then pull the configuration and start the service during join

But TBH I didn't investigated this in detail, so might be I'm missing something 😅

@neolit123
Copy link
Member Author

My first reaction is that Iwe should follow the same approach also on windows/for kube-proxy.
An OS specific package should install kube-proxy and create the service, then Kubeadm should check the service exists in preflight, then pull the configuration and start the service during join

WIndows nodes are blocked on privileged containers at the moment.
so this is the reason kube-proxy is run as a Windows service.

one day kube-proxy will be able to run as a DaemonSet on Windows too, until then we are heavily leaning towards the wrapper approach so that we don't maintain such code in kubeadm.

also i wanted to explain the Windows / kubeadm discussions i had recently with SIG Windows, but i won't be able to attend this week's kubeadm office hours.

@@ -112,5 +112,7 @@ spec:
- key: CriticalAddonsOnly
operator: Exists
- operator: Exists
nodeSelector:
Copy link
Member

Choose a reason for hiding this comment

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

I'm not familiar with windows deployments, but how does service routing work on window then?

Copy link
Member Author

Choose a reason for hiding this comment

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

it works somehow, this is more of a question for the SIG Window folk.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

kube-proxy still runs and handles service routing, it's just that it needs to run on the host directly and can't be run from inside a privileged container.

Copy link
Member

@timothysc timothysc left a comment

Choose a reason for hiding this comment

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

? below

@neolit123
Copy link
Member Author

let's hold this PR regardless, so that we can get the KEP in place that answers the questions.
/approve cancel

@k8s-ci-robot k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 9, 2019
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 2, 2019
Copy link
Member

@timothysc timothysc left a comment

Choose a reason for hiding this comment

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

kubernetes/enhancements#994

/approve
/hold cancel

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: timothysc

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 2, 2019
@fejta-bot
Copy link

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

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

1 similar comment
@fejta-bot
Copy link

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

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit 7defecb into kubernetes:master May 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubeadm cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. 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. sig/windows Categorizes an issue or PR as relevant to SIG Windows. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants