Added the logic for quorum loss scenario

[abdasgupta]

How to categorize this PR?

/area control-plane
/kind enhancement

What this PR does / why we need it:
This PR restores a multinode ETCD cluster after the quorum is lost. The steps are like following:

1. There is already provision for Health check of cluster in ETCD Druid code base. The health check is based on Member Lease renewal time. It also deduct the quorum loss case. This health check is used by Druid
2. Druid custodion controller monitors the cluster health at a regular interval
3. If quorum loss is detected, custodion controller will put an annotation on ETCD CR to indicate that ETCD controller needs to fix quorum loss.
4. ETCD controller scales down the ETCD statefulset to 0 and delete the PVCs
5. ETCD controller deploys the statefulset with replicas = 1
6. ETCD controller scales up the statefulset equal to the replicas mentioned in ETCD CR. ETCD scale up mechanism in ETCD BR makes sure that the first instance of the StatefulSet is up and then the rests are added.
   Which issue(s) this PR fixes:
   Fixes [Feature] Handle quorum loss scenario by Druid in ETCD multinode #362

Special notes for your reviewer:

Release note:

1. A new annotation `gardener.cloud/quorum-loss=true` is used on ETCD CR to indicate the quorum loss happened in the ETCD multi node cluster. If there is no quorum loss, either `gardener.cloud/quorum-loss=false` is set or the annotation is not set at all.

1. ETCD multinode cluster can recover now during quorum loss case. If quorum is lost for multinode cluster and the lost nodes can't be scheduled for some period, then during that period, a temporary one node ETCD cluster will serve the requests and when the new nodes can be scheduled again, rest of the nodes will join the cluster without any manual intervention.

[gardener-robot]

@abdasgupta Labels area/todo, kind/todo do not exist.

[unmarshall]

I could only review one file. Will push further review comments tomorrow.

[unmarshall]

Looks like ImageVector is not used anywhere. Do you plan to use it later?

[unmarshall]

Method not used anywhere. Should ideally be called to register the controller with the manager

[unmarshall]

Function not used anywhere currently

[unmarshall]

ImageVector is not used anywhere. Do you intend to use it later?

[unmarshall]

Minor:
There is a more concise way to implement it. Then you do not have to explicitly handle the error returned from controller.New

	return ctrl.NewControllerManagedBy(mgr).
		WithOptions(controller.Options{
			MaxConcurrentReconciles: workers,
		}).Watches(
		&source.Kind{Type: &coordinationv1.Lease{}},
		&handler.EnqueueRequestForOwner{OwnerType: &druidv1alpha1.Etcd{}, IsController: true},
		builder.WithPredicates(druidpredicates.IsMemberLease()),
	).Complete(cmc)

[unmarshall]

what is the need of continue here?

[unmarshall]

It is written that the loop runs every 5 mins i could not see the delay between 2 loop runs. If there is no error then this loop can run forever and then there is no chance of processing of the next reconcile request. Maybe i miss something. Please check.

[unmarshall]

Ideally these steps of scaling down, deletion of PVC and scaling up to 1 should in separate functions and one can use flow which is also used in gardener for this.

[unmarshall]

use make with initial size. Here you have a fixed size of 2.

[unmarshall]

will label name and instance be always present in sts? If not then you should check for existence of key before adding to the new map.
Suggestion:

func getMatchingLabels(sts *appsv1.StatefulSet) map[string]string {
	const nameLabelKey = "name"
	const instanceLabelKey = "instance"
	labels := make(map[string]string, 2)
	if v, ok := sts.Labels[nameLabelKey]; ok {
		labels[nameLabelKey] = v
	}
	if v, ok := sts.Labels[instanceLabelKey]; ok {
		labels[instanceLabelKey] = v
	}
	return labels
}

[unmarshall]

If there is an error during deletion of PVC, then the request will be re-queued after 10s, after 10 seconds you repeat the above step of setting the replicas to 0. This can be avoided as you already have queries the KAPI to get the sts. If the replicas is already 0 then the above step can be skipped.

[unmarshall]

In the previous step the scale up is done to 1 replica and immediately after that the scale up is again attempted to be equal to the original etcd replicas. Assuming that the above call only changes the spec in etcd and returns and does not wait for the scale up to complete. Is this intended or should you wait for the first replica to be healthy before scaling up from 1-3?

[gardener-robot]

@abdasgupta You need rebase this pull request with latest master branch. Please check.

[abdasgupta]

Added a flag in ETCD druid that will not allow Druid to apply quorum-loss annotation in case of quorum-loss. The flag needs to be set true if somebody wants to allow Druid to handle quorum loss automatically

[aaronfern]

Can you please iterate through the condition list and find the condition with type Ready before using it in the if condition?
Assuming it to be the last condition in the array might be okay now, but future changes might cause this to fail

[abdasgupta]

I don't think we should consider any earlier conditions from the list. It might be very disastrous

[aaronfern]

I don't mean considering any other condition. What I meant is that the Ready condition that gives Reason == "QuorumLost" might not always be at conLength-1 and it might be good if possible to parse through all the conditions and make sure

[abdasgupta]

Suppose, there is a quorum loss. Action is taken on it and then there is some other condition appeared. If we are parsing the whole list every time then we may pick old quorum loss case which is already taken care of. Is it not?

[aaronfern]

The list would have the same conditions. The reasons would change depending on the state of the cluster.
When we recover from quorum loss for instance, the condition would be updated and hence picking up an old quorum loss case will not happen imo

[aaronfern]

Can we rather have the rest of this block inside an if ec.config.EnableAutomaticQuorumLossHandling{...} so that in cases where quorum is lost and the flag is not set, we shouldn't be blocking the rest the code like updateEtcdStatus and/or other functions from running

[abdasgupta]

Why would we want to update ETCD status if the quorum is lost? If quorum is lost and ETCD status is updated, ETCD controller may start working to fix the ETCD . This will give totally unintended result

[aaronfern]

Ah, okay
Fair enough

[aaronfern]

Why is this needed?
Won't returning an empty result set if any lease has an empty RenewTime result in an empty member list?

[abdasgupta]

Couldn't get you what you asked

[aaronfern]

What I meant is, let's say the status already had 2 entries in the member list in the etcd status.
Let's say we add a third member and at the start at lease, the new leave will have a nil renewTime. This returns []Result{} and will result in the member list in the etcd status being completely removed

[aaronfern]

Please add permissions for etcds in charts/etcd/templates/etcd-role.yaml as etcd-backup-restore is now intended to read the etcd resource (ref: here)

[abdasgupta]

Please add permissions for etcds in charts/etcd/templates/etcd-role.yaml as etcd-backup-restore is now intended to read the etcd resource (ref: here)

Like this ? :

- apiGroups:
  - druid.gardener.cloud/v1alpha1
  resources:
  - etcds
  verbs:
  - get
  - list
  - patch
  - update
  - watch

[aaronfern]

yes, but afaik we don't need the v1aplha1

- apiGroups:
  - druid.gardener.cloud
  resources:
  - etcds
  verbs:
  - get
  - list
  - patch
  - update
  - watch

[ashwani2k]

/hold pending test results

[ishan16696]

@abdasgupta I guess we can close this PR ?

[abdasgupta]

We are closing this PR as we decided to handle quorum loss differently. We have identified that two types of quorum loss can happen in the ETCD clusters. One is transient quorum loss. Our ETCD cluster in control plane can recover automatically from transient quorum loss which involves unavailability of most of the ETCD pods due to network problem, resource problem etc. Another is permanent quorum loss. This involves permanent loss of ETCD data directory. A human operator needs to intervene in that case and recover the ETCD cluster. We are writing a playbook for the human operator to follow. The playbook mainly contains the following steps:

1. Scale down the ETCD statefulset to 0
2. Delete all of the PVCs of ETCD pods
3. Scale up the ETCD statefulset to 1
4. Verify that the one ETCD pod recovered correctly
5. Scale up the ETCD statefulset as mentioned in replicas of ETCD CR
6. Verify the cluster