Repository: FoundationDB/fdb-kubernetes-operator
Pull request #1198 (2022-05-04 08:26:08)

Fix backup setup for image config
# Description

Fixes: https://github.com/FoundationDB/fdb-kubernetes-operator/issues/1197 this is a follow up from the other PRs to have the same configuration setup like we use in the cluster CRD

## Type of change

*Please select one of the options below.*

- Bug fix (non-breaking change which fixes an issue)
- New feature (non-breaking change which adds functionality)

## Discussion

-

## Testing

Unit + local setup

## Documentation

-
## Follow-up

-


foundationdb-ci: 
### AWS CodeBuild CI Report for Linux CentOS 7

* CodeBuild project: fdb-kubernetes-operator-pr
* Commit ID: 2657b0c40bc79b427fe80b7d1aeadf5b8fa069a3
* Result: FAILED
* Error: `Error while executing command: make -C tests -kj prOperator. Reason: exit status 2`
* [Build Logs](https://d1e0l78xbkh3xa.cloudfront.net/build-logs/fdb-kubernetes-operator/fdb-kubernetes-operator-pr/db68ec62-e2f6-4bd6-9714-b31e911ba788.gz) (available for 30 days)


Patch:
diff --git a/api/v1beta2/foundationdbbackup_types.go b/api/v1beta2/foundationdbbackup_types.go
index e9bb212c..429dd7f2 100644
--- a/api/v1beta2/foundationdbbackup_types.go
+++ b/api/v1beta2/foundationdbbackup_types.go
@@ -95,9 +95,12 @@ type FoundationDBBackupSpec struct {
 	// This is the configuration of the target blobstore for this backup.
 	BlobStoreConfiguration *BlobStoreConfiguration `json:"blobStoreConfiguration,omitempty"`
 
-	// ImageConfigs allows customizing the image that we use for
-	// a container.
-	ImageConfigs []ImageConfig `json:"imageConfigs,omitempty"`
+	// MainContainer defines customization for the foundationdb container.
+	MainContainer ContainerOverrides `json:"mainContainer,omitempty"`
+
+	// SidecarContainer defines customization for the
+	// foundationdb-kubernetes-sidecar container.
+	SidecarContainer ContainerOverrides `json:"sidecarContainer,omitempty"`
 }
 
 // FoundationDBBackupStatus describes the current status of the backup for a cluster.
diff --git a/api/v1beta2/foundationdbcluster_types.go b/api/v1beta2/foundationdbcluster_types.go
index f8644fcf..acd6f729 100644
--- a/api/v1beta2/foundationdbcluster_types.go
+++ b/api/v1beta2/foundationdbcluster_types.go
@@ -1419,7 +1419,8 @@ func (cluster *FoundationDBCluster) DesiredDatabaseConfiguration() DatabaseConfi
 	configuration.RoleCounts = cluster.GetRoleCountsWithDefaults()
 	configuration.RoleCounts.Storage = 0
 
-	if cluster.Spec.DatabaseConfiguration.AreSeparatedProxiesConfigured() {
+	version, _ := ParseFdbVersion(cluster.GetRunningVersion())
+	if version.HasSeparatedProxies() && cluster.Spec.DatabaseConfiguration.AreSeparatedProxiesConfigured() {
 		configuration.RoleCounts.Proxies = 0
 	} else {
 		configuration.RoleCounts.GrvProxies = 0
diff --git a/api/v1beta2/zz_generated.deepcopy.go b/api/v1beta2/zz_generated.deepcopy.go
index 880fc6d6..83214d09 100644
--- a/api/v1beta2/zz_generated.deepcopy.go
+++ b/api/v1beta2/zz_generated.deepcopy.go
@@ -364,11 +364,8 @@ func (in *FoundationDBBackupSpec) DeepCopyInto(out *FoundationDBBackupSpec) {
 		*out = new(BlobStoreConfiguration)
 		(*in).DeepCopyInto(*out)
 	}
-	if in.ImageConfigs != nil {
-		in, out := &in.ImageConfigs, &out.ImageConfigs
-		*out = make([]ImageConfig, len(*in))
-		copy(*out, *in)
-	}
+	in.MainContainer.DeepCopyInto(&out.MainContainer)
+	in.SidecarContainer.DeepCopyInto(&out.SidecarContainer)
 }
 
 // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new FoundationDBBackupSpec.
diff --git a/config/crd/bases/apps.foundationdb.org_foundationdbbackups.yaml b/config/crd/bases/apps.foundationdb.org_foundationdbbackups.yaml
index 460e5ee7..e6c971f7 100644
--- a/config/crd/bases/apps.foundationdb.org_foundationdbbackups.yaml
+++ b/config/crd/bases/apps.foundationdb.org_foundationdbbackups.yaml
@@ -3197,19 +3197,30 @@ spec:
                   type: string
                 maxItems: 100
                 type: array
-              imageConfigs:
-                items:
-                  properties:
-                    baseImage:
-                      type: string
-                    tag:
-                      type: string
-                    tagSuffix:
-                      type: string
-                    version:
-                      type: string
-                  type: object
-                type: array
+              mainContainer:
+                properties:
+                  enableLivenessProbe:
+                    type: boolean
+                  enableReadinessProbe:
+                    type: boolean
+                  enableTls:
+                    type: boolean
+                  imageConfigs:
+                    items:
+                      properties:
+                        baseImage:
+                          type: string
+                        tag:
+                          type: string
+                        tagSuffix:
+                          type: string
+                        version:
+                          type: string
+                      type: object
+                    type: array
+                  peerVerificationRules:
+                    type: string
+                type: object
               podTemplateSpec:
                 properties:
                   metadata:
@@ -6164,6 +6175,30 @@ spec:
                     - containers
                     type: object
                 type: object
+              sidecarContainer:
+                properties:
+                  enableLivenessProbe:
+                    type: boolean
+                  enableReadinessProbe:
+                    type: boolean
+                  enableTls:
+                    type: boolean
+                  imageConfigs:
+                    items:
+                      properties:
+                        baseImage:
+                          type: string
+                        tag:
+                          type: string
+                        tagSuffix:
+                          type: string
+                        version:
+                          type: string
+                      type: object
+                    type: array
+                  peerVerificationRules:
+                    type: string
+                type: object
               snapshotPeriodSeconds:
                 type: integer
               version:
diff --git a/config/development/debug_logs.yaml b/config/development/debug_logs.yaml
index 186b8662..1fae4dc4 100644
--- a/config/development/debug_logs.yaml
+++ b/config/development/debug_logs.yaml
@@ -7,5 +7,6 @@ spec:
     spec:
       containers:
         - name: manager
+          imagePullPolicy: Never
           args:
             - --zap-log-level=debug
diff --git a/docs/backup_spec.md b/docs/backup_spec.md
index 7af087cc..5aa20e81 100644
--- a/docs/backup_spec.md
+++ b/docs/backup_spec.md
@@ -89,7 +89,8 @@ FoundationDBBackupSpec describes the desired state of the backup for a cluster.
 | customParameters | CustomParameters defines additional parameters to pass to the backup agents. | FoundationDBCustomParameters | false |
 | allowTagOverride | This setting defines if a user provided image can have it's own tag rather than getting the provided version appended. You have to ensure that the specified version in the Spec is compatible with the given version in your custom image. **Deprecated: use ImageConfigs instead.** | *bool | false |
 | blobStoreConfiguration | This is the configuration of the target blobstore for this backup. | *[BlobStoreConfiguration](#blobstoreconfiguration) | false |
-| imageConfigs | ImageConfigs allows customizing the image that we use for a container. | [][ImageConfig](#imageconfig) | false |
+| mainContainer | MainContainer defines customization for the foundationdb container. | ContainerOverrides | false |
+| sidecarContainer | SidecarContainer defines customization for the foundationdb-kubernetes-sidecar container. | ContainerOverrides | false |
 
 [Back to TOC](#table-of-contents)
 
diff --git a/internal/pod_models.go b/internal/pod_models.go
index 2a6aec28..6e97d65c 100644
--- a/internal/pod_models.go
+++ b/internal/pod_models.go
@@ -452,28 +452,17 @@ func GetPodSpec(cluster *fdbv1beta2.FoundationDBCluster, processClass fdbv1beta2
 // configureSidecarContainerForCluster sets up a sidecar container for a sidecar
 // in the FDB cluster.
 func configureSidecarContainerForCluster(cluster *fdbv1beta2.FoundationDBCluster, podName string, container *corev1.Container, initMode bool, processGroupID string) error {
-	versionString := cluster.Status.RunningVersion
-	if versionString == "" {
-		versionString = cluster.Spec.Version
-	}
-
-	return configureSidecarContainer(container, initMode, processGroupID, podName, versionString, cluster)
+	return configureSidecarContainer(container, initMode, processGroupID, podName, cluster.GetRunningVersion(), cluster, cluster.Spec.SidecarContainer.ImageConfigs, false)
 }
 
 // configureSidecarContainerForBackup sets up a sidecar container for the init
 // container for a backup process.
 func configureSidecarContainerForBackup(backup *fdbv1beta2.FoundationDBBackup, container *corev1.Container) error {
-	return configureSidecarContainer(container, true, "", "", backup.Spec.Version, nil)
+	return configureSidecarContainer(container, true, "", "", backup.Spec.Version, nil, backup.Spec.SidecarContainer.ImageConfigs, pointer.BoolDeref(backup.Spec.AllowTagOverride, false))
 }
 
-// configureSidecarContainer sets up a foundationdb-kubernetes-sidecar
-// container.
-func configureSidecarContainer(container *corev1.Container, initMode bool, processGroupID string, podName string, versionString string, optionalCluster *fdbv1beta2.FoundationDBCluster) error {
-	version, err := fdbv1beta2.ParseFdbVersion(versionString)
-	if err != nil {
-		return err
-	}
-
+// configureSidecarContainer sets up a foundationdb-kubernetes-sidecar container.
+func configureSidecarContainer(container *corev1.Container, initMode bool, processGroupID string, podName string, versionString string, optionalCluster *fdbv1beta2.FoundationDBCluster, imageConfigs []fdbv1beta2.ImageConfig, allowTagOverride bool) error {
 	sidecarEnv := make([]corev1.EnvVar, 0, 4)
 
 	hasTrustedCAs := optionalCluster != nil && len(optionalCluster.Spec.TrustedCAs) > 0
@@ -491,7 +480,7 @@ func configureSidecarContainer(container *corev1.Container, initMode bool, proce
 			"--input-monitor-conf", "fdbmonitor.conf",
 			"--copy-binary", "fdbserver",
 			"--copy-binary", "fdbcli",
-			"--main-container-version", version.String(),
+			"--main-container-version", versionString,
 		)
 	}
 
@@ -559,6 +548,10 @@ func configureSidecarContainer(container *corev1.Container, initMode bool, proce
 
 	if optionalCluster != nil {
 		overrides = optionalCluster.Spec.SidecarContainer
+	}
+
+	if len(imageConfigs) > 0 {
+		overrides.ImageConfigs = imageConfigs
 	} else {
 		overrides.ImageConfigs = []fdbv1beta2.ImageConfig{{BaseImage: "foundationdb/foundationdb-kubernetes-sidecar", TagSuffix: "-1"}}
 	}
@@ -576,7 +569,7 @@ func configureSidecarContainer(container *corev1.Container, initMode bool, proce
 		corev1.VolumeMount{Name: "dynamic-conf", MountPath: "/var/output-files"},
 	)
 
-	image, err := GetImage(container.Image, overrides.ImageConfigs, versionString, false)
+	image, err := GetImage(container.Image, overrides.ImageConfigs, versionString, allowTagOverride)
 	if err != nil {
 		return err
 	}
@@ -825,11 +818,13 @@ func GetBackupDeployment(backup *fdbv1beta2.FoundationDBBackup) (*appsv1.Deploym
 		podTemplate.Spec.Containers = containers
 	}
 
-	if backup.Spec.ImageConfigs == nil {
-		backup.Spec.ImageConfigs = []fdbv1beta2.ImageConfig{{BaseImage: "foundationdb/foundationdb"}}
+	if len(backup.Spec.MainContainer.ImageConfigs) == 0 {
+		backup.Spec.MainContainer.ImageConfigs = []fdbv1beta2.ImageConfig{
+			{BaseImage: "foundationdb/foundationdb"},
+		}
 	}
 
-	image, err := GetImage(mainContainer.Image, backup.Spec.ImageConfigs, backup.Spec.Version, pointer.BoolDeref(backup.Spec.AllowTagOverride, false))
+	image, err := GetImage(mainContainer.Image, backup.Spec.MainContainer.ImageConfigs, backup.Spec.Version, pointer.BoolDeref(backup.Spec.AllowTagOverride, false))
 	if err != nil {
 		return nil, err
 	}


Test patch:
diff --git a/api/v1beta2/foundationdbcluster_types_test.go b/api/v1beta2/foundationdbcluster_types_test.go
index c2d3d808..cef7fa36 100644
--- a/api/v1beta2/foundationdbcluster_types_test.go
+++ b/api/v1beta2/foundationdbcluster_types_test.go
@@ -918,6 +918,26 @@ var _ = Describe("[api] FoundationDBCluster", func() {
 					},
 				}))
 			})
+
+			It("should be parsed correctly when both grv_proxies/commit_proxies and proxies are set", func() {
+				cluster.Spec.DatabaseConfiguration.RoleCounts.Proxies = 12
+				cluster.Spec.DatabaseConfiguration.RoleCounts.CommitProxies = 4
+				cluster.Spec.DatabaseConfiguration.RoleCounts.GrvProxies = 4
+				Expect(cluster.DesiredDatabaseConfiguration()).To(Equal(DatabaseConfiguration{
+					RedundancyMode: RedundancyModeDouble,
+					StorageEngine:  StorageEngineSSD2,
+					UsableRegions:  1,
+					RoleCounts: RoleCounts{
+						Logs:          4,
+						Proxies:       0,
+						CommitProxies: 4,
+						GrvProxies:    4,
+						Resolvers:     1,
+						LogRouters:    -1,
+						RemoteLogs:    -1,
+					},
+				}))
+			})
 		})
 
 		When("the version does not support grv and commit proxies", func() {
@@ -975,6 +995,48 @@ var _ = Describe("[api] FoundationDBCluster", func() {
 					},
 				}))
 			})
+
+			When("grv and commit proxies are passed in addition to proxies", func() {
+				BeforeEach(func() {
+					cluster = &FoundationDBCluster{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "foo",
+							Namespace: "default",
+						},
+						Spec: FoundationDBClusterSpec{
+							DatabaseConfiguration: DatabaseConfiguration{
+								RedundancyMode: RedundancyModeDouble,
+								StorageEngine:  StorageEngineSSD,
+								RoleCounts: RoleCounts{
+									Storage:       5,
+									Logs:          4,
+									Proxies:       5,
+									GrvProxies:    1,
+									CommitProxies: 1,
+								},
+							},
+							Version: "6.3.23",
+						},
+					}
+				})
+
+				It("should be only contain the configured proxies", func() {
+					Expect(cluster.DesiredDatabaseConfiguration()).To(Equal(DatabaseConfiguration{
+						RedundancyMode: RedundancyModeDouble,
+						StorageEngine:  StorageEngineSSD2,
+						UsableRegions:  1,
+						RoleCounts: RoleCounts{
+							Logs:          4,
+							Proxies:       5,
+							CommitProxies: 0,
+							GrvProxies:    0,
+							Resolvers:     1,
+							LogRouters:    -1,
+							RemoteLogs:    -1,
+						},
+					}))
+				})
+			})
 		})
 	})
 
diff --git a/internal/pod_models_test.go b/internal/pod_models_test.go
index 209a9e00..ce331797 100644
--- a/internal/pod_models_test.go
+++ b/internal/pod_models_test.go
@@ -2950,7 +2950,12 @@ var _ = Describe("pod_models", func() {
 
 		When("defining an image config", func() {
 			BeforeEach(func() {
-				backup.Spec.ImageConfigs = []fdbv1beta2.ImageConfig{{BaseImage: "foundationdb/foundationdb", Tag: "dev"}}
+				backup.Spec.MainContainer.ImageConfigs = []fdbv1beta2.ImageConfig{
+					{BaseImage: "foundationdb/foundationdb", Tag: "dev"},
+				}
+				backup.Spec.SidecarContainer.ImageConfigs = []fdbv1beta2.ImageConfig{
+					{BaseImage: "foundationdb/foundationdb-kubernetes-sidecar", Tag: "dev-1"},
+				}
 				deployment, err = GetBackupDeployment(backup)
 				Expect(err).NotTo(HaveOccurred())
 				Expect(deployment).NotTo(BeNil())
@@ -2958,6 +2963,39 @@ var _ = Describe("pod_models", func() {
 
 			It("should set the image based on the image configs", func() {
 				Expect(deployment.Spec.Template.Spec.Containers[0].Image).To(Equal("foundationdb/foundationdb:dev"))
+				Expect(deployment.Spec.Template.Spec.InitContainers[0].Image).To(Equal("foundationdb/foundationdb-kubernetes-sidecar:dev-1"))
+			})
+		})
+
+		When("AllowTagOverride is set to true", func() {
+			BeforeEach(func() {
+				backup.Spec.AllowTagOverride = pointer.Bool(true)
+				templateSpec := corev1.PodTemplateSpec{
+					Spec: corev1.PodSpec{
+						Containers: []corev1.Container{
+							{
+								Name:  "foundationdb",
+								Image: "test:main",
+							},
+						},
+						InitContainers: []corev1.Container{
+							{
+								Name:  "foundationdb-kubernetes-init",
+								Image: "test:sidecar",
+							},
+						},
+					},
+				}
+
+				backup.Spec.PodTemplateSpec = &templateSpec
+				deployment, err = GetBackupDeployment(backup)
+				Expect(err).NotTo(HaveOccurred())
+				Expect(deployment).NotTo(BeNil())
+			})
+
+			It("should set the image without validating the tag", func() {
+				Expect(deployment.Spec.Template.Spec.Containers[0].Image).To(Equal("test:main"))
+				Expect(deployment.Spec.Template.Spec.InitContainers[0].Image).To(Equal("test:sidecar"))
 			})
 		})
 	})