Repository: FoundationDB/fdb-kubernetes-operator
Pull request #1831 (2023-10-11 07:15:13)

Allow e2e test suite to specify downgrade versions
# Description

Allow to use downgrade versions in the upgrade tests. This is a preparation for: https://github.com/FoundationDB/fdb-kubernetes-operator/issues/1712

## Type of change

*Please select one of the options below.*

- New feature (non-breaking change which adds functionality)

## Discussion

-

## Testing

Ran a manual test.

## Documentation

-

## Follow-up

-

foundationdb-ci: 
### Result of fdb-kubernetes-operator-pr on Linux CentOS 7

* Commit ID: 4e125ac6b8fb11323c60e6eb22b045df7b652a02
* Duration 0:08:20
* Result: :x: **FAILED**
* Error: `Error while executing command: if $fail_test; then exit 1; fi. Reason: exit status 1`
* [Build Log](https://d1e0l78xbkh3xa.cloudfront.net/fdb-kubernetes-operator/fdb-kubernetes-operator-pr/992f37a8-373f-4cc1-9bfb-dd2e633a5c96.gz) terminal output (available for 30 days)
* [Build Workspace](https://d1e0l78xbkh3xa.cloudfront.net/fdb-kubernetes-operator-pr/992f37a8-373f-4cc1-9bfb-dd2e633a5c96/workspace.zip) zip file of the working directory (available for 30 days)


Patch:
diff --git a/api/v1beta2/foundationdb_version.go b/api/v1beta2/foundationdb_version.go
index 11e41643..b68aff8a 100644
--- a/api/v1beta2/foundationdb_version.go
+++ b/api/v1beta2/foundationdb_version.go
@@ -214,6 +214,11 @@ func (version Version) SupportsDNSInClusterFile() bool {
 	return version.IsAtLeast(Versions.SupportsDNSInClusterFile)
 }
 
+// SupportsVersionChange returns true if the current version can be downgraded or upgraded to provided other version.
+func (version Version) SupportsVersionChange(other Version) bool {
+	return version.IsProtocolCompatible(other) || other.IsAtLeast(version)
+}
+
 // Versions provides a shorthand for known versions.
 // This is only to be used in testing.
 var Versions = struct {
diff --git a/api/v1beta2/foundationdbcluster_types.go b/api/v1beta2/foundationdbcluster_types.go
index 53117790..b400ecfc 100644
--- a/api/v1beta2/foundationdbcluster_types.go
+++ b/api/v1beta2/foundationdbcluster_types.go
@@ -2730,6 +2730,10 @@ func (cluster *FoundationDBCluster) Validate() error {
 		return err
 	}
 
+	if !version.IsSupported() {
+		return fmt.Errorf("version: %s is not supported, minimum supported version is: %s", version.String(), Versions.MinimumVersion.String())
+	}
+
 	if !version.IsStorageEngineSupported(cluster.Spec.DatabaseConfiguration.StorageEngine) {
 		validations = append(validations, fmt.Sprintf("storage engine %s is not supported on version %s", cluster.Spec.DatabaseConfiguration.StorageEngine, cluster.Spec.Version))
 	}
diff --git a/controllers/check_client_compatibility.go b/controllers/check_client_compatibility.go
index c025582d..cbaf86f6 100644
--- a/controllers/check_client_compatibility.go
+++ b/controllers/check_client_compatibility.go
@@ -53,8 +53,8 @@ func (c checkClientCompatibility) reconcile(_ context.Context, r *FoundationDBCl
 		return &requeue{curError: err}
 	}
 
-	if !version.IsAtLeast(runningVersion) && !version.IsProtocolCompatible(runningVersion) {
-		return &requeue{message: fmt.Sprintf("cluster downgrade operation is only supported for protocol compatible versions, running version %s and desired version %s are not compatible", runningVersion, version)}
+	if !runningVersion.SupportsVersionChange(version) {
+		return &requeue{message: fmt.Sprintf("cluster version change from version %s to version %s is not supported", runningVersion, version)}
 	}
 
 	if version.IsProtocolCompatible(runningVersion) {
diff --git a/controllers/cluster_controller.go b/controllers/cluster_controller.go
index 2bd098b6..5ef48034 100644
--- a/controllers/cluster_controller.go
+++ b/controllers/cluster_controller.go
@@ -117,6 +117,12 @@ func (r *FoundationDBClusterReconciler) Reconcile(ctx context.Context, request c
 	}
 	defer adminClient.Close()
 
+	err = cluster.Validate()
+	if err != nil {
+		r.Recorder.Event(cluster, corev1.EventTypeWarning, "ClusterSpec not valid", err.Error())
+		return ctrl.Result{}, fmt.Errorf("ClusterSpec is not valid: %w", err)
+	}
+
 	supportedVersion, err := adminClient.VersionSupported(cluster.Spec.Version)
 	if err != nil {
 		return ctrl.Result{}, err
@@ -125,12 +131,6 @@ func (r *FoundationDBClusterReconciler) Reconcile(ctx context.Context, request c
 		return ctrl.Result{}, fmt.Errorf("version %s is not supported", cluster.Spec.Version)
 	}
 
-	err = cluster.Validate()
-	if err != nil {
-		r.Recorder.Event(cluster, corev1.EventTypeWarning, "ClusterSpec not valid", err.Error())
-		return ctrl.Result{}, fmt.Errorf("ClusterSpec is not valid: %w", err)
-	}
-
 	var status *fdbv1beta2.FoundationDBStatus
 	if cacheStatus {
 		clusterLog.Info("Fetch machine-readable status for reconcilitation loop", "cacheStatus", cacheStatus)
diff --git a/fdbclient/admin_client.go b/fdbclient/admin_client.go
index 15ac014a..01a84775 100644
--- a/fdbclient/admin_client.go
+++ b/fdbclient/admin_client.go
@@ -505,16 +505,7 @@ func (client *cliAdminClient) GetConnectionString() (string, error) {
 // VersionSupported reports whether we can support a cluster with a given
 // version.
 func (client *cliAdminClient) VersionSupported(versionString string) (bool, error) {
-	version, err := fdbv1beta2.ParseFdbVersion(versionString)
-	if err != nil {
-		return false, err
-	}
-
-	if !version.IsSupported() {
-		return false, nil
-	}
-
-	_, err = os.Stat(getBinaryPath(fdbcliStr, versionString))
+	_, err := os.Stat(getBinaryPath(fdbcliStr, versionString))
 	if err != nil {
 		return false, err
 	}


Test patch:
diff --git a/api/v1beta2/foundationdb_version_test.go b/api/v1beta2/foundationdb_version_test.go
index aa559d1e..132a2314 100644
--- a/api/v1beta2/foundationdb_version_test.go
+++ b/api/v1beta2/foundationdb_version_test.go
@@ -154,6 +154,101 @@ var _ = Describe("[api] FDBVersion", func() {
 					expectedResult:         true,
 				}),
 		)
-
 	})
+
+	DescribeTable("validating in a version check is allowed", func(version Version, targetVersion Version, expected bool) {
+		Expect(version.SupportsVersionChange(targetVersion)).To(Equal(expected))
+	},
+		Entry("Same version",
+			Version{
+				Major: 7,
+				Minor: 1,
+				Patch: 0,
+			},
+			Version{
+				Major: 7,
+				Minor: 1,
+				Patch: 0,
+			},
+			true,
+		),
+		Entry("Patch upgrade",
+			Version{
+				Major: 7,
+				Minor: 1,
+				Patch: 0,
+			},
+			Version{
+				Major: 7,
+				Minor: 1,
+				Patch: 1,
+			},
+			true,
+		),
+		Entry("Minor upgrade",
+			Version{
+				Major: 7,
+				Minor: 1,
+				Patch: 0,
+			},
+			Version{
+				Major: 7,
+				Minor: 2,
+				Patch: 0,
+			},
+			true,
+		),
+		Entry("Major upgrade",
+			Version{
+				Major: 7,
+				Minor: 1,
+				Patch: 0,
+			},
+			Version{
+				Major: 8,
+				Minor: 1,
+				Patch: 0,
+			},
+			true,
+		),
+		Entry("Patch downgrade",
+			Version{
+				Major: 7,
+				Minor: 1,
+				Patch: 1,
+			},
+			Version{
+				Major: 7,
+				Minor: 1,
+				Patch: 0,
+			},
+			true,
+		),
+		Entry("Minor downgrade",
+			Version{
+				Major: 7,
+				Minor: 1,
+				Patch: 1,
+			},
+			Version{
+				Major: 7,
+				Minor: 0,
+				Patch: 0,
+			},
+			false,
+		),
+		Entry("Major downgrade",
+			Version{
+				Major: 7,
+				Minor: 1,
+				Patch: 1,
+			},
+			Version{
+				Major: 6,
+				Minor: 1,
+				Patch: 0,
+			},
+			false,
+		),
+	)
 })
diff --git a/api/v1beta2/foundationdbcluster_types_test.go b/api/v1beta2/foundationdbcluster_types_test.go
index 27c52e04..758c7bcc 100644
--- a/api/v1beta2/foundationdbcluster_types_test.go
+++ b/api/v1beta2/foundationdbcluster_types_test.go
@@ -4746,10 +4746,9 @@ var _ = Describe("[api] FoundationDBCluster", func() {
 			func(cluster *FoundationDBCluster, expected error) {
 				if expected == nil {
 					Expect(cluster.Validate()).NotTo(HaveOccurred())
-				} else {
-					Expect(cluster.Validate()).To(Equal(expected))
+					return
 				}
-
+				Expect(cluster.Validate()).To(Equal(expected))
 			},
 			Entry("valid cluster spec",
 				&FoundationDBCluster{
@@ -4851,7 +4850,7 @@ var _ = Describe("[api] FoundationDBCluster", func() {
 			Entry("multiple validations",
 				&FoundationDBCluster{
 					Spec: FoundationDBClusterSpec{
-						Version: "6.1.3",
+						Version: "6.2.20",
 						DatabaseConfiguration: DatabaseConfiguration{
 							StorageEngine: StorageEngineRocksDbV1,
 						},
@@ -4862,7 +4861,7 @@ var _ = Describe("[api] FoundationDBCluster", func() {
 						},
 					},
 				},
-				fmt.Errorf("storage engine ssd-rocksdb-v1 is not supported on version 6.1.3, stateless is not a valid process class for coordinators"),
+				fmt.Errorf("storage engine ssd-rocksdb-v1 is not supported on version 6.2.20, stateless is not a valid process class for coordinators"),
 			),
 			Entry("using invalid version for sharded rocksdb",
 				&FoundationDBCluster{
@@ -4886,6 +4885,17 @@ var _ = Describe("[api] FoundationDBCluster", func() {
 				},
 				nil,
 			),
+			Entry("using an unsupported FDB version",
+				&FoundationDBCluster{
+					Spec: FoundationDBClusterSpec{
+						Version: Versions.IncompatibleVersion.String(),
+						DatabaseConfiguration: DatabaseConfiguration{
+							StorageEngine: StorageEngineSSD2,
+						},
+					},
+				},
+				fmt.Errorf("version: 6.1.0 is not supported, minimum supported version is: 6.2.20"),
+			),
 		)
 	})
 
diff --git a/controllers/exclude_processes_test.go b/controllers/exclude_processes_test.go
index 231960cc..8e7cfc62 100644
--- a/controllers/exclude_processes_test.go
+++ b/controllers/exclude_processes_test.go
@@ -63,7 +63,7 @@ var _ = Describe("exclude_processes", func() {
 			allowedExclusions, missingProcesses = canExcludeNewProcesses(globalControllerLogger, cluster, fdbv1beta2.ProcessClassStorage, processCounts.Storage, ongoingExclusions, false)
 		})
 
-		FWhen("using a small cluster", func() {
+		When("using a small cluster", func() {
 			When("all processes are healthy", func() {
 				When("no additional processes are running", func() {
 					It("should not allow the exclusion", func() {
diff --git a/e2e/fixtures/upgrade_test_configuration.go b/e2e/fixtures/upgrade_test_configuration.go
index 401895d1..cbf43cbd 100644
--- a/e2e/fixtures/upgrade_test_configuration.go
+++ b/e2e/fixtures/upgrade_test_configuration.go
@@ -29,7 +29,7 @@ import (
 )
 
 // UpgradeTestConfiguration represents the configuration for an upgrade test. This includes the initial FoundationDB version
-// and the target FoundationDB version to upgrade the cluster to.
+// and the target FoundationDB version to upgrade or downgrade the cluster to.
 type UpgradeTestConfiguration struct {
 	// InitialVersion represents the version before the upgrade.
 	InitialVersion fdbv1beta2.Version
@@ -37,7 +37,7 @@ type UpgradeTestConfiguration struct {
 	TargetVersion fdbv1beta2.Version
 }
 
-func parseUpgradeVersionPair(upgradeConfig string) UpgradeTestConfiguration {
+func parseUpgradeVersionPair(upgradeConfig string) *UpgradeTestConfiguration {
 	versions := strings.Split(upgradeConfig, ":")
 	if len(versions) != 2 {
 		log.Fatalf(
@@ -46,6 +46,11 @@ func parseUpgradeVersionPair(upgradeConfig string) UpgradeTestConfiguration {
 		)
 	}
 
+	// If both versions are the same ignore it.
+	if versions[0] == versions[1] {
+		return nil
+	}
+
 	initialVersion, err := fdbv1beta2.ParseFdbVersion(versions[0])
 	if err != nil {
 		log.Fatalf("\"%s\" is not a valid FDB version", versions[0])
@@ -56,11 +61,11 @@ func parseUpgradeVersionPair(upgradeConfig string) UpgradeTestConfiguration {
 		log.Fatalf("\"%s\" is not a valid FDB version", versions[1])
 	}
 
-	if initialVersion.IsAtLeast(targetVersion) {
-		log.Fatalf("downgrade from \"%s\" to \"%s\" is not supported", versions[0], versions[1])
+	if !initialVersion.SupportsVersionChange(targetVersion) {
+		log.Fatalf("version change from \"%s\" to \"%s\" is not supported", versions[0], versions[1])
 	}
 
-	return UpgradeTestConfiguration{
+	return &UpgradeTestConfiguration{
 		InitialVersion: initialVersion,
 		TargetVersion:  targetVersion,
 	}
@@ -69,7 +74,7 @@ func parseUpgradeVersionPair(upgradeConfig string) UpgradeTestConfiguration {
 // GetUpgradeVersions returns the upgrade versions as a string slice based on the command line flag. Each entry will be
 // a FoundationDB version. This slice can contain duplicate entries. For upgrade tests it's expected that two versions
 // form one test, e.g. where the odd number is the initial version and the even number is the
-func (factory *Factory) GetUpgradeVersions() []UpgradeTestConfiguration {
+func (factory *Factory) GetUpgradeVersions() []*UpgradeTestConfiguration {
 	return getUpgradeVersions(factory.options.upgradeString)
 }
 
@@ -77,15 +82,20 @@ func (factory *Factory) GetUpgradeVersions() []UpgradeTestConfiguration {
 // a FoundationDB version. This slice can contain duplicate entries. For upgrade tests it's expected that two versions
 // form one test, e.g. where the odd number is the initial version and the even number is the
 // This method is only internally used. Users that import this test suite should use the factory method.
-func getUpgradeVersions(upgradeString string) []UpgradeTestConfiguration {
+func getUpgradeVersions(upgradeString string) []*UpgradeTestConfiguration {
 	if upgradeString == "" {
 		return nil
 	}
 
 	upgradeVersionStrings := strings.Split(upgradeString, ",")
-	upgradeVersions := make([]UpgradeTestConfiguration, 0, len(upgradeVersionStrings))
+	upgradeVersions := make([]*UpgradeTestConfiguration, 0, len(upgradeVersionStrings))
 	for _, upgradeTest := range upgradeVersionStrings {
-		upgradeVersions = append(upgradeVersions, parseUpgradeVersionPair(upgradeTest))
+		upgradeTestConfig := parseUpgradeVersionPair(upgradeTest)
+		if upgradeTestConfig == nil {
+			continue
+		}
+
+		upgradeVersions = append(upgradeVersions, upgradeTestConfig)
 	}
 
 	return upgradeVersions
diff --git a/e2e/test_operator_velocity/operator_velocity_test.go b/e2e/test_operator_velocity/operator_velocity_test.go
index aa0e5ffa..219db0cf 100644
--- a/e2e/test_operator_velocity/operator_velocity_test.go
+++ b/e2e/test_operator_velocity/operator_velocity_test.go
@@ -233,7 +233,6 @@ var _ = Describe("Test Operator Velocity", Label("e2e", "nightly"), func() {
 			).NotTo(HaveOccurred())
 
 			cluster := fdbCluster.GetPrimary().GetCluster()
-
 			CheckKnobRollout(
 				fdbCluster,
 				newGeneralCustomParameters,
@@ -259,9 +258,7 @@ var _ = Describe("Test Operator Velocity", Label("e2e", "nightly"), func() {
 			// Partition a storage Pod from the rest of the cluster
 			pod := fixtures.ChooseRandomPod(fdbCluster.GetPrimary().GetStoragePods())
 			log.Printf("partition Pod: %s", pod.Name)
-			var err error
 			exp = factory.InjectPartition(fixtures.PodSelector(pod))
-			Expect(err).ShouldNot(HaveOccurred())
 		})
 
 		AfterEach(func() {
@@ -287,11 +284,16 @@ var _ = Describe("Test Operator Velocity", Label("e2e", "nightly"), func() {
 				),
 			).NotTo(HaveOccurred())
 
+			// We have to increase the timeout for this test as the maximum time this test might take, depends on the
+			// ordering of the operators taking the kill action. If the operator in the primary cluster is the last one
+			// the test will pass fairly fast. If the primary operator is not the last operator, this test take longer
+			// as the partitioned Pod will block the release of the lock and the next operator has to wait until the
+			// lock is expired.
 			CheckKnobRollout(
 				fdbCluster,
 				newGeneralCustomParameters,
 				newStorageCustomParameters,
-				normalKnobRolloutTimeoutSeconds)
+				normalKnobRolloutTimeoutSeconds+int(fdbCluster.GetPrimary().GetCluster().GetLockDuration().Seconds()))
 		})
 	})
 })