diff --git a/internal/controller/etcd/register.go b/internal/controller/etcd/register.go index 59f9aaa86..1b7bf5329 100644 --- a/internal/controller/etcd/register.go +++ b/internal/controller/etcd/register.go @@ -59,13 +59,13 @@ func (r *Reconciler) buildPredicate() predicate.Predicate { // not been removed (since the last reconcile is not yet successfully completed). onReconcileAnnotationSetPredicate := predicate.And( r.hasReconcileAnnotation(), - predicate.Or(lastReconcileHasFinished(), specUpdated()), + predicate.Or(lastReconcileHasFinished(), specUpdated(), neverReconciled()), ) // If auto-reconcile has been enabled then it should allow reconciliation only on spec change. autoReconcileOnSpecChangePredicate := predicate.And( r.autoReconcileEnabled(), - specUpdated(), + predicate.Or(specUpdated(), neverReconciled()), ) return predicate.Or( @@ -132,6 +132,24 @@ func lastReconcileHasFinished() predicate.Predicate { } } +// neverReconciled handles a specific case which is outlined in https://github.com/gardener/etcd-druid/issues/898 +// It is possible that the initial Create event was not processed. In such cases, the status will not be updated. +// If there is an update event for such a resource then the predicates should allow the event to be processed. +func neverReconciled() predicate.Predicate { + return predicate.Funcs{ + UpdateFunc: func(updateEvent event.UpdateEvent) bool { + newEtcd, ok := updateEvent.ObjectNew.(*druidv1alpha1.Etcd) + if !ok { + return false + } + return newEtcd.Status.LastOperation == nil && newEtcd.Status.ObservedGeneration == nil + }, + CreateFunc: func(_ event.CreateEvent) bool { return false }, + DeleteFunc: func(_ event.DeleteEvent) bool { return false }, + GenericFunc: func(_ event.GenericEvent) bool { return false }, + } +} + func hasLastReconcileFinished(updateEvent event.UpdateEvent) bool { newEtcd, ok := updateEvent.ObjectNew.(*druidv1alpha1.Etcd) // return false if either the object is not an etcd resource or it has not been reconciled yet. diff --git a/internal/controller/etcd/register_test.go b/internal/controller/etcd/register_test.go index 7e3d2f98e..c7b0f9a9b 100644 --- a/internal/controller/etcd/register_test.go +++ b/internal/controller/etcd/register_test.go @@ -20,10 +20,11 @@ import ( ) type predicateTestCase struct { - name string - etcdSpecChanged bool - etcdStatusChanged bool - lastOperationState *druidv1alpha1.LastOperationState + name string + etcdSpecChanged bool + etcdStatusChanged bool + previouslyReconciled bool + lastOperationState *druidv1alpha1.LastOperationState // expected behavior for different event types shouldAllowCreateEvent bool shouldAllowDeleteEvent bool @@ -45,6 +46,7 @@ func TestBuildPredicateWithOnlyAutoReconcileEnabled(t *testing.T) { { name: "only spec has changed and previous reconciliation has completed", etcdSpecChanged: true, + previouslyReconciled: true, lastOperationState: ptr.To(druidv1alpha1.LastOperationStateSucceeded), shouldAllowCreateEvent: true, shouldAllowDeleteEvent: true, @@ -54,6 +56,7 @@ func TestBuildPredicateWithOnlyAutoReconcileEnabled(t *testing.T) { { name: "only spec has changed and previous reconciliation has errored", etcdSpecChanged: true, + previouslyReconciled: true, lastOperationState: ptr.To(druidv1alpha1.LastOperationStateError), shouldAllowCreateEvent: true, shouldAllowDeleteEvent: true, @@ -63,6 +66,7 @@ func TestBuildPredicateWithOnlyAutoReconcileEnabled(t *testing.T) { { name: "only status has changed", etcdStatusChanged: true, + previouslyReconciled: true, shouldAllowCreateEvent: true, shouldAllowDeleteEvent: true, shouldAllowGenericEvent: false, @@ -72,6 +76,7 @@ func TestBuildPredicateWithOnlyAutoReconcileEnabled(t *testing.T) { name: "both spec and status have changed and previous reconciliation is in progress", etcdSpecChanged: true, etcdStatusChanged: true, + previouslyReconciled: true, lastOperationState: ptr.To(druidv1alpha1.LastOperationStateProcessing), shouldAllowCreateEvent: true, shouldAllowDeleteEvent: true, @@ -80,11 +85,21 @@ func TestBuildPredicateWithOnlyAutoReconcileEnabled(t *testing.T) { }, { name: "neither spec nor status has changed", + previouslyReconciled: true, shouldAllowCreateEvent: true, shouldAllowDeleteEvent: true, shouldAllowGenericEvent: false, shouldAllowUpdateEvent: false, }, + { + // This case is described in https://github.com/gardener/etcd-druid/issues/898 + name: "for an existing Etcd resource, neither spec nor status has changed and it has never been reconciled before", + previouslyReconciled: false, + shouldAllowCreateEvent: true, + shouldAllowDeleteEvent: true, + shouldAllowGenericEvent: false, + shouldAllowUpdateEvent: true, + }, } g := NewWithT(t) etcd := createEtcd() @@ -93,7 +108,7 @@ func TestBuildPredicateWithOnlyAutoReconcileEnabled(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { t.Parallel() - updatedEtcd := updateEtcd(etcd, tc.etcdSpecChanged, tc.etcdStatusChanged, tc.lastOperationState, false) + updatedEtcd := updateEtcd(etcd, tc.etcdSpecChanged, tc.etcdStatusChanged, tc.previouslyReconciled, tc.lastOperationState, false) g.Expect(predicate.Create(event.CreateEvent{Object: updatedEtcd})).To(Equal(tc.shouldAllowCreateEvent)) g.Expect(predicate.Delete(event.DeleteEvent{Object: updatedEtcd})).To(Equal(tc.shouldAllowDeleteEvent)) g.Expect(predicate.Generic(event.GenericEvent{Object: updatedEtcd})).To(Equal(tc.shouldAllowGenericEvent)) @@ -107,6 +122,7 @@ func TestBuildPredicateWithNoAutoReconcileAndNoReconcileAnnot(t *testing.T) { { name: "only spec has changed", etcdSpecChanged: true, + previouslyReconciled: true, shouldAllowCreateEvent: true, shouldAllowDeleteEvent: true, shouldAllowGenericEvent: false, @@ -115,6 +131,7 @@ func TestBuildPredicateWithNoAutoReconcileAndNoReconcileAnnot(t *testing.T) { { name: "only status has changed", etcdStatusChanged: true, + previouslyReconciled: true, shouldAllowCreateEvent: true, shouldAllowDeleteEvent: true, shouldAllowGenericEvent: false, @@ -123,6 +140,7 @@ func TestBuildPredicateWithNoAutoReconcileAndNoReconcileAnnot(t *testing.T) { { name: "both spec and status have changed", etcdSpecChanged: true, + previouslyReconciled: true, etcdStatusChanged: true, shouldAllowCreateEvent: true, shouldAllowDeleteEvent: true, @@ -131,6 +149,16 @@ func TestBuildPredicateWithNoAutoReconcileAndNoReconcileAnnot(t *testing.T) { }, { name: "neither spec nor status has changed", + previouslyReconciled: true, + shouldAllowCreateEvent: true, + shouldAllowDeleteEvent: true, + shouldAllowGenericEvent: false, + shouldAllowUpdateEvent: false, + }, + { + // This case is described in https://github.com/gardener/etcd-druid/issues/898 + name: "for an existing Etcd resource, neither spec nor status has changed and it has never been reconciled before", + previouslyReconciled: false, shouldAllowCreateEvent: true, shouldAllowDeleteEvent: true, shouldAllowGenericEvent: false, @@ -144,7 +172,7 @@ func TestBuildPredicateWithNoAutoReconcileAndNoReconcileAnnot(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { t.Parallel() - updatedEtcd := updateEtcd(etcd, tc.etcdSpecChanged, tc.etcdStatusChanged, tc.lastOperationState, false) + updatedEtcd := updateEtcd(etcd, tc.etcdSpecChanged, tc.etcdStatusChanged, tc.previouslyReconciled, tc.lastOperationState, false) g.Expect(predicate.Create(event.CreateEvent{Object: updatedEtcd})).To(Equal(tc.shouldAllowCreateEvent)) g.Expect(predicate.Delete(event.DeleteEvent{Object: updatedEtcd})).To(Equal(tc.shouldAllowDeleteEvent)) g.Expect(predicate.Generic(event.GenericEvent{Object: updatedEtcd})).To(Equal(tc.shouldAllowGenericEvent)) @@ -168,6 +196,7 @@ func TestBuildPredicateWithNoAutoReconcileButReconcileAnnotPresent(t *testing.T) name: "only spec has changed and previous reconciliation is completed", etcdSpecChanged: true, lastOperationState: ptr.To(druidv1alpha1.LastOperationStateSucceeded), + previouslyReconciled: true, shouldAllowCreateEvent: true, shouldAllowDeleteEvent: true, shouldAllowGenericEvent: false, @@ -176,6 +205,7 @@ func TestBuildPredicateWithNoAutoReconcileButReconcileAnnotPresent(t *testing.T) { name: "only status has changed and previous reconciliation is in progress", etcdStatusChanged: true, + previouslyReconciled: true, lastOperationState: ptr.To(druidv1alpha1.LastOperationStateProcessing), shouldAllowCreateEvent: true, shouldAllowDeleteEvent: true, @@ -185,6 +215,7 @@ func TestBuildPredicateWithNoAutoReconcileButReconcileAnnotPresent(t *testing.T) { name: "only status has changed and previous reconciliation is completed", etcdStatusChanged: true, + previouslyReconciled: true, lastOperationState: ptr.To(druidv1alpha1.LastOperationStateSucceeded), shouldAllowCreateEvent: true, shouldAllowDeleteEvent: true, @@ -195,6 +226,7 @@ func TestBuildPredicateWithNoAutoReconcileButReconcileAnnotPresent(t *testing.T) name: "both spec and status have changed", etcdSpecChanged: true, etcdStatusChanged: true, + previouslyReconciled: true, shouldAllowCreateEvent: true, shouldAllowDeleteEvent: true, shouldAllowGenericEvent: false, @@ -203,6 +235,7 @@ func TestBuildPredicateWithNoAutoReconcileButReconcileAnnotPresent(t *testing.T) { name: "neither spec nor status has changed and previous reconciliation is in error", lastOperationState: ptr.To(druidv1alpha1.LastOperationStateError), + previouslyReconciled: true, shouldAllowCreateEvent: true, shouldAllowDeleteEvent: true, shouldAllowGenericEvent: false, @@ -211,6 +244,16 @@ func TestBuildPredicateWithNoAutoReconcileButReconcileAnnotPresent(t *testing.T) { name: "neither spec nor status has changed and previous reconciliation is completed", lastOperationState: ptr.To(druidv1alpha1.LastOperationStateSucceeded), + previouslyReconciled: true, + shouldAllowCreateEvent: true, + shouldAllowDeleteEvent: true, + shouldAllowGenericEvent: false, + shouldAllowUpdateEvent: true, + }, + { + // This case is described in https://github.com/gardener/etcd-druid/issues/898 + name: "for an existing Etcd resource, neither spec nor status has changed and it has never been reconciled before", + previouslyReconciled: false, shouldAllowCreateEvent: true, shouldAllowDeleteEvent: true, shouldAllowGenericEvent: false, @@ -224,7 +267,7 @@ func TestBuildPredicateWithNoAutoReconcileButReconcileAnnotPresent(t *testing.T) for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { t.Parallel() - updatedEtcd := updateEtcd(etcd, tc.etcdSpecChanged, tc.etcdStatusChanged, tc.lastOperationState, true) + updatedEtcd := updateEtcd(etcd, tc.etcdSpecChanged, tc.etcdStatusChanged, tc.previouslyReconciled, tc.lastOperationState, true) g.Expect(predicate.Create(event.CreateEvent{Object: updatedEtcd})).To(Equal(tc.shouldAllowCreateEvent)) g.Expect(predicate.Delete(event.DeleteEvent{Object: updatedEtcd})).To(Equal(tc.shouldAllowDeleteEvent)) g.Expect(predicate.Generic(event.GenericEvent{Object: updatedEtcd})).To(Equal(tc.shouldAllowGenericEvent)) @@ -256,6 +299,7 @@ func TestBuildPredicateWithAutoReconcileAndReconcileAnnotSet(t *testing.T) { name: "only status has changed and previous reconciliation is completed", etcdStatusChanged: true, lastOperationState: ptr.To(druidv1alpha1.LastOperationStateSucceeded), + previouslyReconciled: true, shouldAllowCreateEvent: true, shouldAllowDeleteEvent: true, shouldAllowGenericEvent: false, @@ -265,6 +309,7 @@ func TestBuildPredicateWithAutoReconcileAndReconcileAnnotSet(t *testing.T) { name: "both spec and status have changed", etcdSpecChanged: true, etcdStatusChanged: true, + previouslyReconciled: true, shouldAllowCreateEvent: true, shouldAllowDeleteEvent: true, shouldAllowGenericEvent: false, @@ -273,6 +318,7 @@ func TestBuildPredicateWithAutoReconcileAndReconcileAnnotSet(t *testing.T) { { name: "neither spec nor status has changed and previous reconciliation is in error", lastOperationState: ptr.To(druidv1alpha1.LastOperationStateError), + previouslyReconciled: true, shouldAllowCreateEvent: true, shouldAllowDeleteEvent: true, shouldAllowGenericEvent: false, @@ -289,6 +335,16 @@ func TestBuildPredicateWithAutoReconcileAndReconcileAnnotSet(t *testing.T) { { name: "neither spec nor status has changed and previous reconciliation is completed", lastOperationState: ptr.To(druidv1alpha1.LastOperationStateSucceeded), + previouslyReconciled: true, + shouldAllowCreateEvent: true, + shouldAllowDeleteEvent: true, + shouldAllowGenericEvent: false, + shouldAllowUpdateEvent: true, + }, + { + // This case is described in https://github.com/gardener/etcd-druid/issues/898 + name: "for an existing Etcd resource, neither spec nor status has changed and it has never been reconciled before", + previouslyReconciled: false, shouldAllowCreateEvent: true, shouldAllowDeleteEvent: true, shouldAllowGenericEvent: false, @@ -302,7 +358,7 @@ func TestBuildPredicateWithAutoReconcileAndReconcileAnnotSet(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { t.Parallel() - updatedEtcd := updateEtcd(etcd, tc.etcdSpecChanged, tc.etcdStatusChanged, tc.lastOperationState, true) + updatedEtcd := updateEtcd(etcd, tc.etcdSpecChanged, tc.etcdStatusChanged, tc.previouslyReconciled, tc.lastOperationState, true) g.Expect(predicate.Create(event.CreateEvent{Object: updatedEtcd})).To(Equal(tc.shouldAllowCreateEvent)) g.Expect(predicate.Delete(event.DeleteEvent{Object: updatedEtcd})).To(Equal(tc.shouldAllowDeleteEvent)) g.Expect(predicate.Generic(event.GenericEvent{Object: updatedEtcd})).To(Equal(tc.shouldAllowGenericEvent)) @@ -314,7 +370,7 @@ func TestBuildPredicateWithAutoReconcileAndReconcileAnnotSet(t *testing.T) { func createEtcd() *druidv1alpha1.Etcd { etcd := testutils.EtcdBuilderWithDefaults(testutils.TestEtcdName, testutils.TestNamespace).WithReplicas(3).Build() etcd.Status = druidv1alpha1.EtcdStatus{ - ObservedGeneration: ptr.To[int64](0), + ObservedGeneration: nil, Etcd: &druidv1alpha1.CrossVersionObjectReference{ Kind: "StatefulSet", Name: testutils.TestEtcdName, @@ -328,7 +384,7 @@ func createEtcd() *druidv1alpha1.Etcd { return etcd } -func updateEtcd(originalEtcd *druidv1alpha1.Etcd, specChanged, statusChanged bool, lastOpState *druidv1alpha1.LastOperationState, reconcileAnnotPresent bool) *druidv1alpha1.Etcd { +func updateEtcd(originalEtcd *druidv1alpha1.Etcd, specChanged, statusChanged bool, previouslyReconciled bool, lastOpState *druidv1alpha1.LastOperationState, reconcileAnnotPresent bool) *druidv1alpha1.Etcd { newEtcd := originalEtcd.DeepCopy() annotations := make(map[string]string) if reconcileAnnotPresent { @@ -345,6 +401,9 @@ func updateEtcd(originalEtcd *druidv1alpha1.Etcd, specChanged, statusChanged boo newEtcd.Status.ReadyReplicas = 2 newEtcd.Status.Ready = ptr.To(false) } + if previouslyReconciled && originalEtcd.Status.ObservedGeneration == nil { + newEtcd.Status.ObservedGeneration = ptr.To[int64](1) + } if lastOpState != nil { newEtcd.Status.LastOperation = &druidv1alpha1.LastOperation{ Type: druidv1alpha1.LastOperationTypeReconcile,