Don't count reserved partitions (gpt) when checking if any exist

[wongma7]

What type of PR is this?
/kind bug

What this PR does / why we need it: see #144: with gpt being default, there is some 128MB reserved partition at beginning of disk. LIke

PartitionNumber  DriveLetter Offset                                        Size Type
---------------  ----------- ------                                        ---- ----
1                            1048576                                     128 MB Reserved
2                            135266304                                 19.87 GB Basic

Don't count it when checking if partition exists.

Which issue(s) this PR fixes:

Fixes #144

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

[k8s-ci-robot]

Hi @wongma7. Thanks for your PR.

I'm waiting for a kubernetes-csi member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[Jiawei0227]

/ok-to-test
/cc @jingxu97

[jingxu97]

/lgtm

[wongma7]

rebased (took a few tries xd)

[ddebroy]

Please update the comment at

csi-proxy/internal/os/disk/api.go

Line 36 in 2b7dc43

// PartitionsExist checks if the disk `diskNumber` has any partitions.

to state that reserved partition types are excluded.

Windows allows you to create Reserved partitions as well for GPT: https://docs.microsoft.com/en-us/powershell/module/storage/new-partition?view=windowsserver2019-ps [Microsoft Reserved - "{e3c9e316-0b5c-4db8-817d-f92df00215ae}"]

Instead of the change in the PR, how about listing partitions and skipping a single reserved partition if disk is GPT in

csi-proxy/internal/server/disk/server.go

Line 70 in 2b7dc43

partitioned, err := s.hostAPI.PartitionsExist(diskNumber)

[wongma7]

What about making it just check for Basic partitions? (Type -eq Basic). since this is an internal function only used by PartitionDisk (which is part of the API) to check whether CreatePartition needs to be called, and CreatePartition always creates a Basic partition, I think it is best to narrow the scope to just Basic

[wongma7]

What about making it just check for Basic partitions?

I just pushed a change for what this looks like

52    6156 server.go:113] Request: ListDiskIDs
I0603 20:28:37.491552    6156 api.go:69] Executing command: "C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe /c (Get-Disk | Select Path, SerialNumber) | ConvertTo-Json"
I0603 20:28:40.172585    6156 server.go:122] calling PathExists with path "c:\\var\\lib\\kubelet\\plugins\\kubernetes.io\\csi\\pv\\pvc-ab6391a9-9b7d-4741-9c51-d304d90bf58c\\globalmount"
I0603 20:28:40.173288    6156 server.go:274] GetVolumeIDFromTargetPath: Request: &{TargetPath:c:\var\lib\kubelet\plugins\kubernetes.io\csi\pv\pvc-ab6391a9-9b7d-4741-9c51-d304d90bf58c\globalmount}
I0603 20:28:40.176496    6156 api.go:51] Executing command: "C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe /c (Get-Item -Path c:\\var\\lib\\kubelet\\plugins\\kubernetes.io\\csi\\pv\\pvc-ab6391a9-9b7d-4741-9c51-d304d90bf58c\\globalmount).Target"
E0603 20:28:40.552050    6156 server.go:283] failed GetVolumeIDFromTargetPath: error getting the volume for the mount c:\var\lib\kubelet\plugins\kubernetes.io\csi\pv\pvc-ab6391a9-9b7d-4741-9c51-d304d90bf58c\globalmount, internal error error getting volume from mount. cmd: (Get-Item -Path c:\var\lib\kubelet\plugins\kubernetes.io\csi\pv\pvc-ab6391a9-9b7d-4741-9c51-d304d90bf58c\globalmount).Target, output: , error: <nil>
I0603 20:28:40.560078    6156 server.go:49] Request: PartitionDisk with diskNumber=5
I0603 20:28:40.563608    6156 api.go:69] Executing command: "C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe /c Get-Disk -Number 5 | Where partitionstyle -eq 'raw'"
I0603 20:28:42.931157    6156 server.go:59] Initializing disk 5
I0603 20:28:42.933984    6156 api.go:69] Executing command: "C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe /c Initialize-Disk -Number 5 -PartitionStyle GPT"
I0603 20:28:45.275301    6156 server.go:69] Checking if disk 5 has basic partitions
I0603 20:28:45.277312    6156 api.go:69] Executing command: "C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe /c Get-Partition | Where DiskNumber -eq 5 | Where Type -eq Basic"
I0603 20:28:47.533401    6156 server.go:76] Creating basic partition on disk 5
I0603 20:28:47.536044    6156 api.go:69] Executing command: "C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe /c New-Partition -DiskNumber 5 -UseMaximumSize"
I0603 20:28:49.993549    6156 server.go:25] ListVolumesOnDisk: Request: &{DiskNumber:5 PartitionNumber:0}
I0603 20:28:50.008933    6156 api.go:51] Executing command: "C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe /c (Get-Disk -Number 5 | Get-Partition | Get-Volume).UniqueId"
I0603 20:28:52.604848    6156 server.go:97] IsVolumeFormatted: Request: &{VolumeId:\\?\Volume{0c64d232-5bcd-4cff-b9c3-d0e96a34f515}\}
I0603 20:28:52.607428    6156 api.go:51] Executing command: "C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe /c (Get-Volume -UniqueId \"\\\\?\\Volume{0c64d232-5bcd-4cff-b9c3-d0e96a34f515}\\\" -ErrorAction Stop).FileSystemType"
I0603 20:28:54.944565    6156 server.go:116] FormatVolume: Request: &{VolumeId:\\?\Volume{0c64d232-5bcd-4cff-b9c3-d0e96a34f515}\}
I0603 20:28:54.947178    6156 api.go:51] Executing command: "C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe /c Get-Volume -UniqueId \"\\\\?\\Volume{0c64d232-5bcd-4cff-b9c3-d0e96a34f515}\\\" | Format-Volume -FileSystem ntfs -Confirm:$false"
I0603 20:29:00.160646    6156 server.go:39] MountVolume: Request: &{VolumeId:\\?\Volume{0c64d232-5bcd-4cff-b9c3-d0e96a34f515}\ TargetPath:c:\var\lib\kubelet\plugins\kubernetes.io\csi\pv\pvc-ab6391a9-9b7d-4741-9c51-d304d90bf58c\globalmount}
I0603 20:29:00.161292    6156 api.go:51] Executing command: "C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe /c Get-Volume -UniqueId \"\\\\?\\Volume{0c64d232-5bcd-4cff-b9c3-d0e96a34f515}\\\" | Get-Partition | Add-PartitionAccessPath -AccessPath c:\\var\\lib\\kubelet\\plugins\\kubernetes.io\\csi\\pv\\pvc-ab6391a9-9b7d-4741-9c51-d304d90bf58c\\globalmount"
I0603 20:29:03.286442    6156 server.go:122] calling PathExists with path "c:\\var\\lib\\kubelet\\pods\\ef469fdf-6cab-4535-8cd0-23d911a08e0d\\volumes\\kubernetes.io~csi\\pvc-ab6391a9-9b7d-4741-9c51-d304d90bf58c\\mount"
I0603 20:29:03.287104    6156 server.go:172] calling Rmdir with path "c:\\var\\lib\\kubelet\\pods\\ef469fdf-6cab-4535-8cd0-23d911a08e0d\\volumes\\kubernetes.io~csi\\pvc-ab6391a9-9b7d-4741-9c51-d304d90bf58c\\mount"
I0603 20:29:03.287890    6156 server.go:193] calling LinkPath with targetPath "c:\\var\\lib\\kubelet\\pods\\ef469fdf-6cab-4535-8cd0-23d911a08e0d\\volumes\\kubernetes.io~csi\\pvc-ab6391a9-9b7d-4741-9c51-d304d90bf58c\\mount" sourcePath "c:\\var\\lib\\kubelet\\plugins\\kubernetes.io\\csi\\pv\\pvc-ab6391a9-9b7d-4741-9c51-d304d90bf58c\\globalmount"

[ddebroy]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ddebroy, wongma7

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 [ddebroy]

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

[andyzhangx]

@wongma7 if it's a MBR disk, Type would be IFS, so this change would break existing MBR disk, and before 1.22, default windows disk type would be MBR disk, that's a breaking and incompatible change.