From e98d820d5421b028015c341e3fa2db2e4016dbb0 Mon Sep 17 00:00:00 2001 From: Ti Chi Robot Date: Fri, 23 Jun 2023 08:49:03 +0800 Subject: [PATCH] fix(volume): fix panic bug when enable ModifyVolume feature (#5058) (#5082) Signed-off-by: ti-chi-bot Co-authored-by: Bo Liu Co-authored-by: csuzhangxc --- pkg/manager/volumes/phase.go | 61 +++--- pkg/manager/volumes/phase_test.go | 28 ++- pkg/manager/volumes/pod_vol_modifier.go | 194 +++++++++--------- pkg/manager/volumes/pod_vol_modifier_test.go | 90 +++++--- pkg/manager/volumes/pvc_modifier.go | 3 +- pkg/manager/volumes/sync_volume_status.go | 19 +- .../volumes/sync_volume_status_test.go | 21 +- 7 files changed, 247 insertions(+), 169 deletions(-) diff --git a/pkg/manager/volumes/phase.go b/pkg/manager/volumes/phase.go index d03e6af824..5f5e692f07 100644 --- a/pkg/manager/volumes/phase.go +++ b/pkg/manager/volumes/phase.go @@ -77,44 +77,55 @@ func (p *podVolModifier) getVolumePhase(vol *ActualVolume) VolumePhase { return VolumePhaseModified } - if p.waitForNextTime(vol.PVC, vol.Desired.StorageClass) { + if p.waitForNextTime(vol.PVC, vol.StorageClass, vol.Desired.StorageClass) { return VolumePhasePending } return VolumePhasePreparing } -func isVolumeExpansionSupported(sc *storagev1.StorageClass) bool { +func isVolumeExpansionSupported(sc *storagev1.StorageClass) (bool, error) { + if sc == nil { + // always assume expansion is supported + return true, fmt.Errorf("expansion cap of volume is unknown") + } if sc.AllowVolumeExpansion == nil { - return false + return false, nil } - return *sc.AllowVolumeExpansion + return *sc.AllowVolumeExpansion, nil } func (p *podVolModifier) validate(vol *ActualVolume) error { if vol.Desired == nil { return fmt.Errorf("can't match desired volume") } - if vol.Desired.StorageClass == nil { - // TODO: support default storage class - return fmt.Errorf("can't change storage class to the default one") - } - desired := vol.Desired.Size - actual := getStorageSize(vol.PVC.Spec.Resources.Requests) + desired := vol.Desired.GetStorageSize() + actual := vol.GetStorageSize() result := desired.Cmp(actual) switch { case result == 0: case result < 0: return fmt.Errorf("can't shrunk size from %s to %s", &actual, &desired) case result > 0: - if !isVolumeExpansionSupported(vol.StorageClass) { + supported, err := isVolumeExpansionSupported(vol.StorageClass) + if err != nil { + klog.Warningf("volume expansion of storage class %s may be not supported, but it will be tried", vol.GetStorageClassName()) + } + if !supported { return fmt.Errorf("volume expansion is not supported by storageclass %s", vol.StorageClass.Name) } } - m := p.getVolumeModifier(vol.Desired.StorageClass) + + m := p.getVolumeModifier(vol.StorageClass, vol.Desired.StorageClass) if m == nil { return nil } + + // if no pv permission but have sc permission: cannot change sc + if isStorageClassChanged(vol.GetStorageClassName(), vol.Desired.GetStorageClassName()) && vol.PV == nil { + return fmt.Errorf("cannot change storage class (%s to %s), because there is no permission to get persistent volume", vol.GetStorageClassName(), vol.Desired.GetStorageClassName()) + } + desiredPVC := vol.PVC.DeepCopy() desiredPVC.Spec.Resources.Requests[corev1.ResourceStorage] = desired @@ -128,7 +139,7 @@ func isPVCRevisionChanged(pvc *corev1.PersistentVolumeClaim) bool { return specRevision != statusRevision } -func (p *podVolModifier) waitForNextTime(pvc *corev1.PersistentVolumeClaim, sc *storagev1.StorageClass) bool { +func (p *podVolModifier) waitForNextTime(pvc *corev1.PersistentVolumeClaim, actualSc, desciredSc *storagev1.StorageClass) bool { str, ok := pvc.Annotations[annoKeyPVCLastTransitionTimestamp] if !ok { return false @@ -139,7 +150,7 @@ func (p *podVolModifier) waitForNextTime(pvc *corev1.PersistentVolumeClaim, sc * } d := time.Since(timestamp) - m := p.getVolumeModifier(sc) + m := p.getVolumeModifier(actualSc, desciredSc) waitDur := defaultModifyWaitingDuration if m != nil { @@ -156,23 +167,14 @@ func (p *podVolModifier) waitForNextTime(pvc *corev1.PersistentVolumeClaim, sc * func needModify(pvc *corev1.PersistentVolumeClaim, desired *DesiredVolume) bool { size := desired.Size - scName := "" - if desired.StorageClass != nil { - scName = desired.StorageClass.Name - } + scName := desired.GetStorageClassName() return isPVCStatusMatched(pvc, scName, size) } func isPVCStatusMatched(pvc *corev1.PersistentVolumeClaim, scName string, size resource.Quantity) bool { - isChanged := false - oldSc, ok := pvc.Annotations[annoKeyPVCStatusStorageClass] - if !ok { - oldSc = ignoreNil(pvc.Spec.StorageClassName) - } - if oldSc != scName { - isChanged = true - } + oldSc := getStorageClassNameFromPVC(pvc) + isChanged := isStorageClassChanged(oldSc, scName) oldSize, ok := pvc.Annotations[annoKeyPVCStatusStorageSize] if !ok { @@ -188,3 +190,10 @@ func isPVCStatusMatched(pvc *corev1.PersistentVolumeClaim, scName string, size r return isChanged } + +func isStorageClassChanged(pre, cur string) bool { + if cur != "" && pre != cur { + return true + } + return false +} diff --git a/pkg/manager/volumes/phase_test.go b/pkg/manager/volumes/phase_test.go index 75bc5e1aa2..5ae9843638 100644 --- a/pkg/manager/volumes/phase_test.go +++ b/pkg/manager/volumes/phase_test.go @@ -43,6 +43,11 @@ func newTestPVCForGetVolumePhase(size string, sc *string, annotations map[string }, StorageClassName: sc, }, + Status: corev1.PersistentVolumeClaimStatus{ + Capacity: corev1.ResourceList{ + corev1.ResourceStorage: q, + }, + }, } } @@ -169,13 +174,22 @@ func TestGetVolumePhase(t *testing.T) { expected: VolumePhasePreparing, }, { - desc: "invalid sc", + desc: "sc is not set", pvc: newTestPVCForGetVolumePhase(oldSize, &oldScName, nil), oldSc: newStorageClassForGetVolumePhase(oldScName, "ebs.csi.aws.com", true), sc: nil, size: oldSize, - expected: VolumePhaseCannotModify, + expected: VolumePhaseModified, + }, + { + desc: "sc is not set, but size is changed", + pvc: newTestPVCForGetVolumePhase(oldSize, &oldScName, nil), + oldSc: newStorageClassForGetVolumePhase(oldScName, "ebs.csi.aws.com", true), + sc: nil, + size: newSize, + + expected: VolumePhasePreparing, }, { desc: "invalid size", @@ -217,14 +231,22 @@ func TestGetVolumePhase(t *testing.T) { actual := ActualVolume{ PVC: c.pvc, StorageClass: c.oldSc, + PV: &corev1.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + }, + }, } if !c.noDesired { actual.Desired = &DesiredVolume{ StorageClass: c.sc, Size: resource.MustParse(c.size), } + if c.sc != nil { + actual.Desired.StorageClassName = &c.sc.Name + } } phase := pvm.getVolumePhase(&actual) - g.Expect(phase).Should(Equal(c.expected), c.desc) + g.Expect(phase.String()).Should(Equal(c.expected.String()), c.desc) } } diff --git a/pkg/manager/volumes/pod_vol_modifier.go b/pkg/manager/volumes/pod_vol_modifier.go index 7348bcea17..b7aa4bf67d 100644 --- a/pkg/manager/volumes/pod_vol_modifier.go +++ b/pkg/manager/volumes/pod_vol_modifier.go @@ -42,16 +42,22 @@ type PodVolumeModifier interface { } type DesiredVolume struct { - Name v1alpha1.StorageVolumeName - Size resource.Quantity + Name v1alpha1.StorageVolumeName + Size resource.Quantity + // it may be nil if there is no permission to get storage class StorageClass *storagev1.StorageClass + // it is sc name specified by user + // the sc may not exist + StorageClassName *string } +// get storage class name from tc +// it may return empty because sc is unset or no permission to verify the existence of sc func (v *DesiredVolume) GetStorageClassName() string { - if v.StorageClass == nil { + if v.StorageClassName == nil { return "" } - return v.StorageClass.Name + return *v.StorageClassName } func (v *DesiredVolume) GetStorageSize() resource.Quantity { @@ -59,22 +65,18 @@ func (v *DesiredVolume) GetStorageSize() resource.Quantity { } type ActualVolume struct { - Desired *DesiredVolume - PVC *corev1.PersistentVolumeClaim - PV *corev1.PersistentVolume + Desired *DesiredVolume + PVC *corev1.PersistentVolumeClaim + Phase VolumePhase + // it may be nil if there is no permission to get pvc + PV *corev1.PersistentVolume + // it may be nil if there is no permission to get storage class StorageClass *storagev1.StorageClass - Phase VolumePhase } +// get storage class name from current pvc func (v *ActualVolume) GetStorageClassName() string { - sc := ignoreNil(v.PVC.Spec.StorageClassName) - - scAnno, ok := v.PVC.Annotations[annoKeyPVCStatusStorageClass] - if ok { - sc = scAnno - } - - return sc + return getStorageClassNameFromPVC(v.PVC) } func (v *ActualVolume) GetStorageSize() resource.Quantity { @@ -162,55 +164,39 @@ func (p *podVolModifier) GetDesiredVolumes(tc *v1alpha1.TidbCluster, mt v1alpha1 scLister := p.deps.StorageClassLister storageVolumes := []v1alpha1.StorageVolume{} - var defaultSc *storagev1.StorageClass + var defaultScName *string switch mt { case v1alpha1.TiProxyMemberType: - sc, err := getStorageClass(tc.Spec.TiProxy.StorageClassName, scLister) - if err != nil { - return nil, err - } - defaultSc = sc + defaultScName = tc.Spec.TiProxy.StorageClassName d := DesiredVolume{ - Name: v1alpha1.GetStorageVolumeName("", mt), - Size: getStorageSize(tc.Spec.TiProxy.Requests), - StorageClass: sc, + Name: v1alpha1.GetStorageVolumeName("", mt), + Size: getStorageSize(tc.Spec.TiProxy.Requests), + StorageClassName: defaultScName, } desiredVolumes = append(desiredVolumes, d) storageVolumes = tc.Spec.TiProxy.StorageVolumes case v1alpha1.PDMemberType: - sc, err := getStorageClass(tc.Spec.PD.StorageClassName, scLister) - if err != nil { - return nil, err - } - defaultSc = sc + defaultScName = tc.Spec.PD.StorageClassName d := DesiredVolume{ - Name: v1alpha1.GetStorageVolumeName("", mt), - Size: getStorageSize(tc.Spec.PD.Requests), - StorageClass: sc, + Name: v1alpha1.GetStorageVolumeName("", mt), + Size: getStorageSize(tc.Spec.PD.Requests), + StorageClassName: defaultScName, } desiredVolumes = append(desiredVolumes, d) storageVolumes = tc.Spec.PD.StorageVolumes case v1alpha1.TiDBMemberType: - sc, err := getStorageClass(tc.Spec.TiDB.StorageClassName, scLister) - if err != nil { - return nil, err - } - defaultSc = sc + defaultScName = tc.Spec.TiDB.StorageClassName storageVolumes = tc.Spec.TiDB.StorageVolumes case v1alpha1.TiKVMemberType: - sc, err := getStorageClass(tc.Spec.TiKV.StorageClassName, scLister) - if err != nil { - return nil, err - } - defaultSc = sc + defaultScName = tc.Spec.TiKV.StorageClassName d := DesiredVolume{ - Name: v1alpha1.GetStorageVolumeName("", mt), - Size: getStorageSize(tc.Spec.TiKV.Requests), - StorageClass: sc, + Name: v1alpha1.GetStorageVolumeName("", mt), + Size: getStorageSize(tc.Spec.TiKV.Requests), + StorageClassName: defaultScName, } desiredVolumes = append(desiredVolumes, d) @@ -218,36 +204,24 @@ func (p *podVolModifier) GetDesiredVolumes(tc *v1alpha1.TidbCluster, mt v1alpha1 case v1alpha1.TiFlashMemberType: for i, claim := range tc.Spec.TiFlash.StorageClaims { - sc, err := getStorageClass(claim.StorageClassName, scLister) - if err != nil { - return nil, err - } d := DesiredVolume{ - Name: v1alpha1.GetStorageVolumeNameForTiFlash(i), - Size: getStorageSize(claim.Resources.Requests), - StorageClass: sc, + Name: v1alpha1.GetStorageVolumeNameForTiFlash(i), + Size: getStorageSize(claim.Resources.Requests), + StorageClassName: claim.StorageClassName, } desiredVolumes = append(desiredVolumes, d) } case v1alpha1.TiCDCMemberType: - sc, err := getStorageClass(tc.Spec.TiCDC.StorageClassName, scLister) - if err != nil { - return nil, err - } - defaultSc = sc + defaultScName = tc.Spec.TiCDC.StorageClassName storageVolumes = tc.Spec.TiCDC.StorageVolumes case v1alpha1.PumpMemberType: - sc, err := getStorageClass(tc.Spec.Pump.StorageClassName, scLister) - if err != nil { - return nil, err - } - defaultSc = sc + defaultScName = tc.Spec.Pump.StorageClassName d := DesiredVolume{ - Name: v1alpha1.GetStorageVolumeName("", mt), - Size: getStorageSize(tc.Spec.Pump.Requests), - StorageClass: sc, + Name: v1alpha1.GetStorageVolumeName("", mt), + Size: getStorageSize(tc.Spec.Pump.Requests), + StorageClassName: defaultScName, } desiredVolumes = append(desiredVolumes, d) default: @@ -256,17 +230,13 @@ func (p *podVolModifier) GetDesiredVolumes(tc *v1alpha1.TidbCluster, mt v1alpha1 for _, sv := range storageVolumes { if quantity, err := resource.ParseQuantity(sv.StorageSize); err == nil { - sc, err := getStorageClass(sv.StorageClassName, scLister) - if err != nil { - return nil, err - } - if sc == nil { - sc = defaultSc - } d := DesiredVolume{ - Name: v1alpha1.GetStorageVolumeName(sv.Name, mt), - Size: quantity, - StorageClass: sc, + Name: v1alpha1.GetStorageVolumeName(sv.Name, mt), + Size: quantity, + StorageClassName: sv.StorageClassName, + } + if d.StorageClassName == nil { + d.StorageClassName = defaultScName } desiredVolumes = append(desiredVolumes, d) @@ -276,6 +246,18 @@ func (p *podVolModifier) GetDesiredVolumes(tc *v1alpha1.TidbCluster, mt v1alpha1 } } + if scLister != nil { + for i := range desiredVolumes { + if desiredVolumes[i].StorageClassName != nil { + sc, err := getStorageClass(desiredVolumes[i].StorageClassName, scLister) + if err != nil { + return nil, fmt.Errorf("cannot get sc %s", *desiredVolumes[i].StorageClassName) + } + desiredVolumes[i].StorageClass = sc + } + } + } + return desiredVolumes, nil } @@ -309,18 +291,16 @@ func (p *podVolModifier) getBoundPVFromPVC(pvc *corev1.PersistentVolumeClaim) (* } func (p *podVolModifier) getStorageClassFromPVC(pvc *corev1.PersistentVolumeClaim) (*storagev1.StorageClass, error) { - sc := ignoreNil(pvc.Spec.StorageClassName) - - scAnno, ok := pvc.Annotations[annoKeyPVCStatusStorageClass] - if ok { - sc = scAnno - } - - if sc == "" { + scName := getStorageClassNameFromPVC(pvc) + if p.deps.StorageClassLister == nil { + klog.V(4).Infof("StorageClass is unavailable, skip getting StorageClass for %s. This may be caused by no relevant permissions", scName) return nil, nil } + if scName == "" { + return nil, fmt.Errorf("StorageClass of pvc %s is not set", pvc.Name) + } - return p.deps.StorageClassLister.Get(sc) + return p.deps.StorageClassLister.Get(scName) } func (p *podVolModifier) getPVC(ns string, vol *corev1.Volume) (*corev1.PersistentVolumeClaim, error) { @@ -375,7 +355,11 @@ func (p *podVolModifier) NewActualVolumeOfPod(vs []DesiredVolume, ns string, vol return nil, err } + // no desired volume, it may be a volume which is unmanaged by operator desired := getDesiredVolumeByName(vs, v1alpha1.StorageVolumeName(vol.Name)) + if desired == nil { + return nil, nil + } actual := ActualVolume{ Desired: desired, @@ -412,7 +396,7 @@ func upgradeRevision(pvc *corev1.PersistentVolumeClaim) { func isPVCSpecMatched(pvc *corev1.PersistentVolumeClaim, scName string, size resource.Quantity) bool { isChanged := false oldSc := pvc.Annotations[annoKeyPVCSpecStorageClass] - if oldSc != scName { + if scName != "" && oldSc != scName { isChanged = true } @@ -435,7 +419,9 @@ func snapshotStorageClassAndSize(pvc *corev1.PersistentVolumeClaim, scName strin pvc.Annotations = map[string]string{} } - pvc.Annotations[annoKeyPVCSpecStorageClass] = scName + if scName != "" { + pvc.Annotations[annoKeyPVCSpecStorageClass] = scName + } pvc.Annotations[annoKeyPVCSpecStorageSize] = size.String() return isChanged @@ -454,10 +440,7 @@ func (p *podVolModifier) modifyPVCAnnoSpec(ctx context.Context, vol *ActualVolum pvc := vol.PVC.DeepCopy() size := vol.Desired.Size - scName := "" - if vol.Desired.StorageClass != nil { - scName = vol.Desired.StorageClass.Name - } + scName := vol.Desired.GetStorageClassName() isChanged := snapshotStorageClassAndSize(pvc, scName, size) if isChanged { @@ -510,7 +493,9 @@ func (p *podVolModifier) modifyPVCAnnoStatus(ctx context.Context, vol *ActualVol } pvc.Annotations[annoKeyPVCStatusRevision] = pvc.Annotations[annoKeyPVCSpecRevision] - pvc.Annotations[annoKeyPVCStatusStorageClass] = pvc.Annotations[annoKeyPVCSpecStorageClass] + if scName := pvc.Annotations[annoKeyPVCSpecStorageClass]; scName != "" { + pvc.Annotations[annoKeyPVCStatusStorageClass] = scName + } pvc.Annotations[annoKeyPVCStatusStorageSize] = pvc.Annotations[annoKeyPVCSpecStorageSize] updated, err := p.deps.KubeClientset.CoreV1().PersistentVolumeClaims(pvc.Namespace).Update(ctx, pvc, metav1.UpdateOptions{}) @@ -524,7 +509,7 @@ func (p *podVolModifier) modifyPVCAnnoStatus(ctx context.Context, vol *ActualVol } func (p *podVolModifier) modifyVolume(ctx context.Context, vol *ActualVolume) (bool, error) { - m := p.getVolumeModifier(vol.Desired.StorageClass) + m := p.getVolumeModifier(vol.StorageClass, vol.Desired.StorageClass) if m == nil { // skip modifying volume by delegation.VolumeModifier return false, nil @@ -536,8 +521,16 @@ func (p *podVolModifier) modifyVolume(ctx context.Context, vol *ActualVolume) (b return m.ModifyVolume(ctx, pvc, vol.PV, vol.Desired.StorageClass) } -func (p *podVolModifier) getVolumeModifier(sc *storagev1.StorageClass) delegation.VolumeModifier { - return p.modifiers[sc.Provisioner] +func (p *podVolModifier) getVolumeModifier(actualSc, desiredSc *storagev1.StorageClass) delegation.VolumeModifier { + if actualSc == nil || desiredSc == nil { + return nil + } + // sc is not changed + if actualSc.Name == desiredSc.Name { + return nil + } + + return p.modifiers[desiredSc.Provisioner] } func isLeaderEvictedOrTimeout(tc *v1alpha1.TidbCluster, pod *corev1.Pod) bool { @@ -565,3 +558,14 @@ func isLeaderEvictedOrTimeout(tc *v1alpha1.TidbCluster, pod *corev1.Pod) bool { return false } + +func getStorageClassNameFromPVC(pvc *corev1.PersistentVolumeClaim) string { + sc := ignoreNil(pvc.Spec.StorageClassName) + + scAnno, ok := pvc.Annotations[annoKeyPVCStatusStorageClass] + if ok && scAnno != "" { + sc = scAnno + } + + return sc +} diff --git a/pkg/manager/volumes/pod_vol_modifier_test.go b/pkg/manager/volumes/pod_vol_modifier_test.go index a1b0e6335b..62b4c8abf4 100644 --- a/pkg/manager/volumes/pod_vol_modifier_test.go +++ b/pkg/manager/volumes/pod_vol_modifier_test.go @@ -15,6 +15,7 @@ package volumes import ( "context" + "fmt" "testing" "time" @@ -80,17 +81,18 @@ func TestModify(t *testing.T) { oldSize := "10Gi" newSize := "20Gi" oldSc := "old" - // newSc := "new" + newSc := "new" provisioner := "test" cases := []struct { desc string - pvc *corev1.PersistentVolumeClaim - pv *corev1.PersistentVolume - sc *storagev1.StorageClass - size string + pvc *corev1.PersistentVolumeClaim + pv *corev1.PersistentVolume + oldSc *storagev1.StorageClass + sc *storagev1.StorageClass + size string isModifyVolumeFinished bool @@ -98,72 +100,92 @@ func TestModify(t *testing.T) { expectedHasErr bool }{ { - desc: "volume is not changed", - pvc: newTestPVCForModify(&oldSc, oldSize, oldSize, nil), - pv: newTestPVForModify(), - sc: newTestSCForModify(oldSc, provisioner), - size: oldSize, + desc: "volume is not changed", + pvc: newTestPVCForModify(&oldSc, oldSize, oldSize, nil), + pv: newTestPVForModify(), + oldSc: newTestSCForModify(oldSc, provisioner), + sc: newTestSCForModify(oldSc, provisioner), + size: oldSize, expectedPVC: newTestPVCForModify(&oldSc, oldSize, oldSize, nil), }, { - desc: "volume size is changed, and revision has not been upgraded", + desc: "only volume size is changed", - pvc: newTestPVCForModify(&oldSc, oldSize, oldSize, nil), - pv: newTestPVForModify(), - sc: newTestSCForModify(oldSc, provisioner), - size: newSize, + pvc: newTestPVCForModify(&oldSc, oldSize, oldSize, nil), + pv: newTestPVForModify(), + oldSc: newTestSCForModify(oldSc, provisioner), + sc: newTestSCForModify(oldSc, provisioner), + size: newSize, + + expectedPVC: newTestPVCForModify(&oldSc, newSize, oldSize, map[string]string{ + annoKeyPVCSpecRevision: "1", + annoKeyPVCSpecStorageClass: oldSc, + annoKeyPVCSpecStorageSize: newSize, + }), + expectedHasErr: true, + }, + { + desc: "volume is changed, and revision has not been upgraded", + + pvc: newTestPVCForModify(&oldSc, oldSize, oldSize, nil), + pv: newTestPVForModify(), + oldSc: newTestSCForModify(oldSc, provisioner), + sc: newTestSCForModify(newSc, provisioner), + size: newSize, isModifyVolumeFinished: false, expectedPVC: newTestPVCForModify(&oldSc, oldSize, oldSize, map[string]string{ annoKeyPVCSpecRevision: "1", - annoKeyPVCSpecStorageClass: oldSc, + annoKeyPVCSpecStorageClass: newSc, annoKeyPVCSpecStorageSize: newSize, }), expectedHasErr: true, }, { - desc: "volume size is changed, and delegate modification is finished", + desc: "volume is changed, and delegate modification is finished", pvc: newTestPVCForModify(&oldSc, oldSize, oldSize, map[string]string{ annoKeyPVCSpecRevision: "1", - annoKeyPVCSpecStorageClass: oldSc, + annoKeyPVCSpecStorageClass: newSc, annoKeyPVCSpecStorageSize: newSize, }), - pv: newTestPVForModify(), - sc: newTestSCForModify(oldSc, provisioner), - size: newSize, + pv: newTestPVForModify(), + oldSc: newTestSCForModify(oldSc, provisioner), + sc: newTestSCForModify(newSc, provisioner), + size: newSize, isModifyVolumeFinished: true, expectedPVC: newTestPVCForModify(&oldSc, newSize, oldSize, map[string]string{ annoKeyPVCSpecRevision: "1", - annoKeyPVCSpecStorageClass: oldSc, + annoKeyPVCSpecStorageClass: newSc, annoKeyPVCSpecStorageSize: newSize, }), expectedHasErr: true, }, { - desc: "volume size is changed, and fs resize is finished", + desc: "volume is changed, and fs resize is finished", pvc: newTestPVCForModify(&oldSc, newSize, newSize, map[string]string{ annoKeyPVCSpecRevision: "1", - annoKeyPVCSpecStorageClass: oldSc, + annoKeyPVCSpecStorageClass: newSc, annoKeyPVCSpecStorageSize: newSize, }), - pv: newTestPVForModify(), - sc: newTestSCForModify(oldSc, provisioner), - size: newSize, + pv: newTestPVForModify(), + oldSc: newTestSCForModify(oldSc, provisioner), + sc: newTestSCForModify(newSc, provisioner), + size: newSize, isModifyVolumeFinished: true, expectedPVC: newTestPVCForModify(&oldSc, newSize, newSize, map[string]string{ annoKeyPVCSpecRevision: "1", - annoKeyPVCSpecStorageClass: oldSc, + annoKeyPVCSpecStorageClass: newSc, annoKeyPVCSpecStorageSize: newSize, annoKeyPVCStatusRevision: "1", - annoKeyPVCStatusStorageClass: oldSc, + annoKeyPVCStatusStorageClass: newSc, annoKeyPVCStatusStorageSize: newSize, }), }, @@ -189,6 +211,7 @@ func TestModify(t *testing.T) { m := delegation.NewMockVolumeModifier(provisioner, time.Hour) m.ModifyVolumeFunc = func(_ context.Context, pvc *corev1.PersistentVolumeClaim, pv *corev1.PersistentVolume, sc *storagev1.StorageClass) (bool, error) { + fmt.Println("call modify volume") return !c.isModifyVolumeFinished, nil } @@ -206,13 +229,14 @@ func TestModify(t *testing.T) { actual := ActualVolume{ Desired: &DesiredVolume{ - Name: "test", - Size: resource.MustParse(c.size), - StorageClass: c.sc, + Name: "test", + Size: resource.MustParse(c.size), + StorageClass: c.sc, + StorageClassName: &c.sc.Name, }, PVC: c.pvc, PV: c.pv, - StorageClass: c.sc, + StorageClass: c.oldSc, } phase := pvm.getVolumePhase(&actual) diff --git a/pkg/manager/volumes/pvc_modifier.go b/pkg/manager/volumes/pvc_modifier.go index 60d1e9a194..68f57a8a05 100644 --- a/pkg/manager/volumes/pvc_modifier.go +++ b/pkg/manager/volumes/pvc_modifier.go @@ -220,7 +220,8 @@ func (p *pvcModifier) isStatefulSetSynced(ctx *componentVolumeContext, sts *apps } func isStorageClassMatched(sc *storagev1.StorageClass, scName string) bool { - if sc == nil && scName == "" { + if sc == nil { + // cannot get sc or sc is unset return true } if sc.Name == scName { diff --git a/pkg/manager/volumes/sync_volume_status.go b/pkg/manager/volumes/sync_volume_status.go index 7bf3aa77ee..be59bd0519 100644 --- a/pkg/manager/volumes/sync_volume_status.go +++ b/pkg/manager/volumes/sync_volume_status.go @@ -87,6 +87,16 @@ func observeVolumeStatus(pvm PodVolumeModifier, pods []*v1.Pod, desiredVolumes [ actualCap := volume.GetStorageSize() desiredSC := volume.Desired.GetStorageClassName() actualSC := volume.GetStorageClassName() + scCannotChange := false + + if desiredSC == "" { + // sc is unset + desiredSC = actualSC + } else if volume.Desired.StorageClass == nil { + // sc don't exist or no permission to get sc + desiredSC = "" + scCannotChange = true + } status, exist := observedStatus[volName] if !exist { @@ -100,6 +110,9 @@ func observeVolumeStatus(pvm PodVolumeModifier, pods []*v1.Pod, desiredVolumes [ ModifiedCapacity: desiredCap, // CurrentStorageClass is default to same as desired storage class, and maybe changed later if any // volume is modifying. + // FIXME: CurrentStorageClass may not only one sc in some situations, + // e.g. tikv-0 uses sc aaa, tikv-1 uses sc bbb + // TODO: maybe change it to an array field ? CurrentStorageClass: desiredSC, ModifiedStorageClass: desiredSC, } @@ -109,7 +122,10 @@ func observeVolumeStatus(pvm PodVolumeModifier, pods []*v1.Pod, desiredVolumes [ status.BoundCount++ capModified := actualCap.Cmp(desiredCap) == 0 scModified := actualSC == desiredSC - if capModified && scModified { + if scCannotChange { + status.CurrentStorageClass = actualSC + } + if capModified && (scModified || scCannotChange) { status.ModifiedCount++ } else { status.CurrentCount++ @@ -120,7 +136,6 @@ func observeVolumeStatus(pvm PodVolumeModifier, pods []*v1.Pod, desiredVolumes [ status.CurrentStorageClass = actualSC } } - } } diff --git a/pkg/manager/volumes/sync_volume_status_test.go b/pkg/manager/volumes/sync_volume_status_test.go index db489c70fa..c4703f7fa5 100644 --- a/pkg/manager/volumes/sync_volume_status_test.go +++ b/pkg/manager/volumes/sync_volume_status_test.go @@ -111,19 +111,22 @@ func TestObserveVolumeStatus(t *testing.T) { desiredVolumes := []DesiredVolume{ { - Name: "vol1", - Size: resource.MustParse(desiredSize), - StorageClass: newStorageClass(desiredSC, true), + Name: "vol1", + Size: resource.MustParse(desiredSize), + StorageClass: newStorageClass(desiredSC, true), + StorageClassName: &desiredSC, }, { - Name: "vol2", - Size: resource.MustParse(desiredSize), - StorageClass: newStorageClass(desiredSC, true), + Name: "vol2", + Size: resource.MustParse(desiredSize), + StorageClass: newStorageClass(desiredSC, true), + StorageClassName: &desiredSC, }, { - Name: "vol3", - Size: resource.MustParse(desiredSize), - StorageClass: newStorageClass(desiredSC, true), + Name: "vol3", + Size: resource.MustParse(desiredSize), + StorageClass: newStorageClass(desiredSC, true), + StorageClassName: &desiredSC, }, } pvm.GetActualVolumesFunc = func(pod *corev1.Pod, vs []DesiredVolume) ([]ActualVolume, error) {