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

Fix staging / unmounting volume operations on Windows #1526

Merged
merged 1 commit into from
Mar 31, 2023

Conversation

torredil
Copy link
Member

@torredil torredil commented Mar 9, 2023

Is this a bug fix or adding new feature?

  • Bug fix.

What is this PR about?

This PR fixes several bugs uncovered by running the external e2e tests on Windows. It will allow us to run the tests in parallel and should land before #1521.

  • GetDeviceNameFromMount now returns the disk number for a mount path on Windows.
  • Cleanup the stage path via Rmdir in proxyMounter.Unmount() before offlining the disk.
  • Remove unnecessary call to proxyMounter.WriteVolumeCache() when unpublishing a volume from the node.
  • Return a nil error in GetDeviceNameFromMount if the target path does not exist, per the CSI spec: If the volume corresponding to the volume_id is not staged to the staging_target_path, the Plugin MUST reply 0 OK.

Why do we need it?

To fix the following issues:

I0308 16:30:11.616491    6048 node.go:260] "NodeUnStageVolume: volume operation finished" volumeID="vol-0575b28e63a6f11a5"
I0308 16:30:11.616491    6048 inflight.go:74] "Node Service: volume operation finished" key="vol-0575b28e63a6f11a5"
E0308 16:30:11.616491    6048 driver.go:120] "GRPC error" err=<
	rpc error: code = Internal desc = failed to check if volume is mounted: error getting device name from mount: rpc error: code = Unknown desc = error getting the volume for the mount c:\var\lib\kubelet\plugins\kubernetes.io\csi\ebs.csi.aws.com\1610843f84a51551af6fe5a45ef0fc4afcc8ac37f65a219815fccad076ef7dcc\globalmount, internal error error getting volume from mount. cmd: (Get-Item -Path Get-Item : Cannot find path 'C:\var\lib\kubelet\plugins\kubernetes.io\csi\ebs.csi.aws.com\1610843f84a51551af6fe5a45ef0f
	922efd496ae6e73900f42d7ccc7ef58c003\globalmount' because it does not exist.
	At line:1 char:2
	+ (Get-Item -Path c:\var\lib\kubelet\plugins\kubernetes.io\csi\ebs.csi. ...
	+  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
	    + CategoryInfo          : ObjectNotFound: (C:\var\lib\kube...003\globalmount:String) [Get-Item], ItemNotFoundExcep
	   tion
	    + FullyQualifiedErrorId : PathNotFound,Microsoft.PowerShell.Commands.GetItemCommand).Target, output: At line:2 char:76
	+ ... 496ae6e73900f42d7ccc7ef58c003\globalmount' because it does not exist.
I0303 01:42:25.179841     652 node.go:172] "NodeStageVolume: volume operation finished" volumeID="vol-0ce949dffa97f251a"
I0303 01:42:25.180392     652 inflight.go:74] "Node Service: volume operation finished" key="vol-0ce949dffa97f251a"
E0303 01:42:25.180392     652 driver.go:120] "GRPC error" err=<
	rpc error: code = Internal desc = could not format "3" and mount it at "\\var\\lib\\kubelet\\plugins\\kubernetes.io\\csi\\ebs.csi.aws.com\\914755ec302b648ec6285e1fe8790121c95058c4e191a432214a2a3472aac79c\\globalmount": rpc error: code = Unknown desc = error mount volume to path. cmd: Get-Volume -UniqueId "\\?\Volume{5baf4ea8-203e-477a-845c-77a024150315}\" | Get-Partition | Add-PartitionAccessPath -AccessPath c:\var\lib\kubelet\plugins\kubernetes.io\csi\ebs.csi.aws.com\914755ec302b648ec6285e1fe8790121c95058c4e191a432214a2a3472aac79c\globalmount, output: Add-PartitionAccessPath : The requested access path is already in use.
	Activity ID: {cb64fe87-eb6f-42de-a3dc-d2505bb47395}
	At line:1 char:92
	+ ... Partition | Add-PartitionAccessPath -AccessPath c:\var\lib\kubelet\pl ...
	+                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
	    + CategoryInfo          : InvalidArgument: (StorageWMI:ROOT/Microsoft/.../MSFT_Partition) [Add-PartitionAccessPath
	   ], CimException
	    + FullyQualifiedErrorId : StorageWMI 42002,Add-PartitionAccessPath

	, error: exit status 1

What testing is done?

$ kubetest2 noop \         
  --run-id="e2e-kubernetes" \
  --test=ginkgo \
  -- \
  --test-package-version=v1.27.0-alpha.3 \
  --skip-regex="\[Disruptive\]|\[Serial\]|\[LinuxOnly\]|\[Feature:VolumeSnapshotDataSource\]|(xfs)|(ext4)|(block volmode)|csinodes" \
  --focus-regex="External.Storage" \
  --parallel=15 \
  --test-args="-storage.testdriver=/home/torredil/go/src/github.com/torredil/aws-ebs-csi-driver/tests/e2e-kubernetes/manifests.yaml -node-os-distro=windows"

Screen Shot 2023-03-09 at 5 35 43 PM

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 9, 2023
@k8s-ci-robot k8s-ci-robot requested review from gtxu and rdpsin March 9, 2023 22:37
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 9, 2023
pkg/driver/node_test.go Outdated Show resolved Hide resolved
pkg/driver/mount_windows.go Show resolved Hide resolved
pkg/driver/mount_windows.go Show resolved Hide resolved
pkg/mounter/safe_mounter_windows.go Show resolved Hide resolved
pkg/driver/node.go Show resolved Hide resolved
- `GetDeviceNameFromMount` now returns the disk number for a mount path on Windows
- Cleanup the stage path via Rmdir in `proxyMounter.Unmount()`
- Remove unnecessary call to `proxyMounter.WriteVolumeCache()` when unpublishing a volume
- Return a nil error in `GetDeviceNameFromMount` if the target path does not exist

Signed-off-by: Eddie Torres <[email protected]>
Copy link
Contributor

@ConnorJC3 ConnorJC3 left a comment

Choose a reason for hiding this comment

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

/lgtm

👍

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 22, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gtxu, hanyuel

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 Mar 31, 2023
@k8s-ci-robot k8s-ci-robot merged commit 7db7d1a into kubernetes-sigs:master Mar 31, 2023
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. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants