Add support for etcd cluster scale up #487

aaronfern · 2022-06-03T04:09:58Z

What this PR does / why we need it:
This PR adds support for scaling up an etcd cluster from 1 -> 3
Etcd backup restore will now on bootstrap detect if there is an etcd cluster already running and add itself to it as a learner. It will then promote itself to a full voting member when its logs have caught up with the leader

Which issue(s) this PR fixes:
Fixes gardener/etcd-druid#349

Special notes for your reviewer:
To be reviewed in conjuncture with gardener/etcd-druid#366

Release note:

Etcd can now scale up itself from a single member cluster to a multi member cluster

ishan16696

I take a initial look, direction/design seems correct to me although I have few doubts.
Some nitpicks:

ishan16696 · 2022-06-06T05:44:51Z

pkg/initializer/initializer.go

+	// List members in cluster
+	memListCtx, cancel := context.WithTimeout(context.TODO(), 5*time.Second)
+	etcdMemberList, err := cli.MemberList(memListCtx)
+	cancel()


Suggested change

cancel()

defer cancel()

Why does it need Endpoint to be passed by druid?

pkg/initializer/initializer.go

pkg/server/backuprestoreserver.go

ishan16696 · 2022-06-08T13:24:16Z

pkg/initializer/initializer.go

+			//Create etcd client
+			clientFactory := etcdutil.NewFactory(brtypes.EtcdConnectionConfig{
+				Endpoints:         []string{"http://etcd-main-peer.default.svc:2380"}, //TODO: use ETCD_ENDPOINT env var passed by druid
+				InsecureTransport: true,


Whenever we want to connect to embedded etcd or its corresponding etcd we set InsecureTransport: true to get etcd client.
But here you need to connect to a etcd member of etcd cluster running in a different pod to get etcd client then I guess you should try to have secure connection. WDYT?

pkg/server/backuprestoreserver.go

pkg/server/httpAPI.go

abdasgupta

We may need a discussion on my comments

pkg/initializer/initializer.go

abdasgupta · 2022-06-20T07:29:11Z

pkg/initializer/initializer.go

+	// List members in cluster
+	memListCtx, cancel := context.WithTimeout(context.TODO(), 5*time.Second)
+	etcdMemberList, err := cli.MemberList(memListCtx)
+	cancel()


Why does it need Endpoint to be passed by druid?

abdasgupta · 2022-06-20T07:39:15Z

pkg/initializer/initializer.go

+
+func getMemberURL() string {
+	//end := strings.Split(os.Getenv("ETCD_ENDPOINT"), "//") //TODO: use ETCD_ENDPOINT env var passed by druid
+	memberURL := "http://" + os.Getenv("POD_NAME") + ".etcd-main-peer.default.svc:2380"


Can you form the URL from here

etcd-backup-restore/pkg/server/backuprestoreserver.go

Line 106 in 608b7c4

configYML, err := os.ReadFile(inputFileName)

?

pkg/server/backuprestoreserver.go

abdasgupta · 2022-06-20T07:44:36Z

pkg/server/httpAPI.go

+		}
+
+		config["initial-cluster"] = cluster[:len(cluster)-1]
+	}


Can we edit the config file backuprestoreserver.go instead of httpapi.go ?

I'm not sure if we can
config file need to be edited here so that it can be properly served to the corresponding etcd. I didn't understand how editing it in backuprestoreserver.go would help

unmarshall · 2022-06-20T09:52:14Z

pkg/member/member_add.go

+	for {
+		//Create etcd client
+		//TODO: Use secure transport
+		clientFactory := etcdutil.NewFactory(brtypes.EtcdConnectionConfig{


Is it necessary to create a client factory and then an etcd client every time a method a function is called? This is true for all other functions in this file as well.

Hi @unmarshall ,
Basically NewFactory() takes the EtcdConnectionConfig and returns the clientFactory. The thing is EtcdConnectionConfig can vary according to the use case. so, If we keep using the same factory this might leads to ambiguity and can leads to human error that dev don’t notice about EtcdConfiguration used in factory creation and ends up using same factory again and again. I think with increasing code base it is better to create clientFactory again with EtcdConnectionConfig.

unmarshall · 2022-06-20T09:52:58Z

pkg/member/member_add.go

+	memberURL := getMemberURL()
+	if memberURL == "" {
+		logger.Warn("Could not fetch member URL")
+	}


If the member URL is empty, then a warning is logged but a member is still added as a learner. You decided to continuing, ignoring this error here. When a cluster.memberAdd gets called that in turn calls types.NewURLs passing an empty member URL. This method will return an error. The error handling in general needs to be re-written.

unmarshall · 2022-06-20T09:54:55Z

pkg/member/member_add.go

+}
+
+// PromoteMember promotes an etcd member from a learner to a voting member of the cluster. This will succeed only if its logs are caught up with the leader
+func PromoteMember(ctx context.Context, logger *logrus.Entry) {


ideally an interface should be created for the consumers so that it would be possible to test. This also allows you to create a client once and re-use the same client.

unmarshall · 2022-06-20T10:01:31Z

pkg/member/member_add.go

+		})
+
+		memAddCtx, cancel := context.WithTimeout(context.TODO(), brtypes.DefaultEtcdConnectionTimeout)
+		cli, _ := clientFactory.NewCluster()


clientFactory.NewCluster returns an error which is ignored here. Any reason for ignoring the error?

unmarshall · 2022-06-20T10:32:39Z

pkg/member/member_add.go

+			InsecureTransport: true,
+		})
+
+		memAddCtx, cancel := context.WithTimeout(context.TODO(), brtypes.DefaultEtcdConnectionTimeout)


Create a new merhod which has a timeout passed to it and that does an actual member add, handles the error and it returns an an error also logs the MemberAddResponse for better logging and troubleshooting. The retry can be done in this method.

unmarshall · 2022-06-20T10:58:04Z

pkg/member/member_add.go

+}
+
+// PromoteMember promotes an etcd member from a learner to a voting member of the cluster. This will succeed only if its logs are caught up with the leader
+func PromoteMember(ctx context.Context, logger *logrus.Entry) {


you are not using context anywhere

unmarshall · 2022-06-20T11:01:40Z

pkg/member/member_add.go

+		if memListErr != nil {
+			logger.Info("error listing members: ", memListErr)
+			cli.Close()
+			continue


is it possible to have unrecoverable errors, if yes then you will end up in an infinite loop. I would highly recommend not to do retries in this file. Return proper errors and then let the caller handle the retry.
If you really wish to retry in this file, then have 2 functions one which does not retry and one which retries and delegates to the function which does not retry.

unmarshall · 2022-06-20T11:02:37Z

pkg/member/member_add.go

+		var promoted bool
+		promoted = false
+		for _, y := range etcdList.Members {
+			if y.Name == os.Getenv("POD_NAME") {


There is no need to look out POD_NAME from an environment so many times. Once the environment is set its immutable, doing it just once is good enough.

Also this function should take pod name as an argument, the caller should decide from where it wishes to get the pod name and not this function, makes unit testing difficult.

unmarshall · 2022-06-20T11:09:25Z

pkg/member/member_add.go

+					break
+				}
+				if strings.Contains(rpctypes.ErrGRPCMemberNotLearner.Error(), memPromoteErr.Error()) {
+					//Exit if member is already part of the cluster


If there is an error why do you have to loop through the rest of the members? The intent of this method is to only promote one member

pkg/member/member_add.go

unmarshall · 2022-06-21T04:38:11Z

pkg/member/member_add.go

@@ -0,0 +1,158 @@
+package member


Unfortunately golang does not have a well defined convention for naming the files (golang/go#36060) but you should choose one convention and be consistent throughout the project. in this project there are files names using camelCase, snakecase, all-lower-case. Stick to one.

unmarshall · 2022-06-21T04:39:07Z

pkg/member/member_add.go

+
+const (
+	// RetryPeriod is the peroid after which an operation is retried
+	RetryPeriod time.Duration = 5 * time.Second


there is no need to specify type as it is derived.

Is there a need to expose this when this is only used within this file?

unmarshall · 2022-06-21T04:43:10Z

pkg/server/backuprestoreserver.go

@@ -195,6 +196,9 @@ func (b *BackupRestoreServer) runServer(ctx context.Context, restoreOpts *brtype
 	handler := b.startHTTPServer(etcdInitializer, b.config.SnapstoreConfig.Provider, b.config.EtcdConnectionConfig, nil)
 	defer handler.Stop()

+	// Promotes member if it is a learner
+	member.PromoteMember(context.TODO(), b.logger)


Is there a need to create a context.TODO instead of passing the context that is sent as a method argument?

When do you promote this member? Is it immediately promoted?

that's a thing I missed. Will use a proper context

A member will be allowed to be promoted as soon as its logs have caught up to the leader. There is no way to know exactly whether a member is ready to be promoted, so we immediately try to promote a member and we keep on trying until it succeeds. The promote call will succeed only when its logs have caught up to the leader

unmarshall · 2022-06-21T04:45:05Z

pkg/server/httpAPI.go

@@ -393,7 +395,9 @@ func (h *HTTPHandler) serveLatestSnapshotMetadata(rw http.ResponseWriter, req *h

 func (h *HTTPHandler) serveConfig(rw http.ResponseWriter, req *http.Request) {


ideally inputFileName, outputFileName should be passed as config to this method, else testing is not possible. Alternatively define a struct which captures this and then define methods on it.

unmarshall · 2022-06-21T04:56:14Z

pkg/server/httpAPI.go

+	//Read sts spec for updated replicas to toggle `initial-cluster-state`
+	clientSet, err := miscellaneous.GetKubernetesClientSetOrError()
+	if err != nil {
+		h.Logger.Errorf("failed to create clientset: %v", err)


This method handles an incoming http request and should return a proper error response with a proper error code set so that the client can consume the response and react accordingly.

unmarshall · 2022-06-21T04:59:42Z

pkg/server/httpAPI.go

+		return
+	}
+	curSts := &appsv1.StatefulSet{}
+	errSts := clientSet.Get(context.TODO(), client.ObjectKey{


you should ideally get the context from req.Context() and then pass it to functions that you are going to call, so that if the request context is cancelled so does all external function calls as well

unmarshall · 2022-06-21T05:00:31Z

pkg/server/httpAPI.go

+		Name:      podName[:strings.LastIndex(podName, "-")],
+	}, curSts)
+	if errSts != nil {
+		h.Logger.Warn("error fetching etcd sts ", errSts)


again a proper HttpResponse with a proper code should be returned.

unmarshall · 2022-06-21T05:01:06Z

pkg/server/httpAPI.go

+	if err != nil {
+		h.Logger.Warnf("Error with NewCluster() : %v", err)
+	}
+	ctx, cancel := context.WithTimeout(context.TODO(), brtypes.DefaultEtcdConnectionTimeout)


pass context after getting it from req.Context()

unmarshall · 2022-06-21T05:21:26Z

pkg/server/httpAPI.go

+	memList, err := cli.MemberList(ctx)
+	noOfMembers := 0
+	if err != nil {
+		h.Logger.Warnf("Error with MemberList() : %v", err)


If there is an error then, do you assumed that there will be 0 members? Is that a correct assumption?

Yes, if there's an error here, the assumption is that it is a member bootstrap or a cluster is waking up from hibernation, so we would not want to manipulate the initial-cluster string

ishan16696

some more nitpicks :)

pkg/server/httpAPI.go

ishan16696

Hi @aaronfern , Overall looks good to me. Final few nit picks.

pkg/member/member_control.go

pkg/miscellaneous/miscellaneous.go

aaronfern requested a review from a team as a code owner June 3, 2022 04:09

gardener-robot added needs/review Needs review size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) labels Jun 3, 2022

gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 3, 2022

aaronfern added reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jun 3, 2022

ishan16696 requested changes Jun 6, 2022

View reviewed changes

gardener-robot added the needs/changes Needs (more) changes label Jun 6, 2022

ishan16696 requested changes Jun 8, 2022

View reviewed changes

ishan16696 reviewed Jun 13, 2022

View reviewed changes

pkg/server/httpAPI.go Outdated Show resolved Hide resolved

gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 13, 2022

gardener-robot-ci-1 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jun 13, 2022

aaronfern force-pushed the scale_up_2 branch from 3b8c033 to da446ef Compare June 15, 2022 13:53

gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 15, 2022

gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 15, 2022

aaronfern force-pushed the scale_up_2 branch from da446ef to f232e4d Compare June 15, 2022 14:02

gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 15, 2022

gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 15, 2022

gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 17, 2022

gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 17, 2022

abdasgupta reviewed Jun 20, 2022

View reviewed changes

unmarshall reviewed Jun 20, 2022

View reviewed changes

unmarshall reviewed Jun 21, 2022

View reviewed changes

gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 21, 2022

gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 21, 2022

gardener-robot added size/l Size of pull request is large (see gardener-robot robot/bots/size.py) needs/second-opinion Needs second review by someone else and removed size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) labels Jun 23, 2022

gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 23, 2022

ishan16696 requested changes Jul 7, 2022

View reviewed changes

pkg/server/httpAPI.go Outdated Show resolved Hide resolved

pkg/server/httpAPI.go Show resolved Hide resolved

pkg/server/httpAPI.go Show resolved Hide resolved

gardener-robot added size/l Size of pull request is large (see gardener-robot robot/bots/size.py) and removed size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py) labels Jul 7, 2022

gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 7, 2022

gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 7, 2022

aaronfern added 9 commits July 7, 2022 13:56

Add support for etcd cluster scale up

a14262e

Added member pkg for all etcd member manipulation functions

7c93a6d

Use ETCD_ENDPOINT env var to fetch endpoint to be used be etcd client

d36284a

Add https support for etcd clients

a4284f0

Address review comments

7637e7b

Updated service endpoint URL

68db10e

Update member peer address of old clusters

2f81d39

Added minor error handling

77d527e

Addressed review comments

45ee8af

ishan16696 requested changes Jul 7, 2022

View reviewed changes

pkg/member/member_control.go Outdated Show resolved Hide resolved

pkg/member/member_control.go Show resolved Hide resolved

pkg/miscellaneous/miscellaneous.go Outdated Show resolved Hide resolved

Added unit tests

703f8fe

aaronfern force-pushed the scale_up_2 branch from 5aae944 to 703f8fe Compare July 7, 2022 18:42

gardener-robot added size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py) and removed size/l Size of pull request is large (see gardener-robot robot/bots/size.py) labels Jul 7, 2022

gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 7, 2022

gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 7, 2022

Minor additions and logs

fb07723

gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 8, 2022

gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 8, 2022

ishan16696 approved these changes Jul 8, 2022

View reviewed changes

aaronfern merged commit f1bf59f into gardener:master Jul 8, 2022

gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Jul 8, 2022

timuthy mentioned this pull request Jul 13, 2022

[ci:component:github.com/gardener/etcd-druid:v0.10.0->v0.11.0] gardener/gardener#6292

Closed

aaronfern mentioned this pull request Jul 18, 2022

[Feature] Add/Remove member from etcd cluster #488

Closed

2 tasks

abdasgupta mentioned this pull request Aug 29, 2022

[Feature] Handle quorum loss scenario by Druid in ETCD multinode gardener/etcd-druid#362

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for etcd cluster scale up #487

Add support for etcd cluster scale up #487

aaronfern commented Jun 3, 2022 •

edited

Loading

ishan16696 left a comment

ishan16696 Jun 6, 2022

abdasgupta Jun 20, 2022

ishan16696 Jun 8, 2022

abdasgupta left a comment

abdasgupta Jun 20, 2022

abdasgupta Jun 20, 2022

abdasgupta Jun 20, 2022

aaronfern Jun 27, 2022

unmarshall Jun 20, 2022

ishan16696 Jun 21, 2022

unmarshall Jun 20, 2022

unmarshall Jun 20, 2022

unmarshall Jun 20, 2022

unmarshall Jun 20, 2022

unmarshall Jun 20, 2022

unmarshall Jun 20, 2022

unmarshall Jun 20, 2022

unmarshall Jun 20, 2022

unmarshall Jun 21, 2022

unmarshall Jun 21, 2022

unmarshall Jun 21, 2022

aaronfern Jun 21, 2022

unmarshall Jun 21, 2022

unmarshall Jun 21, 2022

unmarshall Jun 21, 2022

unmarshall Jun 21, 2022

unmarshall Jun 21, 2022

unmarshall Jun 21, 2022

aaronfern Jun 21, 2022

ishan16696 left a comment

ishan16696 left a comment

		@@ -393,7 +395,9 @@ func (h HTTPHandler) serveLatestSnapshotMetadata(rw http.ResponseWriter, req h

		func (h HTTPHandler) serveConfig(rw http.ResponseWriter, req http.Request) {

Add support for etcd cluster scale up #487

Add support for etcd cluster scale up #487

Conversation

aaronfern commented Jun 3, 2022 • edited Loading

ishan16696 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abdasgupta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ishan16696 left a comment

Choose a reason for hiding this comment

ishan16696 left a comment

Choose a reason for hiding this comment

aaronfern commented Jun 3, 2022 •

edited

Loading