Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modularization & Status Enhancements #188

Merged
merged 7 commits into from
Jun 15, 2021
Merged

Modularization & Status Enhancements #188

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0518097
Add etcd status checks
timuthy May 27, 2021
dde320b
Add sync period to custodian controller
timuthy Jun 2, 2021
82fce48
Address minor comments from review
timuthy Jun 4, 2021
134958d
Handle Unknown -> Ready members
timuthy Jun 4, 2021
d55b704
Remove status.podRef
timuthy Jun 14, 2021
63273d9
Add ClusterSize
timuthy Jun 14, 2021
1e76ea4
Check ContainersReady condition
timuthy Jun 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions api/v1alpha1/etcd_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,9 @@ type EtcdStatus struct {
// LastError represents the last occurred error.
// +optional
LastError *string `json:"lastError,omitempty"`
// Cluster size is the size of the etcd cluster.
// +optional
ClusterSize *int32 `json:"clusterSize,omitempty"`
// CurrentReplicas is the current replica count for the etcd cluster.
// +optional
CurrentReplicas int32 `json:"currentReplicas,omitempty"`
Expand Down
5 changes: 5 additions & 0 deletions api/v1alpha1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions config/crd/bases/druid.gardener.cloud_etcds.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -423,6 +423,10 @@ spec:
status:
description: EtcdStatus defines the observed state of Etcd.
properties:
clusterSize:
description: Cluster size is the size of the etcd cluster.
format: int32
type: integer
conditions:
description: Conditions represents the latest available observations
of an etcd's current state.
Expand Down
89 changes: 57 additions & 32 deletions controllers/etcd_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ import (
"k8s.io/apimachinery/pkg/util/yaml"
"k8s.io/client-go/rest"
"k8s.io/client-go/util/retry"
"k8s.io/utils/pointer"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/controller"
Expand Down Expand Up @@ -209,7 +210,7 @@ func (r *EtcdReconciler) reconcile(ctx context.Context, etcd *druidv1alpha1.Etcd
finalizers.Insert(FinalizerName)
etcd.Finalizers = finalizers.UnsortedList()
if err := r.Update(ctx, etcd); err != nil {
if err := r.updateEtcdErrorStatus(ctx, etcd, nil, err); err != nil {
if err := r.updateEtcdErrorStatus(ctx, noOp, etcd, nil, err); err != nil {
return ctrl.Result{
Requeue: true,
}, err
Expand All @@ -220,7 +221,7 @@ func (r *EtcdReconciler) reconcile(ctx context.Context, etcd *druidv1alpha1.Etcd
}
}
if err := r.addFinalizersToDependantSecrets(ctx, logger, etcd); err != nil {
if err := r.updateEtcdErrorStatus(ctx, etcd, nil, err); err != nil {
if err := r.updateEtcdErrorStatus(ctx, noOp, etcd, nil, err); err != nil {
return ctrl.Result{
Requeue: true,
}, err
Expand All @@ -235,9 +236,9 @@ func (r *EtcdReconciler) reconcile(ctx context.Context, etcd *druidv1alpha1.Etcd
Requeue: true,
}, err
}
svc, ss, err := r.reconcileEtcd(ctx, logger, etcd)
op, svc, sts, err := r.reconcileEtcd(ctx, logger, etcd)
if err != nil {
if err := r.updateEtcdErrorStatus(ctx, etcd, ss, err); err != nil {
if err := r.updateEtcdErrorStatus(ctx, op, etcd, sts, err); err != nil {
logger.Error(err, "Error during reconciling ETCD")
return ctrl.Result{
Requeue: true,
Expand All @@ -247,8 +248,7 @@ func (r *EtcdReconciler) reconcile(ctx context.Context, etcd *druidv1alpha1.Etcd
Requeue: true,
}, err
}

if err := r.updateEtcdStatus(ctx, etcd, svc, ss); err != nil {
if err := r.updateEtcdStatus(ctx, op, etcd, svc, sts); err != nil {
return ctrl.Result{
Requeue: true,
}, err
Expand All @@ -264,7 +264,7 @@ func (r *EtcdReconciler) delete(ctx context.Context, etcd *druidv1alpha1.Etcd) (
logger.Info("Starting operation")

if err := r.removeDependantStatefulset(ctx, logger, etcd); err != nil {
if err := r.updateEtcdErrorStatus(ctx, etcd, nil, err); err != nil {
if err := r.updateEtcdErrorStatus(ctx, deleteOp, etcd, nil, err); err != nil {
return ctrl.Result{
Requeue: true,
}, err
Expand All @@ -275,7 +275,7 @@ func (r *EtcdReconciler) delete(ctx context.Context, etcd *druidv1alpha1.Etcd) (
}

if err := r.removeFinalizersToDependantSecrets(ctx, logger, etcd); err != nil {
if err := r.updateEtcdErrorStatus(ctx, etcd, nil, err); err != nil {
if err := r.updateEtcdErrorStatus(ctx, deleteOp, etcd, nil, err); err != nil {
return ctrl.Result{
Requeue: true,
}, err
Expand Down Expand Up @@ -557,7 +557,16 @@ func (r *EtcdReconciler) getConfigMapFromEtcd(etcd *druidv1alpha1.Etcd, rendered
return decoded, nil
}

func (r *EtcdReconciler) reconcileStatefulSet(ctx context.Context, logger logr.Logger, etcd *druidv1alpha1.Etcd, values map[string]interface{}) (*appsv1.StatefulSet, error) {
type operationResult string

const (
bootstrapOp operationResult = "bootstrap"
reconcileOp operationResult = "reconcile"
deleteOp operationResult = "delete"
noOp operationResult = "none"
)

func (r *EtcdReconciler) reconcileStatefulSet(ctx context.Context, logger logr.Logger, etcd *druidv1alpha1.Etcd, values map[string]interface{}) (operationResult, *appsv1.StatefulSet, error) {
logger.Info("Reconciling etcd statefulset")

// If any adoptions are attempted, we should first recheck for deletion with
Expand All @@ -582,19 +591,19 @@ func (r *EtcdReconciler) reconcileStatefulSet(ctx context.Context, logger logr.L
selector, err := metav1.LabelSelectorAsSelector(etcd.Spec.Selector)
if err != nil {
logger.Error(err, "Error converting etcd selector to selector")
return nil, err
return noOp, nil, err
}
dm := NewEtcdDruidRefManager(r.Client, r.Scheme, etcd, selector, etcdGVK, canAdoptFunc)
statefulSets, err := dm.FetchStatefulSet(ctx, etcd)
if err != nil {
logger.Error(err, "Error while fetching StatefulSet")
return nil, err
return noOp, nil, err
}

logger.Info("Claiming existing etcd StatefulSet")
claimedStatefulSets, err := dm.ClaimStatefulsets(ctx, statefulSets)
if err != nil {
return nil, err
return noOp, nil, err
}

if len(claimedStatefulSets) > 0 {
Expand All @@ -612,41 +621,42 @@ func (r *EtcdReconciler) reconcileStatefulSet(ctx context.Context, logger logr.L
// TODO: (timuthy) Check if this is really needed.
sts := &appsv1.StatefulSet{}
if err := r.Get(ctx, types.NamespacedName{Name: claimedStatefulSets[0].Name, Namespace: claimedStatefulSets[0].Namespace}, sts); err != nil {
return nil, err
return noOp, nil, err
}

// Statefulset is claimed by for this etcd. Just sync the specs
if sts, err = r.syncStatefulSetSpec(ctx, logger, sts, etcd, values); err != nil {
return nil, err
return noOp, nil, err
}

// restart etcd pods in crashloop backoff
selector, err := metav1.LabelSelectorAsSelector(sts.Spec.Selector)
if err != nil {
logger.Error(err, "error converting StatefulSet selector to selector")
return nil, err
return noOp, nil, err
}
podList := &v1.PodList{}
if err := r.List(ctx, podList, client.InNamespace(etcd.Namespace), client.MatchingLabelsSelector{Selector: selector}); err != nil {
return nil, err
return noOp, nil, err
}

for _, pod := range podList.Items {
if utils.IsPodInCrashloopBackoff(pod.Status) {
if err := r.Delete(ctx, &pod); err != nil {
logger.Error(err, fmt.Sprintf("error deleting etcd pod in crashloop: %s/%s", pod.Namespace, pod.Name))
return nil, err
return noOp, nil, err
}
}
}

return r.waitUntilStatefulSetReady(ctx, logger, etcd, sts)
sts, err = r.waitUntilStatefulSetReady(ctx, logger, etcd, sts)
return reconcileOp, sts, err
}

// Required statefulset doesn't exist. Create new
sts, err := r.getStatefulSetFromEtcd(etcd, values)
if err != nil {
return nil, err
return noOp, nil, err
}

err = r.Create(ctx, sts)
Expand All @@ -658,10 +668,11 @@ func (r *EtcdReconciler) reconcileStatefulSet(ctx context.Context, logger logr.L
err = nil
}
if err != nil {
return nil, err
return noOp, nil, err
}

return r.waitUntilStatefulSetReady(ctx, logger, etcd, sts)
sts, err = r.waitUntilStatefulSetReady(ctx, logger, etcd, sts)
return bootstrapOp, sts, err
}

func getContainerMapFromPodTemplateSpec(spec v1.PodSpec) map[string]v1.Container {
Expand Down Expand Up @@ -758,40 +769,39 @@ func (r *EtcdReconciler) getStatefulSetFromEtcd(etcd *druidv1alpha1.Etcd, values
return decoded, nil
}

func (r *EtcdReconciler) reconcileEtcd(ctx context.Context, logger logr.Logger, etcd *druidv1alpha1.Etcd) (*corev1.Service, *appsv1.StatefulSet, error) {

func (r *EtcdReconciler) reconcileEtcd(ctx context.Context, logger logr.Logger, etcd *druidv1alpha1.Etcd) (operationResult, *corev1.Service, *appsv1.StatefulSet, error) {
values, err := r.getMapFromEtcd(etcd)
if err != nil {
return nil, nil, err
return noOp, nil, nil, err
}

chartPath := getChartPath()
renderedChart, err := r.chartApplier.Render(chartPath, etcd.Name, etcd.Namespace, values)
if err != nil {
return nil, nil, err
return noOp, nil, nil, err
}
svc, err := r.reconcileServices(ctx, logger, etcd, renderedChart)
if err != nil {
return nil, nil, err
return noOp, nil, nil, err
}
if svc != nil {
values["serviceName"] = svc.Name
}

cm, err := r.reconcileConfigMaps(ctx, logger, etcd, renderedChart)
if err != nil {
return nil, nil, err
return noOp, nil, nil, err
}
if cm != nil {
values["configMapName"] = cm.Name
}

ss, err := r.reconcileStatefulSet(ctx, logger, etcd, values)
op, sts, err := r.reconcileStatefulSet(ctx, logger, etcd, values)
if err != nil {
return nil, nil, err
return noOp, nil, nil, err
}

return svc, ss, nil
return op, svc, sts, nil
}

func checkEtcdOwnerReference(refs []metav1.OwnerReference, etcd *druidv1alpha1.Etcd) bool {
Expand Down Expand Up @@ -1118,14 +1128,24 @@ func canDeleteStatefulset(sts *appsv1.StatefulSet, etcd *druidv1alpha1.Etcd) boo

}

func (r *EtcdReconciler) updateEtcdErrorStatus(ctx context.Context, etcd *druidv1alpha1.Etcd, sts *appsv1.StatefulSet, lastError error) error {
func bootstrapReset(etcd *druidv1alpha1.Etcd) {
etcd.Status.Members = nil
etcd.Status.ClusterSize = pointer.Int32Ptr(int32(etcd.Spec.Replicas))
}

func (r *EtcdReconciler) updateEtcdErrorStatus(ctx context.Context, op operationResult, etcd *druidv1alpha1.Etcd, sts *appsv1.StatefulSet, lastError error) error {
err := kutil.TryUpdateStatus(ctx, retry.DefaultBackoff, r.Client, etcd, func() error {
lastErrStr := fmt.Sprintf("%v", lastError)
etcd.Status.LastError = &lastErrStr
etcd.Status.ObservedGeneration = &etcd.Generation
if sts != nil {
ready := CheckStatefulSet(etcd, sts) == nil
etcd.Status.Ready = &ready

if op == bootstrapOp {
// Reset members in bootstrap phase to ensure depending conditions can be calculated correctly.
bootstrapReset(etcd)
}
}
return nil
})
Expand All @@ -1136,14 +1156,19 @@ func (r *EtcdReconciler) updateEtcdErrorStatus(ctx context.Context, etcd *druidv
return r.removeOperationAnnotation(ctx, etcd)
}

func (r *EtcdReconciler) updateEtcdStatus(ctx context.Context, etcd *druidv1alpha1.Etcd, svc *corev1.Service, sts *appsv1.StatefulSet) error {
func (r *EtcdReconciler) updateEtcdStatus(ctx context.Context, op operationResult, etcd *druidv1alpha1.Etcd, svc *corev1.Service, sts *appsv1.StatefulSet) error {
err := kutil.TryUpdateStatus(ctx, retry.DefaultBackoff, r.Client, etcd, func() error {
ready := CheckStatefulSet(etcd, sts) == nil
etcd.Status.Ready = &ready
svcName := svc.Name
etcd.Status.ServiceName = &svcName
etcd.Status.LastError = nil
etcd.Status.ObservedGeneration = &etcd.Generation

if op == bootstrapOp {
// Reset members in bootstrap phase to ensure depending conditions can be calculated correctly.
bootstrapReset(etcd)
}
return nil
})

Expand Down
6 changes: 6 additions & 0 deletions controllers/etcd_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,12 @@ var _ = Describe("Druid", func() {
err = c.Status().Update(context.TODO(), sts)
Eventually(func() error { return statefulsetIsCorrectlyReconciled(c, instance, sts) }, timeout, pollingInterval).Should(BeNil())
Expect(err).NotTo(HaveOccurred())
Eventually(func() (*int32, error) {
if err := c.Get(context.TODO(), client.ObjectKeyFromObject(instance), instance); err != nil {
return nil, err
}
return instance.Status.ClusterSize, nil
}, timeout, pollingInterval).Should(Equal(pointer.Int32Ptr(int32(instance.Spec.Replicas))))
})
It("should create and adopt statefulset and printing events", func() {
// Check StatefulSet requirements
Expand Down
58 changes: 32 additions & 26 deletions controllers/etcd_custodian_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,9 @@ func (ec *EtcdCustodian) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.
// Requeue if we found more than one or no StatefulSet.
// The Etcd controller needs to decide what to do in such situations.
if len(stsList.Items) != 1 {
ec.updateEtcdStatusWithNoSts(ctx, logger, etcd)
if err := ec.updateEtcdStatus(ctx, logger, etcd, nil); err != nil {
logger.Error(err, "Error while updating ETCD status when no statefulset found")
}
return ctrl.Result{
RequeueAfter: 5 * time.Second,
}, nil
Expand All @@ -131,43 +133,47 @@ func (ec *EtcdCustodian) updateEtcdStatus(ctx context.Context, logger logr.Logge
)

return kutil.TryUpdateStatus(ctx, retry.DefaultBackoff, ec.Client, etcd, func() error {
etcd.Status.Etcd = &druidv1alpha1.CrossVersionObjectReference{
APIVersion: sts.APIVersion,
Kind: sts.Kind,
Name: sts.Name,
}

etcd.Status.Conditions = conditions
etcd.Status.Members = members

ready := CheckStatefulSet(etcd, sts) == nil

// To be changed once we have multiple replicas.
etcd.Status.CurrentReplicas = sts.Status.CurrentReplicas
etcd.Status.ReadyReplicas = sts.Status.ReadyReplicas
etcd.Status.UpdatedReplicas = sts.Status.UpdatedReplicas
etcd.Status.Ready = &ready
logger.Info(fmt.Sprintf("ETCD status updated for statefulset current replicas: %v, ready replicas: %v, updated replicas: %v", sts.Status.CurrentReplicas, sts.Status.ReadyReplicas, sts.Status.UpdatedReplicas))
return nil
})
}
// Bootstrap is a special case which is handled by the etcd controller.
if !inBootstrap(etcd) && len(members) != 0 {
etcd.Status.ClusterSize = pointer.Int32Ptr(int32(len(members)))
}
Comment on lines +139 to +142
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strictly speaking, this is incorrect. Only the initial bootstrapping is triggered by the etcd controller. Later bootstrapping due to quorum failure is to be done by the custodian controller. But this change could be done in #158.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this comment as this reflects the current state.

Tbh, I'm not convinced yet that the Custodian controller performs self healing in the form of re-bootstrapping because the Etcd controller can in the meantime still reconcile etcd resources which can lead to races. In order to avoid a race, you need to introduce some kind of lock which is also not too nice, IMO.

Maybe I didn't check all cases in detail but at the moment I think an alternative would be easier to implement:

  1. Custodian sees that cluster is not healthy and self healing is required.
  2. Custodian triggers a reconciliation for the affected etcd resource.
  3. Etcd controller kicks in and takes necessary action to bring cluster up again (=> desired state). In the meantime any changes to the very same etcd resource cannot lead to a race.

We don't have to decide in this PR of course, but still wanted to give an alternative approach to think about.

Copy link
Collaborator

@amshuman-kr amshuman-kr Jun 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to avoid a race, you need to introduce some kind of lock which is also not too nice, IMO.

Even if only the etcd controller is responsible for bootstrapping non-quorate clusters, some sort of race avoidance is still required because the Etcd spec might get updated while bootstrapping is still on-going, in which case, it should either wait until the on-going bootstrapping completed before applying the latest spec changes or should abort the current bootstrapping (which might unnecessarily prolong the downtime).

  1. Custodian triggers a reconciliation for the affected etcd resource.

This cannot be the regular etcd controller reconciliation which might apply Etcd resource spec changes along with triggering bootstrapping. As a part of the contract of a gardener extension, etcd-druid not apply Etcd resource spec changes unless reconcile operation annotation is present.

On the other hand, it makes no sense for the etcd-druid to wait for the next shoot reconciliation to fix a non-quorate ETCD cluster.

But let's take this discussion out of this PR.


func (ec *EtcdCustodian) updateEtcdStatusWithNoSts(ctx context.Context, logger logr.Logger, etcd *druidv1alpha1.Etcd) {
logger.Info("Updating etcd status when no statefulset found")
conditions := etcd.Status.Conditions
if sts != nil {
etcd.Status.Etcd = &druidv1alpha1.CrossVersionObjectReference{
APIVersion: sts.APIVersion,
Kind: sts.Kind,
Name: sts.Name,
}

ready := CheckStatefulSet(etcd, sts) == nil

// To be changed once we have multiple replicas.
etcd.Status.CurrentReplicas = sts.Status.CurrentReplicas
etcd.Status.ReadyReplicas = sts.Status.ReadyReplicas
etcd.Status.UpdatedReplicas = sts.Status.UpdatedReplicas
etcd.Status.Ready = &ready
logger.Info(fmt.Sprintf("ETCD status updated for statefulset current replicas: %v, ready replicas: %v, updated replicas: %v", sts.Status.CurrentReplicas, sts.Status.ReadyReplicas, sts.Status.UpdatedReplicas))
return nil
}

if err := kutil.TryUpdateStatus(ctx, retry.DefaultBackoff, ec.Client, etcd, func() error {
etcd.Status.Conditions = conditions
// To be changed once we have multiple replicas.
etcd.Status.CurrentReplicas = 0
etcd.Status.ReadyReplicas = 0
etcd.Status.UpdatedReplicas = 0

etcd.Status.Ready = pointer.BoolPtr(false)
return nil
}); err != nil {
logger.Error(err, "Error while updating ETCD status when no statefulset found")
})
}

func inBootstrap(etcd *druidv1alpha1.Etcd) bool {
if etcd.Status.ClusterSize == nil {
return true
}
return len(etcd.Status.Members) == 0 ||
(len(etcd.Status.Members) < etcd.Spec.Replicas && int32(etcd.Spec.Replicas) == *etcd.Status.ClusterSize)
}

// SetupWithManager sets up manager with a new controller and ec as the reconcile.Reconciler
Expand Down
Loading