feat: add LastBackupSucceeded Cluster condition

This patch introduces a new Cluster condition, LastBackupSucceeded,
with the following semantic:
- true if the last backup succeeded
- false if the last backup failed
- not set if no backups were run yet

This allows waiting for at least a backup to be successful upon cluster startup using kubectl:
```
$ kubectl -n <namespace> --for=condition=LastBackupSucceeded=true cluster/<cluster-name>
cluster.postgresql.k8s.enterprisedb.io/<cluster-name> condition met
```

It also allows to easily know whether a Cluster has a recent backup,
by just inspecting the Cluster status.

E2Es were also updated accordingly.

Here is an example of conditions in case of success:
```
status:
  conditions:
  - reason: Continuous Archiving is Working
     status: "True"
     type: ContinuousArchiving
  - reason: Last backup succeeded
     status: "True"
     type: LastBackupSucceeded
```

And here in case of failure:
```
status:
  conditions:
  - reason: Continuous Archiving is Working
     status: "True"
     type: ContinuousArchiving
  - reason: Last backup failed
     status: "False"
     type: LastBackupSucceeded
```

Co-authored-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
Co-authored-by: Philippe Scorsolini <philippe.scorsolini@enterprisedb.com>

Author: 3 people

api/v1/cluster_types.go

@@ -360,6 +360,8 @@ type ClusterStatus struct {
 const (
 	// ConditionContinuousArchiving this condition archiving :owned by InstanceManager.
 	ConditionContinuousArchiving ClusterConditionType = "ContinuousArchiving"
+	// ConditionBackup this condition looking backup status :owned by InstanceManager.
+	ConditionBackup ClusterConditionType = "LastBackupSucceeded"
 )
 
 // ConditionStatus defines conditions of resources

controllers/backup_controller.go

@@ -28,6 +28,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/source"
 
 	apiv1 "github.com/EnterpriseDB/cloud-native-postgresql/api/v1"
+	"github.com/EnterpriseDB/cloud-native-postgresql/internal/cmd/manager"
 	"github.com/EnterpriseDB/cloud-native-postgresql/pkg/management/log"
 	"github.com/EnterpriseDB/cloud-native-postgresql/pkg/management/postgres"
 	"github.com/EnterpriseDB/cloud-native-postgresql/pkg/specs"
@@ -165,7 +166,7 @@ func (r *BackupReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 		"pod", pod.Name)
 
 	// This backup has been started
-	err = StartBackup(ctx, r.Client, &backup, pod)
+	err = StartBackup(ctx, r.Client, &backup, pod, &cluster)
 	if err != nil {
 		r.Recorder.Eventf(&backup, "Warning", "Error", "Backup exit with error %v", err)
 	}
@@ -180,13 +181,14 @@ func StartBackup(
 	client client.Client,
 	backup *apiv1.Backup,
 	pod corev1.Pod,
+	cluster *apiv1.Cluster,
 ) error {
 	// This backup has been started
 	status := backup.GetStatus()
 	status.Phase = apiv1.BackupPhaseStarted
 	status.InstanceID = &apiv1.InstanceID{PodName: pod.Name, ContainerID: pod.Status.ContainerStatuses[0].ContainerID}
 	if err := postgres.UpdateBackupStatusAndRetry(ctx, client, backup); err != nil {
-		backup.GetStatus().SetAsFailed(fmt.Errorf("can't update backup: %w", err))
+		status.SetAsFailed(fmt.Errorf("can't update backup: %w", err))
 		return err
 	}
 	config := ctrl.GetConfigOrDie()
@@ -204,15 +206,21 @@ func StartBackup(
 			nil,
 			"/controller/manager",
 			"backup",
-			backup.GetName())
+			backup.GetName(),
+		)
 		return err
 	})
+
 	if err != nil {
 		log.FromContext(ctx).Error(err, "executing backup", "stdout", stdout, "stderr", stderr)
-		status := backup.GetStatus()
 		status.SetAsFailed(fmt.Errorf("can't execute backup: %w", err))
 		status.CommandError = stderr
 		status.CommandError = stdout
+
+		// Update backup status in cluster conditions
+		if errCond := manager.UpdateCondition(ctx, client, cluster, postgres.BuildBackupCondition(err)); errCond != nil {
+			log.FromContext(ctx).Error(err, "Error status.UpdateCondition()")
+		}
 		return postgres.UpdateBackupStatusAndRetry(ctx, client, backup)
 	}
 

docs/src/troubleshooting.md

@@ -400,12 +400,19 @@ objects [like here](https://kubernetes.io/docs/concepts/workloads/pods/pod-lifec
 Cluster exposes `status.conditions` as well. This allows one to 'wait' for a particular 
 event to occur instead of relying on the overall cluster health state. Available conditions as of now are:
 
+- LastBackupSucceeded
 - ContinuousArchiving
 
 ### How to wait for a particular condition
 
+- Backup:
 ```bash
-$ kubectl wait --for=condition=ContinuousArchiving cluster/<cluster-name>
+$ kubectl wait --for=condition=LastBackupSucceeded cluster/<CLUSTER-NAME> -n <NAMESPACE>
+```
+
+- ContinuousArchiving:
+```bash
+$ kubectl wait --for=condition=ContinuousArchiving cluster/<CLUSTER-NAME> -n <NAMESPACE>
 ```
 
 Below is a snippet of a `cluster.status` that contains a failing condition.
@@ -418,10 +425,15 @@ $ kubectl get cluster/<cluster-name> -o yaml
   status:
     conditions:
     - message: 'unexpected failure invoking barman-cloud-wal-archive: exit status
-        4'
+        2'
       reason: Continuous Archiving is Failing
       status: "False"
       type: ContinuousArchiving
+
+    - message: exit status 2
+      reason: Backup is failed
+      status: "False"
+      type: LastBackupSucceeded
 ```
 
 ## Some common issues

internal/cmd/manager/manager.go

@@ -126,15 +126,15 @@ func customDestination(in *zap.Options) {
 
 // UpdateCondition will allow instance manager to update a particular condition
 // if the condition was already updated in the correct state then this is a no-op
-func UpdateCondition(ctx context.Context, client client.WithWatch,
+func UpdateCondition(ctx context.Context, c client.Client,
 	cluster *apiv1.Cluster, condition *apiv1.ClusterCondition,
 ) error {
 	if cluster == nil && condition == nil {
 		// if cluster or condition is nil nothing to do here.
 		return nil
 	}
 
-	existingClusterStatus := cluster.Status
+	oriCluster := cluster.DeepCopy()
 	var exCondition *apiv1.ClusterCondition
 	for i, c := range cluster.Status.Conditions {
 		if c.Type == condition.Type {
@@ -148,8 +148,9 @@ func UpdateCondition(ctx context.Context, client client.WithWatch,
 		cluster.Status.Conditions = append(cluster.Status.Conditions, *condition)
 	}
 
-	if !reflect.DeepEqual(existingClusterStatus, cluster.Status) {
-		if err := client.Status().Update(ctx, cluster); err != nil {
+	if !reflect.DeepEqual(oriCluster.Status, cluster.Status) {
+		// To avoid conflict using patch instead of update
+		if err := c.Status().Patch(ctx, cluster, client.MergeFrom(oriCluster)); err != nil {
 			return err
 		}
 	}

pkg/management/postgres/backup.go

@@ -26,6 +26,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	apiv1 "github.com/EnterpriseDB/cloud-native-postgresql/api/v1"
+	"github.com/EnterpriseDB/cloud-native-postgresql/internal/cmd/manager"
 	"github.com/EnterpriseDB/cloud-native-postgresql/pkg/fileutils"
 	"github.com/EnterpriseDB/cloud-native-postgresql/pkg/management/barman"
 	barmanCapabilities "github.com/EnterpriseDB/cloud-native-postgresql/pkg/management/barman/capabilities"
@@ -279,6 +280,13 @@ func (b *BackupCommand) run(ctx context.Context) {
 		b.Log.Error(err, "Backup failed")
 		backupStatus.SetAsFailed(err)
 		b.Recorder.Event(b.Backup, "Normal", "Failed", "Backup failed")
+
+		// Update backup status in cluster conditions
+		if errCond := manager.UpdateCondition(ctx, b.Client,
+			b.Cluster, BuildBackupCondition(err)); errCond != nil {
+			b.Log.Error(errCond, "Error status.UpdateCondition()")
+		}
+
 		if err := UpdateBackupStatusAndRetry(ctx, b.Client, b.Backup); err != nil {
 			b.Log.Error(err, "Can't mark backup as failed")
 		}
@@ -290,6 +298,12 @@ func (b *BackupCommand) run(ctx context.Context) {
 	backupStatus.SetAsCompleted()
 	b.Recorder.Event(b.Backup, "Normal", "Completed", "Backup completed")
 
+	// Update backup status in cluster conditions
+	if errCond := manager.UpdateCondition(ctx, b.Client,
+		b.Cluster, BuildBackupCondition(nil)); errCond != nil {
+		b.Log.Error(errCond, "Error status.UpdateCondition()")
+	}
+
 	// Delete backups per policy
 	if b.Cluster.Spec.Backup.RetentionPolicy != "" {
 		b.Log.Info("Applying backup retention policy",
@@ -413,3 +427,21 @@ func (b *BackupCommand) updateCompletedBackupStatus(backupList *catalog.Catalog)
 	backupStatus.BeginLSN = latestBackup.BeginLSN
 	backupStatus.EndLSN = latestBackup.EndLSN
 }
+
+// BuildBackupCondition build a backup conditions
+func BuildBackupCondition(err error) *apiv1.ClusterCondition {
+	if err != nil {
+		return &apiv1.ClusterCondition{
+			Type:    apiv1.ConditionBackup,
+			Status:  apiv1.ConditionFalse,
+			Reason:  "Last backup failed",
+			Message: err.Error(),
+		}
+	}
+	return &apiv1.ClusterCondition{
+		Type:    apiv1.ConditionBackup,
+		Status:  apiv1.ConditionTrue,
+		Reason:  "Last backup succeeded",
+		Message: "",
+	}
+}

tests/e2e/asserts_test.go

@@ -1417,6 +1417,7 @@ func prepareClusterForPITROnMinio(
 	})
 	AssertArchiveWalOnMinio(namespace, clusterName)
 	AssertArchiveConditionMet(namespace, clusterName, "5m")
+	AssertBackupConditionInClusterStatus(namespace, clusterName)
 }
 
 func prepareClusterForPITROnAzureBlob(namespace, clusterName, backupSampleFile,
@@ -1451,6 +1452,7 @@ func prepareClusterForPITROnAzureBlob(namespace, clusterName, backupSampleFile,
 	})
 	AssertArchiveWalOnAzureBlob(namespace, clusterName, azStorageAccount, azStorageKey)
 	AssertArchiveConditionMet(namespace, clusterName, "5m")
+	AssertBackupConditionInClusterStatus(namespace, clusterName)
 }
 
 func prepareClusterOnAzurite(namespace, clusterName, clusterSampleFile string) {
@@ -1497,6 +1499,7 @@ func prepareClusterBackupOnAzurite(namespace, clusterName, clusterSampleFile, ba
 			return cluster.Status.FirstRecoverabilityPoint, err
 		}, 30).ShouldNot(BeEmpty())
 	})
+	AssertBackupConditionInClusterStatus(namespace, clusterName)
 }
 
 func prepareClusterForPITROnAzurite(namespace, clusterName, backupSampleFile string, currentTimestamp *string) {
@@ -2014,3 +2017,16 @@ func CreateResourceFromFile(namespace, sampleFilePath string) {
 		return nil
 	}, 60, 5).Should(BeNil())
 }
+
+func AssertBackupConditionInClusterStatus(namespace, clusterName string) {
+	By(fmt.Sprintf("waiting for backup condition status in cluster '%v'", clusterName), func() {
+		Eventually(func() (string, error) {
+			getBackupCondition, err := testsUtils.GetConditionsInClusterStatus(
+				namespace, clusterName, env, apiv1.ConditionBackup)
+			if err != nil {
+				return "", err
+			}
+			return string(getBackupCondition.Status), nil
+		}, 300, 5).Should(BeEquivalentTo("True"))
+	})
+}

tests/e2e/backup_restore_test.go

@@ -157,7 +157,7 @@ var _ = Describe("Backup and restore", Label(tests.LabelBackupRestore), func() {
 			// There should be a backup resource and
 			By("backing up a cluster and verifying it exists on minio", func() {
 				testUtils.ExecuteBackup(namespace, backupFile, env)
-
+				AssertBackupConditionInClusterStatus(namespace, clusterName)
 				Eventually(func() (int, error) {
 					return testUtils.CountFilesOnMinio(namespace, minioClientName, "data.tar")
 				}, 30).Should(BeEquivalentTo(1))
@@ -313,6 +313,7 @@ var _ = Describe("Backup and restore", Label(tests.LabelBackupRestore), func() {
 			By("uploading a backup", func() {
 				// We create a backup
 				testUtils.ExecuteBackup(namespace, backupFile, env)
+				AssertBackupConditionInClusterStatus(namespace, clusterName)
 
 				// Verifying file called data.tar should be available on Azure blob storage
 				Eventually(func() (int, error) {
@@ -602,6 +603,7 @@ var _ = Describe("Clusters Recovery From Barman Object Store", Label(tests.Label
 			// There should be a backup resource and
 			By("backing up a cluster and verifying it exists on minio", func() {
 				testUtils.ExecuteBackup(namespace, sourceBackupFileMinio, env)
+				AssertBackupConditionInClusterStatus(namespace, clusterName)
 				Eventually(func() (int, error) {
 					return testUtils.CountFilesOnMinio(namespace, minioClientName, "data.tar")
 				}, 30).Should(BeEquivalentTo(1))
@@ -645,6 +647,7 @@ var _ = Describe("Clusters Recovery From Barman Object Store", Label(tests.Label
 			// There should be a backup resource and
 			By("backing up a cluster and verifying it exists on minio", func() {
 				testUtils.ExecuteBackup(namespace, sourceBackupFileMinio, env)
+				AssertBackupConditionInClusterStatus(namespace, clusterName)
 				Eventually(func() (int, error) {
 					return testUtils.CountFilesOnMinio(namespace, minioClientName, "data.tar")
 				}, 30).Should(BeEquivalentTo(1))
@@ -696,6 +699,7 @@ var _ = Describe("Clusters Recovery From Barman Object Store", Label(tests.Label
 				By("backing up a cluster and verifying it exists on azure blob storage", func() {
 					// Create the backup
 					testUtils.ExecuteBackup(namespace, sourceBackupFileAzure, env)
+					AssertBackupConditionInClusterStatus(namespace, clusterName)
 					// Verifying file called data.tar should be available on Azure blob storage
 					Eventually(func() (int, error) {
 						return testUtils.CountFilesOnAzureBlobStorage(azStorageAccount, azStorageKey, clusterName, "data.tar")
@@ -767,6 +771,7 @@ var _ = Describe("Clusters Recovery From Barman Object Store", Label(tests.Label
 				By("backing up a cluster and verifying it exists on azure blob storage", func() {
 					// We create a Backup
 					testUtils.ExecuteBackup(namespace, sourceBackupFileAzureSAS, env)
+					AssertBackupConditionInClusterStatus(namespace, clusterName)
 					// Verifying file called data.tar should be available on Azure blob storage
 					Eventually(func() (int, error) {
 						return testUtils.CountFilesOnAzureBlobStorage(azStorageAccount, azStorageKey, clusterName, "data.tar")

tests/utils/backup.go

@@ -385,3 +385,25 @@ func CountFilesOnAzuriteBlobStorage(
 	err = json.Unmarshal([]byte(out), &arr)
 	return len(arr), err
 }
+
+// GetConditionsInClusterStatus get conditions values as given type from cluster object status
+func GetConditionsInClusterStatus(
+	namespace,
+	clusterName string,
+	env *TestingEnvironment,
+	conditionType apiv1.ClusterConditionType) (*apiv1.ClusterCondition, error) {
+	var cluster *apiv1.Cluster
+	var err error
+	var backupConditionInCluster *apiv1.ClusterCondition
+	cluster, err = env.GetCluster(namespace, clusterName)
+	if err != nil {
+		return &apiv1.ClusterCondition{}, nil
+	}
+	for index, cond := range cluster.Status.Conditions {
+		if cond.Type == conditionType {
+			backupConditionInCluster = &cluster.Status.Conditions[index]
+			break
+		}
+	}
+	return backupConditionInCluster, nil
+}