From 23f718a0bdbfcc73591a24c2f86fffeaff645c88 Mon Sep 17 00:00:00 2001 From: Ti Chi Robot Date: Mon, 19 Jun 2023 14:48:43 +0800 Subject: [PATCH 1/4] Update OWNERS file (#5069) --- OWNERS | 40 ++++++++++++++++++++++++---------------- 1 file changed, 24 insertions(+), 16 deletions(-) diff --git a/OWNERS b/OWNERS index 7b585f45d4..5e4fb469c9 100644 --- a/OWNERS +++ b/OWNERS @@ -1,30 +1,38 @@ # See the OWNERS docs at https://go.k8s.io/owners approvers: - # kubernetes-maintainers: + - AstroProfundis + - aylei - azurezyq + - BornChanger - charleszheng44 + - cofyc - csuzhangxc + - DanielZhangQD + - dragonly + - Ehco1996 + - fengou1 + - gozssky + - gregwebs - grovecai + - handlerww - hanlins + - jlerche + - july2993 - KanShiori + - LinuxGit - liubog2008 - # kubernetes-committers: - - WangLe1321 - - tennix - - shuijing198799 - - sdojjy - mikechengwei - - LinuxGit - - KanShiori - - handlerww - - gozssky - - fengou1 - - Ehco1996 - - dragonly - - BornChanger + - onlymellb + - qiffang + - sdojjy + - shuijing198799 + - tennix + - WangLe1321 + - weekface + - WizardXiao + - Yisaer reviewers: - # kubernetes-reviewers: + - cvvz - howardlau1999 - - KanShiori - lichunzhu - shonge From 3092372d32e8d2b875e6e33378725340eac46341 Mon Sep 17 00:00:00 2001 From: ruoxi Date: Tue, 20 Jun 2023 10:13:23 +0800 Subject: [PATCH 2/4] Remove tcp and http ports for tiflash >= 7.1.0 (#5075) --- pkg/manager/member/tiflash_util.go | 15 ++++-- pkg/manager/member/tiflash_util_test.go | 64 +++++++++++++------------ 2 files changed, 44 insertions(+), 35 deletions(-) diff --git a/pkg/manager/member/tiflash_util.go b/pkg/manager/member/tiflash_util.go index 1c8065beeb..f954385289 100644 --- a/pkg/manager/member/tiflash_util.go +++ b/pkg/manager/member/tiflash_util.go @@ -37,6 +37,8 @@ const ( var ( // the first version that tiflash change default config tiflashEqualOrGreaterThanV540, _ = cmpver.NewConstraint(cmpver.GreaterOrEqual, "v5.4.0") + // the first version that tiflash discards http and tcp ports. + tiflashEqualOrGreaterThanV710, _ = cmpver.NewConstraint(cmpver.GreaterOrEqual, "v7.1.0") ) func buildTiFlashSidecarContainers(tc *v1alpha1.TidbCluster) ([]corev1.Container, error) { @@ -138,6 +140,7 @@ func getTiFlashConfigV2(tc *v1alpha1.TidbCluster) *v1alpha1.TiFlashConfigWraper if tc.Spec.PreferIPv6 { listenHost = listenHostForIPv6 } + version := tc.TiFlashVersion() // common { @@ -161,8 +164,10 @@ func getTiFlashConfigV2(tc *v1alpha1.TidbCluster) *v1alpha1.TiFlashConfigWraper common.SetIfNil("tmp_path", "/data0/tmp") // port - common.SetIfNil("tcp_port", int64(v1alpha1.DefaultTiFlashTcpPort)) - common.SetIfNil("http_port", int64(v1alpha1.DefaultTiFlashHttpPort)) + if ok, err := tiflashEqualOrGreaterThanV710.Check(version); err == nil && !ok { + common.SetIfNil("tcp_port", int64(v1alpha1.DefaultTiFlashTcpPort)) + common.SetIfNil("http_port", int64(v1alpha1.DefaultTiFlashHttpPort)) + } // flash tidbStatusAddr := fmt.Sprintf("%s.%s.svc:%d", controller.TiDBMemberName(name), ns, v1alpha1.DefaultTiDBStatusPort) @@ -224,8 +229,10 @@ func getTiFlashConfigV2(tc *v1alpha1.TidbCluster) *v1alpha1.TiFlashConfigWraper common.Set("security.ca_path", path.Join(tiflashCertPath, corev1.ServiceAccountRootCAKey)) common.Set("security.cert_path", path.Join(tiflashCertPath, corev1.TLSCertKey)) common.Set("security.key_path", path.Join(tiflashCertPath, corev1.TLSPrivateKeyKey)) - common.SetIfNil("tcp_port_secure", int64(v1alpha1.DefaultTiFlashTcpPort)) - common.SetIfNil("https_port", int64(v1alpha1.DefaultTiFlashHttpPort)) + if ok, err := tiflashEqualOrGreaterThanV710.Check(version); err == nil && !ok { + common.SetIfNil("tcp_port_secure", int64(v1alpha1.DefaultTiFlashTcpPort)) + common.SetIfNil("https_port", int64(v1alpha1.DefaultTiFlashHttpPort)) + } common.Del("http_port") common.Del("tcp_port") diff --git a/pkg/manager/member/tiflash_util_test.go b/pkg/manager/member/tiflash_util_test.go index 906ffb1358..cff1648c33 100644 --- a/pkg/manager/member/tiflash_util_test.go +++ b/pkg/manager/member/tiflash_util_test.go @@ -1511,8 +1511,6 @@ func TestTestGetTiFlashConfig(t *testing.T) { tc.Spec.TiFlash.Config = nil }, expectCommonCfg: ` - http_port = 8123 - tcp_port = 9000 tmp_path = "/data0/tmp" [flash] service_addr = "0.0.0.0:3930" @@ -1549,8 +1547,6 @@ func TestTestGetTiFlashConfig(t *testing.T) { tc.Spec.TLSCluster = &v1alpha1.TLSCluster{Enabled: true} }, expectCommonCfg: ` - https_port = 8123 - tcp_port_secure = 9000 tmp_path = "/data0/tmp" [flash] service_addr = "0.0.0.0:3930" @@ -1598,8 +1594,6 @@ func TestTestGetTiFlashConfig(t *testing.T) { tc.Spec.TLSCluster = &v1alpha1.TLSCluster{Enabled: true} }, expectCommonCfg: ` - https_port = 8123 - tcp_port_secure = 9000 tmp_path = "/data0/tmp" [flash] service_addr = "0.0.0.0:3930" @@ -1653,8 +1647,6 @@ func TestTestGetTiFlashConfig(t *testing.T) { }, expectCommonCfg: ` - http_port = 8123 - tcp_port = 9000 tmp_path = "/data0/tmp" [flash] service_addr = "0.0.0.0:3930" @@ -1693,8 +1685,6 @@ func TestTestGetTiFlashConfig(t *testing.T) { tc.Spec.Cluster = &v1alpha1.TidbClusterRef{Name: "cluster-1", Namespace: "default"} }, expectCommonCfg: ` - http_port = 8123 - tcp_port = 9000 tmp_path = "/data0/tmp" [flash] service_addr = "0.0.0.0:3930" @@ -1731,8 +1721,6 @@ func TestTestGetTiFlashConfig(t *testing.T) { tc.Spec.AcrossK8s = true }, expectCommonCfg: ` - http_port = 8123 - tcp_port = 9000 tmp_path = "/data0/tmp" [flash] service_addr = "0.0.0.0:3930" @@ -1772,8 +1760,6 @@ func TestTestGetTiFlashConfig(t *testing.T) { tc.Spec.AcrossK8s = true }, expectCommonCfg: ` - http_port = 8123 - tcp_port = 9000 tmp_path = "/data0/tmp" [flash] service_addr = "0.0.0.0:3930" @@ -1813,8 +1799,6 @@ func TestTestGetTiFlashConfig(t *testing.T) { tc.Spec.AcrossK8s = true }, expectCommonCfg: ` - http_port = 8123 - tcp_port = 9000 tmp_path = "/data0/tmp" [flash] service_addr = "0.0.0.0:3930" @@ -1854,29 +1838,47 @@ func TestTestGetTiFlashConfig(t *testing.T) { tc.Name = "test" tc.Namespace = "default" tc.Spec.TiFlash = &v1alpha1.TiFlashSpec{} + tc.Spec.TiFlash.BaseImage = "pingcap/tiflash" if testcase.setTC != nil { testcase.setTC(tc) } - cfg := getTiFlashConfigV2(tc) + for _, version := range []string{"v7.0.0", "v7.1.0"} { + tc.Spec.Version = version + + expectCommonCfg := testcase.expectCommonCfg + if ok, err := tiflashEqualOrGreaterThanV710.Check(version); err == nil && !ok { + if tc.Spec.TLSCluster != nil && tc.Spec.TLSCluster.Enabled { + expectCommonCfg = ` + https_port = 8123 + tcp_port_secure = 9000` + expectCommonCfg + } else { + expectCommonCfg = ` + http_port = 8123 + tcp_port = 9000` + expectCommonCfg + } + } - commonCfgData, err := cfg.Common.MarshalTOML() - g.Expect(err).Should(Succeed()) - proxyCfgData, err := cfg.Proxy.MarshalTOML() - g.Expect(err).Should(Succeed()) + cfg := getTiFlashConfigV2(tc) - outputCfg := v1alpha1.NewTiFlashConfig() - expectCfg := v1alpha1.NewTiFlashConfig() - outputCfg.Common.UnmarshalTOML(commonCfgData) - outputCfg.Proxy.UnmarshalTOML(proxyCfgData) - expectCfg.Common.UnmarshalTOML([]byte(testcase.expectCommonCfg)) - expectCfg.Proxy.UnmarshalTOML([]byte(testcase.expectProxyCfg)) + commonCfgData, err := cfg.Common.MarshalTOML() + g.Expect(err).Should(Succeed()) + proxyCfgData, err := cfg.Proxy.MarshalTOML() + g.Expect(err).Should(Succeed()) - diff := cmp.Diff(outputCfg.Common.Inner(), expectCfg.Common.Inner()) - g.Expect(diff).Should(BeEmpty()) - diff = cmp.Diff(outputCfg.Proxy.Inner(), expectCfg.Proxy.Inner()) - g.Expect(diff).Should(BeEmpty()) + outputCfg := v1alpha1.NewTiFlashConfig() + expectCfg := v1alpha1.NewTiFlashConfig() + outputCfg.Common.UnmarshalTOML(commonCfgData) + outputCfg.Proxy.UnmarshalTOML(proxyCfgData) + expectCfg.Common.UnmarshalTOML([]byte(expectCommonCfg)) + expectCfg.Proxy.UnmarshalTOML([]byte(testcase.expectProxyCfg)) + + diff := cmp.Diff(outputCfg.Common.Inner(), expectCfg.Common.Inner()) + g.Expect(diff).Should(BeEmpty()) + diff = cmp.Diff(outputCfg.Proxy.Inner(), expectCfg.Proxy.Inner()) + g.Expect(diff).Should(BeEmpty()) + } }) } }) From 0015c826d805f632a3be7e105492112886bdf59a Mon Sep 17 00:00:00 2001 From: Bo Liu Date: Tue, 20 Jun 2023 18:10:52 +0800 Subject: [PATCH 3/4] fix(volume): fix panic bug when enable ModifyVolume feature (#5058) --- pkg/manager/volumes/phase.go | 65 +++--- 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, 248 insertions(+), 172 deletions(-) diff --git a/pkg/manager/volumes/phase.go b/pkg/manager/volumes/phase.go index 1c0f8d98c9..256d11dd91 100644 --- a/pkg/manager/volumes/phase.go +++ b/pkg/manager/volumes/phase.go @@ -71,9 +71,7 @@ func (p VolumePhase) String() string { func (p *podVolModifier) getVolumePhase(vol *ActualVolume) VolumePhase { if err := p.validate(vol); err != nil { - if !errors.Is(err, ErrChangeDefaultStorageClass) { - klog.Warningf("volume %s/%s modification is not allowed: %v", vol.PVC.Namespace, vol.PVC.Name, err) - } + klog.Warningf("volume %s/%s modification is not allowed: %v", vol.PVC.Namespace, vol.PVC.Name, err) return VolumePhaseCannotModify } if isPVCRevisionChanged(vol.PVC) { @@ -84,44 +82,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 ErrChangeDefaultStorageClass - } - 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 @@ -135,7 +144,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 @@ -146,7 +155,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 { @@ -163,23 +172,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 { @@ -195,3 +195,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) { From 4f378772013099667f3f49be7de3cc2f5c1d7fea Mon Sep 17 00:00:00 2001 From: xhe Date: Tue, 20 Jun 2023 22:38:41 +0800 Subject: [PATCH 4/4] tiproxy: revert enable SSL by default (#5084) --- docs/api-references/docs.md | 4 ++-- manifests/crd.yaml | 2 +- manifests/crd/v1/pingcap.com_tidbclusters.yaml | 2 +- manifests/crd/v1beta1/pingcap.com_tidbclusters.yaml | 2 +- manifests/crd_v1beta1.yaml | 2 +- pkg/apis/pingcap/v1alpha1/openapi_generated.go | 4 ++-- pkg/apis/pingcap/v1alpha1/types.go | 4 ++-- pkg/manager/member/tiproxy_member_manager.go | 4 ++-- 8 files changed, 12 insertions(+), 12 deletions(-) diff --git a/docs/api-references/docs.md b/docs/api-references/docs.md index 413a8c4b2f..789440c5fb 100644 --- a/docs/api-references/docs.md +++ b/docs/api-references/docs.md @@ -22592,13 +22592,13 @@ int32 -sslDisableTiDB
+sslEnableTiDB
bool -

Whether disable SSL connection between tiproxy and TiDB server

+

Whether enable SSL connection between tiproxy and TiDB server

diff --git a/manifests/crd.yaml b/manifests/crd.yaml index 449b6d0380..36c7bd0582 100644 --- a/manifests/crd.yaml +++ b/manifests/crd.yaml @@ -34235,7 +34235,7 @@ spec: type: string serviceAccount: type: string - sslDisableTiDB: + sslEnableTiDB: type: boolean statefulSetUpdateStrategy: type: string diff --git a/manifests/crd/v1/pingcap.com_tidbclusters.yaml b/manifests/crd/v1/pingcap.com_tidbclusters.yaml index 8b62bd14c0..8249bfe174 100644 --- a/manifests/crd/v1/pingcap.com_tidbclusters.yaml +++ b/manifests/crd/v1/pingcap.com_tidbclusters.yaml @@ -19617,7 +19617,7 @@ spec: type: string serviceAccount: type: string - sslDisableTiDB: + sslEnableTiDB: type: boolean statefulSetUpdateStrategy: type: string diff --git a/manifests/crd/v1beta1/pingcap.com_tidbclusters.yaml b/manifests/crd/v1beta1/pingcap.com_tidbclusters.yaml index ab2b266f25..0deacc7d5c 100644 --- a/manifests/crd/v1beta1/pingcap.com_tidbclusters.yaml +++ b/manifests/crd/v1beta1/pingcap.com_tidbclusters.yaml @@ -19587,7 +19587,7 @@ spec: type: string serviceAccount: type: string - sslDisableTiDB: + sslEnableTiDB: type: boolean statefulSetUpdateStrategy: type: string diff --git a/manifests/crd_v1beta1.yaml b/manifests/crd_v1beta1.yaml index 060e7f7fef..5e571d724b 100644 --- a/manifests/crd_v1beta1.yaml +++ b/manifests/crd_v1beta1.yaml @@ -34189,7 +34189,7 @@ spec: type: string serviceAccount: type: string - sslDisableTiDB: + sslEnableTiDB: type: boolean statefulSetUpdateStrategy: type: string diff --git a/pkg/apis/pingcap/v1alpha1/openapi_generated.go b/pkg/apis/pingcap/v1alpha1/openapi_generated.go index d2c870a05a..67a2b3410f 100644 --- a/pkg/apis/pingcap/v1alpha1/openapi_generated.go +++ b/pkg/apis/pingcap/v1alpha1/openapi_generated.go @@ -13070,9 +13070,9 @@ func schema_pkg_apis_pingcap_v1alpha1_TiProxySpec(ref common.ReferenceCallback) Format: "int32", }, }, - "sslDisableTiDB": { + "sslEnableTiDB": { SchemaProps: spec.SchemaProps{ - Description: "Whether disable SSL connection between tiproxy and TiDB server", + Description: "Whether enable SSL connection between tiproxy and TiDB server", Type: []string{"boolean"}, Format: "", }, diff --git a/pkg/apis/pingcap/v1alpha1/types.go b/pkg/apis/pingcap/v1alpha1/types.go index 51507fdb16..6004f0f794 100644 --- a/pkg/apis/pingcap/v1alpha1/types.go +++ b/pkg/apis/pingcap/v1alpha1/types.go @@ -774,8 +774,8 @@ type TiProxySpec struct { // +kubebuilder:validation:Minimum=0 Replicas int32 `json:"replicas"` - // Whether disable SSL connection between tiproxy and TiDB server - SSLDisableTiDB bool `json:"sslDisableTiDB,omitempty"` + // Whether enable SSL connection between tiproxy and TiDB server + SSLEnableTiDB bool `json:"sslEnableTiDB,omitempty"` // TLSClientSecretName is the name of secret which stores tidb server client certificate // used by TiProxy to check health status. diff --git a/pkg/manager/member/tiproxy_member_manager.go b/pkg/manager/member/tiproxy_member_manager.go index 02263912cf..7bbec4b102 100644 --- a/pkg/manager/member/tiproxy_member_manager.go +++ b/pkg/manager/member/tiproxy_member_manager.go @@ -134,7 +134,7 @@ func (m *tiproxyMemberManager) syncConfigMap(tc *v1alpha1.TidbCluster, set *apps cfgWrapper.Set("security.server-tls.cert", path.Join(tiproxyServerPath, "tls.crt")) cfgWrapper.Set("security.server-tls.skip-ca", true) - if !tc.Spec.TiProxy.SSLDisableTiDB || !tc.SkipTLSWhenConnectTiDB() { + if tc.Spec.TiProxy.SSLEnableTiDB || !tc.SkipTLSWhenConnectTiDB() { if tc.Spec.TiDB.TLSClient.SkipInternalClientCA { cfgWrapper.Set("security.sql-tls.skip-ca", true) } else { @@ -440,7 +440,7 @@ func (m *tiproxyMemberManager) getNewStatefulSet(tc *v1alpha1.TidbCluster, cm *c }, }) - if !tc.Spec.TiProxy.SSLDisableTiDB || !tc.SkipTLSWhenConnectTiDB() { + if tc.Spec.TiProxy.SSLEnableTiDB || !tc.SkipTLSWhenConnectTiDB() { volMounts = append(volMounts, corev1.VolumeMount{ Name: "tidb-client-tls", ReadOnly: true, MountPath: tiproxySQLPath, })