From 6dc97d6fc1b98f1fd226fe5af6b48fe0a214a184 Mon Sep 17 00:00:00 2001
From: Sergey Dudoladov <sergey.dudoladov@zalando.de>
Date: Fri, 3 May 2019 11:21:09 +0200
Subject: [PATCH] Address first part of the code review

---
 pkg/cluster/cluster.go   | 14 ++++----------
 pkg/cluster/resources.go |  7 +++++++
 pkg/cluster/sync.go      |  3 +--
 3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/pkg/cluster/cluster.go b/pkg/cluster/cluster.go
index 250f65dd1..7921ac739 100644
--- a/pkg/cluster/cluster.go
+++ b/pkg/cluster/cluster.go
@@ -487,9 +487,10 @@ func compareResoucesAssumeFirstNotNil(a *v1.ResourceRequirements, b *v1.Resource
 
 }
 
-// Update changes Kubernetes objects according to the new specification. Unlike the sync case, the missing object.
+// Update changes Kubernetes objects according to the new specification. Unlike the sync case, the missing object
 // (i.e. service) is treated as an error
-// logical backup cron jobs are an exception: they can be freely created/deleted by users
+// logical backup cron jobs are an exception: a user-initiated Update can enable a logical backup job
+// for a cluster that had no such job before. In this case a missing job is not an error.
 func (c *Cluster) Update(oldSpec, newSpec *acidv1.Postgresql) error {
 	updateFailed := false
 
@@ -602,7 +603,7 @@ func (c *Cluster) Update(oldSpec, newSpec *acidv1.Postgresql) error {
 
 		// apply schedule changes
 		// this is the only parameter of logical backups a user can overwrite in the cluster manifest
-		if (newSpec.Spec.EnableLogicalBackup) &&
+		if (oldSpec.Spec.EnableLogicalBackup && newSpec.Spec.EnableLogicalBackup) &&
 			(newSpec.Spec.LogicalBackupSchedule != oldSpec.Spec.LogicalBackupSchedule) {
 			c.logger.Debugf("updating schedule of the backup cron job")
 			if err := c.syncLogicalBackupJob(); err != nil {
@@ -1059,10 +1060,3 @@ func (c *Cluster) deletePatroniClusterConfigMaps() error {
 
 	return c.deleteClusterObject(get, deleteConfigMapFn, "configmap")
 }
-
-func (c *Cluster) deleteLogicalBackupJob() error {
-
-	c.logger.Debug("removing the logical backup job")
-
-	return c.KubeClient.CronJobsGetter.CronJobs(c.Namespace).Delete(c.getLogicalBackupJobName(), c.deleteOptions)
-}
diff --git a/pkg/cluster/resources.go b/pkg/cluster/resources.go
index 19f8c34dc..e8674283a 100644
--- a/pkg/cluster/resources.go
+++ b/pkg/cluster/resources.go
@@ -648,6 +648,13 @@ func (c *Cluster) patchLogicalBackupJob(newJob *batchv1beta1.CronJob) error {
 	return nil
 }
 
+func (c *Cluster) deleteLogicalBackupJob() error {
+
+	c.logger.Info("removing the logical backup job")
+
+	return c.KubeClient.CronJobsGetter.CronJobs(c.Namespace).Delete(c.getLogicalBackupJobName(), c.deleteOptions)
+}
+
 // GetServiceMaster returns cluster's kubernetes master Service
 func (c *Cluster) GetServiceMaster() *v1.Service {
 	return c.Services[Master]
diff --git a/pkg/cluster/sync.go b/pkg/cluster/sync.go
index 93f7ac57a..5f6eb265c 100644
--- a/pkg/cluster/sync.go
+++ b/pkg/cluster/sync.go
@@ -540,7 +540,6 @@ func (c *Cluster) syncLogicalBackupJob() error {
 	c.setProcessName("syncing the logical backup job")
 
 	// sync the job if it exists
-	// NB operator pod at startup syncs all clusters, c.logicalBackupJob will be nil during such Sync
 
 	jobName := c.getLogicalBackupJobName()
 	if job, err = c.KubeClient.CronJobsGetter.CronJobs(c.Namespace).Get(jobName, metav1.GetOptions{}); err == nil {
@@ -554,7 +553,7 @@ func (c *Cluster) syncLogicalBackupJob() error {
 			if err = c.patchLogicalBackupJob(desiredJob); err != nil {
 				return fmt.Errorf("could not update logical backup job to match desired state: %v", err)
 			}
-			c.logger.Info("the logical backup job is in the desired state now")
+			c.logger.Info("the logical backup job is synced")
 		}
 		return nil
 	}