[1.2.x] fix: Make sure to use backward-compatible names for resources…

… created by the operator [RHIDP-2432] (#377) * Add test highlighting the issue and setting the expectations * Revert the DB StatefulSet name for backward compatibility with 1.1 Otherwise, when upgrading from 1.1, a new StatefulSet would be created with a new data PVC, causing the application to boot with from a brand new database. This would break the upgrade path. * Make sure to use backward compatible names for all resources This generalizes the previous commit, as I realized that, otherwise, we might actually be creating duplicate resources like app-config ConfigMaps/Secrets/Services with different names (while keeping the existing ones), which might confuse people upgrading from 1.1. This makes sure we avoid such a similar situation in the future by ensuring naming is done in a backward-compatible manner. * Handle patch error and delete StatefulSet so it can be recreated Some resources like StatefulSets allow patching a limited set of fields. For cases where we are upgrading existing instances, we might need to change the StatefulSet spec. The recommended approach to doing this is to delete the existing StatefulSet, but keep its dependents (like PVCs and Pods) orphan, then recreate the StatefulSet. This way, it will be upgraded (with some downtime) with no data loss. * Revert "Make sure to use backward compatible names for all resources" This reverts commit 0b1be1c2720d96dd6bdd60f230e66819b2bc9e4b. * Address review comments on testing Co-authored-by: Gennady Azarenkov <[email protected]> * Revert useless change to integration_tests/suite_test.go * Address review comments on testing Co-authored-by: Gennady Azarenkov <[email protected]> --------- Co-authored-by: Armel Soro <[email protected]> Co-authored-by: Gennady Azarenkov <[email protected]>
janus-idp · Jun 4, 2024 · 5c75a88 · 5c75a88
1 parent 82c0556
commit 5c75a88
Show file tree

Hide file tree

Showing 10 changed files with 102 additions and 28 deletions.
diff --git a/controllers/backstage_controller.go b/controllers/backstage_controller.go
@@ -181,11 +181,22 @@ func (r *BackstageReconciler) applyObjects(ctx context.Context, objects []model.
 		}
 
 		if err := r.patchObject(ctx, baseObject, obj); err != nil {
+			_, isStatefulSet := obj.Object().(*appsv1.StatefulSet)
+			if isStatefulSet && errors.IsInvalid(err) && errors.HasStatusCause(err, metav1.CauseTypeForbidden) {
+				// Some resources like StatefulSets allow patching a limited set of fields.
+				// That's why we are trying to delete them first, taking care of orphaning the dependents so that resources like PVCs can be retained.
+				// They will be recreated at the next reconciliation.
+				if err = r.Delete(ctx, baseObject, client.PropagationPolicy(metav1.DeletePropagationOrphan)); err != nil {
+					return fmt.Errorf("failed to delete object %s so it can be recreated: %w", obj.Object(), err)
+				}
+				lg.V(1).Info("delete object ", objDispName(obj), obj.Object().GetName())
+				continue
+			}
+
 			return fmt.Errorf("failed to patch object %s: %w", obj.Object(), err)
 		}
 
 		lg.V(1).Info("patch object ", objDispName(obj), obj.Object().GetName())
-
 	}
 	return nil
 }

diff --git a/examples/bs-existing-secret.yaml b/examples/bs-existing-secret.yaml
@@ -13,8 +13,8 @@ metadata:
   name: existing-postgres-secret
 type: Opaque
 stringData:
-  POSTGRES_PASSWORD: admin123
+  POSTGRES_PASSWORD: "admin123"
   POSTGRES_PORT: "5432"
-  POSTGRES_USER: postgres
-  POSTGRESQL_ADMIN_PASSWORD: admin123
-  POSTGRES_HOST: backstage-db-bs-existing-secret
+  POSTGRES_USER: "postgres"
+  POSTGRESQL_ADMIN_PASSWORD: "admin123"
+  POSTGRES_HOST: "backstage-psql-bs-existing-secret"
diff --git a/integration_tests/db_test.go b/integration_tests/db_test.go
@@ -16,10 +16,12 @@ package integration_tests
 
 import (
 	"context"
+	"fmt"
 	"time"
 
-	"k8s.io/apimachinery/pkg/api/errors"
+	"redhat-developer/red-hat-developer-hub-operator/pkg/model"
 
+	"k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/utils/ptr"
 
 	corev1 "k8s.io/api/core/v1"
@@ -28,8 +30,6 @@ import (
 
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
-	"redhat-developer/red-hat-developer-hub-operator/pkg/model"
-
 	bsv1alpha1 "redhat-developer/red-hat-developer-hub-operator/api/v1alpha1"
 
 	"k8s.io/apimachinery/pkg/types"
@@ -60,13 +60,13 @@ var _ = When("create backstage with CR configured", func() {
 		Eventually(func(g Gomega) {
 			By("creating Deployment with database.enableLocalDb=true by default")
 
-			err := k8sClient.Get(ctx, types.NamespacedName{Namespace: ns, Name: model.DbStatefulSetName(backstageName)}, &appsv1.StatefulSet{})
+			err := k8sClient.Get(ctx, types.NamespacedName{Namespace: ns, Name: fmt.Sprintf("backstage-psql-%s", backstageName)}, &appsv1.StatefulSet{})
 			g.Expect(err).To(Not(HaveOccurred()))
 
-			err = k8sClient.Get(ctx, types.NamespacedName{Namespace: ns, Name: model.DbStatefulSetName(backstageName)}, &corev1.Service{})
+			err = k8sClient.Get(ctx, types.NamespacedName{Namespace: ns, Name: fmt.Sprintf("backstage-psql-%s", backstageName)}, &corev1.Service{})
 			g.Expect(err).To(Not(HaveOccurred()))
 
-			err = k8sClient.Get(ctx, types.NamespacedName{Namespace: ns, Name: model.DbStatefulSetName(backstageName)}, &corev1.Secret{})
+			err = k8sClient.Get(ctx, types.NamespacedName{Namespace: ns, Name: fmt.Sprintf("backstage-psql-secret-%s", backstageName)}, &corev1.Secret{})
 			g.Expect(err).To(Not(HaveOccurred()))
 
 		}, time.Minute, time.Second).Should(Succeed())
@@ -86,15 +86,15 @@ var _ = When("create backstage with CR configured", func() {
 
 		Eventually(func(g Gomega) {
 			By("deleting Local Db StatefulSet, Service and Secret")
-			err = k8sClient.Get(ctx, types.NamespacedName{Namespace: ns, Name: model.DbStatefulSetName(backstageName)}, &appsv1.StatefulSet{})
+			err = k8sClient.Get(ctx, types.NamespacedName{Namespace: ns, Name: fmt.Sprintf("backstage-psql-%s", backstageName)}, &appsv1.StatefulSet{})
 			g.Expect(err).To(HaveOccurred())
 			g.Expect(errors.IsNotFound(err))
 
-			err = k8sClient.Get(ctx, types.NamespacedName{Namespace: ns, Name: model.DbServiceName(backstageName)}, &corev1.Service{})
+			err = k8sClient.Get(ctx, types.NamespacedName{Namespace: ns, Name: fmt.Sprintf("backstage-psql-%s", backstageName)}, &corev1.Service{})
 			g.Expect(err).To(HaveOccurred())
 			g.Expect(errors.IsNotFound(err))
 
-			err = k8sClient.Get(ctx, types.NamespacedName{Namespace: ns, Name: model.DbSecretDefaultName(backstageName)}, &corev1.Secret{})
+			err = k8sClient.Get(ctx, types.NamespacedName{Namespace: ns, Name: fmt.Sprintf("backstage-psql-secret-%s", backstageName)}, &corev1.Secret{})
 			g.Expect(err).To(HaveOccurred())
 			g.Expect(errors.IsNotFound(err))
 		}, time.Minute, time.Second).Should(Succeed())
@@ -112,7 +112,7 @@ var _ = When("create backstage with CR configured", func() {
 		Eventually(func(g Gomega) {
 			By("not creating a StatefulSet for the Database")
 			err := k8sClient.Get(ctx,
-				types.NamespacedName{Namespace: ns, Name: model.DbStatefulSetName(backstageName)},
+				types.NamespacedName{Namespace: ns, Name: fmt.Sprintf("backstage-psql-%s", backstageName)},
 				&appsv1.StatefulSet{})
 			g.Expect(err).Should(HaveOccurred())
 			g.Expect(errors.IsNotFound(err))
@@ -133,7 +133,7 @@ var _ = When("create backstage with CR configured", func() {
 		Eventually(func(g Gomega) {
 			By("not creating a StatefulSet for the Database")
 			err := k8sClient.Get(ctx,
-				types.NamespacedName{Namespace: ns, Name: model.DbStatefulSetName(backstageName)},
+				types.NamespacedName{Namespace: ns, Name: fmt.Sprintf("backstage-psql-%s", backstageName)},
 				&appsv1.StatefulSet{})
 			g.Expect(err).Should(HaveOccurred())
 			g.Expect(errors.IsNotFound(err))
@@ -143,4 +143,5 @@ var _ = When("create backstage with CR configured", func() {
 			g.Expect(err).Should(Not(HaveOccurred()))
 		}, time.Minute, time.Second).Should(Succeed())
 	})
+
 })
diff --git a/integration_tests/default-config_test.go b/integration_tests/default-config_test.go
@@ -16,13 +16,16 @@ package integration_tests
 
 import (
 	"context"
-	"redhat-developer/red-hat-developer-hub-operator/pkg/utils"
+	"fmt"
 	"time"
 
-	appsv1 "k8s.io/api/apps/v1"
+	"redhat-developer/red-hat-developer-hub-operator/pkg/utils"
 
 	"redhat-developer/red-hat-developer-hub-operator/pkg/model"
 
+	appsv1 "k8s.io/api/apps/v1"
+	"sigs.k8s.io/controller-runtime/pkg/reconcile"
+
 	bsv1alpha1 "redhat-developer/red-hat-developer-hub-operator/api/v1alpha1"
 
 	corev1 "k8s.io/api/core/v1"
@@ -56,23 +59,23 @@ var _ = When("create default backstage", func() {
 		Eventually(func(g Gomega) {
 			By("creating a secret for accessing the Database")
 			secret := &corev1.Secret{}
-			secretName := model.DbSecretDefaultName(backstageName)
+			secretName := fmt.Sprintf("backstage-psql-secret-%s", backstageName)
 			err := k8sClient.Get(ctx, types.NamespacedName{Namespace: ns, Name: secretName}, secret)
 			g.Expect(err).ShouldNot(HaveOccurred(), controllerMessage())
 			g.Expect(len(secret.Data)).To(Equal(5))
 			g.Expect(secret.Data).To(HaveKeyWithValue("POSTGRES_USER", []uint8("postgres")))
 
 			By("creating a StatefulSet for the Database")
 			ss := &appsv1.StatefulSet{}
-			err = k8sClient.Get(ctx, types.NamespacedName{Namespace: ns, Name: model.DbStatefulSetName(backstageName)}, ss)
+			err = k8sClient.Get(ctx, types.NamespacedName{Namespace: ns, Name: fmt.Sprintf("backstage-psql-%s", backstageName)}, ss)
 			g.Expect(err).ShouldNot(HaveOccurred())
 
 			By("injecting default DB Secret as an env var for Db container")
 			g.Expect(model.DbSecretDefaultName(backstageName)).To(BeEnvFromForContainer(ss.Spec.Template.Spec.Containers[0]))
 			g.Expect(ss.GetOwnerReferences()).To(HaveLen(1))
 
 			By("creating a Service for the Database")
-			err = k8sClient.Get(ctx, types.NamespacedName{Name: model.DbServiceName(backstageName), Namespace: ns}, &corev1.Service{})
+			err = k8sClient.Get(ctx, types.NamespacedName{Name: fmt.Sprintf("backstage-psql-%s", backstageName), Namespace: ns}, &corev1.Service{})
 			g.Expect(err).To(Not(HaveOccurred()))
 
 			By("creating Deployment")
@@ -147,7 +150,7 @@ var _ = When("create default backstage", func() {
 
 			By("creating StatefulSet")
 			ss := &appsv1.StatefulSet{}
-			name := model.DbStatefulSetName(backstageName)
+			name := fmt.Sprintf("backstage-psql-%s", backstageName)
 			err = k8sClient.Get(ctx, types.NamespacedName{Namespace: ns, Name: name}, ss)
 			g.Expect(err).ShouldNot(HaveOccurred())
 			g.Expect(ss.Spec.Template.Spec.Containers).To(HaveLen(1))
@@ -156,4 +159,61 @@ var _ = When("create default backstage", func() {
 
 	})
 
+	It("creates runtime object using raw configuration then updates StatefulSet to replace some immutable fields", func() {
+		if !*testEnv.UseExistingCluster {
+			Skip("Real cluster required to assert actual deletion and replacement of resources")
+		}
+
+		rawStatefulSetYamlContent := readTestYamlFile("raw-statefulset.yaml")
+		dbConf := map[string]string{"db-statefulset.yaml": rawStatefulSetYamlContent}
+
+		dbRaw := generateConfigMap(ctx, k8sClient, "dbraw", ns, dbConf, nil, nil)
+
+		backstageName := createAndReconcileBackstage(ctx, ns, bsv1alpha1.BackstageSpec{
+			RawRuntimeConfig: &bsv1alpha1.RuntimeConfig{
+				LocalDbConfigName: dbRaw,
+			},
+		}, "")
+
+		Eventually(func(g Gomega) {
+			By("creating Deployment")
+			deploy := &appsv1.Deployment{}
+			err := k8sClient.Get(ctx, types.NamespacedName{Namespace: ns, Name: model.DeploymentName(backstageName)}, deploy)
+			g.Expect(err).ShouldNot(HaveOccurred())
+
+			By("creating StatefulSet")
+			dbStatefulSet := &appsv1.StatefulSet{}
+			name := fmt.Sprintf("backstage-psql-%s", backstageName)
+			err = k8sClient.Get(ctx, types.NamespacedName{Namespace: ns, Name: name}, dbStatefulSet)
+			g.Expect(err).ShouldNot(HaveOccurred())
+			g.Expect(dbStatefulSet.Spec.Template.Spec.Containers).To(HaveLen(1))
+			g.Expect(dbStatefulSet.Spec.Template.Spec.Containers[0].Image).To(Equal("busybox"))
+			g.Expect(dbStatefulSet.Spec.PodManagementPolicy).To(Equal(appsv1.ParallelPodManagement))
+		}, time.Minute, time.Second).Should(Succeed())
+
+		By("updating CR to default config")
+		update := &bsv1alpha1.Backstage{}
+		err := k8sClient.Get(ctx, types.NamespacedName{Name: backstageName, Namespace: ns}, update)
+		Expect(err).To(Not(HaveOccurred()))
+		update.Spec.RawRuntimeConfig = nil
+		err = k8sClient.Update(ctx, update)
+		Expect(err).To(Not(HaveOccurred()))
+
+		// Patching StatefulSets is done by the reconciler in two passes: first deleting the StatefulSet first, then recreating it in the next reconcilation.
+		for i := 0; i < 2; i++ {
+			_, err = NewTestBackstageReconciler(ns).ReconcileAny(ctx, reconcile.Request{
+				NamespacedName: types.NamespacedName{Name: backstageName, Namespace: ns},
+			})
+			Expect(err).To(Not(HaveOccurred()))
+		}
+
+		Eventually(func(g Gomega) {
+			By("replacing StatefulSet")
+			dbStatefulSet := &appsv1.StatefulSet{}
+			err = k8sClient.Get(ctx, types.NamespacedName{Namespace: ns, Name: fmt.Sprintf("backstage-psql-%s", backstageName)}, dbStatefulSet)
+			g.Expect(err).ShouldNot(HaveOccurred())
+			g.Expect(dbStatefulSet.Spec.PodManagementPolicy).To(Equal(appsv1.OrderedReadyPodManagement))
+		}, time.Minute, time.Second).Should(Succeed())
+	})
+
 })
diff --git a/integration_tests/testdata/raw-statefulset.yaml b/integration_tests/testdata/raw-statefulset.yaml
@@ -4,6 +4,7 @@ metadata:
   name: db-statefulset
 spec:
   serviceName: ""
+  podManagementPolicy: Parallel
   replicas: 1
   selector:
     matchLabels:

diff --git a/pkg/model/db-secret.go b/pkg/model/db-secret.go
@@ -39,7 +39,7 @@ func init() {
 }
 
 func DbSecretDefaultName(backstageName string) string {
-	return utils.GenerateRuntimeObjectName(backstageName, "backstage-db")
+	return utils.GenerateRuntimeObjectName(backstageName, "backstage-psql-secret")
 }
 
 // implementation of RuntimeObject interface

diff --git a/pkg/model/db-secret_test.go b/pkg/model/db-secret_test.go
@@ -16,6 +16,7 @@ package model
 
 import (
 	"context"
+	"fmt"
 	"testing"
 
 	"k8s.io/utils/ptr"
@@ -50,7 +51,7 @@ func TestEmptyDbSecret(t *testing.T) {
 
 	assert.NoError(t, err)
 	assert.NotNil(t, model.LocalDbSecret)
-	assert.Equal(t, DbSecretDefaultName(bs.Name), model.LocalDbSecret.secret.Name)
+	assert.Equal(t, fmt.Sprintf("backstage-psql-secret-%s", bs.Name), model.LocalDbSecret.secret.Name)
 
 	dbss := model.localDbStatefulSet
 	assert.NotNil(t, dbss)
@@ -68,7 +69,7 @@ func TestDefaultWithGeneratedSecrets(t *testing.T) {
 	model, err := InitObjects(context.TODO(), bs, testObj.externalConfig, true, false, testObj.scheme)
 
 	assert.NoError(t, err)
-	assert.Equal(t, DbSecretDefaultName(bs.Name), model.LocalDbSecret.secret.Name)
+	assert.Equal(t, fmt.Sprintf("backstage-psql-secret-%s", bs.Name), model.LocalDbSecret.secret.Name)
 	//should be generated
 	//	assert.NotEmpty(t, model.LocalDbSecret.secret.StringData["POSTGRES_USER"])
 	//	assert.NotEmpty(t, model.LocalDbSecret.secret.StringData["POSTGRES_PASSWORD"])

diff --git a/pkg/model/db-service.go b/pkg/model/db-service.go
@@ -39,7 +39,7 @@ func init() {
 }
 
 func DbServiceName(backstageName string) string {
-	return utils.GenerateRuntimeObjectName(backstageName, "backstage-db")
+	return utils.GenerateRuntimeObjectName(backstageName, "backstage-psql")
 }
 
 // implementation of RuntimeObject interface

diff --git a/pkg/model/db-statefulset.go b/pkg/model/db-statefulset.go
@@ -44,7 +44,7 @@ func init() {
 }
 
 func DbStatefulSetName(backstageName string) string {
-	return utils.GenerateRuntimeObjectName(backstageName, "backstage-db")
+	return utils.GenerateRuntimeObjectName(backstageName, "backstage-psql")
 }
 
 // implementation of RuntimeObject interface

diff --git a/pkg/utils/utils.go b/pkg/utils/utils.go
@@ -70,7 +70,7 @@ func BackstageAppLabelValue(backstageName string) string {
 }
 
 func BackstageDbAppLabelValue(backstageName string) string {
-	return fmt.Sprintf("backstage-db-%s", backstageName)
+	return fmt.Sprintf("backstage-psql-%s", backstageName)
 }
 
 func ReadYaml(manifest []byte, object interface{}) error {