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

Fixed predicates to allow events for Etcd resource that have never been reconciled before #900

Merged
merged 1 commit into from
Oct 29, 2024
Merged

Fixed predicates to allow events for Etcd resource that have never been reconciled before #900

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
22 changes: 20 additions & 2 deletions internal/controller/etcd/register.go
View file
Open in desktop
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
Open in desktop
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