From bd9d0fcd38b5f692a9f9b2f4bcb13187df0b257e Mon Sep 17 00:00:00 2001 From: Sergey Dudoladov Date: Wed, 17 Apr 2019 16:45:50 +0200 Subject: [PATCH] code cleanup --- pkg/cluster/cluster.go | 11 ++++++----- pkg/cluster/k8sres.go | 5 +++++ pkg/cluster/resources.go | 8 ++------ pkg/cluster/sync.go | 35 ++++++++++++++++------------------- pkg/cluster/util.go | 14 ++++---------- pkg/util/config/config.go | 2 +- pkg/util/k8sutil/k8sutil.go | 4 ++-- run_operator_locally.sh | 2 +- 8 files changed, 37 insertions(+), 44 deletions(-) diff --git a/pkg/cluster/cluster.go b/pkg/cluster/cluster.go index 94399c874..3b4d1517f 100644 --- a/pkg/cluster/cluster.go +++ b/pkg/cluster/cluster.go @@ -582,7 +582,7 @@ func (c *Cluster) Update(oldSpec, newSpec *acidv1.Postgresql) error { func() { // create if it did not exist - if newSpec.Spec.EnableLogicalBackup && !oldSpec.Spec.EnableLogicalBackup { + if !oldSpec.Spec.EnableLogicalBackup && newSpec.Spec.EnableLogicalBackup { c.logger.Debugf("creating backup cron job") if err := c.createLogicalBackupJob(); err != nil { c.logger.Errorf("could not create a k8s cron job for logical backups: %v", err) @@ -603,9 +603,10 @@ func (c *Cluster) Update(oldSpec, newSpec *acidv1.Postgresql) error { } // apply schedule changes - if (c.logicalBackupJob != nil) && + // this is the only parameter of logical backups a user can overwrite in the cluster manifest + if (newSpec.Spec.EnableLogicalBackup) && (newSpec.Spec.LogicalBackupSchedule != oldSpec.Spec.LogicalBackupSchedule) { - c.logger.Debugf("updating backup cron job") + c.logger.Debugf("updating schedule of the backup cron job") if err := c.syncLogicalBackupJob(); err != nil { c.logger.Errorf("could not sync logical backup jobs: %v", err) updateFailed = true @@ -1067,7 +1068,7 @@ func (c *Cluster) deleteLogicalBackupJob() error { return nil } - c.logger.Debugf("removing the logical backup job") + c.logger.Debug("removing the logical backup job") - return c.KubeClient.CronJobsGetter.CronJobs(c.Namespace).Delete(c.logicalBackupJob.ObjectMeta.Name, c.deleteOptions) + return c.KubeClient.CronJobsGetter.CronJobs(c.Namespace).Delete(c.getLogicalBackupJobName(), c.deleteOptions) } diff --git a/pkg/cluster/k8sres.go b/pkg/cluster/k8sres.go index 1cd8cc0c3..df8deee59 100644 --- a/pkg/cluster/k8sres.go +++ b/pkg/cluster/k8sres.go @@ -1407,3 +1407,8 @@ func (c *Cluster) generateLogicalBackupPodEnvVars() []v1.EnvVar { return envVars } + +// getLogicalBackupJobName returns the name; the job itself may not exists +func (c *Cluster) getLogicalBackupJobName() (jobName string) { + return "logical-backup-" + c.clusterName().Namespace + "-" + c.clusterName().Name +} diff --git a/pkg/cluster/resources.go b/pkg/cluster/resources.go index c99931ccf..b808afa54 100644 --- a/pkg/cluster/resources.go +++ b/pkg/cluster/resources.go @@ -629,12 +629,8 @@ func (c *Cluster) createLogicalBackupJob() (err error) { return nil } -func (c *Cluster) updateCronJob(newJob *batchv1beta1.CronJob) error { - c.setProcessName("updating logical backup job") - - if c.logicalBackupJob == nil { - return fmt.Errorf("there is no logical backup job in the cluster") - } +func (c *Cluster) patchLogicalBackupJob(newJob *batchv1beta1.CronJob) error { + c.setProcessName("patching logical backup job") patchData, err := specPatch(newJob.Spec) if err != nil { diff --git a/pkg/cluster/sync.go b/pkg/cluster/sync.go index f44e125e4..93f7ac57a 100644 --- a/pkg/cluster/sync.go +++ b/pkg/cluster/sync.go @@ -94,11 +94,11 @@ func (c *Cluster) Sync(newSpec *acidv1.Postgresql) error { } // create a logical backup job unless we are running without pods or disable that feature explicitly - if c.Spec.EnableLogicalBackup && c.getNumberOfInstances(&newSpec.Spec) > 0 { + if c.Spec.EnableLogicalBackup && c.getNumberOfInstances(&c.Spec) > 0 { - c.logger.Debug("syncing logical backup jobs") + c.logger.Debug("syncing logical backup job") if err = c.syncLogicalBackupJob(); err != nil { - err = fmt.Errorf("could not sync logical backup jobs: %v", err) + err = fmt.Errorf("could not sync the logical backup job: %v", err) return err } } @@ -539,24 +539,20 @@ func (c *Cluster) syncLogicalBackupJob() error { ) c.setProcessName("syncing the logical backup job") - // operator pod at startup syncs all clusters, logicalBackupJob will be nil at this point - if c.Postgresql.Spec.EnableLogicalBackup && c.logicalBackupJob == nil { - c.logicalBackupJob, err = c.generateLogicalBackupJob() - if err != nil { - return fmt.Errorf("could not generate the desired cron job state, presumably during the first Sync on operator pod start-up: %v", err) - } - } + // sync the job if it exists + // NB operator pod at startup syncs all clusters, c.logicalBackupJob will be nil during such Sync - if job, err = c.KubeClient.CronJobsGetter.CronJobs(c.Namespace).Get(c.logicalBackupJob.Name, metav1.GetOptions{}); err == nil { + jobName := c.getLogicalBackupJobName() + if job, err = c.KubeClient.CronJobsGetter.CronJobs(c.Namespace).Get(jobName, metav1.GetOptions{}); err == nil { desiredJob, err = c.generateLogicalBackupJob() if err != nil { - return fmt.Errorf("could not generate the desired cron job state: %v", err) + return fmt.Errorf("could not generate the desired logical backup job state: %v", err) } - if match, reason := k8sutil.SameCronJob(job, desiredJob); !match { - c.logCronJobChanges(job, desiredJob, false, reason) - if err = c.updateCronJob(desiredJob); err != nil { - return fmt.Errorf("could not update job to match desired state: %v", err) + if match, reason := k8sutil.SameLogicalBackupJob(job, desiredJob); !match { + c.logLogicalBackupJobChanges(job, desiredJob, reason) + 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") } @@ -565,17 +561,18 @@ func (c *Cluster) syncLogicalBackupJob() error { if !k8sutil.ResourceNotFound(err) { return fmt.Errorf("could not get logical backp job: %v", err) } + // no existing logical backup job, create new one c.logger.Info("could not find the cluster's logical backup job") if err = c.createLogicalBackupJob(); err == nil { - c.logger.Infof("created missing logical backup job %q", c.logicalBackupJob.Name) + c.logger.Infof("created missing logical backup job %q", jobName) } else { if !k8sutil.ResourceAlreadyExists(err) { return fmt.Errorf("could not create missing logical backup job: %v", err) } - c.logger.Infof("logical backup job %q already exists", util.NameFromMeta(job.ObjectMeta)) - if _, err = c.KubeClient.CronJobsGetter.CronJobs(c.Namespace).Get(c.logicalBackupJob.Name, metav1.GetOptions{}); err != nil { + c.logger.Infof("logical backup job %q already exists", jobName) + if _, err = c.KubeClient.CronJobsGetter.CronJobs(c.Namespace).Get(jobName, metav1.GetOptions{}); err != nil { return fmt.Errorf("could not fetch existing logical backup job: %v", err) } } diff --git a/pkg/cluster/util.go b/pkg/cluster/util.go index 9d6700b22..ab0fc71f4 100644 --- a/pkg/cluster/util.go +++ b/pkg/cluster/util.go @@ -485,17 +485,11 @@ func (c *Cluster) patroniUsesKubernetes() bool { return c.OpConfig.EtcdHost == "" } -func (c *Cluster) logCronJobChanges(old, new *batchv1beta1.CronJob, isUpdate bool, reason string) { - if isUpdate { - c.logger.Infof("logical job %q has been changed", - c.logicalBackupJob.Name, - ) - } else { - c.logger.Infof("logical job %q is not in the desired state and needs to be updated", - c.logicalBackupJob.Name, - ) - } +func (c *Cluster) logLogicalBackupJobChanges(old, new *batchv1beta1.CronJob, reason string) { + c.logger.Infof("logical job %q is not in the desired state and needs to be updated", + c.logicalBackupJob.Name, + ) if reason != "" { c.logger.Infof("reason: %s", reason) } diff --git a/pkg/util/config/config.go b/pkg/util/config/config.go index e5dc04f29..cd02c6ddb 100644 --- a/pkg/util/config/config.go +++ b/pkg/util/config/config.go @@ -71,7 +71,7 @@ type LogicalBackup struct { EnableLogicalBackup bool `name:"enable_logical_backup" default:"false"` LogicalBackupSchedule string `name:"logical_backup_schedule" default:"30 00 * * *"` LogicalBackupDockerImage string `name:"logical_backup_docker_image" default:"registry.opensource.zalan.do/acid/logical-backup"` - LogicalBackupS3Bucket string `name:"logical_backup_s3_bucket"` + LogicalBackupS3Bucket string `name:"logical_backup_s3_bucket" default:""` } // Config describes operator config diff --git a/pkg/util/k8sutil/k8sutil.go b/pkg/util/k8sutil/k8sutil.go index 9e9d5f518..fc5dca1b8 100644 --- a/pkg/util/k8sutil/k8sutil.go +++ b/pkg/util/k8sutil/k8sutil.go @@ -151,8 +151,8 @@ func getJobImage(cronJob *batchv1beta1.CronJob) string { return cronJob.Spec.JobTemplate.Spec.Template.Spec.Containers[0].Image } -// SameCronJob compares Specs of logical backup cron jobs -func SameCronJob(cur, new *batchv1beta1.CronJob) (match bool, reason string) { +// SameLogicalBackupJob compares Specs of logical backup cron jobs +func SameLogicalBackupJob(cur, new *batchv1beta1.CronJob) (match bool, reason string) { if cur.Spec.Schedule != new.Spec.Schedule { return false, fmt.Sprintf("new job's schedule %q doesn't match the current one %q", diff --git a/run_operator_locally.sh b/run_operator_locally.sh index db3d8e50f..ee0768354 100755 --- a/run_operator_locally.sh +++ b/run_operator_locally.sh @@ -256,7 +256,7 @@ function main(){ clean_up start_minikube start_operator - # submit_postgresql_manifest + submit_postgresql_manifest forward_ports check_health