Update volume ID format for metro volumes #345
Conversation
- remove -coverpkg option from test makefile recipe to fix incorrect coverage numbers
- new volume handle {vol-uuid}/{ps-global-id}/{protocol}:{remote-vol-uuid}/{remote-ps-global-id}
- reorganize test structure to add metro test organization - add BeforeEach to set default mocks and vars to metro tests
pkg/array/array.go
Outdated
| @@ -254,30 +254,54 @@ func GetPowerStoreArrays(fs fs.Interface, filePath string) (map[string]*PowerSto | |||
| return arrayMap, mapper, defaultArray, nil | |||
| } | |||
|
|
|||
| // ParseVolumeID parses volume id in from CO (Kubernetes) and tries to understand what in it are PowerStore volume id, and what is ip, protocol. | |||
| // "/" is used as a delimiter. | |||
| // ParseVolumeID parses a volume id from the CO (Kubernetes) and tries to extract the PowerStore volume id, Global ID, and protocol. | |||
| // | |||
| // Example: | |||
| // | |||
| // ParseVolumeID("1cd254s/192.168.0.1/scsi") will return | |||
There was a problem hiding this comment.
IP in the example here can also be updated to reflect global ID.
There was a problem hiding this comment.
Actually, this is one of the supported legacy formats. Give it an IP and it should return the global ID.
I will update the example var names tho.
There was a problem hiding this comment.
I don't think we should still support this legacy format. Not sure if such upgrade paths are supported.
There was a problem hiding this comment.
It would be nice to get away from the legacy format, but I'm also unsure. We could potentially add a k8s job workload that validates and updates existing volume names, but it would be a non-trivial request.
lukeatdell
requested review from
abhi16394,
adarsh-dell,
AkshaySainiDell and
donatwork
as code owners
| // | ||
| // Example: | ||
| // | ||
| // ParseVolumeID("9f840c56-96e6-4de9-b5a3-27e7c20eaa77/PSabcdef0123/scsi:9f840c56-96e6-4de9-b5a3-27e7c20eaa77/PS0123abcdef") returns |
There was a problem hiding this comment.
For any chance can this (volume ID) go beyond 253 char in any case?
There was a problem hiding this comment.
Luke may not be able to respond in time. What are the concerns?
There was a problem hiding this comment.
All components of the volume are known and handled by the CreateVolume k8s gRPC implementation in controller.go, so this is as long as it should ever be.
Description
Volume ID format for metro volumes will build on the existing volume ID format, adding the remote volume and system information separated by a colon.
{vol-uuid}/{PS-globalID}/{protocol}->{vol-uuid}/{PS-globalID}/{protocol}:{remote-vol-uuid}/{remote-PS-globalID}ParseVolumeIDfunction updated to support parsing metro volume IDs.ParseVolumeIDto support the new return format.ParseVolumeIDunit test coverage increased to avoid regressions.CreateVolumefunction updated to apply the new volume ID format to metro volumes on creation.CreateVolumewith respect to metro volumes. Reorganized tests to group metro replication tests together.GitHub Issues
List the GitHub issues impacted by this PR:
Checklist:
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Please also list any relevant details for your test configuration
Current code coverage
For those interested in viewing current coverage: coverage.zip
Integration test: Metro volume provisioned on a k8s cluster. Confirmed corresponding backend resources present on primary and secondary PowerStore array.
Regression test: Full cert-csi with ext4 iSCSI
