fix: backup schedule contains an invalid schedule

docs/user_docs/cli/kbcli_cluster_create.md

@@ -92,7 +92,7 @@ kbcli cluster create [NAME] [flags]
       --backup string                          Set a source backup to restore data
       --backup-cron-expression string          the cron expression for schedule, the timezone is in UTC. see https://en.wikipedia.org/wiki/Cron.
       --backup-enabled                         Specify whether enabled automated backup
-      --backup-method string                   the backup method, support: snapshot, backupTool (default "snapshot")
+      --backup-method string                   the backup method, view it by "kbcli cd describe <cluster-definition>", if not specified, the default backup method will be to take snapshots of the volume
       --backup-repo-name string                the backup repository name
       --backup-retention-period string         a time string ending with the 'd'|'D'|'h'|'H' character to describe how long the Backup should be retained (default "1d")
       --backup-starting-deadline-minutes int   the deadline in minutes for starting the backup job if it misses its scheduled time for any reason

docs/user_docs/cli/kbcli_cluster_update.md

@@ -60,7 +60,7 @@ kbcli cluster update NAME [flags]
       --allow-missing-template-keys            If true, ignore any errors in templates when a field or map key is missing in the template. Only applies to golang and jsonpath output formats. (default true)
       --backup-cron-expression string          the cron expression for schedule, the timezone is in UTC. see https://en.wikipedia.org/wiki/Cron.
       --backup-enabled                         Specify whether enabled automated backup
-      --backup-method string                   the backup method, support: snapshot, backupTool (default "snapshot")
+      --backup-method string                   the backup method, view it by "kbcli cd describe <cluster-definition>", if not specified, the default backup method will be to take snapshots of the volume
       --backup-repo-name string                the backup repository name
       --backup-retention-period string         a time string ending with the 'd'|'D'|'h'|'H' character to describe how long the Backup should be retained (default "1d")
       --backup-starting-deadline-minutes int   the deadline in minutes for starting the backup job if it misses its scheduled time for any reason

go.mod

@@ -60,6 +60,7 @@ require (
 	github.com/mitchellh/mapstructure v1.5.1-0.20220423185008-bf980b35cac4
 	github.com/onsi/ginkgo/v2 v2.11.0
 	github.com/onsi/gomega v1.27.8
+	github.com/opencontainers/image-spec v1.1.0-rc5
 	github.com/pashagolub/pgxmock/v2 v2.11.0
 	github.com/pkg/errors v0.9.1
 	github.com/pmezard/go-difflib v1.0.0
@@ -310,7 +311,6 @@ require (
 	github.com/nsf/termbox-go v0.0.0-20190121233118-02980233997d // indirect
 	github.com/oklog/ulid v1.3.1 // indirect
 	github.com/opencontainers/go-digest v1.0.0 // indirect
-	github.com/opencontainers/image-spec v1.1.0-rc5 // indirect
 	github.com/opencontainers/runc v1.1.7 // indirect
 	github.com/opencontainers/runtime-spec v1.1.0-rc.3 // indirect
 	github.com/opencontainers/selinux v1.11.0 // indirect

internal/cli/cmd/cluster/create.go

@@ -61,6 +61,8 @@ import (
 	"github.com/apecloud/kubeblocks/internal/cli/types"
 	"github.com/apecloud/kubeblocks/internal/cli/util"
 	"github.com/apecloud/kubeblocks/internal/constant"
+	dptypes "github.com/apecloud/kubeblocks/internal/dataprotection/types"
+	"github.com/apecloud/kubeblocks/internal/dataprotection/utils/boolptr"
 	viper "github.com/apecloud/kubeblocks/internal/viperx"
 )
 
@@ -879,10 +881,6 @@ func registerFlagCompletionFunc(cmd *cobra.Command, f cmdutil.Factory) {
 
 // PreCreate before saving yaml to k8s, makes changes on Unstructured yaml
 func (o *CreateOptions) PreCreate(obj *unstructured.Unstructured) error {
-	if !o.EnableAllLogs {
-		// EnableAllLogs is false, nothing will change
-		return nil
-	}
 	c := &appsv1alpha1.Cluster{}
 	if err := runtime.DefaultUnstructuredConverter.FromUnstructured(obj.Object, c); err != nil {
 		return err
@@ -892,7 +890,14 @@ func (o *CreateOptions) PreCreate(obj *unstructured.Unstructured) error {
 	if err != nil {
 		return err
 	}
-	setEnableAllLogs(c, cd)
+
+	if !o.EnableAllLogs {
+		setEnableAllLogs(c, cd)
+	}
+	if o.BackupConfig == nil {
+		// if backup config is not specified, set cluster's backup to nil
+		c.Spec.Backup = nil
+	}
 	data, e := runtime.DefaultUnstructuredConverter.ToUnstructured(c)
 	if e != nil {
 		return e
@@ -1213,7 +1218,7 @@ func (f *UpdatableFlags) addFlags(cmd *cobra.Command) {
 	cmd.Flags().StringVar(&f.Tenancy, "tenancy", "SharedNode", "Tenancy options, one of: (SharedNode, DedicatedNode)")
 	cmd.Flags().BoolVar(&f.BackupEnabled, "backup-enabled", false, "Specify whether enabled automated backup")
 	cmd.Flags().StringVar(&f.BackupRetentionPeriod, "backup-retention-period", "1d", "a time string ending with the 'd'|'D'|'h'|'H' character to describe how long the Backup should be retained")
-	cmd.Flags().StringVar(&f.BackupMethod, "backup-method", "snapshot", "the backup method, support: snapshot, backupTool")
+	cmd.Flags().StringVar(&f.BackupMethod, "backup-method", "", "the backup method, view it by \"kbcli cd describe <cluster-definition>\", if not specified, the default backup method will be to take snapshots of the volume")
 	cmd.Flags().StringVar(&f.BackupCronExpression, "backup-cron-expression", "", "the cron expression for schedule, the timezone is in UTC. see https://en.wikipedia.org/wiki/Cron.")
 	cmd.Flags().Int64Var(&f.BackupStartingDeadlineMinutes, "backup-starting-deadline-minutes", 0, "the deadline in minutes for starting the backup job if it misses its scheduled time for any reason")
 	cmd.Flags().StringVar(&f.BackupRepoName, "backup-repo-name", "", "the backup repository name")
@@ -1379,9 +1384,6 @@ func (o *CreateOptions) buildAnnotation(cls *appsv1alpha1.Cluster) {
 }
 
 func (o *CreateOptions) buildBackupConfig(cls *appsv1alpha1.Cluster) error {
-	// set default backup config
-	o.BackupConfig = &appsv1alpha1.ClusterBackup{}
-
 	// if the cls.Backup isn't nil, use the backup config in cluster
 	if cls != nil && cls.Spec.Backup != nil {
 		o.BackupConfig = cls.Spec.Backup
@@ -1391,36 +1393,114 @@ func (o *CreateOptions) buildBackupConfig(cls *appsv1alpha1.Cluster) error {
 	var flags []*pflag.Flag
 	if o.Cmd != nil {
 		o.Cmd.Flags().Visit(func(flag *pflag.Flag) {
-			flags = append(flags, flag)
+			// only check the backup flags
+			if flag.Name == "backup-enabled" || flag.Name == "backup-retention-period" ||
+				flag.Name == "backup-method" || flag.Name == "backup-cron-expression" ||
+				flag.Name == "backup-starting-deadline-minutes" || flag.Name == "backup-repo-name" ||
+				flag.Name == "pitr-enabled" {
+				flags = append(flags, flag)
+			}
 		})
 	}
 
-	// if the flag is set by user, use the flag value
-	for _, flag := range flags {
-		switch flag.Name {
-		case "backup-enabled":
-			o.BackupConfig.Enabled = &o.BackupEnabled
-		case "backup-retention-period":
-			o.BackupConfig.RetentionPeriod = dpv1alpha1.RetentionPeriod(o.BackupRetentionPeriod)
-		case "backup-method":
-			o.BackupConfig.Method = o.BackupMethod
-		case "backup-cron-expression":
-			if _, err := cron.ParseStandard(o.BackupCronExpression); err != nil {
-				return fmt.Errorf("invalid cron expression: %s, please see https://en.wikipedia.org/wiki/Cron", o.BackupCronExpression)
+	// must set backup method when set backup config in cli
+	if len(flags) > 0 {
+		if o.BackupConfig == nil {
+			o.BackupConfig = &appsv1alpha1.ClusterBackup{}
+		}
+
+		// get default backup method and all backup methods
+		defaultBackupMethod, backupMethodsMap, err := getBackupMethodsFromBackupPolicyTemplates(o.Dynamic, o.ClusterDefRef)
+		if err != nil {
+			return err
+		}
+
+		// if backup method is empty in backup config, use the default backup method
+		if o.BackupConfig.Method == "" {
+			o.BackupConfig.Method = defaultBackupMethod
+		}
+
+		// if the flag is set by user, use the flag value
+		for _, flag := range flags {
+			switch flag.Name {
+			case "backup-enabled":
+				o.BackupConfig.Enabled = &o.BackupEnabled
+			case "backup-retention-period":
+				o.BackupConfig.RetentionPeriod = dpv1alpha1.RetentionPeriod(o.BackupRetentionPeriod)
+			case "backup-method":
+				if _, ok := backupMethodsMap[o.BackupMethod]; !ok {
+					return fmt.Errorf("backup method %s is not supported, please view supported backup methods by \"kbcli cd describe %s\"", o.BackupMethod, o.ClusterDefRef)
+				}
+				o.BackupConfig.Method = o.BackupMethod
+			case "backup-cron-expression":
+				if _, err := cron.ParseStandard(o.BackupCronExpression); err != nil {
+					return fmt.Errorf("invalid cron expression: %s, please see https://en.wikipedia.org/wiki/Cron", o.BackupCronExpression)
+				}
+				o.BackupConfig.CronExpression = o.BackupCronExpression
+			case "backup-starting-deadline-minutes":
+				o.BackupConfig.StartingDeadlineMinutes = &o.BackupStartingDeadlineMinutes
+			case "backup-repo-name":
+				o.BackupConfig.RepoName = o.BackupRepoName
+			case "pitr-enabled":
+				o.BackupConfig.PITREnabled = &o.BackupPITREnabled
 			}
-			o.BackupConfig.CronExpression = o.BackupCronExpression
-		case "backup-starting-deadline-minutes":
-			o.BackupConfig.StartingDeadlineMinutes = &o.BackupStartingDeadlineMinutes
-		case "backup-repo-name":
-			o.BackupConfig.RepoName = o.BackupRepoName
-		case "pitr-enabled":
-			o.BackupConfig.PITREnabled = &o.BackupPITREnabled
 		}
 	}
 
 	return nil
 }
 
+// get backup methods from backup policy template
+// if method's snapshotVolumes is true, use the method as default method
+func getBackupMethodsFromBackupPolicyTemplates(dynamic dynamic.Interface, clusterDefRef string) (string, map[string]struct{}, error) {
+	var backupPolicyTemplates []appsv1alpha1.BackupPolicyTemplate
+	var defaultBackupPolicyTemplate appsv1alpha1.BackupPolicyTemplate
+
+	obj, err := dynamic.Resource(types.BackupPolicyTemplateGVR()).List(context.TODO(), metav1.ListOptions{
+		LabelSelector: fmt.Sprintf("%s=%s", constant.ClusterDefLabelKey, clusterDefRef),
+	})
+	if err != nil {
+		return "", nil, err
+	}
+	for _, item := range obj.Items {
+		var backupPolicyTemplate appsv1alpha1.BackupPolicyTemplate
+		err = runtime.DefaultUnstructuredConverter.FromUnstructured(item.Object, &backupPolicyTemplate)
+		if err != nil {
+			return "", nil, err
+		}
+		backupPolicyTemplates = append(backupPolicyTemplates, backupPolicyTemplate)
+	}
+
+	if len(backupPolicyTemplates) == 0 {
+		return "", nil, fmt.Errorf("failed to find backup policy template for cluster definition %s", clusterDefRef)
+	}
+	// if there is only one backup policy template, use it as default backup policy template
+	if len(backupPolicyTemplates) == 1 {
+		defaultBackupPolicyTemplate = backupPolicyTemplates[0]
+	}
+	for _, backupPolicyTemplate := range backupPolicyTemplates {
+		if backupPolicyTemplate.Annotations[dptypes.DefaultBackupPolicyTemplateAnnotationKey] == annotationTrueValue {
+			defaultBackupPolicyTemplate = backupPolicyTemplate
+			break
+		}
+	}
+
+	var defaultBackupMethod string
+	var backupMethodsMap = make(map[string]struct{})
+	for _, policy := range defaultBackupPolicyTemplate.Spec.BackupPolicies {
+		for _, method := range policy.BackupMethods {
+			if boolptr.IsSetToTrue(method.SnapshotVolumes) {
+				defaultBackupMethod = method.Name
+			}
+			backupMethodsMap[method.Name] = struct{}{}
+		}
+	}
+	if defaultBackupMethod == "" {
+		return "", nil, fmt.Errorf("failed to find default backup method which snapshotVolumes is true, please check backup policy template for cluster definition %s", clusterDefRef)
+	}
+	return defaultBackupMethod, backupMethodsMap, nil
+}
+
 // parse the cluster component spec
 // compatible with old file format that only specifies the components
 func parseClusterComponentSpec(compByte []byte) ([]appsv1alpha1.ClusterComponentSpec, error) {

internal/cli/cmd/cluster/create_test.go

@@ -35,11 +35,13 @@ import (
 	"k8s.io/cli-runtime/pkg/genericclioptions"
 
 	appsv1alpha1 "github.com/apecloud/kubeblocks/apis/apps/v1alpha1"
+	"github.com/apecloud/kubeblocks/apis/dataprotection/v1alpha1"
 	"github.com/apecloud/kubeblocks/internal/class"
 	"github.com/apecloud/kubeblocks/internal/cli/cluster"
 	"github.com/apecloud/kubeblocks/internal/cli/testing"
 	"github.com/apecloud/kubeblocks/internal/cli/types"
 	"github.com/apecloud/kubeblocks/internal/cli/util"
+	"github.com/apecloud/kubeblocks/internal/dataprotection/utils/boolptr"
 	viper "github.com/apecloud/kubeblocks/internal/viperx"
 )
 
@@ -467,18 +469,47 @@ var _ = Describe("create", func() {
 	})
 
 	It("test build backup config", func() {
+		backupPolicyTemplate := testing.FakeBackupPolicyTemplate("backupPolicyTemplate-test", testing.ClusterDefName)
+		backupPolicy := appsv1alpha1.BackupPolicy{
+			BackupMethods: []v1alpha1.BackupMethod{
+				{
+					Name:            "volume-snapshot",
+					SnapshotVolumes: boolptr.True(),
+				},
+				{
+					Name: "xtrabackup",
+				},
+			},
+		}
+		backupPolicyTemplate.Spec.BackupPolicies = append(backupPolicyTemplate.Spec.BackupPolicies, backupPolicy)
+		dynamic := testing.FakeDynamicClient(backupPolicyTemplate)
+
 		o := &CreateOptions{}
 		o.Cmd = NewCreateCmd(o.Factory, o.IOStreams)
+		o.Dynamic = dynamic
+		o.ClusterDefRef = testing.ClusterDefName
 		cluster := testing.FakeCluster("clusterName", testing.Namespace)
 
 		By("test backup is not set")
 		Expect(o.buildBackupConfig(cluster)).To(Succeed())
 
-		By("test backup is with snapshot method")
-		o.BackupMethod = "snapshot"
-		Expect(o.Cmd.Flags().Set("backup-method", "snapshot")).To(Succeed())
+		By("test backup enable")
+		o.BackupEnabled = true
+		Expect(o.Cmd.Flags().Set("backup-enabled", "true")).To(Succeed())
+		Expect(o.buildBackupConfig(cluster)).To(Succeed())
+		Expect(*o.BackupConfig.Enabled).Should(BeTrue())
+		Expect(o.BackupConfig.Method).Should(Equal("volume-snapshot"))
+
+		By("test backup with invalid method")
+		o.BackupMethod = "invalid-method"
+		Expect(o.Cmd.Flags().Set("backup-method", "invalid-method")).To(Succeed())
+		Expect(o.buildBackupConfig(cluster)).To(HaveOccurred())
+
+		By("test backup with xtrabackup method")
+		o.BackupMethod = "xtrabackup"
+		Expect(o.Cmd.Flags().Set("backup-method", "xtrabackup")).To(Succeed())
 		Expect(o.buildBackupConfig(cluster)).To(Succeed())
-		Expect(o.BackupConfig.Method).Should(Equal("snapshot"))
+		Expect(o.BackupConfig.Method).Should(Equal("xtrabackup"))
 
 		By("test backup is with wrong cron expression")
 		o.BackupCronExpression = "wrong-cron-expression"

internal/cli/cmd/cluster/update.go

@@ -35,6 +35,7 @@ import (
 	"github.com/spf13/cobra"
 	"github.com/spf13/pflag"
 	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/cli-runtime/pkg/genericclioptions"
@@ -49,7 +50,10 @@ import (
 	"github.com/apecloud/kubeblocks/internal/cli/types"
 	"github.com/apecloud/kubeblocks/internal/cli/util"
 	cfgcore "github.com/apecloud/kubeblocks/internal/configuration/core"
+	"github.com/apecloud/kubeblocks/internal/constant"
 	"github.com/apecloud/kubeblocks/internal/controller/configuration"
+	dptypes "github.com/apecloud/kubeblocks/internal/dataprotection/types"
+	"github.com/apecloud/kubeblocks/internal/dataprotection/utils/boolptr"
 	"github.com/apecloud/kubeblocks/internal/gotemplate"
 )
 
@@ -257,6 +261,20 @@ func (o *updateOptions) buildPatch(flags []*pflag.Flag) error {
 	}
 
 	if o.cluster != nil {
+		// if update the backup config, the backup method must have value
+		if o.cluster.Spec.Backup != nil {
+			defaultBackupMethod, backupMethodMap, err := o.getBackupMethodsFromBackupPolicy()
+			if err != nil {
+				return err
+			}
+			if o.cluster.Spec.Backup.Method == "" {
+				o.cluster.Spec.Backup.Method = defaultBackupMethod
+			}
+			if _, ok := backupMethodMap[o.cluster.Spec.Backup.Method]; !ok {
+				return fmt.Errorf("backup method %s is not supported, please view the supported backup methods by `kbcli cd describe %s`", o.cluster.Spec.Backup.Method, o.cluster.Spec.ClusterDefRef)
+			}
+		}
+
 		data, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&o.cluster.Spec)
 		if err != nil {
 			return err
@@ -311,6 +329,9 @@ func (o *updateOptions) buildBackup(field string, val string) error {
 		}
 		o.cluster = c
 	}
+	if o.cluster.Spec.Backup == nil {
+		o.cluster.Spec.Backup = &appsv1alpha1.ClusterBackup{}
+	}
 
 	switch field {
 	case "enabled":
@@ -595,3 +616,47 @@ func (o *updateOptions) updateBackupPitrEnabled(val string) error {
 	o.cluster.Spec.Backup.PITREnabled = &boolVal
 	return nil
 }
+
+// get backup methods from cluster's backup policy
+// if method's snapshotVolumes is true, use the method as the default backup method
+func (o *updateOptions) getBackupMethodsFromBackupPolicy() (string, map[string]struct{}, error) {
+	if o.cluster == nil {
+		return "", nil, fmt.Errorf("cluster is nil")
+	}
+
+	var backupPolicy []dpv1alpha1.BackupPolicy
+	obj, err := o.dynamic.Resource(types.BackupPolicyGVR()).Namespace(o.namespace).List(context.Background(), metav1.ListOptions{
+		LabelSelector: fmt.Sprintf("%s=%s", constant.AppInstanceLabelKey, o.cluster.Name),
+	})
+	if err != nil {
+		return "", nil, err
+	}
+	for _, item := range obj.Items {
+		var bp dpv1alpha1.BackupPolicy
+		if err = runtime.DefaultUnstructuredConverter.FromUnstructured(item.Object, &bp); err != nil {
+			return "", nil, err
+		}
+		backupPolicy = append(backupPolicy, bp)
+	}
+
+	var defaultBackupMethod string
+	var backupMethodsMap = make(map[string]struct{})
+	for _, policy := range backupPolicy {
+		if policy.Annotations[dptypes.DefaultBackupPolicyAnnotationKey] != annotationTrueValue {
+			continue
+		}
+		if policy.Status.Phase != dpv1alpha1.AvailablePhase {
+			continue
+		}
+		for _, method := range policy.Spec.BackupMethods {
+			if boolptr.IsSetToTrue(method.SnapshotVolumes) {
+				defaultBackupMethod = method.Name
+			}
+			backupMethodsMap[method.Name] = struct{}{}
+		}
+	}
+	if defaultBackupMethod == "" {
+		return "", nil, fmt.Errorf("failed to find default backup method which snapshotVolumes is true, please check cluster's backup policy")
+	}
+	return defaultBackupMethod, backupMethodsMap, nil
+}