Installing csi-proxy in some windows clusters by marosset · Pull Request #2012 · kubernetes-sigs/cluster-api-provider-azure

marosset · 2022-01-26T22:45:40Z

Signed-off-by: Mark Rossetti marosset@microsoft.com

What type of PR is this?

/kind feature

What this PR does / why we need it:

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:

Installs csi-proxy as a daemonset on Windows nodes for clusters that specify `csi-proxy: enabled`

jsturtevant · 2022-01-27T16:38:37Z

private cluster timed out.
/retest

jsturtevant

Is the plan to follow up with a test config after?

I am not 100% sure as I haven't tried this yet but I think it needs to be supported in a few different templates including this one:

cluster-api-provider-azure/scripts/ci-entrypoint.sh

Line 89 in d41727f

    
           export CLUSTER_TEMPLATE="test/ci/cluster-template-prow-external-cloud-provider.yaml"

marosset · 2022-01-27T17:27:07Z

Is the plan to follow up with a test config after?

Yep

marosset · 2022-01-27T17:29:04Z

I am not 100% sure as I haven't tried this yet but I think it needs to be supported in a few different templates including this one:

Thanks, I wasn't sure which all clusters this needed to be enabled for. Hopefully it will be clearer once I start the test-infra/config changes.

marosset · 2022-01-27T21:36:27Z

/label tide/merge-method-squash

marosset · 2022-01-27T21:37:33Z

/test pull-kubernetes-e2e-capz-azure-disk-windows

It looks like some test jobs for windows + capz + azure-disk were added with kubernetes/test-infra#24065

k8s-ci-robot · 2022-01-27T21:37:35Z

@marosset: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test pull-cluster-api-provider-azure-build
/test pull-cluster-api-provider-azure-e2e
/test pull-cluster-api-provider-azure-e2e-windows-dockershim
/test pull-cluster-api-provider-azure-test
/test pull-cluster-api-provider-azure-verify

The following commands are available to trigger optional jobs:

/test pull-cluster-api-provider-azure-apidiff
/test pull-cluster-api-provider-azure-apiversion-upgrade
/test pull-cluster-api-provider-azure-capi-e2e
/test pull-cluster-api-provider-azure-ci-entrypoint
/test pull-cluster-api-provider-azure-conformance
/test pull-cluster-api-provider-azure-conformance-with-ci-artifacts
/test pull-cluster-api-provider-azure-coverage
/test pull-cluster-api-provider-azure-e2e-exp
/test pull-cluster-api-provider-azure-e2e-full
/test pull-cluster-api-provider-azure-e2e-workload-upgrade-1-21-1-22
/test pull-cluster-api-provider-azure-e2e-workload-upgrade-1-22-latest-main
/test pull-cluster-api-provider-azure-upstream-windows-dockershim
/test pull-cluster-api-provider-azure-windows-containerd-upstream-with-ci-artifacts
/test pull-cluster-api-provider-azure-windows-containerd-upstream-with-ci-artifacts-serial-slow

Use /test all to run the following jobs that were automatically triggered:

pull-cluster-api-provider-azure-apidiff
pull-cluster-api-provider-azure-build
pull-cluster-api-provider-azure-ci-entrypoint
pull-cluster-api-provider-azure-coverage
pull-cluster-api-provider-azure-e2e
pull-cluster-api-provider-azure-e2e-windows-dockershim
pull-cluster-api-provider-azure-test
pull-cluster-api-provider-azure-verify

Details

In response to this:

/test pull-kubernetes-e2e-capz-azure-disk-windows

It looks like some test jobs for windows + capz + azure-disk were added with kubernetes/test-infra#24065

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.

marosset · 2022-01-27T21:39:49Z

I am not 100% sure as I haven't tried this yet but I think it needs to be supported in a few different templates including this one:

Thanks, I wasn't sure which all clusters this needed to be enabled for. Hopefully it will be clearer once I start the test-infra/config changes.

Nevermind, wrong repo :)

/test pull-kubernetes-e2e-capz-azure-disk-windows

It looks like some test jobs for windows + capz + azure-disk were added with kubernetes/test-infra#24065

Nevermind, wrong repo :)

marosset · 2022-01-28T05:40:37Z

/wip

marosset · 2022-01-28T05:41:28Z

/hold
While I validate the dev-cluster scenario

jsturtevant · 2022-01-31T19:31:57Z

 resources:
  - ../default
  - machine-deployment-windows.yaml
+  - ../../addons/windows/csi-proxy/csi-proxy-resource-set.yaml


We have not been installing Cluster Resource Sets for the default templates, only for the E2E tests. This is mostly because cluster api doesn't include those definitions in what is considered a cluster.

We shouldn't include this in the default template as all addon and other components (such as cni) needs to be managed by the user after the base cluster is up and running.

Since CAPI doesn't deploy CNI's if a customer used this template with a CRS I think they would get errors since they clusterctl generate cluster <template> doesn't include a CNI. I think CRS are also an experimental feature?

The way we are handling this as of today with tilt is to deploy during tilt initialization like:

cluster-api-provider-azure/Tiltfile

Lines 223 to 225 in e2abea2

os.putenv("KUBERNETES_VERSION", settings.get("kubernetes_version", {}))

local(kubectl_cmd + " create configmap flannel-windows-addon --from-file=templates/addons/windows/flannel/ --dry-run=client -o yaml | " + envsubst_cmd + " | " + kubectl_cmd + " apply -f -")

local(kubectl_cmd + " create configmap calico-windows-addon --from-file=templates/addons/windows/calico/ --dry-run=client -o yaml | " + envsubst_cmd + " | " + kubectl_cmd + " apply -f -")

Thanks, I was able to get things working this way for dev clusters!

marosset · 2022-01-31T23:00:18Z

I was able to manually validate that the windows-containerd and machinepool-windows-containerd workload clusters had csi-proxy running when deployed through tilt (and not deployed for windows or machinepool-windows as expected.

marosset · 2022-01-31T23:00:50Z

/hold cancel

jsturtevant · 2022-02-01T00:28:23Z

We aren't running the tests against the csi proxy but they are modifying the templates to add them into the CI cluster templates so we should those tests to make sure it doesn't cause any issues

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

marosset · 2022-02-01T21:46:52Z

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

marosset · 2022-02-01T21:59:31Z

It looks like the azure-disk/azure-file csi jobs are targeting cluster-template-prow.yaml based on
https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_azuredisk-csi-driver/1133/pull-kubernetes-e2e-capz-azure-disk-windows/1481582126804504576/build-log.txt

Create workload Cluster.

/home/prow/go/src/sigs.k8s.io/cluster-api-provider-azure/hack/tools/bin/envsubst-v2.0.0-20210730161058-179042472c46 < /home/prow/go/src/sigs.k8s.io/cluster-api-provider-azure/templates/test/ci/cluster-template-prow.yaml

Let me update this PR so csi-proxy is installed for those cluster templates

marosset · 2022-02-01T22:26:59Z

It looks like the azure-disk/azure-file csi jobs are targeting cluster-template-prow.yaml based on https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_azuredisk-csi-driver/1133/pull-kubernetes-e2e-capz-azure-disk-windows/1481582126804504576/build-log.txt

Create workload Cluster.

/home/prow/go/src/sigs.k8s.io/cluster-api-provider-azure/hack/tools/bin/envsubst-v2.0.0-20210730161058-179042472c46 < /home/prow/go/src/sigs.k8s.io/cluster-api-provider-azure/templates/test/ci/cluster-template-prow.yaml

Let me update this PR so csi-proxy is installed for those cluster templates

I made these updates

marosset · 2022-02-02T19:02:08Z

/test pull-cluster-api-provider-azure-ci-entrypoint
Re-triggering test to pick up kubernetes/test-infra#25082

CecileRobertMichon · 2022-02-02T22:18:58Z

@@ -218,6 +218,8 @@ def create_crs():
    local(kubectl_cmd + " delete configmaps calico-ipv6-addon --ignore-not-found=true")
    local(kubectl_cmd + " create configmap calico-ipv6-addon --from-file=templates/addons/calico-ipv6.yaml")
    local(kubectl_cmd + " delete configmaps flannel-windows-addon --ignore-not-found=true")


unrelated to this PR but @jsturtevant do you know why we are deleting the flannel-windows-addon cm but not creating it?

it's created on line https://github.com/kubernetes-sigs/cluster-api-provider-azure/pull/2012/files#diff-c2ee8653e1d6b85f0aadf87cd438a9250806c052877248442be4d434cbc52425R226

though it doesn't look like we delete calico-windows-addon which is added the line after. Not sure if deletion is required... Seems to be working, even on reboots of tilt

I was going to open another PR to fix that

marosset · 2022-02-08T18:01:06Z

I rebased after #2043 merged and we can now see that csi-proxy container is running on the windows nodes

https://gcsweb.k8s.io/gcs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api-provider-azure/2012/pull-cluster-api-provider-azure-ci-entrypoint/1491093865342439424/artifacts/workload-cluster/capz-d51w-kt8hv/kube-system_csi-proxy-xkbt2_56e72841-a720-4539-8b70-b1a1981916a6/

@jsturtevant @mboersma can you all take another look at this PR?

Thanks!

jsturtevant · 2022-02-08T21:54:07Z

@marosset could you squash the commits? lgtm but I would also like to run the conformance tests since those templates are modified though I don't expect anything.

marosset · 2022-02-08T22:00:43Z

@marosset could you squash the commits? lgtm but I would also like to run the conformance tests since those templates are modified though I don't expect anything.

I added the label to have tide squash the commits for me. Is that sufficient?

jsturtevant · 2022-02-08T22:16:51Z

This has come up in past. PR release notes don't show up in the release notes with this label.

marosset · 2022-02-08T22:19:58Z

This has come up in past. PR release notes don't show up in the release notes with this label.

Ah ok.
I'll rebase shortly.

Adding csi-proxy to windows-containerd dev template Moving csi-proxy addon to cluster-template-prow.yaml since that is what azure-disk/file tests use in CI Signed-off-by: Mark Rossetti <marosset@microsoft.com>

marosset · 2022-02-08T22:26:03Z

/remove-label tide/merge-method-squash

jsturtevant · 2022-02-08T22:26:57Z

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

devigned

/lgtm
/approve

k8s-ci-robot · 2022-02-09T23:06:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: devigned

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [devigned]

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 juan-lee and shysank January 26, 2022 22:45

jsturtevant reviewed Jan 27, 2022

View reviewed changes

Comment thread templates/addons/windows/csi-proxy/csi-proxy.yaml Outdated

Comment thread templates/test/ci/patches/windows-csi-proxy-enabled.yaml Outdated

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jan 27, 2022

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 28, 2022

jsturtevant reviewed Jan 31, 2022

View reviewed changes

marosset force-pushed the windows-csi-proxy branch from f078607 to ea16de1 Compare January 31, 2022 22:42

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 31, 2022

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 Feb 1, 2022

marosset force-pushed the windows-csi-proxy branch 2 times, most recently from 5ddbbb5 to fda29a8 Compare February 1, 2022 21:43

CecileRobertMichon reviewed Feb 2, 2022

View reviewed changes

marosset force-pushed the windows-csi-proxy branch 2 times, most recently from 393debc to f839338 Compare February 8, 2022 16:57

Installing csi-proxy in some windows clusters

52f8f9d

Adding csi-proxy to windows-containerd dev template Moving csi-proxy addon to cluster-template-prow.yaml since that is what azure-disk/file tests use in CI Signed-off-by: Mark Rossetti <marosset@microsoft.com>

marosset force-pushed the windows-csi-proxy branch from f839338 to 52f8f9d Compare February 8, 2022 22:22

k8s-ci-robot removed the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Feb 8, 2022

k8s-ci-robot assigned jsturtevant Feb 8, 2022

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

devigned approved these changes Feb 9, 2022

View reviewed changes

k8s-ci-robot assigned devigned Feb 9, 2022

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

k8s-ci-robot merged commit 6e20db5 into kubernetes-sigs:main Feb 10, 2022

k8s-ci-robot added this to the v1.1 milestone Feb 10, 2022

marosset deleted the windows-csi-proxy branch February 10, 2022 18:31

marosset mentioned this pull request Feb 11, 2022

test pr to trigger CAPZ/windows tests kubernetes-sigs/azuredisk-csi-driver#1171

Closed

4 tasks

jsturtevant mentioned this pull request May 13, 2022

[feature request]enable csi-proxy by default on Windows 1.21+ #1814

Closed

	os.putenv("KUBERNETES_VERSION", settings.get("kubernetes_version", {}))
	local(kubectl_cmd + " create configmap flannel-windows-addon --from-file=templates/addons/windows/flannel/ --dry-run=client -o yaml \| " + envsubst_cmd + " \| " + kubectl_cmd + " apply -f -")
	local(kubectl_cmd + " create configmap calico-windows-addon --from-file=templates/addons/windows/calico/ --dry-run=client -o yaml \| " + envsubst_cmd + " \| " + kubectl_cmd + " apply -f -")

Conversation

marosset commented Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsturtevant commented Jan 27, 2022

Uh oh!

jsturtevant left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

marosset commented Jan 27, 2022

Uh oh!

marosset commented Jan 27, 2022

Uh oh!

marosset commented Jan 27, 2022

Uh oh!

marosset commented Jan 27, 2022

Uh oh!

k8s-ci-robot commented Jan 27, 2022

Uh oh!

marosset commented Jan 27, 2022

Uh oh!

marosset commented Jan 28, 2022

Uh oh!

marosset commented Jan 28, 2022

Uh oh!

jsturtevant Jan 31, 2022

Choose a reason for hiding this comment

Uh oh!

marosset Jan 31, 2022

Choose a reason for hiding this comment

Uh oh!

marosset commented Jan 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marosset commented Jan 31, 2022

Uh oh!

jsturtevant commented Feb 1, 2022

Uh oh!

marosset commented Feb 1, 2022

Uh oh!

marosset commented Feb 1, 2022

Create workload Cluster.

Uh oh!

marosset commented Feb 1, 2022

Create workload Cluster.

Uh oh!

marosset commented Feb 2, 2022

Uh oh!

CecileRobertMichon Feb 2, 2022

Choose a reason for hiding this comment

Uh oh!

jsturtevant Feb 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marosset Feb 2, 2022

Choose a reason for hiding this comment

Uh oh!

marosset commented Feb 8, 2022

Uh oh!

jsturtevant commented Feb 8, 2022

Uh oh!

marosset commented Feb 8, 2022

Uh oh!

jsturtevant commented Feb 8, 2022

Uh oh!

marosset commented Feb 8, 2022

Uh oh!

marosset commented Feb 8, 2022

Uh oh!

jsturtevant commented Feb 8, 2022

Uh oh!

devigned left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Feb 9, 2022

Uh oh!

Reviewers

marosset commented Jan 26, 2022 •

edited

Loading

marosset commented Jan 31, 2022 •

edited

Loading

jsturtevant Feb 2, 2022 •

edited

Loading