Merge pull request #3 from cybozu-go/update-RBDPVCBackup-status

update RBDPVCBackup.status

Makefile

@@ -2,7 +2,7 @@
 # Image URL to use all building/pushing image targets
 IMG ?= controller:latest
 # ENVTEST_K8S_VERSION refers to the version of kubebuilder assets to be downloaded by envtest binary.
-ENVTEST_K8S_VERSION = 1.28.0
+ENVTEST_K8S_VERSION = 1.28.3
 
 # Get the currently used golang install path (in GOPATH/bin, unless GOBIN is set)
 ifeq (,$(shell go env GOBIN))

api/v1/rbdpvcbackup_types.go

@@ -30,15 +30,15 @@ type RBDPVCBackupStatus struct {
 	CreatedAt metav1.Time `json:"createdAt,omitempty"`
 
 	// 'conditions' specifies current backup conditions
-	// +kubebuilder:validation:Enum=Creating;Bound;Failed;Deleting
-	Conditions string `json:"conditions,omitempty"`
+	Conditions []metav1.Condition `json:"conditions,omitempty"`
 }
 
 const (
-	RBDPVCBackupConditionsCreating = "Creating"
-	RBDPVCBackupConditionsBound    = "Bound"
-	RBDPVCBackupConditionsFailed   = "Failed"
-	RBDPVCBackupConditionsDeleting = "Deleting"
+	ConditionReadyToUse = "ReadyToUse"
+
+	// Reasons for ConditionReadyToUse
+	ReasonNone                 = "NoProblem"
+	ReasonFailedToCreateBackup = "FailedToCreateBackup"
 )
 
 //+kubebuilder:object:root=true

config/crd/bases/backup.cybozu.com_rbdpvcbackups.yaml

@@ -46,12 +46,73 @@ spec:
             properties:
               conditions:
                 description: '''conditions'' specifies current backup conditions'
-                enum:
-                - Creating
-                - Bound
-                - Failed
-                - Deleting
-                type: string
+                items:
+                  description: "Condition contains details for one aspect of the current
+                    state of this API Resource. --- This struct is intended for direct
+                    use as an array at the field path .status.conditions.  For example,
+                    \n type FooStatus struct{ // Represents the observations of a
+                    foo's current state. // Known .status.conditions.type are: \"Available\",
+                    \"Progressing\", and \"Degraded\" // +patchMergeKey=type // +patchStrategy=merge
+                    // +listType=map // +listMapKey=type Conditions []metav1.Condition
+                    `json:\"conditions,omitempty\" patchStrategy:\"merge\" patchMergeKey:\"type\"
+                    protobuf:\"bytes,1,rep,name=conditions\"` \n // other fields }"
+                  properties:
+                    lastTransitionTime:
+                      description: lastTransitionTime is the last time the condition
+                        transitioned from one status to another. This should be when
+                        the underlying condition changed.  If that is not known, then
+                        using the time when the API field changed is acceptable.
+                      format: date-time
+                      type: string
+                    message:
+                      description: message is a human readable message indicating
+                        details about the transition. This may be an empty string.
+                      maxLength: 32768
+                      type: string
+                    observedGeneration:
+                      description: observedGeneration represents the .metadata.generation
+                        that the condition was set based upon. For instance, if .metadata.generation
+                        is currently 12, but the .status.conditions[x].observedGeneration
+                        is 9, the condition is out of date with respect to the current
+                        state of the instance.
+                      format: int64
+                      minimum: 0
+                      type: integer
+                    reason:
+                      description: reason contains a programmatic identifier indicating
+                        the reason for the condition's last transition. Producers
+                        of specific condition types may define expected values and
+                        meanings for this field, and whether the values are considered
+                        a guaranteed API. The value should be a CamelCase string.
+                        This field may not be empty.
+                      maxLength: 1024
+                      minLength: 1
+                      pattern: ^[A-Za-z]([A-Za-z0-9_,:]*[A-Za-z0-9_])?$
+                      type: string
+                    status:
+                      description: status of the condition, one of True, False, Unknown.
+                      enum:
+                      - "True"
+                      - "False"
+                      - Unknown
+                      type: string
+                    type:
+                      description: type of condition in CamelCase or in foo.example.com/CamelCase.
+                        --- Many .condition.type values are consistent across resources
+                        like Available, but because arbitrary conditions can be useful
+                        (see .node.status.conditions), the ability to deconflict is
+                        important. The regex it matches is (dns1123SubdomainFmt/)?(qualifiedNameFmt)
+                      maxLength: 316
+                      pattern: ^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*/)?(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])$
+                      type: string
+                  required:
+                  - lastTransitionTime
+                  - message
+                  - reason
+                  - status
+                  - type
+                  type: object
+                type: array
               createdAt:
                 description: '''createdAt'' specifies the creation date and time'
                 format: date-time

docs/design.md

@@ -71,7 +71,11 @@ kind: RBDPVCBackup
 metadata:
   name: <RBDPVCBackup resource name>
 spec:
+  # The name of the backup target PVC
   pvc: <target PVC name>
+status:
+  conditions:
+    # The corresponding backup data is ready to use if `status` is "True"
+    - type: "ReadyToUse"
+      status: "True"
 ```
-
-- `<target PVC name>` is the name of the backup target PVC.

e2e/suite_test.go

@@ -16,6 +16,9 @@ import (
 	. "github.com/onsi/gomega"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/meta"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
 	"sigs.k8s.io/yaml"
 )
 
@@ -308,7 +311,8 @@ var _ = Describe("rbd backup system", func() {
 		var backup backupv1.RBDPVCBackup
 		err = yaml.Unmarshal(stdout, &backup)
 		Expect(err).NotTo(HaveOccurred())
-		Expect(backup.Status.Conditions).To(Equal(backupv1.RBDPVCBackupConditionsBound))
+		Expect(meta.FindStatusCondition(backup.Status.Conditions, backupv1.ConditionReadyToUse).Status).
+			To(Equal(metav1.ConditionTrue))
 	})
 
 	It("should delete RBDPVCBackup resource", func() {

internal/controller/rbdpvcbackup_controller.go

@@ -11,6 +11,7 @@ import (
 
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
@@ -96,11 +97,11 @@ func executeCommandImpl(command []string, input io.Reader) ([]byte, error) {
 
 var executeCommand = executeCommandImpl
 
-func (r *RBDPVCBackupReconciler) updateConditions(ctx context.Context, backup *backupv1.RBDPVCBackup, conditions string) error {
-	backup.Status.Conditions = conditions
+func (r *RBDPVCBackupReconciler) updateStatus(ctx context.Context, backup *backupv1.RBDPVCBackup, condition metav1.Condition) error {
+	meta.SetStatusCondition(&backup.Status.Conditions, condition)
 	err := r.Status().Update(ctx, backup)
 	if err != nil {
-		logger.Error("failed to update status", "conditions", backup.Status.Conditions, "error", err)
+		logger.Error("failed to update status", "status", backup.Status, "error", err)
 		return err
 	}
 	return nil
@@ -114,7 +115,7 @@ func (r *RBDPVCBackupReconciler) createRBDSnapshot(ctx context.Context, poolName
 		out, err := executeCommand(command, nil)
 		if err != nil {
 			logger.Info("failed to run `rbd snap ls`", "poolName", poolName, "imageName", imageName, "error", err)
-			err2 := r.updateConditions(ctx, backup, backupv1.RBDPVCBackupConditionsFailed)
+			err2 := r.updateStatus(ctx, backup, metav1.Condition{Type: backupv1.ConditionReadyToUse, Status: metav1.ConditionFalse, Reason: backupv1.ReasonFailedToCreateBackup})
 			if err2 != nil {
 				return ctrl.Result{}, err2
 			}
@@ -124,7 +125,7 @@ func (r *RBDPVCBackupReconciler) createRBDSnapshot(ctx context.Context, poolName
 		err = json.Unmarshal(out, &snapshots)
 		if err != nil {
 			logger.Error("failed to unmarshal json", "json", out, "error", err)
-			err2 := r.updateConditions(ctx, backup, backupv1.RBDPVCBackupConditionsFailed)
+			err2 := r.updateStatus(ctx, backup, metav1.Condition{Type: backupv1.ConditionReadyToUse, Status: metav1.ConditionFalse, Reason: backupv1.ReasonFailedToCreateBackup})
 			if err2 != nil {
 				return ctrl.Result{}, err2
 			}
@@ -138,8 +139,8 @@ func (r *RBDPVCBackupReconciler) createRBDSnapshot(ctx context.Context, poolName
 			}
 		}
 		if !existSnapshot {
-			logger.Info("snapshot not exists", "snapshotName", backup.Name)
-			err2 := r.updateConditions(ctx, backup, backupv1.RBDPVCBackupConditionsFailed)
+			logger.Info("snapshot does not exists", "snapshotName", backup.Name)
+			err2 := r.updateStatus(ctx, backup, metav1.Condition{Type: backupv1.ConditionReadyToUse, Status: metav1.ConditionFalse, Reason: backupv1.ReasonFailedToCreateBackup})
 			if err2 != nil {
 				return ctrl.Result{}, err2
 			}
@@ -184,6 +185,10 @@ func (r *RBDPVCBackupReconciler) Reconcile(ctx context.Context, req ctrl.Request
 	err = r.Get(ctx, types.NamespacedName{Namespace: pvcNamespace, Name: pvcName}, &pvc)
 	if err != nil {
 		logger.Error("failed to get PVC", "namespace", pvcNamespace, "name", pvcName, "error", err)
+		err2 := r.updateStatus(ctx, &backup, metav1.Condition{Type: backupv1.ConditionReadyToUse, Status: metav1.ConditionFalse, Reason: backupv1.ReasonFailedToCreateBackup})
+		if err2 != nil {
+			return ctrl.Result{}, err2
+		}
 		if errors.IsNotFound(err) {
 			if !backup.ObjectMeta.DeletionTimestamp.IsZero() {
 				if controllerutil.ContainsFinalizer(&backup, RBDPVCBackupFinalizerName) {
@@ -202,12 +207,17 @@ func (r *RBDPVCBackupReconciler) Reconcile(ctx context.Context, req ctrl.Request
 	}
 
 	if pvc.Status.Phase != corev1.ClaimBound {
+		err := r.updateStatus(ctx, &backup, metav1.Condition{Type: backupv1.ConditionReadyToUse, Status: metav1.ConditionFalse, Reason: backupv1.ReasonFailedToCreateBackup})
+		if err != nil {
+			return ctrl.Result{}, err
+		}
+
 		if pvc.Status.Phase == corev1.ClaimPending {
 			logger.Info("waiting for PVC bound.")
 			return ctrl.Result{Requeue: true}, nil
 		} else {
-			logger.Error("failed to bound PVC", "status.phase", pvc.Status.Phase)
-			return ctrl.Result{}, fmt.Errorf("failed to bound PVC (status.phase: %s)", pvc.Status.Phase)
+			logger.Error("PVC phase is neither bound nor pending", "status.phase", pvc.Status.Phase)
+			return ctrl.Result{}, fmt.Errorf("PVC phase is neither bound nor pending (status.phase: %s)", pvc.Status.Phase)
 		}
 	}
 
@@ -216,6 +226,11 @@ func (r *RBDPVCBackupReconciler) Reconcile(ctx context.Context, req ctrl.Request
 	err = r.Get(ctx, types.NamespacedName{Namespace: req.NamespacedName.Namespace, Name: pvName}, &pv)
 	if err != nil {
 		logger.Error("failed to get PV", "namespace", req.NamespacedName.Namespace, "name", pvName, "error", err)
+		err2 := r.updateStatus(ctx, &backup, metav1.Condition{Type: backupv1.ConditionReadyToUse, Status: metav1.ConditionFalse, Reason: backupv1.ReasonFailedToCreateBackup})
+		if err2 != nil {
+			return ctrl.Result{}, err2
+		}
+
 		return ctrl.Result{}, err
 	}
 
@@ -229,14 +244,6 @@ func (r *RBDPVCBackupReconciler) Reconcile(ctx context.Context, req ctrl.Request
 	}
 
 	if !backup.ObjectMeta.DeletionTimestamp.IsZero() {
-		if backup.Status.Conditions != backupv1.RBDPVCBackupConditionsDeleting {
-			err = r.updateConditions(ctx, &backup, backupv1.RBDPVCBackupConditionsDeleting)
-			if err != nil {
-				return ctrl.Result{}, err
-			}
-			return ctrl.Result{Requeue: true}, nil
-		}
-
 		if controllerutil.ContainsFinalizer(&backup, RBDPVCBackupFinalizerName) {
 			command := []string{"rbd", "snap", "rm", poolName + "/" + imageName + "@" + backup.Name}
 			_, err = executeCommand(command, nil)
@@ -270,31 +277,25 @@ func (r *RBDPVCBackupReconciler) Reconcile(ctx context.Context, req ctrl.Request
 			logger.Error("failed to add finalizer", "finalizer", RBDPVCBackupFinalizerName, "error", err)
 			return ctrl.Result{}, err
 		}
-		return ctrl.Result{Requeue: true}, nil
-	}
-
-	if backup.Status.Conditions == backupv1.RBDPVCBackupConditionsBound || backup.Status.Conditions == backupv1.RBDPVCBackupConditionsDeleting {
-		return ctrl.Result{}, nil
-	}
-
-	if backup.Status.Conditions != backupv1.RBDPVCBackupConditionsCreating {
-		err := r.updateConditions(ctx, &backup, backupv1.RBDPVCBackupConditionsCreating)
+		err := r.updateStatus(ctx, &backup, metav1.Condition{Type: backupv1.ConditionReadyToUse, Status: metav1.ConditionFalse, Reason: backupv1.ReasonNone})
 		if err != nil {
 			return ctrl.Result{}, err
 		}
 		return ctrl.Result{Requeue: true}, nil
 	}
 
+	if meta.FindStatusCondition(backup.Status.Conditions, backupv1.ConditionReadyToUse).Status == metav1.ConditionTrue {
+		return ctrl.Result{}, nil
+	}
+
 	result, err := r.createRBDSnapshot(ctx, poolName, imageName, &backup)
 	if err != nil {
 		return result, err
 	}
 
 	backup.Status.CreatedAt = metav1.NewTime(time.Now())
-	backup.Status.Conditions = backupv1.RBDPVCBackupConditionsBound
-	err = r.Status().Update(ctx, &backup)
+	err = r.updateStatus(ctx, &backup, metav1.Condition{Type: backupv1.ConditionReadyToUse, Status: metav1.ConditionTrue, Reason: backupv1.ReasonNone})
 	if err != nil {
-		logger.Error("failed to update status", "createdAt", backup.Status.CreatedAt, "conditions", backup.Status.Conditions, "error", err)
 		return ctrl.Result{}, err
 	}