From 6a19e2648a8dd5c1faeb6485051a76d7f61d9cc9 Mon Sep 17 00:00:00 2001 From: Ingo Gottwald Date: Tue, 20 Jul 2021 17:12:16 +0200 Subject: [PATCH 1/2] Make eco recognize podTemplate changes Changes are handled like version upgrades with a rolling re-deployment cycle of all etcd members. --- api/v1alpha1/validation.go | 1 + api/v1alpha1/validation_test.go | 14 ----------- controllers/etcdcluster_controller.go | 35 ++++++++++++++++++--------- 3 files changed, 25 insertions(+), 25 deletions(-) diff --git a/api/v1alpha1/validation.go b/api/v1alpha1/validation.go index 7933cde1..cbe17a3b 100644 --- a/api/v1alpha1/validation.go +++ b/api/v1alpha1/validation.go @@ -67,6 +67,7 @@ func (o *EtcdCluster) ValidateUpdate(old runtime.Object) error { // Overwrite the fields which are allowed to change oldO.Spec.Replicas = o.Spec.Replicas oldO.Spec.Version = o.Spec.Version + oldO.Spec.PodTemplate = o.Spec.PodTemplate if diff := cmp.Diff(oldO.Spec, o.Spec); diff != "" { return fmt.Errorf("Unsupported changes: (- current, + new) %s", diff) diff --git a/api/v1alpha1/validation_test.go b/api/v1alpha1/validation_test.go index c9fceb39..2052e06a 100644 --- a/api/v1alpha1/validation_test.go +++ b/api/v1alpha1/validation_test.go @@ -156,20 +156,6 @@ func TestEtcdCluster_ValidateUpdate(t *testing.T) { }, err: "^Unsupported changes:", }, - { - // TODO Support pod annotation modification https://github.com/improbable-eng/etcd-cluster-operator/issues/109 - name: "ModifyPodSpecAnnotation", - modifier: func(o *v1alpha1.EtcdCluster) { - o.Spec.PodTemplate = &v1alpha1.EtcdPodTemplateSpec{ - Metadata: &v1alpha1.EtcdPodTemplateObjectMeta{ - Annotations: map[string]string{ - "new-annotation": "some-value", - }, - }, - } - }, - err: "^Unsupported changes:", - }, { name: "UnsupportedChange/StorageClassName", modifier: func(o *v1alpha1.EtcdCluster) { diff --git a/controllers/etcdcluster_controller.go b/controllers/etcdcluster_controller.go index b40d812a..203d6c8c 100644 --- a/controllers/etcdcluster_controller.go +++ b/controllers/etcdcluster_controller.go @@ -13,6 +13,7 @@ import ( "github.com/go-logr/logr" etcdclient "go.etcd.io/etcd/client" v1 "k8s.io/api/core/v1" + apiequality "k8s.io/apimachinery/pkg/api/equality" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -404,13 +405,15 @@ func (r *EtcdClusterReconciler) reconcile( } } - // Upgrade Etcd. + // Upgrade Etcd or incorporate changes. // Remove EtcdPeers which have a different version than EtcdCluster; in reverse name order, one-at-a-time. - // The EtcdPeers will be recreated in the next reconcile, with the correct version. - if peer := nextOutdatedPeer(cluster, peers); peer != nil { - if peer.Spec.Version == cluster.Spec.Version { - log.Info("Waiting for EtcdPeer to report expected server version", "etcdpeer-name", peer.Name) - return result, nil, nil + // The EtcdPeers will be recreated in the next reconcile, with the correct state. + if peer, reason := nextOutdatedPeer(cluster, peers); peer != nil { + if reason == outdatedVersion { + if peer.Spec.Version == cluster.Spec.Version { + log.Info("Waiting for EtcdPeer to report expected server version", "etcdpeer-name", peer.Name) + return result, nil, nil + } } if !peer.DeletionTimestamp.IsZero() { log.Info("Waiting for EtcdPeer be deleted", "etcdpeer-name", peer.Name) @@ -473,10 +476,17 @@ func (r *EtcdClusterReconciler) reconcile( return result, nil, nil } -// nextOutdatedPeer returns an EtcdPeer which has a different version than the -// EtcdCluster. +type outdatedReason int + +const ( + outdatedVersion outdatedReason = iota + 1 + outdatedPodTemplate +) + +// nextOutdatedPeer returns an EtcdPeer which is different to the spec of the +// EtcdCluster. It also returns the reason why it's outdated. // It searches EtcdPeers in reverse name order. -func nextOutdatedPeer(cluster *etcdv1alpha1.EtcdCluster, peers *etcdv1alpha1.EtcdPeerList) *etcdv1alpha1.EtcdPeer { +func nextOutdatedPeer(cluster *etcdv1alpha1.EtcdCluster, peers *etcdv1alpha1.EtcdPeerList) (*etcdv1alpha1.EtcdPeer, outdatedReason) { peerNames := make([]string, len(peers.Items)) peersByName := map[string]etcdv1alpha1.EtcdPeer{} for i, peer := range peers.Items { @@ -491,10 +501,13 @@ func nextOutdatedPeer(cluster *etcdv1alpha1.EtcdCluster, peers *etcdv1alpha1.Etc continue } if peer.Status.ServerVersion != cluster.Spec.Version { - return peer.DeepCopy() + return peer.DeepCopy(), outdatedVersion + } + if !apiequality.Semantic.DeepEqual(peer.Spec.PodTemplate, cluster.Spec.PodTemplate) { + return peer.DeepCopy(), outdatedPodTemplate } } - return nil + return nil, 0 } func hasTooFewPeers(cluster *etcdv1alpha1.EtcdCluster, peers *etcdv1alpha1.EtcdPeerList) bool { From 032993e9f0d49476cd9376468aefb87f84174506 Mon Sep 17 00:00:00 2001 From: Ingo Gottwald Date: Wed, 10 Nov 2021 13:55:12 +0100 Subject: [PATCH 2/2] Address code review comments --- controllers/etcdcluster_controller.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/controllers/etcdcluster_controller.go b/controllers/etcdcluster_controller.go index 203d6c8c..63c74179 100644 --- a/controllers/etcdcluster_controller.go +++ b/controllers/etcdcluster_controller.go @@ -365,7 +365,7 @@ func (r *EtcdClusterReconciler) reconcile( if isAllMembersStable(*members) { // The remaining operations can now be performed. In order: // * Remove surplus EtcdPeers - // * Upgrade Etcd + // * Upgrade Etcd or make changes that need rolling deployment // * Scale-up // * Scale-down // @@ -406,11 +406,15 @@ func (r *EtcdClusterReconciler) reconcile( } // Upgrade Etcd or incorporate changes. - // Remove EtcdPeers which have a different version than EtcdCluster; in reverse name order, one-at-a-time. + // Remove EtcdPeers which have a different version than EtcdCluster; in reverse name order, one-at-a-time. // The EtcdPeers will be recreated in the next reconcile, with the correct state. if peer, reason := nextOutdatedPeer(cluster, peers); peer != nil { if reason == outdatedVersion { if peer.Spec.Version == cluster.Spec.Version { + // The peer returned by nextOutdatedPeer is outdated based on the status.Version but the version + // in the spec is already matching the desired cluster version. This means the peer spec is + // already up to date and we need to wait until the status is updated. Once the status is up to date + // nextOutdatedPeer will return the next outdated peer. log.Info("Waiting for EtcdPeer to report expected server version", "etcdpeer-name", peer.Name) return result, nil, nil }