Skip to content

Commit 39816e4

Browse files
xing-yangxing-yang
committed
Add topology annotation to pre-existing PVC if needed
1 parent aabc278 commit 39816e4

File tree

2 files changed

+141
-15
lines changed

2 files changed

+141
-15
lines changed

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

Lines changed: 21 additions & 4 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

@@ -562,7 +563,7 @@ func (r *ReconcileCnsRegisterVolume) Reconcile(ctx context.Context,
562563

563564
// Validate topology compatibility if PVC exists and can be reused
564565
if topologyMgr != nil {
565-
err = validatePVCTopologyCompatibility(ctx, pvc, volume.DatastoreUrl, topologyMgr, vc)
566+
err = validatePVCTopologyCompatibility(ctx, pvc, volume.DatastoreUrl, topologyMgr, vc, datastoreAccessibleTopology)
566567
if err != nil {
567568
msg := fmt.Sprintf("PVC topology validation failed: %v", err)
568569
log.Error(msg)
@@ -811,15 +812,31 @@ func (r *ReconcileCnsRegisterVolume) Reconcile(ctx context.Context,
811812
// with the volume's actual placement zone.
812813
func validatePVCTopologyCompatibility(ctx context.Context, pvc *v1.PersistentVolumeClaim,
813814
volumeDatastoreURL string, topologyMgr commoncotypes.ControllerTopologyService,
814-
vc *cnsvsphere.VirtualCenter) error {
815+
vc *cnsvsphere.VirtualCenter, datastoreAccessibleTopology []map[string]string) error {
815816
log := logger.GetLogger(ctx)
816817

817818
// Check if PVC has topology annotation
818819
topologyAnnotation, exists := pvc.Annotations[common.AnnVolumeAccessibleTopology]
819820
if !exists || topologyAnnotation == "" {
820-
// No topology annotation on PVC, skip validation
821-
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",
822823
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+
// Return nil as we just added the topology annotation based on actual volume placement
823840
return nil
824841
}
825842

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

Lines changed: 120 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,18 +792,22 @@ 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
799801
)
800802

801803
BeforeEach(func() {
802804
ctx = context.Background()
803805
volumeDatastoreURL = "dummy-datastore-url"
804806
mockTopologyMgr = &mockTopologyService{}
805807
mockVC = &cnsvsphere.VirtualCenter{}
808+
datastoreAccessibleTopology = []map[string]string{
809+
{"topology.kubernetes.io/zone": "zone-1"},
810+
}
806811

807812
pvc = &corev1.PersistentVolumeClaim{
808813
ObjectMeta: metav1.ObjectMeta{
@@ -814,7 +819,8 @@ var _ = Describe("validatePVCTopologyCompatibility", func() {
814819

815820
Context("when PVC has no topology annotation", func() {
816821
It("should return nil without error", func() {
817-
err := validatePVCTopologyCompatibility(ctx, pvc, volumeDatastoreURL, mockTopologyMgr, mockVC)
822+
err := validatePVCTopologyCompatibility(ctx, pvc, volumeDatastoreURL, mockTopologyMgr, mockVC,
823+
datastoreAccessibleTopology)
818824
Expect(err).To(BeNil())
819825
})
820826
})
@@ -827,7 +833,8 @@ var _ = Describe("validatePVCTopologyCompatibility", func() {
827833
})
828834

829835
It("should return nil without error", func() {
830-
err := validatePVCTopologyCompatibility(ctx, pvc, volumeDatastoreURL, mockTopologyMgr, mockVC)
836+
err := validatePVCTopologyCompatibility(ctx, pvc, volumeDatastoreURL, mockTopologyMgr, mockVC,
837+
datastoreAccessibleTopology)
831838
Expect(err).To(BeNil())
832839
})
833840
})
@@ -840,7 +847,8 @@ var _ = Describe("validatePVCTopologyCompatibility", func() {
840847
})
841848

842849
It("should return error for invalid JSON", func() {
843-
err := validatePVCTopologyCompatibility(ctx, pvc, volumeDatastoreURL, mockTopologyMgr, mockVC)
850+
err := validatePVCTopologyCompatibility(ctx, pvc, volumeDatastoreURL, mockTopologyMgr, mockVC,
851+
datastoreAccessibleTopology)
844852
Expect(err).ToNot(BeNil())
845853
Expect(err.Error()).To(ContainSubstring("failed to parse topology annotation"))
846854
})
@@ -855,7 +863,8 @@ var _ = Describe("validatePVCTopologyCompatibility", func() {
855863
})
856864

857865
It("should return error from topology manager", func() {
858-
err := validatePVCTopologyCompatibility(ctx, pvc, volumeDatastoreURL, mockTopologyMgr, mockVC)
866+
err := validatePVCTopologyCompatibility(ctx, pvc, volumeDatastoreURL, mockTopologyMgr, mockVC,
867+
datastoreAccessibleTopology)
859868
Expect(err).ToNot(BeNil())
860869
Expect(err.Error()).To(ContainSubstring("failed to get topology for volume datastore"))
861870
})
@@ -872,7 +881,8 @@ var _ = Describe("validatePVCTopologyCompatibility", func() {
872881
})
873882

874883
It("should return nil without error", func() {
875-
err := validatePVCTopologyCompatibility(ctx, pvc, volumeDatastoreURL, mockTopologyMgr, mockVC)
884+
err := validatePVCTopologyCompatibility(ctx, pvc, volumeDatastoreURL, mockTopologyMgr, mockVC,
885+
datastoreAccessibleTopology)
876886
Expect(err).To(BeNil())
877887
})
878888
})
@@ -888,11 +898,110 @@ var _ = Describe("validatePVCTopologyCompatibility", func() {
888898
})
889899

890900
It("should return error for incompatible zones", func() {
891-
err := validatePVCTopologyCompatibility(ctx, pvc, volumeDatastoreURL, mockTopologyMgr, mockVC)
901+
err := validatePVCTopologyCompatibility(ctx, pvc, volumeDatastoreURL, mockTopologyMgr, mockVC,
902+
datastoreAccessibleTopology)
892903
Expect(err).ToNot(BeNil())
893904
Expect(err.Error()).To(ContainSubstring("is not compatible with volume placement"))
894905
})
895906
})
907+
908+
Context("when PVC exists without topology annotation and annotation needs to be added", func() {
909+
var originalPVC *corev1.PersistentVolumeClaim
910+
911+
BeforeEach(func() {
912+
// Create a PVC with some existing annotations but no topology annotation
913+
originalPVC = &corev1.PersistentVolumeClaim{
914+
ObjectMeta: metav1.ObjectMeta{
915+
Name: "existing-pvc",
916+
Namespace: "test-namespace",
917+
Annotations: map[string]string{
918+
"some.other/annotation": "existing-value",
919+
"another/annotation": "another-value",
920+
},
921+
},
922+
Spec: corev1.PersistentVolumeClaimSpec{
923+
AccessModes: []corev1.PersistentVolumeAccessMode{corev1.ReadWriteOnce},
924+
},
925+
}
926+
pvc = originalPVC
927+
})
928+
929+
It("should add topology annotation to existing PVC and return nil", func() {
930+
// Verify PVC initially has no topology annotation
931+
_, exists := pvc.Annotations["csi.vsphere.volume-accessible-topology"]
932+
Expect(exists).To(BeFalse())
933+
934+
// Call the function
935+
err := validatePVCTopologyCompatibility(ctx, pvc, volumeDatastoreURL, mockTopologyMgr, mockVC,
936+
datastoreAccessibleTopology)
937+
Expect(err).To(BeNil())
938+
939+
// Verify topology annotation was added
940+
topologyAnnotation, exists := pvc.Annotations["csi.vsphere.volume-accessible-topology"]
941+
Expect(exists).To(BeTrue())
942+
Expect(topologyAnnotation).ToNot(BeEmpty())
943+
944+
// Verify the annotation contains the expected topology data
945+
expectedAnnotation := `[{"topology.kubernetes.io/zone":"zone-1"}]`
946+
Expect(topologyAnnotation).To(Equal(expectedAnnotation))
947+
948+
// Verify existing annotations are preserved
949+
Expect(pvc.Annotations["some.other/annotation"]).To(Equal("existing-value"))
950+
Expect(pvc.Annotations["another/annotation"]).To(Equal("another-value"))
951+
})
952+
953+
It("should handle PVC with nil annotations map", func() {
954+
// Create PVC with nil annotations
955+
pvcWithNilAnnotations := &corev1.PersistentVolumeClaim{
956+
ObjectMeta: metav1.ObjectMeta{
957+
Name: "pvc-nil-annotations",
958+
Namespace: "test-namespace",
959+
Annotations: nil,
960+
},
961+
}
962+
963+
// Call the function
964+
err := validatePVCTopologyCompatibility(ctx, pvcWithNilAnnotations, volumeDatastoreURL, mockTopologyMgr,
965+
mockVC, datastoreAccessibleTopology)
966+
Expect(err).To(BeNil())
967+
968+
// Verify annotations map was created and topology annotation was added
969+
Expect(pvcWithNilAnnotations.Annotations).ToNot(BeNil())
970+
topologyAnnotation, exists := pvcWithNilAnnotations.Annotations["csi.vsphere.volume-accessible-topology"]
971+
Expect(exists).To(BeTrue())
972+
Expect(topologyAnnotation).To(Equal(`[{"topology.kubernetes.io/zone":"zone-1"}]`))
973+
})
974+
975+
It("should handle complex topology data with multiple zones", func() {
976+
// Use more complex topology data
977+
complexTopology := []map[string]string{
978+
{"topology.kubernetes.io/zone": "zone-a", "topology.kubernetes.io/region": "us-west"},
979+
{"topology.kubernetes.io/zone": "zone-b", "topology.kubernetes.io/region": "us-west"},
980+
}
981+
982+
err := validatePVCTopologyCompatibility(ctx, pvc, volumeDatastoreURL, mockTopologyMgr, mockVC, complexTopology)
983+
Expect(err).To(BeNil())
984+
985+
// Verify the complex topology was properly serialized
986+
topologyAnnotation := pvc.Annotations["csi.vsphere.volume-accessible-topology"]
987+
Expect(topologyAnnotation).ToNot(BeEmpty())
988+
989+
// Parse the annotation to verify it contains both topology segments
990+
var parsedTopology []map[string]string
991+
err = json.Unmarshal([]byte(topologyAnnotation), &parsedTopology)
992+
Expect(err).To(BeNil())
993+
Expect(len(parsedTopology)).To(Equal(2))
994+
Expect(parsedTopology[0]).To(Equal(map[string]string{
995+
"topology.kubernetes.io/zone": "zone-a",
996+
"topology.kubernetes.io/region": "us-west",
997+
}))
998+
Expect(parsedTopology[1]).To(Equal(map[string]string{
999+
"topology.kubernetes.io/zone": "zone-b",
1000+
"topology.kubernetes.io/region": "us-west",
1001+
}))
1002+
})
1003+
1004+
})
8961005
})
8971006

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

0 commit comments

Comments
 (0)