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 for Windows KEP #994

Merged
merged 1 commit into from
May 3, 2019
Merged

Kubeadm for Windows KEP #994

merged 1 commit into from
May 3, 2019

Conversation

ksubrmnn
Copy link
Contributor

@ksubrmnn ksubrmnn commented Apr 24, 2019

KEP PR for Issue #995

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 24, 2019
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. labels Apr 24, 2019
@ksubrmnn
Copy link
Contributor Author

/assign @timothysc

@neolit123
Copy link
Member

@kubernetes/sig-cluster-lifecycle-pr-reviews

@neolit123
Copy link
Member

/assign @michmike @fabriziopandini @rosti

Copy link
Contributor

@rosti rosti left a comment

Choose a reason for hiding this comment

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

Thanks for this proposal!

The KEP in its shape and form is looking like imposing a wrapper script and Flannel in the long run. These are both unacceptable as a long term solution, but I can see the point of their use in a few releases.

I think, that the long term goal of this KEP should be to have kubeadm join and reset be implemented on Windows with as similar and as simple UX as on Linux.
For that matter we'll have to employ privileged containers on Windows, get kube-proxy & CNI plugins to run in pods again, and ideally, get rid of the wrapper script.

Of course, this rosy dream of mine relies on getting privileged Windows containers.
@PatrickLang is there any possibility of getting privileged containers on Windows in the future?

@benmoss
Copy link
Member

benmoss commented Apr 25, 2019

Of course, this rosy dream of mine relies on getting privileged Windows containers.

I have a questionably acceptable idea to workaround this, but basically a RPC server running on the host that gets mounted into the container via a named pipe. It obviously is still less ideal than if privileged containers could be added to Windows, but assuming we can get a general-enough RPC API, we could then be in the position of distributing/maintaining the provisioning code via pods.

@neolit123
Copy link
Member

@benmoss

Of course, this rosy dream of mine relies on getting privileged Windows containers.

I have a questionably acceptable idea to workaround this, but basically a RPC server running on the host that gets mounted into the container via a named pipe. It obviously is still less ideal than if privileged containers could be added to Windows, but assuming we can get a general-enough RPC API, we could then be in the position of distributing/maintaining the provisioning code via pods.

we need to evaluate the roadmap of privilege containers on Windows.
the question here is when are they coming, and if hey are not coming anytime soon we can start thinking about alternatives.

the RPC idea seems like something that can be proposed in a document, but workarounds may still block the Beta. for this KEP it seems that we might want to hold onto the Windows services idea.

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.

So I stopped reviewing part way through b/c imo this effort should span releases. I don't want to force this through given other higher priority efforts. I'd much rather us refactor properly.


To support Windows specific flags for the kubelet, there is a requirement to split kubeadm’s app/phases/kubelet/flags.go files into two:
* app/phases/kubelet/flags_windows.go
* app/phases/kubelet/flags_linux.go
Copy link
Member

Choose a reason for hiding this comment

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

I'd like work from the componentconfig folks.

Copy link
Member

Choose a reason for hiding this comment

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

could you elaborate on your ideas?
this could end up being something quite isolated to kubeadm -> kubelet.
if os == "windows" { use flagset A } else { use flag set B }

Copy link
Member

Choose a reason for hiding this comment

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

You could build meta-data in apis ~= +omitempty

Copy link
Contributor

Choose a reason for hiding this comment

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

At the current state of kubelet's component config (which is at v1beta1), there are a few command line flags that don't have a corresponding field in the config. Those are:

  • dockershim related flags
  • container-runtime && container-runtime-endpoint flags
  • register-with-taints flag
  • hostname-override

Only the resolv-conf flag has a representation in the component config. However, we would have to patch it, after fetching the config from the config map, for the local machine setting to take place.


Kubeadm makes a number of non-portable assumptions about paths. E.g. “/etc/kubernetes” is a hardcoded path in kubeadm.

We need to evaluate the kubeadm codebase for such instances of non-portable paths - CRI sockets, Cert paths, etc. Such paths need to be defaulted properly in the kubeadm configuration API.
Copy link
Member

Choose a reason for hiding this comment

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

IMO this should span several releases and be done judiciously, I want us todo this wisely and not in a rush.

@timothysc
Copy link
Member

timothysc commented Apr 26, 2019

/hold
Please come to the next kubeadm office hours.

@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 Apr 26, 2019
@ksubrmnn
Copy link
Contributor Author

Of course, this rosy dream of mine relies on getting privileged Windows containers.

I have a questionably acceptable idea to workaround this, but basically a RPC server running on the host that gets mounted into the container via a named pipe. It obviously is still less ideal than if privileged containers could be added to Windows, but assuming we can get a general-enough RPC API, we could then be in the position of distributing/maintaining the provisioning code via pods.

@benmoss @neolit123

This is a feasible idea, and I completed a POC for this a month ago. It works. I think we can propose both ideas in the doc, and I can advocate for both. This is a more immediate option than waiting on privileged containers, but I think the community should discuss.

@PatrickLang
Copy link
Contributor

There's a lot of circling around "privileged containers on Windows". There are some Windows engineers looking into this, but I don't have a clear answer on whether some form of privileged containers could be made to work on Windows Server 2019, or if it would take a new OS version. I'm trying to get them to write up a public proposal but don't expect anything for at least a few weeks.

@rosti
Copy link
Contributor

rosti commented Apr 30, 2019

Thanks for the clarification @PatrickLang !
With or without privileged containers on Windows, there is a bunch of stuff that needs to be done either way to get kubeadm running on Windows - CRI detection, kubelet drop-in params, correct handling of Windows paths, etc.
Therefore, I think, that the focus of the initial effort should be on those grounds.


To support Windows specific flags for the kubelet, there is a requirement to split kubeadm’s app/phases/kubelet/flags.go files into two:
* app/phases/kubelet/flags_windows.go
* app/phases/kubelet/flags_linux.go
Copy link
Member

Choose a reason for hiding this comment

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

could you elaborate on your ideas?
this could end up being something quite isolated to kubeadm -> kubelet.
if os == "windows" { use flagset A } else { use flag set B }

@timothysc
Copy link
Member

poke me on slack after it's been updated to include the content changes from yesterdays review.

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.

/approve
/hold

Please address the comments and I'll let @neolit123 do the final lgtm.


To support Windows specific flags for the kubelet, there is a requirement to split kubeadm’s app/phases/kubelet/flags.go files into two:
* app/phases/kubelet/flags_windows.go
* app/phases/kubelet/flags_linux.go
Copy link
Member

Choose a reason for hiding this comment

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

You could build meta-data in apis ~= +omitempty


This proposal plans for FlannelD as the default option. Currently, FlannelD has to be started before the kube-proxy Windows service is started. FlannelD creates an HNS network on the Windows host, and kube-proxy will crash if it cannot find the network. This should be fixed in the scope of this project so that kube-proxy will wait until the network comes up. Therefore, kube proxy can be started at any time.

However, if FlannelD is deployed in VXLAN (Overlay) mode, then we need to rewrite the KubeProxyConfiguration with the correct Overlay specific values, and kube-proxy will need to read this config again.
Copy link
Member

Choose a reason for hiding this comment

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

You're going to need to specify the default path settings that flow through the kubelet config as well here.

@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
Copy link
Member

@fabriziopandini fabriziopandini left a comment

Choose a reason for hiding this comment

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

Few minors,
At first sight, this requires a considerable amount of work, so let's get this started!

Copy link
Contributor

@rosti rosti left a comment

Choose a reason for hiding this comment

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

I am fine with how things are ATM. It's clear that this is still provisional and would require a lot of work done before things are set in stone.
However, let's be mindful, that we should update the KEP when the actual implementation deviates from initial designs here.


To support Windows specific flags for the kubelet, there is a requirement to split kubeadm’s app/phases/kubelet/flags.go files into two:
* app/phases/kubelet/flags_windows.go
* app/phases/kubelet/flags_linux.go
Copy link
Contributor

Choose a reason for hiding this comment

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

At the current state of kubelet's component config (which is at v1beta1), there are a few command line flags that don't have a corresponding field in the config. Those are:

  • dockershim related flags
  • container-runtime && container-runtime-endpoint flags
  • register-with-taints flag
  • hostname-override

Only the resolv-conf flag has a representation in the component config. However, we would have to patch it, after fetching the config from the config map, for the local machine setting to take place.

* Installing the Container Runtime (e.g. Docker or containerd)
* Implement kubeadm init for Windows
* Implement kubeadm join --control-plane for Windows (at this time)
* Supporting upgrades using kubeadm upgrade for Windows (to be revisited for Beta)
Copy link
Contributor

Choose a reason for hiding this comment

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

Going through the code, I think, that this would be quite easy to do for a worker node. The only kubeadm portion here that need revising is kubeadm upgrade node config. It has a tiny bit of OS specific code, that is shared with join and, thus, it would have been taken care of with the kubeadm join porting effort.

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

@rosti rosti left a comment

Choose a reason for hiding this comment

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

Thanks for the updates and squash @ksubrmnn !
Let's get this in.
/lgtm
/hold cancel

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ksubrmnn, rosti, 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 merged commit 557352e 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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants