From 4efddc907fa416507e080c5674a9757cf3ece833 Mon Sep 17 00:00:00 2001 From: gnolong <2391353625@qq.com> Date: Wed, 8 May 2024 16:04:06 +0800 Subject: [PATCH 1/2] fix: check existence of backuprepo before creating a backup --- pkg/cmd/backuprepo/common.go | 5 +++-- pkg/cmd/cluster/dataprotection.go | 26 ++++++++++++++++++++++++++ pkg/cmd/cluster/dataprotection_test.go | 19 +++++++++++++------ 3 files changed, 42 insertions(+), 8 deletions(-) diff --git a/pkg/cmd/backuprepo/common.go b/pkg/cmd/backuprepo/common.go index 4c667cd90..f794776e0 100644 --- a/pkg/cmd/backuprepo/common.go +++ b/pkg/cmd/backuprepo/common.go @@ -29,10 +29,11 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/dynamic" - "github.com/apecloud/kbcli/pkg/types" - "github.com/apecloud/kbcli/pkg/util" dpv1alpha1 "github.com/apecloud/kubeblocks/apis/dataprotection/v1alpha1" storagev1alpha1 "github.com/apecloud/kubeblocks/apis/storage/v1alpha1" + + "github.com/apecloud/kbcli/pkg/types" + "github.com/apecloud/kbcli/pkg/util" ) const ( diff --git a/pkg/cmd/cluster/dataprotection.go b/pkg/cmd/cluster/dataprotection.go index f3cf87f2b..7040404a5 100644 --- a/pkg/cmd/cluster/dataprotection.go +++ b/pkg/cmd/cluster/dataprotection.go @@ -206,6 +206,32 @@ func (o *CreateBackupOptions) Validate() error { o.BackupSpec.BackupMethod, backupPolicy.Name, strings.Join(availableMethods, ", ")) } + // check if the backup repo exists in backup policy + backupRepoName := backupPolicy.Spec.BackupRepoName + if backupRepoName != nil { + _, err := o.Dynamic.Resource(types.BackupRepoGVR()).Get(context.Background(), *backupRepoName, metav1.GetOptions{}) + if err != nil { + return err + } + } else { + backupRepoList, err := o.Dynamic.Resource(types.BackupRepoGVR()).List(context.Background(), metav1.ListOptions{}) + if err != nil { + return err + } + if len(backupRepoList.Items) == 0 { + return fmt.Errorf("No backup repository found") + } + existDefaultBackupRepo := false + for _, item := range backupRepoList.Items { + if item.GetAnnotations()[dptypes.DefaultBackupRepoAnnotationKey] == "true" { + existDefaultBackupRepo = true + break + } + } + if !existDefaultBackupRepo { + return fmt.Errorf("No default backup repository exists") + } + } // TODO: check if pvc exists // valid retention period diff --git a/pkg/cmd/cluster/dataprotection_test.go b/pkg/cmd/cluster/dataprotection_test.go index 303fac5eb..4264d0d13 100644 --- a/pkg/cmd/cluster/dataprotection_test.go +++ b/pkg/cmd/cluster/dataprotection_test.go @@ -97,7 +97,7 @@ var _ = Describe("DataProtection", func() { }) Context("backup", func() { - initClient := func(policies ...*dpv1alpha1.BackupPolicy) { + initClient := func(objs ...runtime.Object) { clusterDef := testing.FakeClusterDef() cluster := testing.FakeCluster(testing.ClusterName, testing.Namespace) clusterDefLabel := map[string]string{ @@ -108,9 +108,7 @@ var _ = Describe("DataProtection", func() { objects := []runtime.Object{ cluster, clusterDef, &pods.Items[0], } - for _, v := range policies { - objects = append(objects, v) - } + objects = append(objects, objs...) tf.FakeDynamicClient = testing.FakeDynamicClient(objects...) } @@ -216,18 +214,27 @@ var _ = Describe("DataProtection", func() { o.Dynamic = tf.FakeDynamicClient Expect(o.Validate().Error()).Should(ContainSubstring("backup method can not be empty, you can specify it by --method")) + By("test without default backup repo") + repo := testing.FakeBackupRepo(repoName, false) + initClient(defaultBackupPolicy, repo) + o.Dynamic = tf.FakeDynamicClient + o.BackupSpec.BackupMethod = testing.BackupMethodName + Expect(o.Validate()).Should(MatchError(fmt.Errorf("No default backup repository exists"))) + By("test with one default backupPolicy") - initClient(defaultBackupPolicy) + defaultRepo := testing.FakeBackupRepo("default-repo", true) + initClient(defaultBackupPolicy, defaultRepo) o.Dynamic = tf.FakeDynamicClient o.BackupSpec.BackupMethod = testing.BackupMethodName Expect(o.Validate()).Should(Succeed()) }) It("run backup command", func() { + defaultRepo := testing.FakeBackupRepo("default-repo", true) defaultBackupPolicy := testing.FakeBackupPolicy(policyName, testing.ClusterName) otherBackupPolicy := testing.FakeBackupPolicy("otherPolicy", testing.ClusterName) otherBackupPolicy.Annotations = map[string]string{} - initClient(defaultBackupPolicy, otherBackupPolicy) + initClient(defaultBackupPolicy, otherBackupPolicy, defaultRepo) By("test backup with default backupPolicy") cmd := NewCreateBackupCmd(tf, streams) Expect(cmd).ShouldNot(BeNil()) From c0f492c86f391b353abecc304281f7ec0676d81d Mon Sep 17 00:00:00 2001 From: gnolong <2391353625@qq.com> Date: Thu, 9 May 2024 13:11:55 +0800 Subject: [PATCH 2/2] fix: check existence of backuprepo before creating a backup --- pkg/cmd/cluster/dataprotection.go | 14 ++++++++------ pkg/cmd/cluster/dataprotection_test.go | 10 +++++++++- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/pkg/cmd/cluster/dataprotection.go b/pkg/cmd/cluster/dataprotection.go index 7040404a5..217d7f44e 100644 --- a/pkg/cmd/cluster/dataprotection.go +++ b/pkg/cmd/cluster/dataprotection.go @@ -219,17 +219,19 @@ func (o *CreateBackupOptions) Validate() error { return err } if len(backupRepoList.Items) == 0 { - return fmt.Errorf("No backup repository found") + return fmt.Errorf("No backuprepo found") } - existDefaultBackupRepo := false + var defaultBackupRepos []unstructured.Unstructured for _, item := range backupRepoList.Items { if item.GetAnnotations()[dptypes.DefaultBackupRepoAnnotationKey] == "true" { - existDefaultBackupRepo = true - break + defaultBackupRepos = append(defaultBackupRepos, item) } } - if !existDefaultBackupRepo { - return fmt.Errorf("No default backup repository exists") + if len(defaultBackupRepos) == 0 { + return fmt.Errorf("No default backuprepo exists") + } + if len(defaultBackupRepos) > 1 { + return fmt.Errorf("Cluster %s has multiple default backuprepos", o.Name) } } // TODO: check if pvc exists diff --git a/pkg/cmd/cluster/dataprotection_test.go b/pkg/cmd/cluster/dataprotection_test.go index 4264d0d13..e01a5559c 100644 --- a/pkg/cmd/cluster/dataprotection_test.go +++ b/pkg/cmd/cluster/dataprotection_test.go @@ -219,7 +219,15 @@ var _ = Describe("DataProtection", func() { initClient(defaultBackupPolicy, repo) o.Dynamic = tf.FakeDynamicClient o.BackupSpec.BackupMethod = testing.BackupMethodName - Expect(o.Validate()).Should(MatchError(fmt.Errorf("No default backup repository exists"))) + Expect(o.Validate()).Should(MatchError(fmt.Errorf("No default backuprepo exists"))) + + By("test with two default backup repos") + repo1 := testing.FakeBackupRepo("repo1", true) + repo2 := testing.FakeBackupRepo("repo2", true) + initClient(defaultBackupPolicy, repo1, repo2) + o.Dynamic = tf.FakeDynamicClient + o.BackupSpec.BackupMethod = testing.BackupMethodName + Expect(o.Validate()).Should(MatchError(fmt.Errorf("Cluster %s has multiple default backuprepos", o.Name))) By("test with one default backupPolicy") defaultRepo := testing.FakeBackupRepo("default-repo", true)