Add first upgrade test with DNS usage in cluster file and correct sta…

…tement about DNS (#1820) * Add first upgrade test with DNS usage in cluster file and correct statement about DNS
FoundationDB · Oct 5, 2023 · 22ddb7d · 22ddb7d
1 parent b493bcc
commit 22ddb7d
Show file tree

Hide file tree

Showing 12 changed files with 194 additions and 305 deletions.
diff --git a/api/v1beta2/foundationdb_version.go b/api/v1beta2/foundationdb_version.go
@@ -209,6 +209,11 @@ func (version Version) SupportsRecoveryState() bool {
 	return version.IsAtLeast(Versions.SupportsRecoveryState)
 }
 
+// SupportsDNSInClusterFile returns true if the version of FDB supports the usage of DNS names in the cluster file.
+func (version Version) SupportsDNSInClusterFile() bool {
+	return version.IsAtLeast(Versions.SupportsDNSInClusterFile)
+}
+
 // Versions provides a shorthand for known versions.
 // This is only to be used in testing.
 var Versions = struct {
@@ -222,6 +227,7 @@ var Versions = struct {
 	IncompatibleVersion,
 	PreviousPatchVersion,
 	SupportsRecoveryState,
+	SupportsDNSInClusterFile,
 	Default Version
 }{
 	Default:                      Version{Major: 6, Minor: 2, Patch: 21},
@@ -235,4 +241,5 @@ var Versions = struct {
 	SupportsShardedRocksDB:       Version{Major: 7, Minor: 2, Patch: 0},
 	SupportsRedwood1Experimental: Version{Major: 7, Minor: 0, Patch: 0},
 	SupportsRecoveryState:        Version{Major: 7, Minor: 1, Patch: 22},
+	SupportsDNSInClusterFile:     Version{Major: 7, Minor: 0, Patch: 0},
 }
diff --git a/api/v1beta2/foundationdbcluster_types.go b/api/v1beta2/foundationdbcluster_types.go
@@ -1976,9 +1976,8 @@ type RoutingConfig struct {
 	PodIPFamily *int `json:"podIPFamily,omitempty"`
 
 	// UseDNSInClusterFile determines whether to use DNS names rather than IP
-	// addresses to identify coordinators in the cluster file.
-	// NOTE: This is an experimental feature, and is not supported in the
-	// latest stable version of FoundationDB.
+	// addresses to identify coordinators in the cluster file. This requires
+	// FoundationDB 7.0+.
 	UseDNSInClusterFile *bool `json:"useDNSInClusterFile,omitempty"`
 
 	// DefineDNSLocalityFields determines whether to define pod DNS names on pod
@@ -2281,7 +2280,13 @@ func (cluster *FoundationDBCluster) NeedsHeadlessService() bool {
 // UseDNSInClusterFile determines whether we need to use DNS entries in the
 // cluster file for this cluster.
 func (cluster *FoundationDBCluster) UseDNSInClusterFile() bool {
-	return pointer.BoolDeref(cluster.Spec.Routing.UseDNSInClusterFile, false)
+	runningVersion, err := ParseFdbVersion(cluster.Status.RunningVersion)
+	// If the version cannot be parsed fall back to false.
+	if err != nil {
+		return false
+	}
+
+	return runningVersion.SupportsDNSInClusterFile() && pointer.BoolDeref(cluster.Spec.Routing.UseDNSInClusterFile, false)
 }
 
 // DefineDNSLocalityFields determines whether we need to put DNS entries in the

diff --git a/api/v1beta2/foundationdbcluster_types_test.go b/api/v1beta2/foundationdbcluster_types_test.go
@@ -4332,57 +4332,76 @@ var _ = Describe("[api] FoundationDBCluster", func() {
 			cluster = &FoundationDBCluster{}
 		})
 
-		When("checking whether we need a headless service", func() {
-			It("respects the headless service setting", func() {
-				Expect(cluster.NeedsHeadlessService()).To(BeFalse())
-
-				cluster.Spec.Routing.HeadlessService = pointer.Bool(true)
-				Expect(cluster.NeedsHeadlessService()).To(BeTrue())
+		When("FoundationDB supports DNS in the cluster file", func() {
+			BeforeEach(func() {
+				cluster.Status.RunningVersion = Versions.SupportsDNSInClusterFile.String()
 			})
 
-			It("can be overridden by the DNS in cluster file setting", func() {
-				cluster.Spec.Routing.HeadlessService = pointer.Bool(false)
-				cluster.Spec.Routing.UseDNSInClusterFile = pointer.Bool(true)
-				Expect(cluster.NeedsHeadlessService()).To(BeTrue())
-			})
+			When("checking whether we need a headless service", func() {
+				It("respects the headless service setting", func() {
+					Expect(cluster.NeedsHeadlessService()).To(BeFalse())
+
+					cluster.Spec.Routing.HeadlessService = pointer.Bool(true)
+					Expect(cluster.NeedsHeadlessService()).To(BeTrue())
+				})
 
-			It("can be overridden by the DNS in locality setting", func() {
-				cluster.Spec.Routing.HeadlessService = pointer.Bool(false)
-				cluster.Spec.Routing.DefineDNSLocalityFields = pointer.Bool(true)
-				Expect(cluster.NeedsHeadlessService()).To(BeTrue())
+				It("can be overridden by the DNS in cluster file setting", func() {
+					cluster.Spec.Routing.HeadlessService = pointer.Bool(false)
+					cluster.Spec.Routing.UseDNSInClusterFile = pointer.Bool(true)
+					Expect(cluster.NeedsHeadlessService()).To(BeTrue())
+				})
+
+				It("can be overridden by the DNS in locality setting", func() {
+					cluster.Spec.Routing.HeadlessService = pointer.Bool(false)
+					cluster.Spec.Routing.DefineDNSLocalityFields = pointer.Bool(true)
+					Expect(cluster.NeedsHeadlessService()).To(BeTrue())
+				})
 			})
-		})
 
-		When("checking whether we use DNS in the cluster file", func() {
-			It("respects the value in the flag", func() {
-				Expect(cluster.UseDNSInClusterFile()).To(BeFalse())
+			When("checking whether we use DNS in the cluster file", func() {
+				It("respects the value in the flag", func() {
+					Expect(cluster.UseDNSInClusterFile()).To(BeFalse())
 
-				cluster.Spec.Routing.UseDNSInClusterFile = pointer.Bool(true)
-				Expect(cluster.UseDNSInClusterFile()).To(BeTrue())
+					cluster.Spec.Routing.UseDNSInClusterFile = pointer.Bool(true)
+					Expect(cluster.UseDNSInClusterFile()).To(BeTrue())
+				})
 			})
-		})
 
-		When("checking whether we use DNS in the locality fields", func() {
-			It("respects the value in the flag", func() {
-				Expect(cluster.DefineDNSLocalityFields()).To(BeFalse())
+			When("checking whether we use DNS in the locality fields", func() {
+				It("respects the value in the flag", func() {
+					Expect(cluster.DefineDNSLocalityFields()).To(BeFalse())
+
+					cluster.Spec.Routing.DefineDNSLocalityFields = pointer.Bool(true)
+					Expect(cluster.DefineDNSLocalityFields()).To(BeTrue())
+				})
 
-				cluster.Spec.Routing.DefineDNSLocalityFields = pointer.Bool(true)
-				Expect(cluster.DefineDNSLocalityFields()).To(BeTrue())
+				It("can be overridden by the DNS in cluster file setting", func() {
+					cluster.Spec.Routing.DefineDNSLocalityFields = pointer.Bool(false)
+					cluster.Spec.Routing.UseDNSInClusterFile = pointer.Bool(true)
+					Expect(cluster.DefineDNSLocalityFields()).To(BeTrue())
+				})
 			})
 
-			It("can be overridden by the DNS in cluster file setting", func() {
-				cluster.Spec.Routing.DefineDNSLocalityFields = pointer.Bool(false)
-				cluster.Spec.Routing.UseDNSInClusterFile = pointer.Bool(true)
-				Expect(cluster.DefineDNSLocalityFields()).To(BeTrue())
+			When("getting the DNS domain", func() {
+				It("allows overrides in the spec", func() {
+					Expect(cluster.GetDNSDomain()).To(Equal("cluster.local"))
+					suffix := "cluster.example"
+					cluster.Spec.Routing.DNSDomain = &suffix
+					Expect(cluster.GetDNSDomain()).To(Equal(suffix))
+				})
 			})
 		})
 
-		When("getting the DNS domain", func() {
-			It("allows overrides in the spec", func() {
-				Expect(cluster.GetDNSDomain()).To(Equal("cluster.local"))
-				suffix := "cluster.example"
-				cluster.Spec.Routing.DNSDomain = &suffix
-				Expect(cluster.GetDNSDomain()).To(Equal(suffix))
+		When("FoundationDB does not support DNS in the cluster file", func() {
+			BeforeEach(func() {
+				cluster.Status.RunningVersion = Versions.MinimumVersion.String()
+			})
+
+			When("checking if DNS should be used", func() {
+				It("should return false", func() {
+					cluster.Spec.Routing.HeadlessService = pointer.Bool(true)
+					Expect(cluster.UseDNSInClusterFile()).To(BeFalse())
+				})
 			})
 		})
 	})

diff --git a/controllers/change_coordinators_test.go b/controllers/change_coordinators_test.go
@@ -656,6 +656,8 @@ var _ = Describe("Change coordinators", func() {
 		When("enabling DNS in the cluster file", func() {
 			BeforeEach(func() {
 				cluster.Spec.Routing.UseDNSInClusterFile = pointer.Bool(true)
+				cluster.Spec.Version = fdbv1beta2.Versions.SupportsDNSInClusterFile.String()
+				cluster.Status.RunningVersion = fdbv1beta2.Versions.SupportsDNSInClusterFile.String()
 			})
 
 			When("the Pods do not have DNS names", func() {

diff --git a/controllers/cluster_controller_test.go b/controllers/cluster_controller_test.go
@@ -324,8 +324,9 @@ var _ = Describe("cluster_controller", func() {
 		When("enabling the DNS names in the cluster file", func() {
 			BeforeEach(func() {
 				cluster.Spec.Routing.UseDNSInClusterFile = pointer.Bool(true)
-				err = k8sClient.Update(context.TODO(), cluster)
-				Expect(err).NotTo(HaveOccurred())
+				cluster.Spec.Version = fdbv1beta2.Versions.SupportsDNSInClusterFile.String()
+				cluster.Status.RunningVersion = fdbv1beta2.Versions.SupportsDNSInClusterFile.String()
+				Expect(k8sClient.Update(context.TODO(), cluster)).NotTo(HaveOccurred())
 			})
 
 			It("should update the pods", func() {

diff --git a/docs/cluster_spec.md b/docs/cluster_spec.md
@@ -466,7 +466,7 @@ RoutingConfig allows configuring routing to our pods, and services that sit in f
 | headlessService | Headless determines whether we want to run a headless service for the cluster. | *bool | false |
 | publicIPSource | PublicIPSource specifies what source a process should use to get its public IPs.  This supports the values `pod` and `service`. | *[PublicIPSource](#publicipsource) | false |
 | podIPFamily | PodIPFamily tells the pod which family of IP addresses to use. You can use 4 to represent IPv4, and 6 to represent IPv6. This feature is only supported in FDB 7.0 or later, and requires dual-stack support in your Kubernetes environment. | *int | false |
-| useDNSInClusterFile | UseDNSInClusterFile determines whether to use DNS names rather than IP addresses to identify coordinators in the cluster file. NOTE: This is an experimental feature, and is not supported in the latest stable version of FoundationDB. | *bool | false |
+| useDNSInClusterFile | UseDNSInClusterFile determines whether to use DNS names rather than IP addresses to identify coordinators in the cluster file. This requires FoundationDB 7.0+. | *bool | false |
 | defineDNSLocalityFields | DefineDNSLocalityFields determines whether to define pod DNS names on pod specs and provide them in the locality arguments to fdbserver.  This is ignored if UseDNSInCluster is true. | *bool | false |
 | dnsDomain | DNSDomain defines the cluster domain used in a DNS name generated for a service. The default is `cluster.local`. | *string | false |
 

diff --git a/e2e/fixtures/cluster_config.go b/e2e/fixtures/cluster_config.go
@@ -74,6 +74,8 @@ type ClusterConfig struct {
 	UseMaintenanceMode bool
 	// UseLocalityBasedExclusions if enabled the FoundationDBCluster resource will enable the locality based exclusions.
 	UseLocalityBasedExclusions bool
+	// UseDNS if enabled the FoundationDBCluster resource will enable the DNS feature.
+	UseDNS bool
 	// CreationTracker if specified will be used to log the time between the creations steps.
 	CreationTracker CreationTrackerLogger
 	// Number of machines, this is used for calculating the number of Pods and is not correlated to the actual number
@@ -498,22 +500,24 @@ func calculateProxies(proxies int) (int, int) {
 // Copy will return a new struct of the ClusterConfig.
 func (config *ClusterConfig) Copy() *ClusterConfig {
 	return &ClusterConfig{
-		Performance:         config.Performance,
-		DebugSymbols:        config.DebugSymbols,
-		UseMaintenanceMode:  config.UseMaintenanceMode,
-		CreationTracker:     config.CreationTracker,
-		MachineCount:        config.MachineCount,
-		DisksPerMachine:     config.DisksPerMachine,
-		StorageServerPerPod: config.StorageServerPerPod,
-		LogServersPerPod:    config.LogServersPerPod,
-		VolumeSize:          config.VolumeSize,
-		Namespace:           config.Namespace,
-		Name:                config.Name,
-		cloudProvider:       config.cloudProvider,
-		StorageEngine:       config.StorageEngine,
-		NodeSelector:        config.NodeSelector,
-		HaMode:              config.HaMode,
-		CustomParameters:    config.CustomParameters,
-		CreationCallback:    config.CreationCallback,
+		Performance:                config.Performance,
+		DebugSymbols:               config.DebugSymbols,
+		UseMaintenanceMode:         config.UseMaintenanceMode,
+		CreationTracker:            config.CreationTracker,
+		MachineCount:               config.MachineCount,
+		DisksPerMachine:            config.DisksPerMachine,
+		StorageServerPerPod:        config.StorageServerPerPod,
+		LogServersPerPod:           config.LogServersPerPod,
+		VolumeSize:                 config.VolumeSize,
+		Namespace:                  config.Namespace,
+		Name:                       config.Name,
+		cloudProvider:              config.cloudProvider,
+		StorageEngine:              config.StorageEngine,
+		NodeSelector:               config.NodeSelector,
+		HaMode:                     config.HaMode,
+		CustomParameters:           config.CustomParameters,
+		CreationCallback:           config.CreationCallback,
+		UseDNS:                     config.UseDNS,
+		UseLocalityBasedExclusions: config.UseLocalityBasedExclusions,
 	}
 }
diff --git a/e2e/fixtures/fdb_cluster_specs.go b/e2e/fixtures/fdb_cluster_specs.go
@@ -86,7 +86,7 @@ func (factory *Factory) createFDBClusterSpec(
 				UseLocalitiesForExclusion: pointer.Bool(config.UseLocalityBasedExclusions),
 			},
 			Routing: fdbv1beta2.RoutingConfig{
-				UseDNSInClusterFile: pointer.Bool(false),
+				UseDNSInClusterFile: pointer.Bool(config.UseDNS),
 				HeadlessService: pointer.Bool(
 					true,
 				), // to make switching between hostname <-> IP smooth