WINC-505: Windows containerd runtime enablement #962

selansen · 2021-11-23T03:25:26Z

Enhancement proposal for making containerd as default runtime in windows node.

Signed-off-by: selansen esiva@redhat.com

enhancements/windows-containers/container-runtime-containerd.md

saifshaikh48

Looks good on a first pass through. I have a few comments

enhancements/windows-containers/container-runtime-containerd.md

aravindhp

Thanks for working on this, @selansen. Please address my comments.

enhancements/windows-containers/container-runtime-containerd.md

aravindhp · 2021-11-23T17:55:55Z

enhancements/windows-containers/container-runtime-containerd.md

+#### Dev Preview -> Tech Preview
+None
+
+#### Tech Preview -> GA
+None


Please remove as this does not apply here.

CI throws error when I remove few headings . I think I should keep them and add N/A if it is not applicable.
enhancements/windows-containers/container-runtime-containerd.md missing "#### Removing a deprecated feature"
enhancements/windows-containers/container-runtime-containerd.md missing "### Operational Aspects of API Extensions"
enhancements/windows-containers/container-runtime-containerd.md missing "#### Failure Modes"
enhancements/windows-containers/container-runtime-containerd.md missing "#### Support Procedures"
enhancements/windows-containers/container-runtime-containerd.md missing "## Drawbacks"

enhancements/windows-containers/container-runtime-containerd.md

aravindhp

Thanks for working on this, @selansen. Please address my comments.

enhancements/windows-containers/container-runtime-containerd.md

aravindhp · 2021-12-01T00:22:32Z

enhancements/windows-containers/container-runtime-containerd.md

+community operator for customers and developers to try it out.  This will become default for openshift 
+4.11. At any given point in time we have one runtime support and do not allow switching between runtime.
+
+## logging


That needs to be investigated as we need to figure out if console logs and oc pod logs etc continue to work.

enhancements/windows-containers/container-runtime-containerd.md

mtnbikenc · 2021-12-09T16:14:43Z

enhancements/windows-containers/container-runtime-containerd.md

+## Motivation
+
+In Kubernetes, the kubelet talks to a container runtime using the Container Runtime Interface. From
+Kubernetes 1.24 onwards dockershim will be removed from kubelet code via upstream [PR](kubernetes/kubernetes#97252).


Broken link when rendered. Should use https://github.com/kubernetes/kubernetes/pull/97252.

alinaryan · 2021-12-09T17:14:48Z

enhancements/windows-containers/container-runtime-containerd.md

+  in the 4.10 community branch(OKD) and 4.10 branch(ccm OCP)to capture any regression.
+  This will help us to identify any issues well before containerd becomes default runtime.
+* containerd doesn't support image-pull-progress-deadline as of now. There is a PR
+  https://github.com/containerd/containerd/pull/6150 work in progress. Until this


Link should have text:
PR

alinaryan · 2021-12-09T17:19:09Z

enhancements/windows-containers/container-runtime-containerd.md

+There are few alternatives, but they are either not cost-effective or depend on the competitor's less modular
+components.
+* Implementing CRI-O runtime for Windows involves huge engineering effort and there is no community
+  support ( most community supporters already moved to containerd).


nit: remove extra space after '( '

alinaryan · 2021-12-09T17:22:32Z

enhancements/windows-containers/container-runtime-containerd.md

+
+#### Removing a deprecated feature
+Once containerd becomes the default runtime, Docker will no longer be needed in Kubernetes stack. This is discussed in
+design section.


Can link to design section here

aravindhp · 2021-12-09T16:22:54Z

enhancements/windows-containers/container-runtime-containerd.md

+changes. MachineSet and BYOH (Bring Your Own Host) upgrade details are discussed in the upgrade section.
+There is no difference between MachineSet and BYOH on how we enable containerd as the default runtime.
+
+Golden Image:


Yeah make this ### Golden Image

aravindhp · 2021-12-09T16:23:41Z

enhancements/windows-containers/container-runtime-containerd.md

+runtime or continue to use the same Windows image with Docker. Once containerd becomes the default runtime, WMCO will
+support Windows golden image with or without Docker.
+
+Containerd Migration plan:


@mrunalp is not asking for the line to be removed but to make it a subheader i.e. ### Containerd migration plan

aravindhp · 2021-12-09T16:26:57Z

enhancements/windows-containers/container-runtime-containerd.md

@@ -0,0 +1,205 @@
+---
+title: container-runtime-containerd


Given the main header is containerd - Windows container runtime this needs to be containerd-windows-container runtime. My comment was changing the title does not imply changing the filename.

enhancements/windows-containers/container-runtime-containerd.md

Enhancement proposal for making containerd as default runtime in windows node. Signed-off-by: selansen <esiva@redhat.com>

aravindhp

Thanks for working on this, @selansen. I am leaving the hold in case other @openshift/openshift-team-windows-containers members want to review.

/lgtm

openshift-ci · 2021-12-10T00:20:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aravindhp, mrunalp

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:

~~OWNERS~~ [aravindhp,mrunalp]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

sgaoshang · 2021-12-13T04:19:51Z

/lgtm, thanks.

rrasouli · 2021-12-13T07:16:32Z

/lgtm

sebsoto · 2021-12-13T14:45:39Z

/lgtm

alinaryan · 2021-12-13T14:46:15Z

/lgtm

jrvaldes · 2021-12-13T14:46:43Z

/lgtm

selansen · 2021-12-13T14:52:58Z

/hold cancel

openshift-ci · 2021-12-13T15:11:29Z

@selansen: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

saifshaikh48 · 2021-12-13T15:24:47Z

💯

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 23, 2021

openshift-ci bot requested review from markmc and sttts November 23, 2021 03:25

selansen force-pushed the containerd_enhancement branch from ffb0c32 to 36cad3c Compare November 23, 2021 03:31

saifshaikh48 reviewed Nov 23, 2021

View reviewed changes

enhancements/windows-containers/container-runtime-containerd.md Outdated Show resolved Hide resolved

saifshaikh48 reviewed Nov 23, 2021

View reviewed changes

enhancements/windows-containers/container-runtime-containerd.md Outdated Show resolved Hide resolved

enhancements/windows-containers/container-runtime-containerd.md Show resolved Hide resolved

aravindhp suggested changes Nov 23, 2021

View reviewed changes

selansen force-pushed the containerd_enhancement branch from 36cad3c to 2e712d8 Compare November 23, 2021 21:05

sebsoto suggested changes Nov 24, 2021

View reviewed changes

selansen force-pushed the containerd_enhancement branch 2 times, most recently from a1d9d1d to f92a9a8 Compare November 24, 2021 16:47

mansikulkarni96 reviewed Nov 24, 2021

View reviewed changes

selansen force-pushed the containerd_enhancement branch 6 times, most recently from c070529 to 05dfc98 Compare November 29, 2021 04:44

aravindhp suggested changes Dec 1, 2021

View reviewed changes

selansen force-pushed the containerd_enhancement branch 9 times, most recently from 093020d to fec7d3a Compare December 6, 2021 03:05

selansen changed the title ~~[WIP] WINC-505: Windows containerd runtime enablement~~ WINC-505: Windows containerd runtime enablement Dec 6, 2021

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 6, 2021

mtnbikenc reviewed Dec 9, 2021

View reviewed changes

alinaryan reviewed Dec 9, 2021

View reviewed changes

aravindhp suggested changes Dec 9, 2021

View reviewed changes

selansen force-pushed the containerd_enhancement branch 5 times, most recently from cb51120 to 71367c0 Compare December 10, 2021 00:02

WINC-505: Windows containerd runtime enablement

6ebfdc8

Enhancement proposal for making containerd as default runtime in windows node. Signed-off-by: selansen <esiva@redhat.com>

selansen force-pushed the containerd_enhancement branch from 71367c0 to 6ebfdc8 Compare December 10, 2021 00:15

aravindhp approved these changes Dec 10, 2021

View reviewed changes

openshift-ci bot assigned aravindhp Dec 10, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 10, 2021

openshift-ci bot assigned rrasouli Dec 13, 2021

openshift-ci bot assigned sebsoto Dec 13, 2021

openshift-ci bot assigned jrvaldes and alinaryan Dec 13, 2021

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 13, 2021

openshift-merge-robot merged commit 90dafe7 into openshift:master Dec 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WINC-505: Windows containerd runtime enablement #962

WINC-505: Windows containerd runtime enablement #962

selansen commented Nov 23, 2021

saifshaikh48 left a comment

aravindhp left a comment

aravindhp Nov 23, 2021

selansen Nov 26, 2021

aravindhp left a comment

aravindhp Dec 1, 2021

mtnbikenc Dec 9, 2021 •

edited

alinaryan Dec 9, 2021

alinaryan Dec 9, 2021

alinaryan Dec 9, 2021

aravindhp Dec 9, 2021

aravindhp Dec 9, 2021

aravindhp Dec 9, 2021

aravindhp left a comment •

edited

openshift-ci bot commented Dec 10, 2021

sgaoshang commented Dec 13, 2021

rrasouli commented Dec 13, 2021

sebsoto commented Dec 13, 2021

alinaryan commented Dec 13, 2021

jrvaldes commented Dec 13, 2021

selansen commented Dec 13, 2021

openshift-ci bot commented Dec 13, 2021

saifshaikh48 commented Dec 13, 2021

WINC-505: Windows containerd runtime enablement #962

WINC-505: Windows containerd runtime enablement #962

Conversation

selansen commented Nov 23, 2021

saifshaikh48 left a comment

Choose a reason for hiding this comment

aravindhp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aravindhp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtnbikenc Dec 9, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aravindhp left a comment • edited

Choose a reason for hiding this comment

openshift-ci bot commented Dec 10, 2021

sgaoshang commented Dec 13, 2021

rrasouli commented Dec 13, 2021

sebsoto commented Dec 13, 2021

alinaryan commented Dec 13, 2021

jrvaldes commented Dec 13, 2021

selansen commented Dec 13, 2021

openshift-ci bot commented Dec 13, 2021

saifshaikh48 commented Dec 13, 2021

mtnbikenc Dec 9, 2021 •

edited

aravindhp left a comment •

edited