Skip to content

Commit 5e0d211

Browse files
xing-yangxing-yang
committed
Add topology annotation to pre-existing PVC if needed
1 parent a68cb53 commit 5e0d211

File tree

2 files changed

+177
-18
lines changed

2 files changed

+177
-18
lines changed

‎pkg/syncer/cnsoperator/controller/cnsregistervolume/cnsregistervolume_controller.go‎

Lines changed: 33 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import (
2222
"errors"
2323
"fmt"
2424
"reflect"
25+
"strings"
2526
"sync"
2627
"time"
2728

@@ -44,9 +45,8 @@ import (
4445
cnsoperatortypes "sigs.k8s.io/vsphere-csi-driver/v3/pkg/syncer/cnsoperator/types"
4546
"sigs.k8s.io/vsphere-csi-driver/v3/pkg/syncer/cnsoperator/util"
4647

47-
clientConfig "sigs.k8s.io/controller-runtime/pkg/client/config"
48-
4948
clientset "k8s.io/client-go/kubernetes"
49+
clientConfig "sigs.k8s.io/controller-runtime/pkg/client/config"
5050
apis "sigs.k8s.io/vsphere-csi-driver/v3/pkg/apis/cnsoperator"
5151
cnsregistervolumev1alpha1 "sigs.k8s.io/vsphere-csi-driver/v3/pkg/apis/cnsoperator/cnsregistervolume/v1alpha1"
5252
storagepolicyusagev1alpha2 "sigs.k8s.io/vsphere-csi-driver/v3/pkg/apis/cnsoperator/storagepolicy/v1alpha2"
@@ -563,7 +563,7 @@ func (r *ReconcileCnsRegisterVolume) Reconcile(ctx context.Context,
563563

564564
// Validate topology compatibility if PVC exists and can be reused
565565
if topologyMgr != nil {
566-
err = validatePVCTopologyCompatibility(ctx, pvc, volume.DatastoreUrl, topologyMgr, vc)
566+
err = validatePVCTopologyCompatibility(ctx, k8sclient, pvc, volume.DatastoreUrl, topologyMgr, vc, datastoreAccessibleTopology)
567567
if err != nil {
568568
msg := fmt.Sprintf("PVC topology validation failed: %v", err)
569569
log.Error(msg)
@@ -810,17 +810,43 @@ func (r *ReconcileCnsRegisterVolume) Reconcile(ctx context.Context,
810810

811811
// validatePVCTopologyCompatibility checks if the existing PVC's topology annotation is compatible
812812
// with the volume's actual placement zone.
813-
func validatePVCTopologyCompatibility(ctx context.Context, pvc *v1.PersistentVolumeClaim,
813+
func validatePVCTopologyCompatibility(ctx context.Context, k8sclient clientset.Interface, pvc *v1.PersistentVolumeClaim,
814814
volumeDatastoreURL string, topologyMgr commoncotypes.ControllerTopologyService,
815-
vc *cnsvsphere.VirtualCenter) error {
815+
vc *cnsvsphere.VirtualCenter, datastoreAccessibleTopology []map[string]string) error {
816816
log := logger.GetLogger(ctx)
817817

818818
// Check if PVC has topology annotation
819819
topologyAnnotation, exists := pvc.Annotations[common.AnnVolumeAccessibleTopology]
820820
if !exists || topologyAnnotation == "" {
821-
// No topology annotation on PVC, skip validation
822-
log.Debugf("PVC %s/%s has no topology annotation, skipping topology validation",
821+
// No topology annotation on PVC, add topology annotation and skip validation
822+
log.Debugf("PVC %s/%s has no topology annotation, adding topology annotation",
823823
pvc.Namespace, pvc.Name)
824+
var segmentsArray []string
825+
for _, topologyTerm := range datastoreAccessibleTopology {
826+
jsonSegment, err := json.Marshal(topologyTerm)
827+
if err != nil {
828+
return logger.LogNewErrorf(log,
829+
"failed to marshal topology segment: %+v to json. Error: %+v", topologyTerm, err)
830+
}
831+
segmentsArray = append(segmentsArray, string(jsonSegment))
832+
}
833+
topologyAnnotation = "[" + strings.Join(segmentsArray, ",") + "]"
834+
835+
if pvc.Annotations == nil {
836+
pvc.Annotations = make(map[string]string)
837+
}
838+
pvc.Annotations[common.AnnVolumeAccessibleTopology] = topologyAnnotation
839+
840+
// Update the PVC in Kubernetes to persist the topology annotation
841+
_, err := k8sclient.CoreV1().PersistentVolumeClaims(pvc.Namespace).Update(ctx, pvc, metav1.UpdateOptions{})
842+
if err != nil {
843+
return logger.LogNewErrorf(log, "failed to update PVC %s/%s with topology annotation: %+v",
844+
pvc.Namespace, pvc.Name, err)
845+
}
846+
847+
log.Infof("Successfully added topology annotation %s to PVC %s/%s",
848+
topologyAnnotation, pvc.Namespace, pvc.Name)
849+
// Return nil as we just added the topology annotation based on actual volume placement
824850
return nil
825851
}
826852

‎pkg/syncer/cnsoperator/controller/cnsregistervolume/cnsregistervolume_controller_test.go‎

Lines changed: 144 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ package cnsregistervolume
1818

1919
import (
2020
"context"
21+
"encoding/json"
2122
"fmt"
2223
"reflect"
2324
"testing"
@@ -791,30 +792,43 @@ var _ = Describe("checkExistingPVCDataSourceRef", func() {
791792

792793
var _ = Describe("validatePVCTopologyCompatibility", func() {
793794
var (
794-
ctx context.Context
795-
pvc *corev1.PersistentVolumeClaim
796-
volumeDatastoreURL string
797-
mockTopologyMgr *mockTopologyService
798-
mockVC *cnsvsphere.VirtualCenter
795+
ctx context.Context
796+
pvc *corev1.PersistentVolumeClaim
797+
volumeDatastoreURL string
798+
mockTopologyMgr *mockTopologyService
799+
mockVC *cnsvsphere.VirtualCenter
800+
datastoreAccessibleTopology []map[string]string
801+
mockK8sClient *k8sfake.Clientset
799802
)
800803

801804
BeforeEach(func() {
802805
ctx = context.Background()
803806
volumeDatastoreURL = "dummy-datastore-url"
804807
mockTopologyMgr = &mockTopologyService{}
805808
mockVC = &cnsvsphere.VirtualCenter{}
809+
datastoreAccessibleTopology = []map[string]string{
810+
{"topology.kubernetes.io/zone": "zone-1"},
811+
}
806812

807813
pvc = &corev1.PersistentVolumeClaim{
808814
ObjectMeta: metav1.ObjectMeta{
809815
Name: "test-pvc",
810816
Namespace: "test-namespace",
811817
},
812818
}
819+
820+
// Create a fake Kubernetes client
821+
mockK8sClient = k8sfake.NewSimpleClientset()
813822
})
814823

815824
Context("when PVC has no topology annotation", func() {
816825
It("should return nil without error", func() {
817-
err := validatePVCTopologyCompatibility(ctx, pvc, volumeDatastoreURL, mockTopologyMgr, mockVC)
826+
// Add the PVC to the fake client so it can be updated
827+
_, err := mockK8sClient.CoreV1().PersistentVolumeClaims(pvc.Namespace).Create(ctx, pvc, metav1.CreateOptions{})
828+
Expect(err).To(BeNil())
829+
830+
err = validatePVCTopologyCompatibility(ctx, mockK8sClient, pvc, volumeDatastoreURL, mockTopologyMgr, mockVC,
831+
datastoreAccessibleTopology)
818832
Expect(err).To(BeNil())
819833
})
820834
})
@@ -827,7 +841,12 @@ var _ = Describe("validatePVCTopologyCompatibility", func() {
827841
})
828842

829843
It("should return nil without error", func() {
830-
err := validatePVCTopologyCompatibility(ctx, pvc, volumeDatastoreURL, mockTopologyMgr, mockVC)
844+
// Add the PVC to the fake client so it can be updated
845+
_, err := mockK8sClient.CoreV1().PersistentVolumeClaims(pvc.Namespace).Create(ctx, pvc, metav1.CreateOptions{})
846+
Expect(err).To(BeNil())
847+
848+
err = validatePVCTopologyCompatibility(ctx, mockK8sClient, pvc, volumeDatastoreURL, mockTopologyMgr, mockVC,
849+
datastoreAccessibleTopology)
831850
Expect(err).To(BeNil())
832851
})
833852
})
@@ -840,7 +859,8 @@ var _ = Describe("validatePVCTopologyCompatibility", func() {
840859
})
841860

842861
It("should return error for invalid JSON", func() {
843-
err := validatePVCTopologyCompatibility(ctx, pvc, volumeDatastoreURL, mockTopologyMgr, mockVC)
862+
err := validatePVCTopologyCompatibility(ctx, mockK8sClient, pvc, volumeDatastoreURL, mockTopologyMgr, mockVC,
863+
datastoreAccessibleTopology)
844864
Expect(err).ToNot(BeNil())
845865
Expect(err.Error()).To(ContainSubstring("failed to parse topology annotation"))
846866
})
@@ -855,7 +875,8 @@ var _ = Describe("validatePVCTopologyCompatibility", func() {
855875
})
856876

857877
It("should return error from topology manager", func() {
858-
err := validatePVCTopologyCompatibility(ctx, pvc, volumeDatastoreURL, mockTopologyMgr, mockVC)
878+
err := validatePVCTopologyCompatibility(ctx, mockK8sClient, pvc, volumeDatastoreURL, mockTopologyMgr, mockVC,
879+
datastoreAccessibleTopology)
859880
Expect(err).ToNot(BeNil())
860881
Expect(err.Error()).To(ContainSubstring("failed to get topology for volume datastore"))
861882
})
@@ -872,7 +893,8 @@ var _ = Describe("validatePVCTopologyCompatibility", func() {
872893
})
873894

874895
It("should return nil without error", func() {
875-
err := validatePVCTopologyCompatibility(ctx, pvc, volumeDatastoreURL, mockTopologyMgr, mockVC)
896+
err := validatePVCTopologyCompatibility(ctx, mockK8sClient, pvc, volumeDatastoreURL, mockTopologyMgr, mockVC,
897+
datastoreAccessibleTopology)
876898
Expect(err).To(BeNil())
877899
})
878900
})
@@ -888,11 +910,122 @@ var _ = Describe("validatePVCTopologyCompatibility", func() {
888910
})
889911

890912
It("should return error for incompatible zones", func() {
891-
err := validatePVCTopologyCompatibility(ctx, pvc, volumeDatastoreURL, mockTopologyMgr, mockVC)
913+
err := validatePVCTopologyCompatibility(ctx, mockK8sClient, pvc, volumeDatastoreURL, mockTopologyMgr, mockVC,
914+
datastoreAccessibleTopology)
892915
Expect(err).ToNot(BeNil())
893916
Expect(err.Error()).To(ContainSubstring("is not compatible with volume placement"))
894917
})
895918
})
919+
920+
Context("when PVC exists without topology annotation and annotation needs to be added", func() {
921+
var originalPVC *corev1.PersistentVolumeClaim
922+
923+
BeforeEach(func() {
924+
// Create a PVC with some existing annotations but no topology annotation
925+
originalPVC = &corev1.PersistentVolumeClaim{
926+
ObjectMeta: metav1.ObjectMeta{
927+
Name: "existing-pvc",
928+
Namespace: "test-namespace",
929+
Annotations: map[string]string{
930+
"some.other/annotation": "existing-value",
931+
"another/annotation": "another-value",
932+
},
933+
},
934+
Spec: corev1.PersistentVolumeClaimSpec{
935+
AccessModes: []corev1.PersistentVolumeAccessMode{corev1.ReadWriteOnce},
936+
},
937+
}
938+
pvc = originalPVC
939+
})
940+
941+
It("should add topology annotation to existing PVC and return nil", func() {
942+
// Verify PVC initially has no topology annotation
943+
_, exists := pvc.Annotations["csi.vsphere.volume-accessible-topology"]
944+
Expect(exists).To(BeFalse())
945+
946+
// Add the PVC to the fake client so it can be updated
947+
_, err := mockK8sClient.CoreV1().PersistentVolumeClaims(pvc.Namespace).Create(ctx, pvc, metav1.CreateOptions{})
948+
Expect(err).To(BeNil())
949+
950+
// Call the function
951+
err = validatePVCTopologyCompatibility(ctx, mockK8sClient, pvc, volumeDatastoreURL, mockTopologyMgr, mockVC,
952+
datastoreAccessibleTopology)
953+
Expect(err).To(BeNil())
954+
955+
// Verify topology annotation was added
956+
topologyAnnotation, exists := pvc.Annotations["csi.vsphere.volume-accessible-topology"]
957+
Expect(exists).To(BeTrue())
958+
Expect(topologyAnnotation).ToNot(BeEmpty())
959+
960+
// Verify the annotation contains the expected topology data
961+
expectedAnnotation := `[{"topology.kubernetes.io/zone":"zone-1"}]`
962+
Expect(topologyAnnotation).To(Equal(expectedAnnotation))
963+
964+
// Verify existing annotations are preserved
965+
Expect(pvc.Annotations["some.other/annotation"]).To(Equal("existing-value"))
966+
Expect(pvc.Annotations["another/annotation"]).To(Equal("another-value"))
967+
})
968+
969+
It("should handle PVC with nil annotations map", func() {
970+
// Create PVC with nil annotations
971+
pvcWithNilAnnotations := &corev1.PersistentVolumeClaim{
972+
ObjectMeta: metav1.ObjectMeta{
973+
Name: "pvc-nil-annotations",
974+
Namespace: "test-namespace",
975+
Annotations: nil,
976+
},
977+
}
978+
979+
// Add the PVC to the fake client so it can be updated
980+
_, err := mockK8sClient.CoreV1().PersistentVolumeClaims(pvcWithNilAnnotations.Namespace).Create(ctx, pvcWithNilAnnotations, metav1.CreateOptions{})
981+
Expect(err).To(BeNil())
982+
983+
// Call the function
984+
err = validatePVCTopologyCompatibility(ctx, mockK8sClient, pvcWithNilAnnotations, volumeDatastoreURL, mockTopologyMgr,
985+
mockVC, datastoreAccessibleTopology)
986+
Expect(err).To(BeNil())
987+
988+
// Verify annotations map was created and topology annotation was added
989+
Expect(pvcWithNilAnnotations.Annotations).ToNot(BeNil())
990+
topologyAnnotation, exists := pvcWithNilAnnotations.Annotations["csi.vsphere.volume-accessible-topology"]
991+
Expect(exists).To(BeTrue())
992+
Expect(topologyAnnotation).To(Equal(`[{"topology.kubernetes.io/zone":"zone-1"}]`))
993+
})
994+
995+
It("should handle complex topology data with multiple zones", func() {
996+
// Use more complex topology data
997+
complexTopology := []map[string]string{
998+
{"topology.kubernetes.io/zone": "zone-a", "topology.kubernetes.io/region": "us-west"},
999+
{"topology.kubernetes.io/zone": "zone-b", "topology.kubernetes.io/region": "us-west"},
1000+
}
1001+
1002+
// Add the PVC to the fake client so it can be updated
1003+
_, err := mockK8sClient.CoreV1().PersistentVolumeClaims(pvc.Namespace).Create(ctx, pvc, metav1.CreateOptions{})
1004+
Expect(err).To(BeNil())
1005+
1006+
err = validatePVCTopologyCompatibility(ctx, mockK8sClient, pvc, volumeDatastoreURL, mockTopologyMgr, mockVC, complexTopology)
1007+
Expect(err).To(BeNil())
1008+
1009+
// Verify the complex topology was properly serialized
1010+
topologyAnnotation := pvc.Annotations["csi.vsphere.volume-accessible-topology"]
1011+
Expect(topologyAnnotation).ToNot(BeEmpty())
1012+
1013+
// Parse the annotation to verify it contains both topology segments
1014+
var parsedTopology []map[string]string
1015+
err = json.Unmarshal([]byte(topologyAnnotation), &parsedTopology)
1016+
Expect(err).To(BeNil())
1017+
Expect(len(parsedTopology)).To(Equal(2))
1018+
Expect(parsedTopology[0]).To(Equal(map[string]string{
1019+
"topology.kubernetes.io/zone": "zone-a",
1020+
"topology.kubernetes.io/region": "us-west",
1021+
}))
1022+
Expect(parsedTopology[1]).To(Equal(map[string]string{
1023+
"topology.kubernetes.io/zone": "zone-b",
1024+
"topology.kubernetes.io/region": "us-west",
1025+
}))
1026+
})
1027+
1028+
})
8961029
})
8971030

8981031
var _ = Describe("isTopologyCompatible", func() {

0 commit comments

Comments
 (0)