Handle corrupted etcd members

[ArkaSaha30]

Fixes: #474

This PR will:

• Replace a member when etcd flags it with a CORRUPT alarm
• Disarm CORRUPT alarms left behind by removed members
• Poll etcd every 10s when idle, so alarms are noticed
• Add unit, e2e and failpoint tests

[ahrtr]

Overall the production code change of this PR looks good. Please resolve the test failure and mark the PR as ready to review.

cc @nwnt @silentred

[ArkaSaha30]

/test pull-etcd-operator-test-e2e

[silentred]

Overall looks good, with only 2 minor comments.

[nwnt]

In e2e, for the failpoint to work, you will need to specify the failpoint name with the fail term, and then call enable/disableGoFailPoint helpers during the setup and tear down to trigger it. You can see TestScaling as an example of how to do this:

etcd-operator/test/e2e/e2e_test.go

Line 132 in 9d16e80

func TestScaling(t *testing.T) {

[ahrtr]

@ArkaSaha30 I simplified the implementation below, please apply it if it makes sense to you.

// handleCorruptAlarm handles CORRUPT alarm if any with three steps:
// 1. mark the corrupted member as replacing
// 2. remove the etcd member from the cluster
// 3. disarming the CORRUPT alarm
func (r *EtcdClusterReconciler) handleCorruptAlarm(ctx context.Context, s *reconcileState) (ctrl.Result, error) {
	if s.health == nil {
		return ctrl.Result{}, nil
	}

	i := slices.IndexFunc(s.health.Alarms, func(a *etcdserverpb.AlarmMember) bool {
		return a.Alarm == etcdserverpb.AlarmType_CORRUPT
	})
	if i < 0 {
		return ctrl.Result{}, nil
	}
	alarm := s.health.Alarms[i]
	logger := log.FromContext(ctx).WithValues("memberID", fmt.Sprintf("%x", alarm.MemberID))
	logger.Info("CORRUPT alarm found")
	if alarm.MemberID == 0 {
		// The only case that etcd may raise a CORRUPT alarm with 0 MemberID
		// is when there is no quorum and hashes mismatch detected. But if
		// there is no quorum, the `AlarmList` call won't be successful. So
		// it should never run into this situation.
		return ctrl.Result{}, fmt.Errorf("CORRUPT alarm detected, but the MemberID is 0")
	}

	if s.memberListResp == nil {
		return ctrl.Result{}, fmt.Errorf("cannot handle the CORRUPT alarm of etcd member %x without a MemberList response", alarm.MemberID)
	}
	member := findEtcdMemberForEtcdNodeID(s, alarm.MemberID)
	// An EtcdMember always exists while its etcd node is in the membership, so a nil member
	// means the node has already been removed, e.g. by a crash before the disarm; skip straight to the disarm.
	if member != nil {
		// Replacing is persisted before the removal so an interruption can't strand a removed member as Ready.
		if member.Status.Phase != ecv1alpha1.EtcdMemberReplacing {
			logger.Info("replacing a corrupted EtcdMember", "EtcdMember", member.Name)
			if err := r.markMemberReplacing(ctx, member); err != nil {
				return ctrl.Result{}, err
			}
		}
		logger.Info("Removing etcd member for handling CORRUPT alarm", "EtcdMember", member.Name)
		if err := removeEtcdNode(s, member); err != nil {
			return ctrl.Result{}, err
		}

		// gofail: var corruptAlarmAfterMemberRemove struct{}
	}

	logger.Info("Disarming the CORRUPT alarm")
	// The removed member has stopped, so its endpoint can't serve any requests.
	pods := slices.DeleteFunc(slices.Clone(s.pods), func(p *corev1.Pod) bool { return member != nil && p.Name == member.Name })
	endpoints := clientEndpointsFromPods(s.cluster.Name, s.cluster.Namespace, pods, clusterTLSEnabled(s.cluster))
	cfg := etcdutils.ClientConfig{Endpoints: endpoints, TLS: s.tlsConfig}
	if err := etcdutils.AlarmDisarm(cfg, []*etcdserverpb.AlarmMember{alarm}); err != nil {
		return ctrl.Result{}, fmt.Errorf("failed to disarm the CORRUPT alarm of etcd member %x: %w", alarm.MemberID, err)
	}
	return ctrl.Result{RequeueAfter: requeueDuration}, nil
}

I also re-considered @silentred 's comment #530 (comment), ideally we might need to handle the recreating case first. For example, if one etcd member is in progress of recreating, then other member is corrupted, then we should finish the recreation first. This is a extreme case, if we really run into such case, we might need human intervene. Let's leave it as it's for now

@nwnt @silentred please also take a look at the test case in case you have any comments.

[silentred]

After a crash between MemberRemove and the Disarm, findEtcdMemberForEtcdNodeID returns nil because the node is no longer in memberListResp, so the current Pod filter excludes nothing. The removed member's still-running Pod stays in the disarm endpoint list. With the pick_first balancer (clientv3 3.7.x goes through grpc.NewClient, which defaults to pick_first), the client sticks to the first endpoint. If the removed member was ordinal 0, the disarm fails on every retry.

EtcdMember.Status.MemberID is retained after the removal of the etcd node, so we can try to match the MemberID with alarm.MemberID.

[ahrtr]

good point.

[ahrtr]

@ArkaSaha30 can you squash the commits or I squash them for you when merging.

[ArkaSaha30]

@ArkaSaha30 can you squash the commits or I squash them for you when merging.

Sure, I have squashed the commits.

[ahrtr]

Merging this PR for now, if anyone has any more comments, feel free to resolve them in separate PRs.

[kubernetes-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahrtr, ArkaSaha30

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [ahrtr]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[ahrtr]

# 1. First run: provision everything, but leave it standing afterwards.
ETCD_E2E_SKIP_TEARDOWN=true go test ./test/e2e/... -run TestSomething -v

# 2. Iterate: point individual runs at the environment left up by step 1.
ETCD_E2E_SKIP_SETUP=true ETCD_E2E_SKIP_TEARDOWN=true go test ./test/e2e/... -run TestCorruptAlarm -v