Skip to content
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

Add Windows daemonset to the helm template #275

Merged
merged 2 commits into from
Nov 4, 2021

Conversation

mauriciopoppe
Copy link
Member

@mauriciopoppe mauriciopoppe commented Oct 26, 2021

What type of PR is this?
/kind feature

What this PR does / why we need it:
Adds a version of the daemonset that works in Windows nodes through a different NodeSelector

Which issue(s) this PR fixes:

Fixes #270

Release note:

Add Windows daemonset to the helm template 

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 26, 2021
@mauriciopoppe
Copy link
Member Author

/cc @jingxu97

useNodeNameOnly: true
classes:
- name: local-scsi
hostDir: "/mnt/disks"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this class for both Linux and Windows?

Copy link
Member Author

@mauriciopoppe mauriciopoppe Nov 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, initially I had classesWindows but it's not needed

@jingxu97
Copy link
Contributor

jingxu97 commented Nov 4, 2021

/lgtm
/approve
A few TODO in the PR, you could update those later.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 4, 2021
@mauriciopoppe
Copy link
Member Author

/assign @msau42

@msau42
Copy link
Contributor

msau42 commented Nov 4, 2021

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jingxu97, mauriciopoppe, msau42

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 4, 2021
@k8s-ci-robot k8s-ci-robot merged commit a5ccd2c into kubernetes-sigs:master Nov 4, 2021
mountPath: /etc/provisioner/config
readOnly: true
- name: provisioner-dev
mountPath: /dev
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @mauriciopoppe have you verified that this local volume provisioner works on Windows? on Windows, there is no such /dev, the device discovery should be quite different on Windows IMO

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I remember correctly /dev isn't read by the Windows impl because it doesn't have any mount points, during development I set the mount points in:

			- name: local-storage
              mountPath: /mnt/disks
              mountPropagation: HostToContainer

the device discovery should be quite different on Windows IMO

This is true, I precreated mountPoints (symlinks to the actual device) in /mnt/disks in the host.

I run all the e2e tests successfully with CSI Proxy v1.1.0 locally and got the presubmits tests to pass in #279 with Windows too, I think the remaining item was to bump kubernetes/kubernetes to use this version of CSI Proxy.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mauriciopoppe did you hit following error when start the provisioner:

I1007 14:38:40.991041    8780 common.go:348] StorageClass "local-disk" configured with MountDir "c:\\dev", HostDir "c:\\dev", VolumeMode "Filesystem", FsType "ext4", BlockCleanerCommand ["/scripts/shred.sh" "2"], NamePattern "nvme*"
I1007 14:38:41.544387    8780 main.go:66] Loaded configuration: {StorageClassConfig:map[local-disk:{HostDir:c:\dev MountDir:c:\dev BlockCleanerCommand:[/scripts/shred.sh 2] VolumeMode:Filesystem FsType:ext4 NamePattern:nvme*}] NodeLabelsForPV:[] UseAlphaAPI:false UseJobForCleaning:false MinResyncPeriod:{Duration:5m0s} UseNodeNameOnly:false LabelsForPV:map[] SetPVOwnerRef:false}
I1007 14:38:41.544387    8780 main.go:67] Ready to run...
I1007 14:38:41.544387    8780 common.go:425] Creating client using in-cluster config
I1007 14:38:41.593060    8780 main.go:132] Could not get node information (remaining retries: 2): Get "https://andy-aks1219-dns-a2701255.hcp.uksouth.azmk8s.io:443/api/v1/nodes/akswin000000": dial tcp: lookup andy-aks1219-dns-a2701255.hcp.uksouth.azmk8s.io: no such host
I1007 14:38:42.653329    8780 main.go:88] Starting controller
I1007 14:38:42.653329    8780 main.go:105] Starting metrics server at :8080
I1007 14:38:42.653329    8780 controller.go:47] Initializing volume cache
E1007 14:38:42.653329    8780 csi_proxy_windows.go:59] failed to connect to csi-proxy v1beta with error=open \\.\\pipe\\csi-proxy-volume-v1beta2: Access is denied.
F1007 14:38:42.653329    8780 controller.go:68] Error initializing VolumeUtil: open \\.\\pipe\\csi-proxy-volume-v1beta2: Access is denied.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nvm, this PR should fix the issue: #338

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support to run in Windows nodes
5 participants