From 037d7120efc332558dec430f5fe78e64439816d0 Mon Sep 17 00:00:00 2001 From: Dmitrii Dolgov <9erthalion6@gmail.com> Date: Mon, 2 Mar 2020 16:32:15 +0100 Subject: [PATCH] Sync due to defaults Since we can miss it while checking only spec --- pkg/cluster/cluster.go | 68 +++++++++++++++++++++++++++++++++++- pkg/cluster/k8sres.go | 2 +- pkg/cluster/sync.go | 6 ++-- pkg/cluster/sync_test.go | 20 +++++++++++ pkg/util/constants/pooler.go | 2 ++ pkg/util/k8sutil/k8sutil.go | 12 +++++++ 6 files changed, 106 insertions(+), 4 deletions(-) diff --git a/pkg/cluster/cluster.go b/pkg/cluster/cluster.go index 627c15481..6fe94d08a 100644 --- a/pkg/cluster/cluster.go +++ b/pkg/cluster/cluster.go @@ -1206,7 +1206,7 @@ func (c *Cluster) deletePatroniClusterConfigMaps() error { // Test if two connection pool configuration needs to be synced. For simplicity // compare not the actual K8S objects, but the configuration itself and request // sync if there is any difference. -func (c *Cluster) needSyncConnPoolDeployments(oldSpec, newSpec *acidv1.ConnectionPool) (sync bool, reasons []string) { +func (c *Cluster) needSyncConnPoolSpecs(oldSpec, newSpec *acidv1.ConnectionPool) (sync bool, reasons []string) { reasons = []string{} sync = false @@ -1228,3 +1228,69 @@ func (c *Cluster) needSyncConnPoolDeployments(oldSpec, newSpec *acidv1.Connectio return sync, reasons } + +func syncResources(a, b *v1.ResourceRequirements) bool { + for _, res := range []v1.ResourceName{ + v1.ResourceCPU, + v1.ResourceMemory, + } { + if !a.Limits[res].Equal(b.Limits[res]) || + !a.Requests[res].Equal(b.Requests[res]) { + return true + } + } + + return false +} + +// Check if we need to synchronize connection pool deploymend due to new +// defaults, that are different from what we see in the DeploymentSpec +func (c *Cluster) needSyncConnPoolDefaults( + spec *acidv1.ConnectionPool, + deployment *appsv1.Deployment) (sync bool, reasons []string) { + + reasons = []string{} + sync = false + + config := c.OpConfig.ConnectionPool + podTemplate := deployment.Spec.Template + poolContainer := podTemplate.Spec.Containers[constants.ConnPoolContainer] + + if spec.NumberOfInstances == nil && + deployment.Spec.Replicas != config.NumberOfInstances { + + sync = true + msg := fmt.Sprintf("NumberOfInstances is different (%d vs %d)", + deployment.Spec.Replicas, config.NumberOfInstances) + reasons = append(reasons, msg) + } + + if spec.DockerImage == "" && + poolContainer.Image != config.Image { + + sync = true + msg := fmt.Sprintf("DockerImage is different (%s vs %s)", + poolContainer.Image, config.Image) + reasons = append(reasons, msg) + } + + expectedResources, err := generateResourceRequirements(spec.Resources, + c.makeDefaultConnPoolResources()) + + // An error to generate expected resources means something is not quite + // right, but for the purpose of robustness do not panic here, just report + // and ignore resources comparison (in the worst case there will be no + // updates for new resource values). + if err == nil && syncResources(&poolContainer.Resources, expectedResources) { + sync = true + msg := fmt.Sprintf("Resources are different (%+v vs %+v)", + poolContainer.Resources, expectedResources) + reasons = append(reasons, msg) + } + + if err != nil { + c.logger.Warningf("Cannot generate expected resources, %v", err) + } + + return sync, reasons +} diff --git a/pkg/cluster/k8sres.go b/pkg/cluster/k8sres.go index b60204233..53c608e1d 100644 --- a/pkg/cluster/k8sres.go +++ b/pkg/cluster/k8sres.go @@ -1754,7 +1754,7 @@ func (c *Cluster) generateConnPoolPodTemplate(spec *acidv1.PostgresSpec) ( secretSelector := func(key string) *v1.SecretKeySelector { return &v1.SecretKeySelector{ LocalObjectReference: v1.LocalObjectReference{ - Name: c.credentialSecretName(c.OpConfig.SuperUsername), + Name: c.credentialSecretName(c.OpConfig.ConnectionPool.User), }, Key: key, } diff --git a/pkg/cluster/sync.go b/pkg/cluster/sync.go index f8d242d99..307f703ee 100644 --- a/pkg/cluster/sync.go +++ b/pkg/cluster/sync.go @@ -702,8 +702,10 @@ func (c *Cluster) syncConnectionPoolWorker(oldSpec, newSpec *acidv1.Postgresql) // actual synchronization oldConnPool := oldSpec.Spec.ConnectionPool newConnPool := newSpec.Spec.ConnectionPool - sync, reason := c.needSyncConnPoolDeployments(oldConnPool, newConnPool) - if sync { + specSync, specReason := c.needSyncConnPoolSpecs(oldConnPool, newConnPool) + defaultsSync, defaultsReason := c.needSyncConnPoolDefaults(newConnPool, deployment) + reason := append(specReason, defaultsReason...) + if specSync || defaultsSync { c.logger.Infof("Update connection pool deployment %s, reason: %s", c.connPoolName(), reason) diff --git a/pkg/cluster/sync_test.go b/pkg/cluster/sync_test.go index ab58b2074..8087a7db4 100644 --- a/pkg/cluster/sync_test.go +++ b/pkg/cluster/sync_test.go @@ -88,6 +88,11 @@ func TestConnPoolSynchronization(t *testing.T) { Service: &v1.Service{}, } + clusterNewDefaultsMock := *cluster + clusterNewDefaultsMock.KubeClient = k8sutil.NewMockKubernetesClient() + cluster.OpConfig.ConnectionPool.Image = "pooler:2.0" + cluster.OpConfig.ConnectionPool.NumberOfInstances = int32ToPointer(2) + tests := []struct { subTest string oldSpec *acidv1.Postgresql @@ -166,6 +171,21 @@ func TestConnPoolSynchronization(t *testing.T) { cluster: &clusterMock, check: deploymentUpdated, }, + { + subTest: "update image from changed defaults", + oldSpec: &acidv1.Postgresql{ + Spec: acidv1.PostgresSpec{ + ConnectionPool: &acidv1.ConnectionPool{}, + }, + }, + newSpec: &acidv1.Postgresql{ + Spec: acidv1.PostgresSpec{ + ConnectionPool: &acidv1.ConnectionPool{}, + }, + }, + cluster: &clusterNewDefaultsMock, + check: deploymentUpdated, + }, } for _, tt := range tests { err := tt.cluster.syncConnectionPool(tt.oldSpec, tt.newSpec) diff --git a/pkg/util/constants/pooler.go b/pkg/util/constants/pooler.go index 4d5edaa57..aa8171110 100644 --- a/pkg/util/constants/pooler.go +++ b/pkg/util/constants/pooler.go @@ -10,4 +10,6 @@ const ( ConnectionPoolDefaultCpuLimit = "100m" ConnectionPoolDefaultMemoryRequest = "100Mi" ConnectionPoolDefaultMemoryLimit = "100Mi" + + ConnPoolContainer = 0 ) diff --git a/pkg/util/k8sutil/k8sutil.go b/pkg/util/k8sutil/k8sutil.go index dd062b209..6e0798312 100644 --- a/pkg/util/k8sutil/k8sutil.go +++ b/pkg/util/k8sutil/k8sutil.go @@ -300,6 +300,18 @@ func (mock *mockDeployment) Get(name string, opts metav1.GetOptions) (*apiappsv1 ObjectMeta: metav1.ObjectMeta{ Name: "test-deployment", }, + Spec: apiappsv1.DeploymentSpec{ + Replicas: int32ToPointer(1), + Template: v1.PodTemplateSpec{ + Spec: v1.PodSpec{ + Containers: []v1.Container{ + v1.Container{ + Image: "pooler:1.0", + }, + }, + }, + }, + }, }, nil }