From da4cc2705b2dfc4ebc98eceacf3222cc9dbf2e0a Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Thu, 17 May 2018 16:05:12 +0200 Subject: [PATCH] Use deepcopy to propagate the spec to clusters. Avoid sharing pointers to the same spec data between the informer and the clusters. The only catch is that the error field is cleared during deepcopy, since it is an interface that may contain private fields that cannot be copied, however, the error is only used when the manifest is parsed and before it is queued, therefore, we never refer to that field in the cluster structure. --- glide.lock | 14 +++++++++----- glide.yaml | 1 + pkg/controller/postgresql.go | 20 +++++--------------- pkg/spec/postgresql.go | 14 ++++++++++++++ pkg/spec/postgresql_test.go | 15 +++++++++++++-- 5 files changed, 42 insertions(+), 22 deletions(-) diff --git a/glide.lock b/glide.lock index bb94e693b..27ee9beba 100644 --- a/glide.lock +++ b/glide.lock @@ -1,8 +1,8 @@ -hash: aa008e00a8cf34fa59a081dd67644319f9d5d077bf4de81aa4a25a2d5b8781a1 -updated: 2018-01-15T15:00:53.231516+01:00 +hash: 688e15147f1217da635b83ee33f20a3741a400a493787d79992d1650f6e4c514 +updated: 2018-05-17T10:46:49.090929+02:00 imports: - name: github.com/aws/aws-sdk-go - version: 0cebc639926eb91b0192dae4b28bc808417e764c + version: ee7b4b1162937cba700de23bd90acb742982e626 subpackages: - aws - aws/awserr @@ -20,6 +20,8 @@ imports: - aws/request - aws/session - aws/signer/v4 + - internal/sdkio + - internal/sdkrand - internal/shareddefaults - private/protocol - private/protocol/ec2query @@ -97,6 +99,8 @@ imports: - buffer - jlexer - jwriter +- name: github.com/mohae/deepcopy + version: c48cc78d482608239f6c4c92a4abd87eb8761c90 - name: github.com/motomux/pretty version: b2aad2c9a95d14eb978f29baa6e3a5c3c20eef30 - name: github.com/PuerkitoBio/purell @@ -104,7 +108,7 @@ imports: - name: github.com/PuerkitoBio/urlesc version: 5bd2802263f21d8788851d5305584c82a5c75d7e - name: github.com/Sirupsen/logrus - version: d682213848ed68c0a260ca37d6dd5ace8423f5ba + version: c155da19408a8799da419ed3eeb0cb5db0ad5dbc - name: github.com/spf13/pflag version: 9ff6c6923cfffbcd502984b8e0c80539a94968b7 - name: github.com/ugorji/go @@ -153,7 +157,7 @@ imports: - pkg/client/clientset/clientset/scheme - pkg/client/clientset/clientset/typed/apiextensions/v1beta1 - name: k8s.io/apimachinery - version: 8ab5f3d8a330c2e9baaf84e39042db8d49034ae2 + version: 1cb2cdd78d38df243e686d1b572b76e190469842 subpackages: - pkg/api/equality - pkg/api/errors diff --git a/glide.yaml b/glide.yaml index 5000dcaaf..3d79d8f68 100644 --- a/glide.yaml +++ b/glide.yaml @@ -44,3 +44,4 @@ import: - tools/clientcmd - tools/remotecommand - package: gopkg.in/yaml.v2 +- package: github.com/mohae/deepcopy diff --git a/pkg/controller/postgresql.go b/pkg/controller/postgresql.go index 45645a709..8b890129b 100644 --- a/pkg/controller/postgresql.go +++ b/pkg/controller/postgresql.go @@ -348,10 +348,9 @@ func (c *Controller) mergeDeprecatedPostgreSQLSpecParameters(spec *spec.Postgres func (c *Controller) queueClusterEvent(informerOldSpec, informerNewSpec *spec.Postgresql, eventType spec.EventType) { var ( - uid types.UID - clusterName spec.NamespacedName - clusterError error - oldSpec, newSpec *spec.Postgresql + uid types.UID + clusterName spec.NamespacedName + clusterError error ) if informerOldSpec != nil { //update, delete @@ -380,22 +379,13 @@ func (c *Controller) queueClusterEvent(informerOldSpec, informerNewSpec *spec.Po // 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: oldSpec, - NewSpec: newSpec, + OldSpec: informerOldSpec.Clone(), + NewSpec: informerNewSpec.Clone(), WorkerID: workerID, } diff --git a/pkg/spec/postgresql.go b/pkg/spec/postgresql.go index c7b5df902..bd5d06127 100644 --- a/pkg/spec/postgresql.go +++ b/pkg/spec/postgresql.go @@ -3,6 +3,7 @@ package spec import ( "encoding/json" "fmt" + "github.com/mohae/deepcopy" "regexp" "strings" "time" @@ -138,6 +139,19 @@ var ( serviceNameRegex = regexp.MustCompile(serviceNameRegexString) ) +// Clone makes a deepcopy of the Postgresql structure. The Error field is nulled-out, +// as there is no guaratee that the actual implementation of the error interface +// will not contain any private fields not-reachable to deepcopy. This should be ok, +// since Error is never read from a Kubernetes object. +func (p *Postgresql) Clone() *Postgresql { + if p == nil { + return nil + } + c := deepcopy.Copy(p).(*Postgresql) + c.Error = nil + return c +} + func parseTime(s string) (time.Time, error) { parts := strings.Split(s, ":") if len(parts) != 2 { diff --git a/pkg/spec/postgresql_test.go b/pkg/spec/postgresql_test.go index 07251edeb..aef698782 100644 --- a/pkg/spec/postgresql_test.go +++ b/pkg/spec/postgresql_test.go @@ -4,11 +4,10 @@ import ( "bytes" "encoding/json" "errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "reflect" "testing" "time" - - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) var parseTimeTests = []struct { @@ -546,3 +545,15 @@ func TestPostgresListMeta(t *testing.T) { return } } + +func TestPostgresqlClone(t *testing.T) { + for _, tt := range unmarshalCluster { + cp := &tt.out + cp.Error = nil + clone := cp.Clone() + if !reflect.DeepEqual(clone, cp) { + t.Errorf("TestPostgresqlClone expected: \n%#v\n, got \n%#v", cp, clone) + } + + } +}