Skip to content

Commit

Permalink
Fixed predicates to allow events for Etcd resource that have never be…
Browse files Browse the repository at this point in the history 
…en (#900) (#904)

reconciled before (#898)

Co-authored-by: Madhav Bhargava <[email protected]>
  • Loading branch information
@anveshreddy18 @unmarshall
anveshreddy18 and unmarshall authored Oct 29, 2024
1 parent b31c378 commit 43227ab
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 43227ab

Please sign in to comment.