-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
doc: Containerd alpha support on Windows #19208
doc: Containerd alpha support on Windows #19208
Conversation
Deploy preview for kubernetes-io-vnext-staging processing. Building with commit 43ebc28 https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/5e6a5f9c2c5511000a83b3ae |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
/milestone 1.18 |
I'll mark this as a work in progress |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's some informal feedback that I hope helps shape the PR.
@@ -571,9 +571,11 @@ If filing a bug, please include detailed information about how to reproduce the | |||
|
|||
We have a lot of features in our roadmap. An abbreviated high level list is included below, but we encourage you to view our [roadmap project](https://github.com/orgs/kubernetes/projects/8) and help us make Windows support better by [contributing](https://github.com/kubernetes/community/blob/master/sig-windows/). | |||
|
|||
### CRI-ContainerD | |||
### CRI-ContainerD (alpha in v1.18) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PatrickLang this will break incoming links at least twice: once when adding the feature state, then again when it changes.
Use a feature state shortcode instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks! fixed
{{< glossary_tooltip term_id="containerd" >}} is another OCI-compliant runtime that recently graduated as a {{< glossary_tooltip text="CNCF" term_id="cncf" >}} project. It's currently tested on Linux, but 1.3 will bring support for Windows and Hyper-V. [[reference](https://blog.docker.com/2019/02/containerd-graduates-within-the-cncf/)] | ||
{{< glossary_tooltip term_id="containerd" >}} is another OCI-compliant runtime that recently graduated as a {{< glossary_tooltip text="CNCF" term_id="cncf" >}} project. | ||
|
||
Kubernetes v1.18 is tested along with the upcoming ContainerD 1.4 release. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please avoid statements about the future. I would also avoid hinting at the recent past (“recently graduated”). This kind of text tends to go stale.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks! just fixed
/lgtm cancel |
/sig node |
48e2907
to
33a08fd
Compare
/hold cancel This is ready for review |
I recommend taking a look at the previous feedback I provided; there's a few things still to address. |
@sftim - can you be more specific or quote what's wrong? I admit I am not a good writer and could use some help here since I believe I already removed the last forward-looking statements. The last feedback I see you added is about whether or not a feature should go to alpha. What we have is consistent with what was agreed between SIG-Node and SIG-Windows. We need to have a working implementation in Kubernetes in place so we can continue working with the ContainerD maintainers on remaining issues and eventually get to a working release of containerd to recommend with k8s. cc @m2 @craiglpeters @marosset - I may need some help here |
The style guide asks authors to avoid making promises or giving hints about the future. Can you revise these changes so you're not adding those kind of promises / hints? |
- Azure - [AKS-Engine](https://github.com/Azure/aks-engine/blob/master/docs/topics/features.md#windows-containerd) | ||
|
||
|
||
In future milestones, the CRI-ContainerD interface will be able to manage sandboxes based on Hyper-V. This provides a foundation where RuntimeClass could be implemented for new use cases including: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a forward looking statement.
ContainerD on Windows is suitable for development purposes only for a few reasons: | ||
|
||
- Kubernetes 1.18 was not tested with an official ContainerD release, only the master development branch. Production deployments should use official releases that have been fully tested and are supported with security fixes. | ||
- Group-Managed Service Accounts are not yet implemented with ContainerD - see [containerd/cri#1276](https://github.com/containerd/cri/issues/1276). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a forward looking statement.
|
||
The CRI-ContainerD interface will be able to manage sandboxes based on Hyper-V. This provides a foundation where RuntimeClass could be implemented for new use cases including: | ||
{{< glossary_tooltip term_id="containerd" >}} is another OCI-compliant runtime that works with Kubernetes on Linux. Kubernetes v1.18 is the first version tested with ContainerD on Windows. It's still a work-in-progress, and you can track progress in [enhancements#1001](https://github.com/kubernetes/enhancements/issues/1001). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This includes a hint about future improvements.
I will be picking up the remaining work for this PR |
Cool. @marosset would you like any help? |
Let me look over the style guide. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's unusual to have a feature move into alpha in a page's What's Next section.
@marosset for a minimum viable change, can you also reword line 572:
We have a lot of features in our roadmap. An abbreviated high level list is included below, but we encourage you to view our roadmap project and help us make Windows support better by contributing.
I think that'd be enough.
@@ -575,28 +575,29 @@ We have a lot of features in our roadmap. An abbreviated high level list is incl | |||
|
|||
{{< feature-state for_k8s_version="v1.18" state="alpha" >}} | |||
|
|||
{{< glossary_tooltip term_id="containerd" >}} is another OCI-compliant runtime that works with Kubernetes on Linux. Kubernetes v1.18 is the first version tested with ContainerD on Windows. It's still a work-in-progress, and you can track progress in [enhancements#1001](https://github.com/kubernetes/enhancements/issues/1001). | |||
{{< glossary_tooltip term_id="containerd" >}} is an OCI-compliant runtime that works with Kubernetes on Linux. Kubernetes v1.18 adds support for ContainerD on Windows. Progress for ContainerD on Windows can be tracked at [enhancements#1001](https://github.com/kubernetes/enhancements/issues/1001). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PatrickLang @marosset, Some general input:
Is there a good reason to place this content in the What's next
section?
Why not introduce a ContainerD for Windows
heading (level2)?
OR
There is already a sub section on the page,
https://deploy-preview-19208--kubernetes-io-vnext-staging.netlify.com/docs/setup/production-environment/windows/intro-windows-in-kubernetes/#container-runtime
Line 578, If the name of this feature is, ContainerD
, then I suggest starting the sentence with that name, not the tooltip. You could use the tooltip in another place in the first sentence. Also, add the text
attribute to the glossary_tooltip
shortcode such as: text="ContainerD".
Line 580, The caution
shortcode should contain the list of isssues/things.
Line 584. Perhaps modify statement about official releases indicating that ContainerD does not officially support Windows
.
Line 594, Do you need the word, new
Look through the content and try to remove content that refers to possible future use or deprecations, etc.
.
Such as,
Windows node support in kubeadm will come in a future release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll move the ContainerD info under the Container Runtimes - good idea!
I'll address some of the other feedback as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that's a much better page.
For Kubernetes v1.18, you can use the test scripts to set up a cluster: | ||
|
||
* GCE - [kube-up.sh](https://github.com/kubernetes/kubernetes/blob/master/cluster/gce/windows/README-GCE-Windows-kube-up.md) | ||
* Azure - [AKS-Engine](https://github.com/Azure/aks-engine/blob/master/docs/topics/features.md#windows-containerd) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd omit this: it's cloud provider specific.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great changes. thank you @PatrickLang and @marosset
/lgtm
6698db4
to
43ebc28
Compare
My last push just squashed the commit - no new changes were introduced. |
/lgtm |
/approve |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: michmike, VineethReddy02 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 |
This updates the status of ContainerD on Windows to alpha for v1.18 (see kubernetes/enhancements#1001)