Member Peer URL not updated when scaled to multi-node, scale identification is also not correct

pkg/miscellaneous/miscellaneous.go

@@ -39,6 +39,8 @@ import (
 	"go.etcd.io/etcd/etcdserver/etcdserverpb"
 	"go.etcd.io/etcd/pkg/types"
 	"gopkg.in/yaml.v2"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/utils/pointer"
 
 	appsv1 "k8s.io/api/apps/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -416,25 +418,30 @@ func IsBackupBucketEmpty(snapStoreConfig *brtypes.SnapstoreConfig, logger *logru
 	return true
 }
 
+const scaledToMultiNodeAnnotationKey = "gardener.cloud/scaled-to-multi-node"
+
 // GetInitialClusterStateIfScaleup checks if it is the Scale-up scenario and returns the cluster state either `new` or `existing`.
-func GetInitialClusterStateIfScaleup(ctx context.Context, logger logrus.Entry, clientSet client.Client, podName string, podNS string) string {
+func GetInitialClusterStateIfScaleup(ctx context.Context, logger logrus.Entry, clientSet client.Client, podName string, podNS string) (*string, error) {
 	//Read sts spec for updated replicas to toggle `initial-cluster-state`
 	curSts := &appsv1.StatefulSet{}
-	errSts := clientSet.Get(ctx, client.ObjectKey{
+	err := clientSet.Get(ctx, client.ObjectKey{
 		Namespace: podNS,
 		Name:      podName[:strings.LastIndex(podName, "-")],
 	}, curSts)
-	if errSts != nil {
-		logger.Warn("error fetching etcd sts ", errSts)
-		return ClusterStateNew
+	if err != nil {
+		logger.Warn("error fetching etcd sts ", err)
+		return nil, err
 	}
 
-	//TODO: achieve this without an sts?
-	if *curSts.Spec.Replicas > 1 && *curSts.Spec.Replicas > curSts.Status.UpdatedReplicas {
-		return ClusterStateExisting
+	if metav1.HasAnnotation(curSts.ObjectMeta, scaledToMultiNodeAnnotationKey) {
+		return pointer.StringPtr(ClusterStateExisting), nil
 	}
 
-	return ""
+	// fallback - preserves backward compatibility
+	if *curSts.Spec.Replicas > 1 && *curSts.Spec.Replicas > curSts.Status.UpdatedReplicas {
+		return pointer.StringPtr(ClusterStateExisting), nil
+	}
+	return nil, nil
 }
 
 // DoPromoteMember promotes a given learner to a voting member.

pkg/miscellaneous/miscellaneous_test.go

@@ -26,8 +26,11 @@ import (
 	"github.com/gardener/etcd-backup-restore/pkg/snapstore"
 	brtypes "github.com/gardener/etcd-backup-restore/pkg/types"
 	"github.com/golang/mock/gomock"
+
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
+	. "github.com/onsi/gomega/gstruct"
+
 	"go.etcd.io/etcd/clientv3"
 	"go.etcd.io/etcd/etcdserver/etcdserverpb"
 	appsv1 "k8s.io/api/apps/v1"
@@ -463,92 +466,106 @@ var _ = Describe("Miscellaneous Tests", func() {
 			statefulSetName = "etcd-test"
 			podName         = "etcd-test-0"
 			namespace       = "test_namespace"
-			emptyString     = ""
 		)
+
+		BeforeEach(func() {
+			sts = emptyStatefulSet(statefulSetName, namespace)
+		})
+
 		Context("In single node etcd: no scale-up", func() {
-			It("Should return the cluster state as empty string ", func() {
-				sts = &appsv1.StatefulSet{
-					TypeMeta: metav1.TypeMeta{
-						Kind:       "StatefulSet",
-						APIVersion: "apps/v1",
-					},
-					ObjectMeta: metav1.ObjectMeta{
-						Name:      statefulSetName,
-						Namespace: namespace,
-					},
-					Spec: appsv1.StatefulSetSpec{
-						Replicas: pointer.Int32Ptr(1),
-					},
-					Status: appsv1.StatefulSetStatus{
-						UpdatedReplicas: 1,
-					},
+			BeforeEach(func() {
+				sts.Spec = appsv1.StatefulSetSpec{
+					Replicas: pointer.Int32Ptr(1),
 				}
+				sts.Status = appsv1.StatefulSetStatus{
+					UpdatedReplicas: 1,
+				}
+			})
+
+			It("Should return the cluster state as nil", func() {
 				clientSet := GetFakeKubernetesClientSet()
 
 				err := clientSet.Create(testCtx, sts)
 				Expect(err).ShouldNot(HaveOccurred())
 
-				clusterState := GetInitialClusterStateIfScaleup(testCtx, *logger, clientSet, podName, namespace)
-				Expect(clusterState).Should(Equal(emptyString))
+				clusterState, err := GetInitialClusterStateIfScaleup(testCtx, *logger, clientSet, podName, namespace)
+				Expect(err).ToNot(HaveOccurred())
+				Expect(clusterState).To(BeNil())
 			})
 		})
+
 		Context("In multi-node etcd bootstrap: no scale-up", func() {
-			It("Should return the cluster state as empty string ", func() {
-				sts = &appsv1.StatefulSet{
-					TypeMeta: metav1.TypeMeta{
-						Kind:       "StatefulSet",
-						APIVersion: "apps/v1",
-					},
-					ObjectMeta: metav1.ObjectMeta{
-						Name:      statefulSetName,
-						Namespace: namespace,
-					},
-					Spec: appsv1.StatefulSetSpec{
-						Replicas: pointer.Int32Ptr(3),
-					},
-					Status: appsv1.StatefulSetStatus{
-						UpdatedReplicas: 3,
-					},
+			BeforeEach(func() {
+				sts.Spec = appsv1.StatefulSetSpec{
+					Replicas: pointer.Int32Ptr(3),
+				}
+				sts.Status = appsv1.StatefulSetStatus{
+					UpdatedReplicas: 3,
 				}
+			})
+
+			It("Should return the cluster state as nil", func() {
 				clientSet := GetFakeKubernetesClientSet()
 
 				err := clientSet.Create(testCtx, sts)
 				Expect(err).ShouldNot(HaveOccurred())
 
-				clusterState := GetInitialClusterStateIfScaleup(testCtx, *logger, clientSet, podName, namespace)
-				Expect(clusterState).Should(Equal(emptyString))
+				clusterState, err := GetInitialClusterStateIfScaleup(testCtx, *logger, clientSet, podName, namespace)
+				Expect(err).ToNot(HaveOccurred())
+				Expect(clusterState).Should(BeNil())
 			})
 		})
-		Context("In case of Scaling up from single node to multi-node etcd", func() {
+
+		Context("In case of Scaling up from single node to multi-node etcd with no scale-up annotation set", func() {
+			BeforeEach(func() {
+				sts.Spec = appsv1.StatefulSetSpec{
+					Replicas: pointer.Int32Ptr(3),
+				}
+				sts.Status = appsv1.StatefulSetStatus{
+					UpdatedReplicas: 1,
+				}
+			})
+
 			It("Should return clusterState as `existing` ", func() {
-				sts = &appsv1.StatefulSet{
-					TypeMeta: metav1.TypeMeta{
-						Kind:       "StatefulSet",
-						APIVersion: "apps/v1",
-					},
-					ObjectMeta: metav1.ObjectMeta{
-						Name:      statefulSetName,
-						Namespace: namespace,
-					},
-					Spec: appsv1.StatefulSetSpec{
-						Replicas: pointer.Int32Ptr(3),
-					},
-					Status: appsv1.StatefulSetStatus{
-						UpdatedReplicas: 1,
-					},
+				clientSet := GetFakeKubernetesClientSet()
+
+				err := clientSet.Create(testCtx, sts)
+				Expect(err).ShouldNot(HaveOccurred())
+
+				clusterState, err := GetInitialClusterStateIfScaleup(testCtx, *logger, clientSet, podName, namespace)
+				Expect(err).ToNot(HaveOccurred())
+				Expect(clusterState).Should(PointTo(Equal(ClusterStateExisting)))
+			})
+		})
+
+		Context("scaling of single node to multi-node etcd with scale-up annotation set", func() {
+			BeforeEach(func() {
+				sts.Spec = appsv1.StatefulSetSpec{
+					Replicas: pointer.Int32Ptr(3),
+				}
+				sts.Status = appsv1.StatefulSetStatus{
+					UpdatedReplicas: 3,
+				}
+				sts.Annotations = map[string]string{
+					scaledToMultiNodeAnnotationKey: "",
 				}
+			})
+
+			It("should return existing cluster", func() {
 				clientSet := GetFakeKubernetesClientSet()
 
 				err := clientSet.Create(testCtx, sts)
 				Expect(err).ShouldNot(HaveOccurred())
 
-				clusterState := GetInitialClusterStateIfScaleup(testCtx, *logger, clientSet, podName, namespace)
-				Expect(clusterState).Should(Equal(ClusterStateExisting))
+				clusterState, err := GetInitialClusterStateIfScaleup(testCtx, *logger, clientSet, podName, namespace)
+				Expect(err).ToNot(HaveOccurred())
+				Expect(clusterState).Should(PointTo(Equal(ClusterStateExisting)))
+
 			})
 		})
 
 		Context("Unable to fetch statefulset", func() {
-			It("Should return clusterState as `new` ", func() {
+			It("Should return error", func() {
 				sts = &appsv1.StatefulSet{
 					TypeMeta: metav1.TypeMeta{
 						Kind:       "StatefulSet",
@@ -572,60 +589,71 @@ var _ = Describe("Miscellaneous Tests", func() {
 				err := clientSet.Create(testCtx, sts)
 				Expect(err).ShouldNot(HaveOccurred())
 
-				clusterState := GetInitialClusterStateIfScaleup(testCtx, *logger, clientSet, podName, wrongNamespace)
-				Expect(clusterState).Should(Equal(ClusterStateNew))
+				_, err = GetInitialClusterStateIfScaleup(testCtx, *logger, clientSet, podName, wrongNamespace)
+				Expect(err).To(HaveOccurred())
 			})
 		})
 	})
+
 	Describe("parse peer urls config", func() {
 		var (
 			initialAdPeerURL string
 			podName          string
 		)
+
 		BeforeEach(func() {
 			podName = "etcd-test-pod-0"
 		})
+
 		Context("parse peer url", func() {
 			It("parsing well-defined initial-advertise-peer-urls", func() {
 				initialAdPeerURL = "https@etcd-events-peer@shoot--dev--test@2380"
 				peerURL, err := ParsePeerURL(initialAdPeerURL, podName)
 				Expect(err).To(BeNil())
 				Expect(peerURL).To(Equal("https://etcd-test-pod-0.etcd-events-peer.shoot--dev--test.svc:2380"))
 			})
+
 			It("parsing malformed initial-advertise-peer-urls", func() {
 				initialAdPeerURL = "https@etcd-events-peer@shoot--dev--test"
 				_, err := ParsePeerURL(initialAdPeerURL, podName)
 				Expect(err).ToNot(BeNil())
 			})
 		})
 	})
+
 	Describe("read config file into a map", func() {
 		const testdataPath = "testdata"
 		var (
 			configPath string
 		)
+
 		Context("valid config file", func() {
+
 			BeforeEach(func() {
 				configPath = filepath.Join(testdataPath, "valid_config.yaml")
 			})
+
 			It("test read and parse yaml", func() {
 				configAsMap, err := ReadConfigFileAsMap(configPath)
 				Expect(err).To(BeNil())
 				Expect(configAsMap).ToNot(BeNil())
 				Expect(configAsMap["name"]).To(Equal("etcd-57c38d")) //just testing one property
 			})
 		})
+
 		Context("invalid file path", func() {
 			It("test read and parse for a non-existent path", func() {
 				configPath = "file-does-not-exist.yaml"
 				_, err := ReadConfigFileAsMap(configPath)
 				Expect(err).ToNot(BeNil())
 			})
 		})
+
 		Context("invalid yaml file", func() {
 			BeforeEach(func() {
 				configPath = filepath.Join(testdataPath, "invalid_config.yaml")
 			})
+
 			It("test read and parse an invalid config yaml", func() {
 				_, err := ReadConfigFileAsMap(configPath)
 				Expect(err).ToNot(BeNil())
@@ -635,6 +663,15 @@ var _ = Describe("Miscellaneous Tests", func() {
 
 })
 
+func emptyStatefulSet(name, namespace string) *appsv1.StatefulSet {
+	return &appsv1.StatefulSet{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      name,
+			Namespace: namespace,
+		},
+	}
+}
+
 func generateSnapshotList(n int) brtypes.SnapList {
 	snapList := brtypes.SnapList{}
 	for i := 0; i < n; i++ {

pkg/server/backrestoreserver_test.go

@@ -3,34 +3,42 @@ package server
 import (
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
+	"github.com/sirupsen/logrus"
 )
 
 var _ = Describe("backrestoreserver tests", func() {
-	const podName = "etcd-test-pod-0"
 	var (
-		initialAdvertisePeerURLs string
+		memberPeerURL string
+		brServer      *BackupRestoreServer
+		err           error
 	)
 
+	BeforeEach(func() {
+		brServer, err = NewBackupRestoreServer(logrus.New(), &BackupRestoreComponentConfig{})
+		Expect(err).ToNot(HaveOccurred())
+	})
+
 	Describe("testing isPeerURLTLSEnabled", func() {
 		Context("testing with non-TLS enabled peer url", func() {
 			BeforeEach(func() {
-				initialAdvertisePeerURLs = "http@etcd-main-peer@default@2380"
+				memberPeerURL = "http://etcd-main-peer.default.svc:2380"
 			})
 			It("test", func() {
-				enabled, err := isPeerURLTLSEnabled(initialAdvertisePeerURLs, podName)
+				enabled := brServer.isPeerURLTLSEnabled(memberPeerURL)
 				Expect(err).To(BeNil())
 				Expect(enabled).To(BeFalse())
 			})
 
 		})
+
 		Context("testing with TLS enabled peer url", func() {
 			BeforeEach(func() {
-				initialAdvertisePeerURLs = "https@etcd-main-peer@default@2380"
+				memberPeerURL = "https://etcd-main-peer.default.svc:2380"
 			})
 			It("test", func() {
-				enabled, err := isPeerURLTLSEnabled(initialAdvertisePeerURLs, podName)
+				enabled := brServer.isPeerURLTLSEnabled(memberPeerURL)
 				Expect(err).To(BeNil())
-				Expect(enabled).To(BeFalse())
+				Expect(enabled).To(BeTrue())
 			})
 		})
 	})