From ebe50abccb029e700964ce68cda4c78f6c2ae071 Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Wed, 16 May 2018 18:23:31 +0200 Subject: [PATCH] Make sure we never modify informer cached manifest. (#290) 987b434 introduced a new function that modifies the cluster spec in memory before the cluster processes it. Unfortunately, the instance being modified appeared to be the one stored internally in the PostgresInformer, resulting in those modifications to be propagated with futher cluster events and producing update loops in some occasions. This commit makes sure we copy the spec before putting it into the clusterEventQueues. --- pkg/controller/postgresql.go | 78 ++++++++++++++++++++----------- pkg/controller/postgresql_test.go | 6 +-- 2 files changed, 53 insertions(+), 31 deletions(-) diff --git a/pkg/controller/postgresql.go b/pkg/controller/postgresql.go index 0968eb758..45645a709 100644 --- a/pkg/controller/postgresql.go +++ b/pkg/controller/postgresql.go @@ -172,9 +172,9 @@ func (c *Controller) processEvent(event spec.ClusterEvent) { c.mergeDeprecatedPostgreSQLSpecParameters(&event.OldSpec.Spec) } if event.NewSpec != nil { + c.warnOnDeprecatedPostgreSQLSpecParameters(&event.NewSpec.Spec) c.mergeDeprecatedPostgreSQLSpecParameters(&event.NewSpec.Spec) } - c.warnOnDeprecatedPostgreSQLSpecParameters(&event.NewSpec.Spec) } switch event.EventType { @@ -318,46 +318,55 @@ func (c *Controller) warnOnDeprecatedPostgreSQLSpecParameters(spec *spec.Postgre if len(spec.MaintenanceWindows) > 0 { noeffect("maintenanceWindows", "Not implemented.") } + + if (spec.UseLoadBalancer != nil || spec.ReplicaLoadBalancer != nil) && + (spec.EnableReplicaLoadBalancer != nil || spec.EnableMasterLoadBalancer != nil) { + c.logger.Warnf("Both old and new load balancer parameters are present in the manifest, ignoring old ones") + } } +// mergeDeprecatedPostgreSQLSpecParameters modifies the spec passed to the cluster by setting current parameter +// values from the obsolete ones. Note: while the spec that is modified is a copy made in queueClusterEvent, it is +// still a shallow copy, so be extra careful not to modify values pointer fields point to, but copy them instead. func (c *Controller) mergeDeprecatedPostgreSQLSpecParameters(spec *spec.PostgresSpec) *spec.PostgresSpec { - if spec.UseLoadBalancer != nil || spec.ReplicaLoadBalancer != nil { - if spec.EnableReplicaLoadBalancer != nil || spec.EnableMasterLoadBalancer != nil { - c.logger.Warnf("Both old and new load balancer options are present, ignoring old ones") - } else { - if spec.UseLoadBalancer != nil { - spec.EnableMasterLoadBalancer = new(bool) - *spec.EnableMasterLoadBalancer = *spec.UseLoadBalancer - } - if spec.ReplicaLoadBalancer != nil { - spec.EnableReplicaLoadBalancer = new(bool) - *spec.EnableReplicaLoadBalancer = *spec.ReplicaLoadBalancer - } + if (spec.UseLoadBalancer != nil || spec.ReplicaLoadBalancer != nil) && + (spec.EnableReplicaLoadBalancer == nil && spec.EnableMasterLoadBalancer == nil) { + if spec.UseLoadBalancer != nil { + spec.EnableMasterLoadBalancer = new(bool) + *spec.EnableMasterLoadBalancer = *spec.UseLoadBalancer + } + if spec.ReplicaLoadBalancer != nil { + spec.EnableReplicaLoadBalancer = new(bool) + *spec.EnableReplicaLoadBalancer = *spec.ReplicaLoadBalancer } } + spec.ReplicaLoadBalancer = nil + spec.UseLoadBalancer = nil + return spec } -func (c *Controller) queueClusterEvent(old, new *spec.Postgresql, eventType spec.EventType) { +func (c *Controller) queueClusterEvent(informerOldSpec, informerNewSpec *spec.Postgresql, eventType spec.EventType) { var ( - uid types.UID - clusterName spec.NamespacedName - clusterError error + uid types.UID + clusterName spec.NamespacedName + clusterError error + oldSpec, newSpec *spec.Postgresql ) - if old != nil { //update, delete - uid = old.GetUID() - clusterName = util.NameFromMeta(old.ObjectMeta) - if eventType == spec.EventUpdate && new.Error == nil && old.Error != nil { + if informerOldSpec != nil { //update, delete + uid = informerOldSpec.GetUID() + clusterName = util.NameFromMeta(informerOldSpec.ObjectMeta) + if eventType == spec.EventUpdate && informerNewSpec.Error == nil && informerOldSpec.Error != nil { eventType = spec.EventSync - clusterError = new.Error + clusterError = informerNewSpec.Error } else { - clusterError = old.Error + clusterError = informerOldSpec.Error } } else { //add, sync - uid = new.GetUID() - clusterName = util.NameFromMeta(new.ObjectMeta) - clusterError = new.Error + uid = informerNewSpec.GetUID() + clusterName = util.NameFromMeta(informerNewSpec.ObjectMeta) + clusterError = informerNewSpec.Error } if clusterError != nil && eventType != spec.EventDelete { @@ -367,13 +376,26 @@ func (c *Controller) queueClusterEvent(old, new *spec.Postgresql, eventType spec return } + // Don't pass the spec directly from the informer, since subsequent modifications of it would be reflected + // in the informer internal state, making it incohherent with the actual Kubernetes object (and, as a side + // effect, the modified state will be returned together with subsequent events). + + if informerOldSpec != nil { + t := *informerOldSpec + oldSpec = &t + } + if informerNewSpec != nil { + t := *informerNewSpec + newSpec = &t + } + workerID := c.clusterWorkerID(clusterName) clusterEvent := spec.ClusterEvent{ EventTime: time.Now(), EventType: eventType, UID: uid, - OldSpec: old, - NewSpec: new, + OldSpec: oldSpec, + NewSpec: newSpec, WorkerID: workerID, } diff --git a/pkg/controller/postgresql_test.go b/pkg/controller/postgresql_test.go index 967166ecb..7fa7d842f 100644 --- a/pkg/controller/postgresql_test.go +++ b/pkg/controller/postgresql_test.go @@ -23,14 +23,14 @@ func TestMergeDeprecatedPostgreSQLSpecParameters(t *testing.T) { { "Check that old parameters propagate values to the new ones", &spec.PostgresSpec{UseLoadBalancer: &True, ReplicaLoadBalancer: &True}, - &spec.PostgresSpec{UseLoadBalancer: &True, ReplicaLoadBalancer: &True, + &spec.PostgresSpec{UseLoadBalancer: nil, ReplicaLoadBalancer: nil, EnableMasterLoadBalancer: &True, EnableReplicaLoadBalancer: &True}, "New parameters should be set from the values of old ones", }, { "Check that new parameters are not set when both old and new ones are present", - &spec.PostgresSpec{UseLoadBalancer: &True, EnableReplicaLoadBalancer: &True}, - &spec.PostgresSpec{UseLoadBalancer: &True, EnableReplicaLoadBalancer: &True}, + &spec.PostgresSpec{UseLoadBalancer: &True, EnableMasterLoadBalancer: &False}, + &spec.PostgresSpec{UseLoadBalancer: nil, EnableMasterLoadBalancer: &False}, "New parameters should remain unchanged when both old and new are present", }, }