Refactor volume mount logic #1264
Conversation
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: pvName, | ||
| Labels: getCsiAzurePvLabels(appName, namespace, componentName, radixVolumeMount), | ||
| Annotations: getCsiAzurePvAnnotations(csiVolumeCredSecretName, namespace, useAzureIdentity), |
There was a problem hiding this comment.
I feel we should skip setting these annotations since none of them are documented anywhere. They are probably just for internal use for automatic privisioning from SC
| return "" | ||
| } | ||
|
|
||
| func getCsiAzurePvAnnotations(csiVolumeCredSecretName, namespace string, useAzureIdentity bool) map[string]string { |
There was a problem hiding this comment.
Remove this function after removing call from buildCsiAzurePv func
| CsiAnnotationProvisionedBy = "pv.kubernetes.io/provisioned-by" | ||
| CsiAnnotationProvisionerDeletionSecretName = "volume.kubernetes.io/provisioner-deletion-secret-name" | ||
| CsiAnnotationProvisionerDeletionSecretNamespace = "volume.kubernetes.io/provisioner-deletion-secret-namespace" |
There was a problem hiding this comment.
| CsiAnnotationProvisionedBy = "pv.kubernetes.io/provisioned-by" | |
| CsiAnnotationProvisionerDeletionSecretName = "volume.kubernetes.io/provisioner-deletion-secret-name" | |
| CsiAnnotationProvisionerDeletionSecretNamespace = "volume.kubernetes.io/provisioner-deletion-secret-namespace" |
Can be removed after getCsiAzurePvAnnotations function is deleted
| if !utils.EqualStringMaps(getPvAnnotations(pv1), getPvAnnotations(pv2)) { | ||
| return false | ||
| } |
There was a problem hiding this comment.
| if !utils.EqualStringMaps(getPvAnnotations(pv1), getPvAnnotations(pv2)) { | |
| return false | |
| } |
Remove this comparison since we don't set any annotations
| if !utils.EqualStringMaps(getAnnotations(pvc1), getAnnotations(pvc2)) { | ||
| return false | ||
| } |
There was a problem hiding this comment.
| if !utils.EqualStringMaps(getAnnotations(pvc1), getAnnotations(pvc2)) { | |
| return false | |
| } |
Should probably remove this comparison since we don't set any annotations.
| expectedClonedVolumeAttrs := cloneMap(pv1.Spec.CSI.VolumeAttributes, CsiVolumeMountAttributePvName, CsiVolumeMountAttributePvcName, CsiVolumeMountAttributeProvisionerIdentity) | ||
| actualClonedVolumeAttrs := cloneMap(pv2.Spec.CSI.VolumeAttributes, CsiVolumeMountAttributePvName, CsiVolumeMountAttributePvcName, CsiVolumeMountAttributeProvisionerIdentity) | ||
| if !utils.EqualStringMaps(expectedClonedVolumeAttrs, actualClonedVolumeAttrs) { | ||
| return false | ||
| } |
There was a problem hiding this comment.
If I comment out these lines, all tests still pass. Either we miss some tests, or the tests incorrectly use this function (EqualPersistentVolumes) to compare actual and expected
| if pv1.Spec.Capacity[corev1.ResourceStorage] != pv2.Spec.Capacity[corev1.ResourceStorage] || | ||
| len(pv1.Spec.AccessModes) != len(pv2.Spec.AccessModes) || | ||
| (len(pv1.Spec.AccessModes) != 1 && pv1.Spec.AccessModes[0] != pv2.Spec.AccessModes[0]) || | ||
| pv1.Spec.CSI.Driver != pv2.Spec.CSI.Driver { | ||
| return false | ||
| } | ||
|
|
||
| if pv1.Spec.CSI.NodeStageSecretRef != nil { | ||
| if pv2.Spec.CSI.NodeStageSecretRef == nil || pv1.Spec.CSI.NodeStageSecretRef.Name != pv2.Spec.CSI.NodeStageSecretRef.Name { | ||
| return false | ||
| } | ||
| } else if pv2.Spec.CSI.NodeStageSecretRef != nil { | ||
| return false | ||
| } | ||
|
|
||
| if pv1.Spec.ClaimRef != nil { | ||
| if pv2.Spec.ClaimRef == nil || | ||
| !internal.EqualTillPostfix(pv1.Spec.ClaimRef.Name, pv2.Spec.ClaimRef.Name, 5) || | ||
| pv1.Spec.ClaimRef.Namespace != pv2.Spec.ClaimRef.Namespace || | ||
| pv1.Spec.ClaimRef.Kind != pv2.Spec.ClaimRef.Kind { | ||
| return false | ||
| } | ||
| } else if pv2.Spec.ClaimRef != nil { | ||
| return false | ||
| } |
There was a problem hiding this comment.
If I comment out these lines, all tests still pass. Either we miss some tests, or the tests incorrectly use this function (EqualPersistentVolumes) to compare actual and expected
| package internal | ||
|
|
||
| // EqualTillPostfix Compares two strings till the postfix | ||
| func EqualTillPostfix(value1, value2 string, postfixLength int) bool { |
There was a problem hiding this comment.
Can we move (and make private) this function to the volumemount package since it is only used there.
There was a problem hiding this comment.
It is here because it is used in two files: volumemount and persistentvolume
| @@ -0,0 +1,64 @@ | |||
| package persistentvolume | |||
There was a problem hiding this comment.
Can we move everything here into the volumemount package since this is the only place it is used?
There was a problem hiding this comment.
Added a few comments regarding annotations on PV and PVC. We should probably not set them since they are not documented anywhere, and is probably only used internally by the driver.
Also added some optional suggestions about moving code from internal package to the volumemount package, then as private props/functions
No description provided.