From 5ba044f1bf90e0eb371f695969506e97666533c1 Mon Sep 17 00:00:00 2001 From: Satoru Takeuchi Date: Wed, 3 Apr 2024 08:08:56 +0000 Subject: [PATCH] update RBDPVCBackup.status Update RBDPVCBackup status to follow the offical convention: ref. https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#typical-status-properties Signed-off-by: Satoru Takeuchi --- Makefile | 2 +- api/v1/rbdpvcbackup_types.go | 12 +-- api/v1/zz_generated.deepcopy.go | 8 ++ .../backup.cybozu.com_rbdpvcbackups.yaml | 73 +++++++++++++-- docs/design.md | 8 +- e2e/suite_test.go | 6 +- .../controller/rbdpvcbackup_controller.go | 59 +++++++------ .../rbdpvcbackup_controller_test.go | 88 +++++++------------ 8 files changed, 153 insertions(+), 103 deletions(-) diff --git a/Makefile b/Makefile index 08581a9a..a63c2874 100644 --- a/Makefile +++ b/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)) diff --git a/api/v1/rbdpvcbackup_types.go b/api/v1/rbdpvcbackup_types.go index 74ced2a5..322d8167 100644 --- a/api/v1/rbdpvcbackup_types.go +++ b/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 diff --git a/api/v1/zz_generated.deepcopy.go b/api/v1/zz_generated.deepcopy.go index a34f74f2..8a491a9b 100644 --- a/api/v1/zz_generated.deepcopy.go +++ b/api/v1/zz_generated.deepcopy.go @@ -5,6 +5,7 @@ package v1 import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" runtime "k8s.io/apimachinery/pkg/runtime" ) @@ -86,6 +87,13 @@ func (in *RBDPVCBackupSpec) DeepCopy() *RBDPVCBackupSpec { func (in *RBDPVCBackupStatus) DeepCopyInto(out *RBDPVCBackupStatus) { *out = *in in.CreatedAt.DeepCopyInto(&out.CreatedAt) + if in.Conditions != nil { + in, out := &in.Conditions, &out.Conditions + *out = make([]metav1.Condition, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new RBDPVCBackupStatus. diff --git a/config/crd/bases/backup.cybozu.com_rbdpvcbackups.yaml b/config/crd/bases/backup.cybozu.com_rbdpvcbackups.yaml index 7b188734..2c4ffec7 100644 --- a/config/crd/bases/backup.cybozu.com_rbdpvcbackups.yaml +++ b/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 diff --git a/docs/design.md b/docs/design.md index e113a0e5..14f15de9 100644 --- a/docs/design.md +++ b/docs/design.md @@ -71,7 +71,11 @@ kind: RBDPVCBackup metadata: name: spec: + # The name of the backup target PVC pvc: +status: + conditions: + # The corresponding backup data is ready to use if `status` is "True" + - type: "ReadyToUse" + status: "True" ``` - -- `` is the name of the backup target PVC. diff --git a/e2e/suite_test.go b/e2e/suite_test.go index 9b62d41d..5738196a 100644 --- a/e2e/suite_test.go +++ b/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() { diff --git a/internal/controller/rbdpvcbackup_controller.go b/internal/controller/rbdpvcbackup_controller.go index 767bafa6..a4b42729 100644 --- a/internal/controller/rbdpvcbackup_controller.go +++ b/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 } diff --git a/internal/controller/rbdpvcbackup_controller_test.go b/internal/controller/rbdpvcbackup_controller_test.go index 61cbae17..0e85524e 100644 --- a/internal/controller/rbdpvcbackup_controller_test.go +++ b/internal/controller/rbdpvcbackup_controller_test.go @@ -11,6 +11,7 @@ import ( . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -19,13 +20,6 @@ import ( ) func mockExecuteCommand(command []string, input io.Reader) ([]byte, error) { - if command[0] == "rbd" && command[1] == "snap" && (command[2] == "create" || command[2] == "rm") { - // The actual `rbd snap create` or `rbd snap rm` is momentary, - // but to check the transition of the `status.conditions` of the RBDPVCBackups resource, - // a DefaultEventuallyPollingInterval (1 second) or longer sleep is set to. - time.Sleep(1500 * time.Millisecond) - } - return nil, nil } @@ -65,7 +59,7 @@ var _ = Describe("RBDPVCBackup controller", func() { Expect(<-errCh).NotTo(HaveOccurred()) }) - It("should set/unset finalizer and status fields properly on the resource creation/deletion", func() { + It("should be ready to use", func() { ctx := context.Background() ns := createNamespace() @@ -153,10 +147,6 @@ var _ = Describe("RBDPVCBackup controller", func() { return fmt.Errorf("finalizer does not set yet") } - if backup.Status.Conditions != backupv1.RBDPVCBackupConditionsCreating { - return fmt.Errorf("status.conditions does not set \"Creating\" yet (status.conditions: %s)", backup.Status.Conditions) - } - return nil }).Should(Succeed()) @@ -170,13 +160,8 @@ var _ = Describe("RBDPVCBackup controller", func() { return err } - if backup.Status.Conditions != backupv1.RBDPVCBackupConditionsBound { - return fmt.Errorf("status.conditions does not set \"Bound\" yet") - } - - timeDiff := time.Since(backup.Status.CreatedAt.Time).Seconds() - if timeDiff > 5 { - return fmt.Errorf("invalid status.createdAt (time difference from createdAt(%f sec) exceeds 5 seconds.)", timeDiff) + if meta.FindStatusCondition(backup.Status.Conditions, backupv1.ConditionReadyToUse).Status != metav1.ConditionTrue { + return fmt.Errorf("not ready to use yet") } return nil @@ -185,23 +170,6 @@ var _ = Describe("RBDPVCBackup controller", func() { err = k8sClient.Delete(ctx, &backup) Expect(err).NotTo(HaveOccurred()) - Eventually(func() error { - namespacedName := types.NamespacedName{ - Namespace: ns, - Name: backup.Name, - } - err = k8sClient.Get(ctx, namespacedName, &backup) - if err != nil { - return err - } - - if backup.Status.Conditions != backupv1.RBDPVCBackupConditionsDeleting { - return fmt.Errorf("status.conditions does not set \"Deleting\" yet (status.conditions: %s)", backup.Status.Conditions) - } - - return nil - }).Should(Succeed()) - Eventually(func() error { namespacedName := types.NamespacedName{ Namespace: ns, @@ -214,11 +182,11 @@ var _ = Describe("RBDPVCBackup controller", func() { if err != nil { return err } - return fmt.Errorf("\"%s\" does not deleted yet", backup.Name) + return fmt.Errorf("\"%s\" is not deleted yet", backup.Name) }).Should(Succeed()) }) - It("should remain \"Bound\" in RBDPVCBackup resource even if the PVC is not bound", func() { + It("should still be ready to use even if the PVC lost", func() { ctx := context.Background() ns := createNamespace() @@ -306,10 +274,6 @@ var _ = Describe("RBDPVCBackup controller", func() { return fmt.Errorf("finalizer does not set yet") } - if backup.Status.Conditions != backupv1.RBDPVCBackupConditionsCreating { - return fmt.Errorf("status.conditions does not set \"Creating\" yet (status.conditions: %s)", backup.Status.Conditions) - } - return nil }).Should(Succeed()) @@ -323,19 +287,14 @@ var _ = Describe("RBDPVCBackup controller", func() { return err } - if backup.Status.Conditions != backupv1.RBDPVCBackupConditionsBound { - return fmt.Errorf("status.conditions does not set \"Bound\" yet") - } - - timeDiff := time.Since(backup.Status.CreatedAt.Time).Seconds() - if timeDiff > 5 { - return fmt.Errorf("invalid status.createdAt (time difference from createdAt(%f sec) exceeds 5 seconds.)", timeDiff) + if meta.FindStatusCondition(backup.Status.Conditions, backupv1.ConditionReadyToUse).Status != metav1.ConditionTrue { + return fmt.Errorf("not ready to use yet") } return nil }).Should(Succeed()) - pvc.Status.Phase = corev1.ClaimLost // simulate broken PVC + pvc.Status.Phase = corev1.ClaimLost // simulate lost PVC err = k8sClient.Status().Update(ctx, &pvc) Expect(err).NotTo(HaveOccurred()) @@ -349,15 +308,20 @@ var _ = Describe("RBDPVCBackup controller", func() { return err } - if backup.Status.Conditions != backupv1.RBDPVCBackupConditionsBound { - return fmt.Errorf("status.conditions changed from \"Bound\"") + condition := meta.FindStatusCondition(backup.Status.Conditions, backupv1.ConditionReadyToUse) + if condition == nil { + return fmt.Errorf("condition is not set") + } + + if condition.Status != metav1.ConditionTrue { + return fmt.Errorf("should keep ready to use") } return nil }).Should(Succeed()) }) - It("should not be \"Bound\" conditions in RBDPVCBackup resource if the status.phase of the PVC is in the lost state from the beginning", func() { + It("should not be ready to use if the PVC is the lost state from the beginning", func() { ctx := context.Background() ns := createNamespace() @@ -434,15 +398,19 @@ var _ = Describe("RBDPVCBackup controller", func() { return err } - if backup.Status.Conditions == backupv1.RBDPVCBackupConditionsBound { - return fmt.Errorf("status.conditions should not be \"Bound\"") + condition := meta.FindStatusCondition(backup.Status.Conditions, backupv1.ConditionReadyToUse) + if condition == nil { + return fmt.Errorf("condition is not set") + } + if !(condition.Status == metav1.ConditionFalse && condition.Reason != backupv1.ReasonNone) { + return fmt.Errorf("should not be ready to use") } return nil }).Should(Succeed()) }) - It("should not be \"Bound\" conditions in RBDPVCBackup resource if specified non-existent PVC name", func() { + It("should not be ready to use if specified non-existent PVC name", func() { ctx := context.Background() ns := createNamespace() @@ -468,8 +436,12 @@ var _ = Describe("RBDPVCBackup controller", func() { return err } - if backup.Status.Conditions == backupv1.RBDPVCBackupConditionsBound { - return fmt.Errorf("status.conditions should not be \"Bound\"") + condition := meta.FindStatusCondition(backup.Status.Conditions, backupv1.ConditionReadyToUse) + if condition == nil { + return fmt.Errorf("condition is not set") + } + if !(condition.Status == metav1.ConditionFalse && condition.Reason != backupv1.ReasonNone) { + return fmt.Errorf("should not be ready to use") } return nil