feat: Run Windows containers as host-process containers by marosset · Pull Request #1201 · kubernetes-sigs/azuredisk-csi-driver

marosset · 2022-03-03T19:16:43Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR allows for running the Windows azuredisk csi driver containers as host-process containers.
This simplifies the deployment of the drivers and aligns with the long-term CSI plugin goals of the Kubernetes community.

This also works around kubernetes-sigs/cluster-api-provider-azure#2132 allowing azuredisk csi drivers to be used in CAPZ clusters.

Note: this still requires csi-proxy to be installed and running on Windows nodes in the cluster
Which issue(s) this PR fixes:

Fixes #

Requirements:

uses conventional commit messages
includes documentation
adds unit tests
tested upgrade from previous version

Special notes for your reviewer:

Release note:

Adds the ability to run windows node containers as host-process containers.

marosset · 2022-03-03T19:17:09Z

I will work on updating the helm charts now too.

marosset · 2022-03-03T23:22:26Z

I updated the helm charts.
I still need to do some local validation and then also update

azuredisk-csi-driver/Makefile

Lines 110 to 128 in 24e5bb7

    
           .PHONY: e2e-bootstrap 
        
           e2e-bootstrap: install-helm 
        
           	docker pull $(IMAGE_TAG) || make container-all push-manifest 
        
           ifdef TEST_WINDOWS 
        
           	helm install azuredisk-csi-driver charts/${CHART_VERSION}/azuredisk-csi-driver --namespace kube-system --wait --timeout=15m -v=5 --debug \ 
        
           		${E2E_HELM_OPTIONS} \ 
        
           		--set windows.enabled=true \ 
        
           		--set linux.enabled=false \ 
        
           		--set controller.runOnMaster=true \ 
        
           		--set controller.replicas=1 \ 
        
           		--set controller.logLevel=6 \ 
        
           		--set node.logLevel=6 \ 
        
           		--set cloud=$(CLOUD) 
        
           else 
        
           	helm install azuredisk-csi-driver charts/${CHART_VERSION}/azuredisk-csi-driver --namespace kube-system --wait --timeout=15m -v=5 --debug \ 
        
           		${E2E_HELM_OPTIONS} \ 
        
           		--set snapshot.enabled=true \ 
        
           		--set cloud=$(CLOUD) 
        
           endif

so we can use host process containers based install if requested

marosset · 2022-03-07T18:43:27Z

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

marosset · 2022-03-07T18:57:04Z

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

marosset · 2022-03-07T18:58:08Z

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

marosset · 2022-03-07T20:31:43Z

/retest

marosset · 2022-03-09T23:07:11Z

/retest

andyzhangx · 2022-03-11T02:24:52Z

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

marosset · 2022-03-11T17:02:36Z

Let me fix the verify job and then squash all my commits.

marosset · 2022-03-11T17:16:55Z

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

marosset · 2022-03-11T17:41:02Z

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

jsturtevant · 2022-03-11T18:12:14Z

I don't have a ton of experience with the CSI drivers but generally lgtm and the test pull-kubernetes-e2e-capz-azure-disk-windows is passing using this template.

marosset · 2022-03-11T19:36:38Z

@andyzhangx it looks like pull-kubernetes-e2e-capz-azure-disk is timing out running windows test cases

ex: Should create and attach a volume with basic perfProfile [enableBursting][disk.csi.azure.com] [Windows] [It]

Do we need to set ginkgo.skip somewhere?

andyzhangx · 2022-03-12T02:37:35Z

@andyzhangx it looks like pull-kubernetes-e2e-capz-azure-disk is timing out running windows test cases

ex: Should create and attach a volume with basic perfProfile [enableBursting][disk.csi.azure.com] [Windows] [It]

Do we need to set ginkgo.skip somewhere?

@marosset that's zone issue:

"Disk /subscriptions/0e46bd28-a80f-4d3a-8200-d9eb8d80cb2e/resourceGroups/CAPZ-9B3W2C/providers/Microsoft.Compute/disks/reattach-disk-multiple-nodes cannot be attached to the VM because it is not in zone '1'."

currently master node is in zone 1, while agent node are non-zone, the test case would create a PV disk first with zone 1, and then it could not be attached to non-zone node, I will check how to fix it later

andyzhangx · 2022-03-18T06:13:12Z

/test pull-azuredisk-csi-driver-e2e-capz
/test pull-azuredisk-csi-driver-e2e-capz-windows

andyzhangx · 2022-03-18T13:53:59Z

/test pull-azuredisk-csi-driver-e2e-capz

marosset · 2022-03-18T18:22:04Z

/test pull-azuredisk-csi-driver-e2e-capz
/test pull-azuredisk-csi-driver-e2e-capz-windows

andyzhangx · 2022-03-21T14:47:04Z

 {{- else }}
          image: "{{ .Values.image.livenessProbe.repository }}:{{ .Values.image.livenessProbe.tag }}"
 {{- end }}
+          imagePullPolicy: {{ .Values.image.livenessProbe.pullPolicy }}


I thought it was a small diff, while it looks like these two deployments are quite different now, then I think we should keep two deployment files, otherwise it may be hard to understand and maintain.

No problem. I dropped the commit to split merge the manifests.
Let me rebase again.
The manifest do have many subtle differences.

… containers Signed-off-by: Mark Rossetti <marosset@microsoft.com>

marosset · 2022-03-21T18:29:36Z

/test pull-azuredisk-csi-driver-e2e-capz-windows

marosset · 2022-03-22T21:09:11Z

@andyzhangx do you have any other concerns or feedback here?
I'd like to try get CAPZ + azuredisk + Windows periodic jobs enabled.

andyzhangx

/lgtm
/approve

k8s-ci-robot · 2022-03-23T07:51:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyzhangx, marosset

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

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

andyzhangx · 2022-03-23T07:58:29Z

/retest

andyzhangx · 2022-03-23T08:35:30Z

/retest

andyzhangx · 2022-03-23T10:54:23Z

/retest

andyzhangx · 2022-03-23T12:08:28Z

/retest

k8s-ci-robot · 2022-03-23T12:11:23Z

@marosset: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-e2e-capz-azure-disk	`639905a`	link	false	`/test pull-kubernetes-e2e-capz-azure-disk`
pull-azuredisk-csi-driver-e2e-windows	`aea6ca1`	link	true	`/test pull-azuredisk-csi-driver-e2e-windows`
pull-azuredisk-csi-driver-e2e-capz	`aea6ca1`	link	false	`/test pull-azuredisk-csi-driver-e2e-capz`
pull-azuredisk-csi-driver-verify	`38d86ab`	link	true	`/test pull-azuredisk-csi-driver-verify`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

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.

k8s-ci-robot requested review from ZeroMagic and andyzhangx March 3, 2022 19:17

marosset changed the title ~~WIP: Run Windows containers as host-process containers~~ WIP: feat: Run Windows containers as host-process containers Mar 3, 2022

marosset mentioned this pull request Mar 4, 2022

Set WINDOWS_USE_HOST_PROCESS_CONTAINERS for pull-kubernetes-e2e-capz-azure-disk-windows kubernetes/test-infra#25513

Merged

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

marosset force-pushed the windows-capz-work branch from 79fa1e8 to e25eba9 Compare March 5, 2022 00:14

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

marosset changed the title ~~WIP: feat: Run Windows containers as host-process containers~~ feat: Run Windows containers as host-process containers Mar 5, 2022

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 5, 2022

marosset mentioned this pull request Mar 7, 2022

test pr to trigger CAPZ/windows tests #1171

Closed

4 tasks

marosset force-pushed the windows-capz-work branch from e25eba9 to 0ab7eb4 Compare March 7, 2022 18:51

marosset mentioned this pull request Mar 9, 2022

feat: Run Windows containers as host-process containers kubernetes-sigs/azurefile-csi-driver#936

Closed

4 tasks

marosset force-pushed the windows-capz-work branch from acf22c8 to e2f6403 Compare March 11, 2022 17:13

marosset force-pushed the windows-capz-work branch from e2f6403 to 639905a Compare March 11, 2022 17:39

andyzhangx reviewed Mar 12, 2022

View reviewed changes

Comment thread charts/latest/azuredisk-csi-driver/templates/csi-azuredisk-node-windows-hostprocess.yaml

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

andyzhangx reviewed Mar 21, 2022

View reviewed changes

marosset force-pushed the windows-capz-work branch from aea6ca1 to 639905a Compare March 21, 2022 17:53

Adding deployment file for running windows containers as host process…

38d86ab

… containers Signed-off-by: Mark Rossetti <marosset@microsoft.com>

marosset force-pushed the windows-capz-work branch from 639905a to 38d86ab Compare March 21, 2022 18:12

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

andyzhangx approved these changes Mar 23, 2022

View reviewed changes

k8s-ci-robot assigned andyzhangx Mar 23, 2022

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

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

andyzhangx merged commit b10c564 into kubernetes-sigs:master Mar 23, 2022

CecileRobertMichon mentioned this pull request Feb 9, 2023

Use hostprocess for Windows node manager in Helm chart kubernetes-sigs/cloud-provider-azure#3283

Merged

Conversation

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

Uh oh!

Uh oh!

marosset commented Mar 3, 2022

Uh oh!

marosset commented Mar 3, 2022

Uh oh!

marosset commented Mar 7, 2022

Uh oh!

marosset commented Mar 7, 2022

Uh oh!

marosset commented Mar 7, 2022

Uh oh!

marosset commented Mar 7, 2022

Uh oh!

marosset commented Mar 9, 2022

Uh oh!

andyzhangx commented Mar 11, 2022

Uh oh!

marosset commented Mar 11, 2022

Uh oh!

marosset commented Mar 11, 2022

Uh oh!

marosset commented Mar 11, 2022

Uh oh!

jsturtevant commented Mar 11, 2022

Uh oh!

marosset commented Mar 11, 2022

Uh oh!

andyzhangx commented Mar 12, 2022

Uh oh!

Uh oh!

andyzhangx commented Mar 18, 2022

Uh oh!

andyzhangx commented Mar 18, 2022

Uh oh!

marosset commented Mar 18, 2022

Uh oh!

andyzhangx Mar 21, 2022

Choose a reason for hiding this comment

Uh oh!

marosset Mar 21, 2022

Choose a reason for hiding this comment

Uh oh!

marosset commented Mar 21, 2022

Uh oh!

marosset commented Mar 22, 2022

Uh oh!

andyzhangx left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Mar 23, 2022

Uh oh!

andyzhangx commented Mar 23, 2022

Uh oh!

andyzhangx commented Mar 23, 2022

Uh oh!

andyzhangx commented Mar 23, 2022

Uh oh!

andyzhangx commented Mar 23, 2022

Uh oh!

k8s-ci-robot commented Mar 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

marosset commented Mar 3, 2022 •

edited

Loading

k8s-ci-robot commented Mar 23, 2022 •

edited

Loading