Skip to content

Commit

Permalink
Fixed predicates to allow events for Etcd resource that have never been
Browse files Browse the repository at this point in the history 
reconciled before (gardener#898)
  • Loading branch information
@unmarshall
unmarshall committed Oct 29, 2024
1 parent df3ff21 commit aa77936
Show file tree
Hide file tree
Showing 2 changed files with 89 additions and 12 deletions.
22 changes: 20 additions & 2 deletions internal/controller/etcd/register.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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.
Expand Down
79 changes: 69 additions & 10 deletions internal/controller/etcd/register_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -63,6 +66,7 @@ func TestBuildPredicateWithOnlyAutoReconcileEnabled(t *testing.T) {
{
name: "only status has changed",
etcdStatusChanged: true,
previouslyReconciled: true,
shouldAllowCreateEvent: true,
shouldAllowDeleteEvent: true,
shouldAllowGenericEvent: false,
Expand All @@ -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,
Expand All @@ -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()
Expand All @@ -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))
Expand All @@ -107,6 +122,7 @@ func TestBuildPredicateWithNoAutoReconcileAndNoReconcileAnnot(t *testing.T) {
{
name: "only spec has changed",
etcdSpecChanged: true,
previouslyReconciled: true,
shouldAllowCreateEvent: true,
shouldAllowDeleteEvent: true,
shouldAllowGenericEvent: false,
Expand All @@ -115,6 +131,7 @@ func TestBuildPredicateWithNoAutoReconcileAndNoReconcileAnnot(t *testing.T) {
{
name: "only status has changed",
etcdStatusChanged: true,
previouslyReconciled: true,
shouldAllowCreateEvent: true,
shouldAllowDeleteEvent: true,
shouldAllowGenericEvent: false,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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))
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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))
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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))
Expand All @@ -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,
Expand All @@ -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 {
Expand All @@ -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,
Expand Down

0 comments on commit aa77936

Please sign in to comment.