Skip to content

Commit a13ecd0

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

File tree

2 files changed

+180
-18
lines changed

2 files changed

+180
-18
lines changed

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

Lines changed: 34 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,8 @@ 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,
567+
datastoreAccessibleTopology)
567568
if err != nil {
568569
msg := fmt.Sprintf("PVC topology validation failed: %v", err)
569570
log.Error(msg)
@@ -810,17 +811,43 @@ func (r *ReconcileCnsRegisterVolume) Reconcile(ctx context.Context,
810811

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

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

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

Lines changed: 146 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,124 @@ 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,
981+
pvcWithNilAnnotations, metav1.CreateOptions{})
982+
Expect(err).To(BeNil())
983+
984+
// Call the function
985+
err = validatePVCTopologyCompatibility(ctx, mockK8sClient, pvcWithNilAnnotations, volumeDatastoreURL,
986+
mockTopologyMgr, mockVC, datastoreAccessibleTopology)
987+
Expect(err).To(BeNil())
988+
989+
// Verify annotations map was created and topology annotation was added
990+
Expect(pvcWithNilAnnotations.Annotations).ToNot(BeNil())
991+
topologyAnnotation, exists := pvcWithNilAnnotations.Annotations["csi.vsphere.volume-accessible-topology"]
992+
Expect(exists).To(BeTrue())
993+
Expect(topologyAnnotation).To(Equal(`[{"topology.kubernetes.io/zone":"zone-1"}]`))
994+
})
995+
996+
It("should handle complex topology data with multiple zones", func() {
997+
// Use more complex topology data
998+
complexTopology := []map[string]string{
999+
{"topology.kubernetes.io/zone": "zone-a", "topology.kubernetes.io/region": "us-west"},
1000+
{"topology.kubernetes.io/zone": "zone-b", "topology.kubernetes.io/region": "us-west"},
1001+
}
1002+
1003+
// Add the PVC to the fake client so it can be updated
1004+
_, err := mockK8sClient.CoreV1().PersistentVolumeClaims(pvc.Namespace).Create(ctx, pvc, metav1.CreateOptions{})
1005+
Expect(err).To(BeNil())
1006+
1007+
err = validatePVCTopologyCompatibility(ctx, mockK8sClient, pvc, volumeDatastoreURL, mockTopologyMgr, mockVC,
1008+
complexTopology)
1009+
Expect(err).To(BeNil())
1010+
1011+
// Verify the complex topology was properly serialized
1012+
topologyAnnotation := pvc.Annotations["csi.vsphere.volume-accessible-topology"]
1013+
Expect(topologyAnnotation).ToNot(BeEmpty())
1014+
1015+
// Parse the annotation to verify it contains both topology segments
1016+
var parsedTopology []map[string]string
1017+
err = json.Unmarshal([]byte(topologyAnnotation), &parsedTopology)
1018+
Expect(err).To(BeNil())
1019+
Expect(len(parsedTopology)).To(Equal(2))
1020+
Expect(parsedTopology[0]).To(Equal(map[string]string{
1021+
"topology.kubernetes.io/zone": "zone-a",
1022+
"topology.kubernetes.io/region": "us-west",
1023+
}))
1024+
Expect(parsedTopology[1]).To(Equal(map[string]string{
1025+
"topology.kubernetes.io/zone": "zone-b",
1026+
"topology.kubernetes.io/region": "us-west",
1027+
}))
1028+
})
1029+
1030+
})
8961031
})
8971032

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

0 commit comments

Comments
 (0)