test: remove legacy Windows dockershim templates #2292

jackfrancis · 2022-05-09T19:00:04Z

What type of PR is this?

/kind cleanup
/kind deprecation

What this PR does / why we need it:

This PR updates the Windows test templates surface area in the following ways:

Remove the E2E test job that validates Windows + dockershim cluster scenario. For capz development going forward, we no longer want to recommend users create clusters w/ Windows nodes running dockershim as the next release of capz will occur after 1.21 (the last version of Kubernetes recommended for Windows + dockershim) has reached its end-of-life.
Preserve the existing "windows" templates and convert them to use containerd. The templates/test/dev/cluster-template-custom-builds.yaml template still uses the vanilla "windows" template as its base, and we can still benefit by referencing a simple template that includes a Windows AzureMachineTemplate for users.
Remove the ci-conformance dockershim testing surface area (this azure: remove Windows + dockershim conformance test job kubernetes/test-infra#26242 PR does the rest of this work
Ensure that all remaining Windows node-providing templates use containerd as CRI

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

squashed commits
includes documentation
adds unit tests

Release note:

remove dockershim- and containerd-specific reference templates, standardize to containerd by default

jackfrancis · 2022-05-09T19:01:50Z

/test pull-cluster-api-provider-azure-e2e-optional

CecileRobertMichon

@jsturtevant can confirm but there might also be some possible cleanup around the CNI area? Since Windows containerd tests use calico and we no longer need the special flannel CNI for windows setup

templates/cluster-template-windows.yaml

jsturtevant

This brings up an issue if we need to retain the https://github.com/kubernetes-sigs/cluster-api-provider-azure/releases/download/v1.3.0/cluster-template-windows-containerd.yaml or if it can just be renamed to cluster-template-windows.yaml.

templates/flavors/windows/machine-deployment-windows.yaml

templates/flavors/machinepool-windows/machine-pool-deployment-windows.yaml

jackfrancis · 2022-05-16T19:26:26Z

@jsturtevant PTAL

jackfrancis · 2022-05-16T20:50:24Z

/test pull-cluster-api-provider-azure-e2e-optional

docs/book/src/topics/windows.md

templates/test/ci/cluster-template-prow-ci-version-windows.yaml

templates/test/ci/cluster-template-prow-windows.yaml

templates/test/dev/cluster-template-custom-builds-windows.yaml

jsturtevant

feels good to be removing these :-)

jsturtevant · 2022-05-17T17:33:18Z

can confirm but there might also be some possible cleanup around the CNI area? Since Windows containerd tests use calico and we no longer need the special flannel CNI for windows setup

Yes we can drop the files in https://github.com/kubernetes-sigs/cluster-api-provider-azure/tree/main/templates/addons/windows/flannel as they were only used for dockershim

jackfrancis · 2022-05-17T19:57:50Z

@jsturtevant above cleanup tasks (and others discovered during) applied

jackfrancis · 2022-05-17T20:21:02Z

/test pull-cluster-api-provider-azure-e2e-optional

jsturtevant · 2022-05-17T22:23:58Z

/test pull-cluster-api-provider-azure-windows-containerd-upstream-with-ci-artifacts

jsturtevant · 2022-05-18T15:29:12Z

looks like we got it all! 💯

One last open question:

Shouldn't this template be a rename of cluster-template-windows-containerd.yaml now? I wouldn't expect us to keep both side by side

This brings up an issue if we need to retain the https://github.com/kubernetes-sigs/cluster-api-provider-azure/releases/download/v1.3.0/cluster-template-windows-containerd.yaml or if it can just be renamed to cluster-template-windows.yaml.

It will be a breaking change to not publish cluster-template-windows-containerd.yaml in the 1.4.0 release. It already is breaking change to have cluster-template-windows.yaml go from dockershim to containerd. So this maybe ok. The other alternative I can think of is to do a symlink for one release of cluster-template-windows.yaml to cluster-template-windows-containerd.yaml in

cluster-api-provider-azure/Makefile

Lines 564 to 566 in 2a9b44c

    
           .PHONY: release-templates 
        
           release-templates: $(RELEASE_DIR) 
        
           	cp templates/cluster-template* $(RELEASE_DIR)/

that would give anyone using it a chance to move on the release.

Either way we should make sure the release notes are clear this is a breaking change in the release notes.

thoughts @jackfrancis @CecileRobertMichon ?

jackfrancis · 2022-05-18T16:33:56Z

@jsturtevant I vote we move forward with the template name change. For folks who want to insist on retaining a canonical template under that name going forward, they can refer to an immutable link such as this one:

https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/v1.3.0/templates/cluster-template-windows-containerd.yaml

I will update release notes so that it gets a call-out at release time.

CecileRobertMichon · 2022-05-19T17:45:55Z

test/e2e/config/azure-dev.yaml

@@ -165,10 +163,6 @@ providers:
        targetName: "cluster-template-conformance-presubmit-artifacts.yaml"
      - sourcePath: "${PWD}/templates/test/dev/cluster-template-custom-builds.yaml"
        targetName: "cluster-template-conformance-presubmit-artifacts-windows-containerd.yaml"


not related to this change but @jsturtevant do you know why we're writing the cluster-template-custom-builds.yaml template above twice with two different names?

Prior to this we needed different templates for windows and so we tacked windows to the end:

cluster-api-provider-azure/test/e2e/conformance_test.go

Lines 125 to 127 in 30bfbb5

if isWindows(kubetestConfigFilePath) {

flavor = flavor + "-" + getWindowsFlavor()

}

After this merged we might not need this logic anymore as we can re-use the same templates. I will create an issue to follow up on this after this merges

CecileRobertMichon · 2022-05-19T17:45:56Z

test/e2e/config/azure-dev.yaml

@@ -165,10 +163,6 @@ providers:
        targetName: "cluster-template-conformance-presubmit-artifacts.yaml"
      - sourcePath: "${PWD}/templates/test/dev/cluster-template-custom-builds.yaml"
        targetName: "cluster-template-conformance-presubmit-artifacts-windows-containerd.yaml"


not related to this change but @jsturtevant do you know why we're writing the cluster-template-custom-builds.yaml template above twice with two different names?

CecileRobertMichon · 2022-05-19T17:46:46Z

templates/flavors/machinepool-windows/kustomization.yaml

  - machine-pool-deployment-windows.yaml

 patchesStrategicMerge:
-  - ../../azure-cluster-identity/azurecluster-identity-ref.yaml
+  - ../base-windows-containerd/kubeadm-control-plane.yaml


should we rename this to just base-windows?

should we do this as part of #2315

CecileRobertMichon · 2022-05-19T17:46:46Z

templates/flavors/machinepool-windows/kustomization.yaml

  - machine-pool-deployment-windows.yaml

 patchesStrategicMerge:
-  - ../../azure-cluster-identity/azurecluster-identity-ref.yaml
+  - ../base-windows-containerd/kubeadm-control-plane.yaml


should we rename this to just base-windows?

CecileRobertMichon · 2022-05-19T17:47:25Z

test/e2e/config/azure-dev.yaml

@@ -155,8 +155,6 @@ providers:
        targetName: "cluster-template-private.yaml"
      - sourcePath: "${PWD}/templates/test/ci/cluster-template-prow-ci-version.yaml"
        targetName: "cluster-template-conformance-ci-artifacts.yaml"
-      - sourcePath: "${PWD}/templates/test/ci/cluster-template-prow-ci-version-windows.yaml"
-        targetName: "cluster-template-conformance-ci-artifacts-windows.yaml"
      - sourcePath: "${PWD}/templates/test/ci/cluster-template-prow-ci-version-windows-containerd-2022.yaml"
        targetName: "cluster-template-conformance-ci-artifacts-windows-containerd-2022.yaml"


can we also rename cluster-template-prow-ci-version-windows-containerd-2022 to cluster-template-prow-ci-version-windows-2022 ?

CecileRobertMichon · 2022-05-19T17:47:25Z

test/e2e/config/azure-dev.yaml

@@ -155,8 +155,6 @@ providers:
        targetName: "cluster-template-private.yaml"
      - sourcePath: "${PWD}/templates/test/ci/cluster-template-prow-ci-version.yaml"
        targetName: "cluster-template-conformance-ci-artifacts.yaml"
-      - sourcePath: "${PWD}/templates/test/ci/cluster-template-prow-ci-version-windows.yaml"
-        targetName: "cluster-template-conformance-ci-artifacts-windows.yaml"
      - sourcePath: "${PWD}/templates/test/ci/cluster-template-prow-ci-version-windows-containerd-2022.yaml"
        targetName: "cluster-template-conformance-ci-artifacts-windows-containerd-2022.yaml"


can we also rename cluster-template-prow-ci-version-windows-containerd-2022 to cluster-template-prow-ci-version-windows-2022 ?

I think this will rely on change the logic in the e2e_conformance: #2315

Lets follow up with a separate pr for that?

jsturtevant · 2022-05-19T18:27:08Z

I vote we move forward with the template name change.

Ok, I think we are all in agreement

jsturtevant · 2022-05-19T18:27:31Z

/lgtm

CecileRobertMichon

/lgtm
/approve

thank you @jackfrancis!

k8s-ci-robot · 2022-05-19T18:28:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

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~~ [CecileRobertMichon]

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 requested review from alexeldeib and Jont828 May 9, 2022 19:00

CecileRobertMichon reviewed May 9, 2022

View reviewed changes

templates/cluster-template-windows.yaml Show resolved Hide resolved

jsturtevant reviewed May 10, 2022

View reviewed changes

templates/flavors/windows/machine-deployment-windows.yaml Show resolved Hide resolved

templates/flavors/machinepool-windows/machine-pool-deployment-windows.yaml Show resolved Hide resolved

CecileRobertMichon mentioned this pull request May 11, 2022

Use MSI ClientID as userAssignedIdentityID in azure.json #2214

Merged

3 tasks

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 14, 2022

jackfrancis force-pushed the windows-dockershim-test branch from 984602d to 8260e48 Compare May 16, 2022 19:24

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 16, 2022

jsturtevant reviewed May 17, 2022

View reviewed changes

jsturtevant suggested changes May 17, 2022

View reviewed changes

test: remove legacy Windows dockershim templates

80a6ddb

jackfrancis force-pushed the windows-dockershim-test branch from 8260e48 to 80a6ddb Compare May 17, 2022 19:57

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels May 18, 2022

CecileRobertMichon reviewed May 19, 2022

View reviewed changes

jsturtevant mentioned this pull request May 19, 2022

Review e2e logic for selecting templates on Windows after dockershim templates are removed #2315

Closed

k8s-ci-robot assigned jsturtevant May 19, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 19, 2022

CecileRobertMichon reviewed May 19, 2022

View reviewed changes

k8s-ci-robot assigned CecileRobertMichon May 19, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 19, 2022

CecileRobertMichon mentioned this pull request May 19, 2022

fix broken link for Windows flavor template #2312

Merged

3 tasks

k8s-ci-robot merged commit d28d51c into kubernetes-sigs:main May 19, 2022

k8s-ci-robot added this to the v1.4 milestone May 19, 2022

CecileRobertMichon mentioned this pull request May 19, 2022

Check configmap existence before creation #2311

Merged

3 tasks

CecileRobertMichon mentioned this pull request May 27, 2022

Use new naming scheme for SKUs and versions #2302

Merged

3 tasks

CecileRobertMichon mentioned this pull request Jun 16, 2022

Find current Kubernetes versions for e2e testing #2388

Merged

3 tasks

jackfrancis deleted the windows-dockershim-test branch December 9, 2022 22:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: remove legacy Windows dockershim templates #2292

test: remove legacy Windows dockershim templates #2292

jackfrancis commented May 9, 2022 •

edited

Loading

jackfrancis commented May 9, 2022

CecileRobertMichon left a comment

jsturtevant left a comment

jackfrancis commented May 16, 2022

jackfrancis commented May 16, 2022

jsturtevant left a comment

jsturtevant commented May 17, 2022

jackfrancis commented May 17, 2022

jackfrancis commented May 17, 2022

jsturtevant commented May 17, 2022

jsturtevant commented May 18, 2022

jackfrancis commented May 18, 2022

CecileRobertMichon May 19, 2022

jsturtevant May 19, 2022

jsturtevant May 19, 2022

CecileRobertMichon May 19, 2022

CecileRobertMichon May 19, 2022

jsturtevant May 19, 2022

CecileRobertMichon May 19, 2022

CecileRobertMichon May 19, 2022

CecileRobertMichon May 19, 2022

jsturtevant May 19, 2022

jsturtevant commented May 19, 2022

jsturtevant commented May 19, 2022

CecileRobertMichon left a comment

k8s-ci-robot commented May 19, 2022

	if isWindows(kubetestConfigFilePath) {
	flavor = flavor + "-" + getWindowsFlavor()
	}

test: remove legacy Windows dockershim templates #2292

test: remove legacy Windows dockershim templates #2292

Conversation

jackfrancis commented May 9, 2022 • edited Loading

jackfrancis commented May 9, 2022

CecileRobertMichon left a comment

Choose a reason for hiding this comment

jsturtevant left a comment

Choose a reason for hiding this comment

jackfrancis commented May 16, 2022

jackfrancis commented May 16, 2022

jsturtevant left a comment

Choose a reason for hiding this comment

jsturtevant commented May 17, 2022

jackfrancis commented May 17, 2022

jackfrancis commented May 17, 2022

jsturtevant commented May 17, 2022

jsturtevant commented May 18, 2022

jackfrancis commented May 18, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsturtevant commented May 19, 2022

jsturtevant commented May 19, 2022

CecileRobertMichon left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 19, 2022

jackfrancis commented May 9, 2022 •

edited

Loading