Remove operator checks that prevent PG major version upgrade (#1160)

* remove checks that prevent major version upgrade

Co-authored-by: Sergey Dudoladov <sergey.dudoladov@zalando.de>

docs/administrator.md

@@ -11,17 +11,15 @@ switchover (planned failover) of the master to the Pod with new minor version.
 The switch should usually take less than 5 seconds, still clients have to
 reconnect.
 
-Major version upgrades are supported via [cloning](user.md#how-to-clone-an-existing-postgresql-cluster).
-The new cluster manifest must have a higher `version` string than the source
+Major version upgrades are supported either via [cloning](user.md#how-to-clone-an-existing-postgresql-cluster)or in-place.
+
+With cloning, the new cluster manifest must have a higher `version` string than the source
 cluster and will be created from a basebackup. Depending of the cluster size,
 downtime in this case can be significant as writes to the database should be
 stopped and all WAL files should be archived first before cloning is started.
 
-Note, that simply changing the version string in the `postgresql` manifest does
-not work at present and leads to errors. Neither Patroni nor Postgres Operator
-can do in place `pg_upgrade`. Still, it can be executed manually in the Postgres
-container, which is tricky (i.e. systems need to be stopped, replicas have to be
-synced) but of course faster than cloning.
+Starting with Spilo 13, Postgres Operator can do in-place major version upgrade, which should be faster than cloning. To trigger the upgrade, simply increase the version in the cluster manifest. As the very last step of
+processing the manifest update event, the operator will call the `inplace_upgrade.py` script in Spilo. The upgrade is usually fast, well under one minute for most DBs. Note the changes become irrevertible once `pg_upgrade` is called. To understand the upgrade procedure, refer to the [corresponding PR in Spilo](https://github.com/zalando/spilo/pull/488).
 
 ## CRD Validation
 

docs/user.md

@@ -542,6 +542,10 @@ section in the spec. There are two options here:
 Note, that cloning can also be used for [major version upgrades](administrator.md#minor-and-major-version-upgrade)
 of PostgreSQL.
 
+## In-place major version upgrade
+
+Starting with Spilo 13, operator supports in-place major version upgrade to a higher major version (e.g. from PG 10 to PG 12). To trigger the upgrade, simply increase the version in the manifest. It is your responsibility to test your applications against the new version before the upgrade; downgrading is not supported. The easiest way to do so is to try the upgrade on the cloned cluster first. For details of how Spilo does the upgrade [see here](https://github.com/zalando/spilo/pull/488), operator implementation is described [in the admin docs](administrator.md#minor-and-major-version-upgrade).
+
 ### Clone from S3
 
 Cloning from S3 has the advantage that there is no impact on your production

pkg/cluster/cluster.go

@@ -626,13 +626,16 @@ func (c *Cluster) Update(oldSpec, newSpec *acidv1.Postgresql) error {
 		}
 	}()
 
-	if oldSpec.Spec.PostgresqlParam.PgVersion != newSpec.Spec.PostgresqlParam.PgVersion { // PG versions comparison
+	if oldSpec.Spec.PostgresqlParam.PgVersion >= newSpec.Spec.PostgresqlParam.PgVersion {
 		c.logger.Warningf("postgresql version change(%q -> %q) has no effect",
 			oldSpec.Spec.PostgresqlParam.PgVersion, newSpec.Spec.PostgresqlParam.PgVersion)
 		c.eventRecorder.Eventf(c.GetReference(), v1.EventTypeWarning, "PostgreSQL", "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.PostgresqlParam.PgVersion = oldSpec.Spec.PostgresqlParam.PgVersion
+	} else {
+		c.logger.Infof("postgresql version increased (%q -> %q), major version upgrade can be done manually after StatefulSet Sync",
+			oldSpec.Spec.PostgresqlParam.PgVersion, newSpec.Spec.PostgresqlParam.PgVersion)
 	}
 
 	// Service

pkg/cluster/k8sres.go

@@ -909,41 +909,6 @@ func extractPgVersionFromBinPath(binPath string, template string) (string, error
 	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 (

pkg/cluster/k8sres_test.go

@@ -557,95 +557,6 @@ func TestExtractPgVersionFromBinPath(t *testing.T) {
 	}
 }
 
-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, eventRecorder)
-
-	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 {

pkg/cluster/sync.go

@@ -332,18 +332,6 @@ 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)