From b66734a0a9a94147bc99bffa6844e146b036a7f7 Mon Sep 17 00:00:00 2001 From: Felix Kunde Date: Fri, 13 Mar 2020 11:48:19 +0100 Subject: [PATCH] omit PgVersion diff on sync (#860) * use PostgresParam.PgVersion everywhere * on sync compare pgVersion with SpiloConfiguration * update getNewPgVersion and added tests --- kubectl-pg/cmd/list.go | 25 ++++--- pkg/cluster/cluster.go | 7 +- pkg/cluster/k8sres.go | 48 +++++++++++++- pkg/cluster/k8sres_test.go | 129 +++++++++++++++++++++++++++++++++++++ pkg/cluster/sync.go | 12 ++++ 5 files changed, 208 insertions(+), 13 deletions(-) diff --git a/kubectl-pg/cmd/list.go b/kubectl-pg/cmd/list.go index df827ffaf..f4dea882d 100644 --- a/kubectl-pg/cmd/list.go +++ b/kubectl-pg/cmd/list.go @@ -24,13 +24,14 @@ package cmd import ( "fmt" - "github.com/spf13/cobra" - "github.com/zalando/postgres-operator/pkg/apis/acid.zalan.do/v1" - PostgresqlLister "github.com/zalando/postgres-operator/pkg/generated/clientset/versioned/typed/acid.zalan.do/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "log" "strconv" "time" + + "github.com/spf13/cobra" + v1 "github.com/zalando/postgres-operator/pkg/apis/acid.zalan.do/v1" + PostgresqlLister "github.com/zalando/postgres-operator/pkg/generated/clientset/versioned/typed/acid.zalan.do/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) const ( @@ -95,8 +96,12 @@ func listAll(listPostgres *v1.PostgresqlList) { template := "%-32s%-16s%-12s%-12s%-12s%-12s%-12s\n" fmt.Printf(template, "NAME", "STATUS", "INSTANCES", "VERSION", "AGE", "VOLUME", "NAMESPACE") for _, pgObjs := range listPostgres.Items { - fmt.Printf(template, pgObjs.Name, pgObjs.Status.PostgresClusterStatus, strconv.Itoa(int(pgObjs.Spec.NumberOfInstances)), - pgObjs.Spec.PgVersion, time.Since(pgObjs.CreationTimestamp.Time).Truncate(TrimCreateTimestamp), pgObjs.Spec.Size, pgObjs.Namespace) + fmt.Printf(template, pgObjs.Name, + pgObjs.Status.PostgresClusterStatus, + strconv.Itoa(int(pgObjs.Spec.NumberOfInstances)), + pgObjs.Spec.PostgresqlParam.PgVersion, + time.Since(pgObjs.CreationTimestamp.Time).Truncate(TrimCreateTimestamp), + pgObjs.Spec.Size, pgObjs.Namespace) } } @@ -104,8 +109,12 @@ func listWithNamespace(listPostgres *v1.PostgresqlList) { template := "%-32s%-16s%-12s%-12s%-12s%-12s\n" fmt.Printf(template, "NAME", "STATUS", "INSTANCES", "VERSION", "AGE", "VOLUME") for _, pgObjs := range listPostgres.Items { - fmt.Printf(template, pgObjs.Name, pgObjs.Status.PostgresClusterStatus, strconv.Itoa(int(pgObjs.Spec.NumberOfInstances)), - pgObjs.Spec.PgVersion, time.Since(pgObjs.CreationTimestamp.Time).Truncate(TrimCreateTimestamp), pgObjs.Spec.Size) + fmt.Printf(template, pgObjs.Name, + pgObjs.Status.PostgresClusterStatus, + strconv.Itoa(int(pgObjs.Spec.NumberOfInstances)), + pgObjs.Spec.PostgresqlParam.PgVersion, + time.Since(pgObjs.CreationTimestamp.Time).Truncate(TrimCreateTimestamp), + pgObjs.Spec.Size) } } diff --git a/pkg/cluster/cluster.go b/pkg/cluster/cluster.go index 91e7a5195..d740260d2 100644 --- a/pkg/cluster/cluster.go +++ b/pkg/cluster/cluster.go @@ -554,10 +554,11 @@ func (c *Cluster) Update(oldSpec, newSpec *acidv1.Postgresql) error { } }() - if oldSpec.Spec.PgVersion != newSpec.Spec.PgVersion { // PG versions comparison - c.logger.Warningf("postgresql version change(%q -> %q) has no effect", oldSpec.Spec.PgVersion, newSpec.Spec.PgVersion) + if oldSpec.Spec.PostgresqlParam.PgVersion != newSpec.Spec.PostgresqlParam.PgVersion { // PG versions comparison + c.logger.Warningf("postgresql version change(%q -> %q) has no effect", + oldSpec.Spec.PostgresqlParam.PgVersion, newSpec.Spec.PostgresqlParam.PgVersion) //we need that hack to generate statefulset with the old version - newSpec.Spec.PgVersion = oldSpec.Spec.PgVersion + newSpec.Spec.PostgresqlParam.PgVersion = oldSpec.Spec.PostgresqlParam.PgVersion } // Service diff --git a/pkg/cluster/k8sres.go b/pkg/cluster/k8sres.go index 84c407700..aaa27384a 100644 --- a/pkg/cluster/k8sres.go +++ b/pkg/cluster/k8sres.go @@ -27,7 +27,7 @@ import ( ) const ( - pgBinariesLocationTemplate = "/usr/lib/postgresql/%s/bin" + pgBinariesLocationTemplate = "/usr/lib/postgresql/%v/bin" patroniPGBinariesParameterName = "bin_dir" patroniPGParametersParameterName = "parameters" patroniPGHBAConfParameterName = "pg_hba" @@ -722,6 +722,50 @@ func makeResources(cpuRequest, memoryRequest, cpuLimit, memoryLimit string) acid } } +func extractPgVersionFromBinPath(binPath string, template string) (string, error) { + var pgVersion float32 + _, err := fmt.Sscanf(binPath, template, &pgVersion) + if err != nil { + return "", err + } + return fmt.Sprintf("%v", pgVersion), nil +} + +func (c *Cluster) getNewPgVersion(container v1.Container, newPgVersion string) (string, error) { + var ( + spiloConfiguration spiloConfiguration + runningPgVersion string + err error + ) + + for _, env := range container.Env { + if env.Name != "SPILO_CONFIGURATION" { + continue + } + err = json.Unmarshal([]byte(env.Value), &spiloConfiguration) + if err != nil { + return newPgVersion, err + } + } + + if len(spiloConfiguration.PgLocalConfiguration) > 0 { + currentBinPath := fmt.Sprintf("%v", spiloConfiguration.PgLocalConfiguration[patroniPGBinariesParameterName]) + runningPgVersion, err = extractPgVersionFromBinPath(currentBinPath, pgBinariesLocationTemplate) + if err != nil { + return "", fmt.Errorf("could not extract Postgres version from %v in SPILO_CONFIGURATION", currentBinPath) + } + } else { + return "", fmt.Errorf("could not find %q setting in SPILO_CONFIGURATION", patroniPGBinariesParameterName) + } + + if runningPgVersion != newPgVersion { + c.logger.Warningf("postgresql version change(%q -> %q) has no effect", runningPgVersion, newPgVersion) + newPgVersion = runningPgVersion + } + + return newPgVersion, nil +} + func (c *Cluster) generateStatefulSet(spec *acidv1.PostgresSpec) (*appsv1.StatefulSet, error) { var ( @@ -1680,7 +1724,7 @@ func (c *Cluster) generateLogicalBackupPodEnvVars() []v1.EnvVar { // Postgres env vars { Name: "PG_VERSION", - Value: c.Spec.PgVersion, + Value: c.Spec.PostgresqlParam.PgVersion, }, { Name: "PGPORT", diff --git a/pkg/cluster/k8sres_test.go b/pkg/cluster/k8sres_test.go index 7fd4cd3e6..25e0f7af4 100644 --- a/pkg/cluster/k8sres_test.go +++ b/pkg/cluster/k8sres_test.go @@ -382,6 +382,135 @@ func TestCloneEnv(t *testing.T) { } } +func TestExtractPgVersionFromBinPath(t *testing.T) { + testName := "TestExtractPgVersionFromBinPath" + tests := []struct { + subTest string + binPath string + template string + expected string + }{ + { + subTest: "test current bin path with decimal against hard coded template", + binPath: "/usr/lib/postgresql/9.6/bin", + template: pgBinariesLocationTemplate, + expected: "9.6", + }, + { + subTest: "test current bin path against hard coded template", + binPath: "/usr/lib/postgresql/12/bin", + template: pgBinariesLocationTemplate, + expected: "12", + }, + { + subTest: "test alternative bin path against a matching template", + binPath: "/usr/pgsql-12/bin", + template: "/usr/pgsql-%v/bin", + expected: "12", + }, + } + + for _, tt := range tests { + pgVersion, err := extractPgVersionFromBinPath(tt.binPath, tt.template) + if err != nil { + t.Errorf("Unexpected error: %v", err) + } + if pgVersion != tt.expected { + t.Errorf("%s %s: Expected version %s, have %s instead", + testName, tt.subTest, tt.expected, pgVersion) + } + } +} + +func TestGetPgVersion(t *testing.T) { + testName := "TestGetPgVersion" + tests := []struct { + subTest string + pgContainer v1.Container + currentPgVersion string + newPgVersion string + }{ + { + subTest: "new version with decimal point differs from current SPILO_CONFIGURATION", + pgContainer: v1.Container{ + Name: "postgres", + Env: []v1.EnvVar{ + { + Name: "SPILO_CONFIGURATION", + Value: "{\"postgresql\": {\"bin_dir\": \"/usr/lib/postgresql/9.6/bin\"}}", + }, + }, + }, + currentPgVersion: "9.6", + newPgVersion: "12", + }, + { + subTest: "new version differs from current SPILO_CONFIGURATION", + pgContainer: v1.Container{ + Name: "postgres", + Env: []v1.EnvVar{ + { + Name: "SPILO_CONFIGURATION", + Value: "{\"postgresql\": {\"bin_dir\": \"/usr/lib/postgresql/11/bin\"}}", + }, + }, + }, + currentPgVersion: "11", + newPgVersion: "12", + }, + { + subTest: "new version is lower than the one found in current SPILO_CONFIGURATION", + pgContainer: v1.Container{ + Name: "postgres", + Env: []v1.EnvVar{ + { + Name: "SPILO_CONFIGURATION", + Value: "{\"postgresql\": {\"bin_dir\": \"/usr/lib/postgresql/12/bin\"}}", + }, + }, + }, + currentPgVersion: "12", + newPgVersion: "11", + }, + { + subTest: "new version is the same like in the current SPILO_CONFIGURATION", + pgContainer: v1.Container{ + Name: "postgres", + Env: []v1.EnvVar{ + { + Name: "SPILO_CONFIGURATION", + Value: "{\"postgresql\": {\"bin_dir\": \"/usr/lib/postgresql/12/bin\"}}", + }, + }, + }, + currentPgVersion: "12", + newPgVersion: "12", + }, + } + + var cluster = New( + Config{ + OpConfig: config.Config{ + ProtectedRoles: []string{"admin"}, + Auth: config.Auth{ + SuperUsername: superUserName, + ReplicationUsername: replicationUserName, + }, + }, + }, k8sutil.KubernetesClient{}, acidv1.Postgresql{}, logger) + + for _, tt := range tests { + pgVersion, err := cluster.getNewPgVersion(tt.pgContainer, tt.newPgVersion) + if err != nil { + t.Errorf("Unexpected error: %v", err) + } + if pgVersion != tt.currentPgVersion { + t.Errorf("%s %s: Expected version %s, have %s instead", + testName, tt.subTest, tt.currentPgVersion, pgVersion) + } + } +} + func TestSecretVolume(t *testing.T) { testName := "TestSecretVolume" tests := []struct { diff --git a/pkg/cluster/sync.go b/pkg/cluster/sync.go index 053db9ff7..b04ff863b 100644 --- a/pkg/cluster/sync.go +++ b/pkg/cluster/sync.go @@ -285,6 +285,18 @@ func (c *Cluster) syncStatefulSet() error { // statefulset is already there, make sure we use its definition in order to compare with the spec. c.Statefulset = sset + // check if there is no Postgres version mismatch + for _, container := range c.Statefulset.Spec.Template.Spec.Containers { + if container.Name != "postgres" { + continue + } + pgVersion, err := c.getNewPgVersion(container, c.Spec.PostgresqlParam.PgVersion) + if err != nil { + return fmt.Errorf("could not parse current Postgres version: %v", err) + } + c.Spec.PostgresqlParam.PgVersion = pgVersion + } + desiredSS, err := c.generateStatefulSet(&c.Spec) if err != nil { return fmt.Errorf("could not generate statefulset: %v", err)