Update Scheduled Backup CronJob Name and Reconcile by Labels

This commit updates the scheduled backup CronJob name format,
previously '<cluster name>-pgbackrest-repo#-<upgrade type>', to a
new shorter format, '<cluster name>-repo#-<upgrade type>', to
better support longer PostgresCluster names. Also, to facilitate
moving from older PGO versions using the previous format, the
scheduled backup CronJobs are now reconciled by Label to ensure
any existing CronJobs can continue to be used by the cluster.

For more information on name limitations, see:
https://kubernetes.io/docs/concepts/workloads/controllers/cron-jobs/

Issue: [sc-14068]

Author: tjmoore4

docs/content/tutorial/disaster-recovery.md

@@ -315,7 +315,7 @@ NAME                             READY   AGE
 statefulset.apps/hippo-00-lwgx   0/0     1h
 
 NAME                                        SCHEDULE   SUSPEND   ACTIVE
-cronjob.batch/hippo-pgbackrest-repo1-full   @daily     True      0
+cronjob.batch/hippo-repo1-full   @daily     True      0
 ```
 
 We can then promote the standby cluster by removing or disabling its

internal/controller/postgrescluster/pgbackrest.go

@@ -1332,7 +1332,7 @@ func (r *Reconciler) reconcilePGBackRest(ctx context.Context,
 		result = updateReconcileResult(result, reconcile.Result{RequeueAfter: 10 * time.Second})
 	}
 	// reconcile the pgBackRest backup CronJobs
-	requeue := r.reconcileScheduledBackups(ctx, postgresCluster, sa)
+	requeue := r.reconcileScheduledBackups(ctx, postgresCluster, sa, repoResources.cronjobs)
 	// If the pgBackRest backup CronJob reconciliation function has encountered an error, requeue
 	// after 10 seconds. The error will not bubble up to allow the reconcile loop to continue.
 	// An error is not logged because an event was already created.
@@ -2748,6 +2748,7 @@ func getRepoVolumeStatus(repoStatus []v1beta1.RepoStatus, repoVolumes []*corev1.
 // schedules configured in the cluster definition
 func (r *Reconciler) reconcileScheduledBackups(
 	ctx context.Context, cluster *v1beta1.PostgresCluster, sa *corev1.ServiceAccount,
+	cronjobs []*batchv1beta1.CronJob,
 ) bool {
 	log := logging.FromContext(ctx).WithValues("reconcileResource", "repoCronJob")
 	// requeue if there is an error during creation
@@ -2760,21 +2761,21 @@ func (r *Reconciler) reconcileScheduledBackups(
 			// next if the repo level schedule is not nil, create the CronJob.
 			if repo.BackupSchedules.Full != nil {
 				if err := r.reconcilePGBackRestCronJob(ctx, cluster, repo,
-					full, repo.BackupSchedules.Full, sa); err != nil {
+					full, repo.BackupSchedules.Full, sa, cronjobs); err != nil {
 					log.Error(err, "unable to reconcile Full backup for "+repo.Name)
 					requeue = true
 				}
 			}
 			if repo.BackupSchedules.Differential != nil {
 				if err := r.reconcilePGBackRestCronJob(ctx, cluster, repo,
-					differential, repo.BackupSchedules.Differential, sa); err != nil {
+					differential, repo.BackupSchedules.Differential, sa, cronjobs); err != nil {
 					log.Error(err, "unable to reconcile Differential backup for "+repo.Name)
 					requeue = true
 				}
 			}
 			if repo.BackupSchedules.Incremental != nil {
 				if err := r.reconcilePGBackRestCronJob(ctx, cluster, repo,
-					incremental, repo.BackupSchedules.Incremental, sa); err != nil {
+					incremental, repo.BackupSchedules.Incremental, sa, cronjobs); err != nil {
 					log.Error(err, "unable to reconcile Incremental backup for "+repo.Name)
 					requeue = true
 				}
@@ -2791,6 +2792,7 @@ func (r *Reconciler) reconcileScheduledBackups(
 func (r *Reconciler) reconcilePGBackRestCronJob(
 	ctx context.Context, cluster *v1beta1.PostgresCluster, repo v1beta1.PGBackRestRepo,
 	backupType string, schedule *string, serviceAccount *corev1.ServiceAccount,
+	cronjobs []*batchv1beta1.CronJob,
 ) error {
 
 	log := logging.FromContext(ctx).WithValues("reconcileResource", "repoCronJob")
@@ -2804,6 +2806,25 @@ func (r *Reconciler) reconcilePGBackRestCronJob(
 		naming.PGBackRestCronJobLabels(cluster.Name, repo.Name, backupType),
 	)
 	objectmeta := naming.PGBackRestCronJob(cluster, backupType, repo.Name)
+
+	// Look for an existing CronJob by the associated Labels. If one exists,
+	// update the ObjectMeta accordingly.
+	for _, cronjob := range cronjobs {
+		// ignore CronJobs that are terminating
+		if cronjob.GetDeletionTimestamp() != nil {
+			continue
+		}
+
+		if cronjob.GetLabels()[naming.LabelCluster] == cluster.Name &&
+			cronjob.GetLabels()[naming.LabelPGBackRestCronJob] == backupType &&
+			cronjob.GetLabels()[naming.LabelPGBackRestRepo] == repo.Name {
+			objectmeta = metav1.ObjectMeta{
+				Namespace: cluster.GetNamespace(),
+				Name:      cronjob.Name,
+			}
+		}
+	}
+
 	objectmeta.Labels = labels
 	objectmeta.Annotations = annotations
 

internal/controller/postgrescluster/pgbackrest_test.go

@@ -173,6 +173,14 @@ func fakePostgresCluster(clusterName, namespace, clusterUID string,
 	return postgresCluster
 }
 
+func fakeObservedCronJobs() []*batchv1beta1.CronJob {
+	return []*batchv1beta1.CronJob{
+		{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: "fake-cronjob",
+			}}}
+}
+
 func TestReconcilePGBackRest(t *testing.T) {
 	// Garbage collector cleans up test resources before the test completes
 	if strings.EqualFold(os.Getenv("USE_EXISTING_CLUSTER"), "true") {
@@ -507,19 +515,19 @@ topologySpreadConstraints:
 				Type: condition, Reason: "testing", Status: status})
 		}
 
-		requeue := r.reconcileScheduledBackups(ctx, postgresCluster, serviceAccount)
+		requeue := r.reconcileScheduledBackups(ctx, postgresCluster, serviceAccount, fakeObservedCronJobs())
 		assert.Assert(t, !requeue)
 
 		returnedCronJob := &batchv1beta1.CronJob{}
 		if err := tClient.Get(ctx, types.NamespacedName{
-			Name:      postgresCluster.Name + "-pgbackrest-repo1-full",
+			Name:      postgresCluster.Name + "-repo1-full",
 			Namespace: postgresCluster.GetNamespace(),
 		}, returnedCronJob); err != nil {
 			assert.NilError(t, err)
 		}
 
 		// check returned cronjob matches set spec
-		assert.Equal(t, returnedCronJob.Name, "hippocluster-pgbackrest-repo1-full")
+		assert.Equal(t, returnedCronJob.Name, "hippocluster-repo1-full")
 		assert.Equal(t, returnedCronJob.Spec.Schedule, testCronSchedule)
 		assert.Equal(t, returnedCronJob.Spec.JobTemplate.Spec.Template.Spec.Containers[0].Name,
 			"pgbackrest")
@@ -558,7 +566,7 @@ topologySpreadConstraints:
 
 		returnedCronJob := &batchv1beta1.CronJob{}
 		if err := tClient.Get(ctx, types.NamespacedName{
-			Name:      postgresCluster.Name + "-pgbackrest-repo1-full",
+			Name:      postgresCluster.Name + "-repo1-full",
 			Namespace: postgresCluster.GetNamespace(),
 		}, returnedCronJob); err != nil {
 			assert.NilError(t, err)
@@ -573,11 +581,11 @@ topologySpreadConstraints:
 			postgresCluster.Spec.Standby = nil
 
 			requeue := r.reconcileScheduledBackups(ctx,
-				postgresCluster, serviceAccount)
+				postgresCluster, serviceAccount, fakeObservedCronJobs())
 			assert.Assert(t, !requeue)
 
 			assert.NilError(t, tClient.Get(ctx, types.NamespacedName{
-				Name:      postgresCluster.Name + "-pgbackrest-repo1-full",
+				Name:      postgresCluster.Name + "-repo1-full",
 				Namespace: postgresCluster.GetNamespace(),
 			}, returnedCronJob))
 
@@ -591,11 +599,11 @@ topologySpreadConstraints:
 			}
 
 			requeue := r.reconcileScheduledBackups(ctx,
-				postgresCluster, serviceAccount)
+				postgresCluster, serviceAccount, fakeObservedCronJobs())
 			assert.Assert(t, !requeue)
 
 			assert.NilError(t, tClient.Get(ctx, types.NamespacedName{
-				Name:      postgresCluster.Name + "-pgbackrest-repo1-full",
+				Name:      postgresCluster.Name + "-repo1-full",
 				Namespace: postgresCluster.GetNamespace(),
 			}, returnedCronJob))
 
@@ -3327,6 +3335,8 @@ func TestReconcileScheduledBackups(t *testing.T) {
 		expectedEventReason string
 		// the observed instances
 		instances *observedInstances
+		// CronJobs exist
+		cronJobs bool
 	}{
 		{
 			testDesc: "should reconcile, no requeue",
@@ -3341,6 +3351,20 @@ func TestReconcileScheduledBackups(t *testing.T) {
 			},
 			expectReconcile: true,
 			expectRequeue:   false,
+		}, {
+			testDesc: "should reconcile, no requeue, existing cronjob",
+			clusterConditions: map[string]metav1.ConditionStatus{
+				ConditionRepoHostReady: metav1.ConditionTrue,
+				ConditionReplicaCreate: metav1.ConditionTrue,
+			},
+			status: &v1beta1.PostgresClusterStatus{
+				Patroni: v1beta1.PatroniStatus{SystemIdentifier: "12345abcde"},
+				PGBackRest: &v1beta1.PGBackRestStatus{
+					Repos: []v1beta1.RepoStatus{{Name: "repo1", StanzaCreated: true}}},
+			},
+			expectReconcile: true,
+			expectRequeue:   false,
+			cronJobs:        true,
 		}, {
 			testDesc: "cluster not bootstrapped, should not reconcile",
 			status: &v1beta1.PostgresClusterStatus{
@@ -3432,12 +3456,38 @@ func TestReconcileScheduledBackups(t *testing.T) {
 				assert.NilError(t, tClient.Status().Update(ctx, postgresCluster))
 
 				var requeue bool
-				if tc.instances != nil {
-					requeue = r.reconcileScheduledBackups(ctx, postgresCluster, sa)
+				if tc.cronJobs {
+					existingCronJobs := []*batchv1beta1.CronJob{
+						{
+							ObjectMeta: metav1.ObjectMeta{
+								Name: "existingcronjob-repo1-full",
+								Labels: map[string]string{
+									naming.LabelCluster:           clusterName,
+									naming.LabelPGBackRestCronJob: "full",
+									naming.LabelPGBackRestRepo:    "repo1",
+								}},
+						}, {
+							ObjectMeta: metav1.ObjectMeta{
+								Name: "existingcronjob-repo1-incr",
+								Labels: map[string]string{
+									naming.LabelCluster:           clusterName,
+									naming.LabelPGBackRestCronJob: "incr",
+									naming.LabelPGBackRestRepo:    "repo1",
+								}},
+						}, {
+							ObjectMeta: metav1.ObjectMeta{
+								Name: "existingcronjob-repo1-diff",
+								Labels: map[string]string{
+									naming.LabelCluster:           clusterName,
+									naming.LabelPGBackRestCronJob: "diff",
+									naming.LabelPGBackRestRepo:    "repo1",
+								}},
+						},
+					}
+					requeue = r.reconcileScheduledBackups(ctx, postgresCluster, sa, existingCronJobs)
 				} else {
-					requeue = r.reconcileScheduledBackups(ctx, postgresCluster, sa)
+					requeue = r.reconcileScheduledBackups(ctx, postgresCluster, sa, fakeObservedCronJobs())
 				}
-
 				if !tc.expectReconcile && !tc.expectRequeue {
 					// expect no reconcile, no requeue
 					assert.Assert(t, !requeue)
@@ -3475,16 +3525,23 @@ func TestReconcileScheduledBackups(t *testing.T) {
 
 					for _, backupType := range backupTypes {
 
+						var cronJobName string
+						if tc.cronJobs {
+							cronJobName = "existingcronjob-repo1-" + backupType
+						} else {
+							cronJobName = postgresCluster.Name + "-repo1-" + backupType
+						}
+
 						returnedCronJob := &batchv1beta1.CronJob{}
 						if err := tClient.Get(ctx, types.NamespacedName{
-							Name:      postgresCluster.Name + "-pgbackrest-repo1-" + backupType,
+							Name:      cronJobName,
 							Namespace: postgresCluster.GetNamespace(),
 						}, returnedCronJob); err != nil {
 							assert.NilError(t, err)
 						}
 
 						// check returned cronjob matches set spec
-						assert.Equal(t, returnedCronJob.Name, clusterName+"-pgbackrest-repo1-"+backupType)
+						assert.Equal(t, returnedCronJob.Name, cronJobName)
 						assert.Equal(t, returnedCronJob.Spec.Schedule, testCronSchedule)
 						assert.Equal(t, returnedCronJob.Spec.JobTemplate.Spec.Template.Spec.PriorityClassName, "some-priority-class")
 						assert.Equal(t, returnedCronJob.Spec.JobTemplate.Spec.Template.Spec.Containers[0].Name,

internal/naming/names.go

@@ -418,7 +418,7 @@ func PGBackRestBackupJob(cluster *v1beta1.PostgresCluster) metav1.ObjectMeta {
 func PGBackRestCronJob(cluster *v1beta1.PostgresCluster, backuptype, repoName string) metav1.ObjectMeta {
 	return metav1.ObjectMeta{
 		Namespace: cluster.GetNamespace(),
-		Name:      cluster.Name + "-pgbackrest-" + repoName + "-" + backuptype,
+		Name:      cluster.Name + "-" + repoName + "-" + backuptype,
 	}
 }