Prep for Windows support: Copy pkg/mounter and refactor to use k8s.io/mount-utils #786
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wongma7 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 |
k8s-ci-robot
added
approved
Pull Request Test Coverage Report for Build 1708
💛 - Coveralls |
wongma7
changed the title
migration is failing because external-resizer can't mark PVC required? This test doesnt involve the node plugin at all so I don't think it's related to my changes Also unsure if external-resizer will retry in case of transient patch error like this or if it's a legit bug /retest |
|
/test pull-aws-ebs-csi-driver-migration-test-latest |
| ExistsPath(filename string) (bool, error) | ||
|
|
||
| // Implemented by NodeMounter.SafeFormatAndMount.Exec | ||
| // TODO this won't make sense on Windows with csi-proxy |
There was a problem hiding this comment.
What are the TODOs here? Remove them?
There was a problem hiding this comment.
i don't know yet what if anything needs to be done on windows until i actually test it which could take a while so I droped a lot of these windows-related TODOS throughout the code.
There was a problem hiding this comment.
so far I've only tested up to the point where it gets the device path (device number on windows) . All of the subsequent execs and mkdirs and mkfiles, i expect to fail, but exactly how to fix that I don't kno
| return "", err | ||
| } | ||
|
|
||
| // If the path exists, assume it is not nvme device |
There was a problem hiding this comment.
Can we put a link to the source of this assumption and/or explain why it's safe to do so?
There was a problem hiding this comment.
i have no clue, keep in mind this is copy/pasted from node.go
There was a problem hiding this comment.
I think this is it, will link: https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/device_naming.html#available-ec2-device-names
There was a problem hiding this comment.
Thanks. You can do it on the next PR if you like, up to you.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
Is this a bug fix or adding new feature? new
What is this PR about? / Why do we need it? As mentioned in pkg/mounter/README I have copied the csi-proxy implementation of mount.Interface from https://github.com/kubernetes-sigs/azuredisk-csi-driver/tree/master/pkg/mounter . Basically it is a platform independent package where on unix the mounter will be the same SafeFormatAndMount implementation as before and on windows the mounter will be the newer csi-proxy implementation. Since GCE PD also has their own similar implementation, we might want to consider moving it to its own k8s repo, but in meantime copy/pasting the code is the least painful way to consume it IMO. I don't want to depend on azure/gce repos and pull in a mountain of dependencies.
Since pkg/mounter implements the latest mount interface from k8s.io/mount-utils, not k8s.io/utils/mount, I have to do some refactoring to use it.
I also refactored NodeStage tests to be table driven but it's too much effort to refactor them all. The length of the test file and amount of code repetition is becoming too much IMO.
What testing is done?
unit testing locally go test ./pkg/driver/...