-
Notifications
You must be signed in to change notification settings - Fork 65
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.
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
Try to reduce the log verbosity of the SPC controller. #1131
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
package mapper | ||
|
||
import ( | ||
"context" | ||
|
||
"github.com/google/go-cmp/cmp" | ||
"k8s.io/client-go/util/workqueue" | ||
"sigs.k8s.io/controller-runtime/pkg/event" | ||
"sigs.k8s.io/controller-runtime/pkg/handler" | ||
) | ||
|
||
// FilterIgnoredFields produces an event handler that wraps the provided even handler. | ||
// Before forwarding the event handling to it, it first checks if there were any updates | ||
// outside of the provided ignore fields. If the changes made to the object are only to | ||
// one or more of the ignored fields, the event handling is not forwarded and the event | ||
// effectively discarded. | ||
// | ||
// This means that only the update events are potentially discarded. All other event types | ||
// are always forwarded. | ||
func FilterIgnoredFields(eventHandler handler.EventHandler, ignoredFieldPaths []string) handler.EventHandler { | ||
cmpOpts := []cmp.Option{cmp.FilterPath(func(p cmp.Path) bool { | ||
str := p.String() | ||
for _, f := range ignoredFieldPaths { | ||
if str == f { | ||
return true | ||
} | ||
} | ||
return false | ||
}, cmp.Ignore())} | ||
|
||
return &ifChangedIgnoring{eventHandler: eventHandler, cmpOpts: cmpOpts} | ||
} | ||
|
||
type ifChangedIgnoring struct { | ||
cmpOpts []cmp.Option | ||
eventHandler handler.EventHandler | ||
} | ||
|
||
// Create implements handler.EventHandler. | ||
func (i *ifChangedIgnoring) Create(ctx context.Context, evt event.CreateEvent, q workqueue.RateLimitingInterface) { | ||
i.eventHandler.Create(ctx, evt, q) | ||
} | ||
|
||
// Delete implements handler.EventHandler. | ||
func (i *ifChangedIgnoring) Delete(ctx context.Context, evt event.DeleteEvent, q workqueue.RateLimitingInterface) { | ||
i.eventHandler.Delete(ctx, evt, q) | ||
} | ||
|
||
// Generic implements handler.EventHandler. | ||
func (i *ifChangedIgnoring) Generic(ctx context.Context, evt event.GenericEvent, q workqueue.RateLimitingInterface) { | ||
i.eventHandler.Generic(ctx, evt, q) | ||
} | ||
|
||
// Update implements handler.EventHandler. | ||
func (i *ifChangedIgnoring) Update(ctx context.Context, evt event.UpdateEvent, q workqueue.RateLimitingInterface) { | ||
if !cmp.Equal(evt.ObjectOld, evt.ObjectNew, i.cmpOpts...) { | ||
i.eventHandler.Update(ctx, evt, q) | ||
} | ||
} | ||
|
||
var _ handler.EventHandler = (*ifChangedIgnoring)(nil) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,98 @@ | ||
package mapper | ||
|
||
import ( | ||
"context" | ||
"testing" | ||
|
||
"github.com/stretchr/testify/assert" | ||
"github.com/stretchr/testify/require" | ||
corev1 "k8s.io/api/core/v1" | ||
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
"k8s.io/apimachinery/pkg/types" | ||
"k8s.io/client-go/util/workqueue" | ||
"sigs.k8s.io/controller-runtime/pkg/client" | ||
"sigs.k8s.io/controller-runtime/pkg/event" | ||
"sigs.k8s.io/controller-runtime/pkg/handler" | ||
"sigs.k8s.io/controller-runtime/pkg/reconcile" | ||
) | ||
|
||
func TestFilterIgnoredFieldsMapper(t *testing.T) { | ||
orig := &corev1.Secret{ | ||
ObjectMeta: metav1.ObjectMeta{ | ||
Name: "sec", | ||
Namespace: "ns", | ||
}, | ||
Data: map[string][]byte{"k": []byte("v")}, | ||
} | ||
|
||
t.Run("ignores fields", func(t *testing.T) { | ||
// given | ||
updated := orig.DeepCopy() | ||
updated.Finalizers = []string{"some-finalizer"} | ||
|
||
queue := workqueue.NewRateLimitingQueue(workqueue.DefaultItemBasedRateLimiter()) | ||
|
||
// when | ||
handler := FilterIgnoredFields(acceptingHandler, []string{"ObjectMeta.Finalizers"}) | ||
handler.Update(context.TODO(), event.UpdateEvent{ObjectNew: updated, ObjectOld: orig}, queue) | ||
|
||
// then | ||
require.Zero(t, queue.Len()) | ||
}) | ||
|
||
t.Run("doesn't ignore changes outside of ignored fields", func(t *testing.T) { | ||
// given | ||
updated := orig.DeepCopy() | ||
updated.Data = map[string][]byte{"k": []byte("vv")} | ||
|
||
queue := workqueue.NewRateLimitingQueue(workqueue.DefaultItemBasedRateLimiter()) | ||
|
||
// when | ||
handler := FilterIgnoredFields(acceptingHandler, []string{"ObjectMeta.Name"}) | ||
handler.Update(context.TODO(), event.UpdateEvent{ObjectNew: updated, ObjectOld: orig}, queue) | ||
|
||
// then | ||
require.Positive(t, queue.Len()) | ||
item, _ := queue.Get() | ||
require.NotNil(t, item) | ||
req, ok := item.(reconcile.Request) | ||
require.True(t, ok) | ||
assert.Equal(t, "sec", req.Name) | ||
assert.Equal(t, "ns", req.Namespace) | ||
}) | ||
|
||
t.Run("doesn't ignore changes outside of ignored fields while ignored fields also changed", func(t *testing.T) { | ||
// given | ||
updated := orig.DeepCopy() | ||
updated.Data = map[string][]byte{"k": []byte("vv")} | ||
updated.Finalizers = []string{"some-finalizer"} | ||
|
||
queue := workqueue.NewRateLimitingQueue(workqueue.DefaultItemBasedRateLimiter()) | ||
|
||
// when | ||
handler := FilterIgnoredFields(acceptingHandler, []string{"ObjectMeta.Finalizers"}) | ||
handler.Update(context.TODO(), event.UpdateEvent{ObjectNew: updated, ObjectOld: orig}, queue) | ||
|
||
// then | ||
require.Positive(t, queue.Len()) | ||
item, _ := queue.Get() | ||
require.NotNil(t, item) | ||
req, ok := item.(reconcile.Request) | ||
require.True(t, ok) | ||
assert.Equal(t, "sec", req.Name) | ||
assert.Equal(t, "ns", req.Namespace) | ||
}) | ||
} | ||
|
||
var acceptingHandler handler.EventHandler = handler.EnqueueRequestsFromMapFunc(accept) | ||
|
||
func accept(ctx context.Context, o client.Object) []reconcile.Request { | ||
return []reconcile.Request{ | ||
{ | ||
NamespacedName: types.NamespacedName{ | ||
Name: o.GetName(), | ||
Namespace: o.GetNamespace(), | ||
}, | ||
}, | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
package mapper | ||
|
||
import ( | ||
"sigs.k8s.io/controller-runtime/pkg/handler" | ||
) | ||
|
||
// IgnoringTimeChangesInToolchainConditions wraps the provided event handler such that it only reacts | ||
// to updates discarding the changes in the condition timestamps. Only updates are affected, all other | ||
// event types are always forwarded. | ||
func IgnoringTimeChangesInToolchainConditions(eventHandler handler.EventHandler) handler.EventHandler { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. minor - should we keep the name more generic ? Just |
||
return FilterIgnoredFields(eventHandler, []string{"Status.Conditions.LastTransitionTime", "Status.Conditions.LastUpdatedTime"}) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,94 @@ | ||
package mapper | ||
|
||
import ( | ||
"context" | ||
"testing" | ||
"time" | ||
|
||
toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" | ||
"github.com/stretchr/testify/assert" | ||
"github.com/stretchr/testify/require" | ||
corev1 "k8s.io/api/core/v1" | ||
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
"k8s.io/client-go/util/workqueue" | ||
"sigs.k8s.io/controller-runtime/pkg/event" | ||
"sigs.k8s.io/controller-runtime/pkg/reconcile" | ||
) | ||
|
||
func TestIfConditionsChangedMapper(t *testing.T) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we also have a scenario for multiple conditions, for example one where there is a change in the ignored fields and but also in one of the not ignored fields ( like the |
||
orig := &toolchainv1alpha1.ToolchainCluster{ | ||
ObjectMeta: metav1.ObjectMeta{ | ||
Name: "tc", | ||
Namespace: "ns", | ||
}, | ||
Spec: toolchainv1alpha1.ToolchainClusterSpec{ | ||
SecretRef: toolchainv1alpha1.LocalSecretReference{ | ||
Name: "secret", | ||
}, | ||
}, | ||
Status: toolchainv1alpha1.ToolchainClusterStatus{ | ||
APIEndpoint: "https://api", | ||
OperatorNamespace: "hostopns", | ||
Conditions: []toolchainv1alpha1.Condition{ | ||
{ | ||
Type: toolchainv1alpha1.ConditionReady, | ||
Status: corev1.ConditionTrue, | ||
Reason: "truth", | ||
Message: "is objective", | ||
LastTransitionTime: metav1.NewTime(time.Date(2025, time.January, 1, 0, 0, 0, 0, time.UTC)), | ||
}, | ||
}, | ||
}, | ||
} | ||
|
||
t.Run("ignores last transition time", func(t *testing.T) { | ||
// given | ||
updated := orig.DeepCopy() | ||
updated.Status.Conditions[0].LastTransitionTime.Time = updated.Status.Conditions[0].LastTransitionTime.Add(5 * time.Second) | ||
|
||
queue := workqueue.NewRateLimitingQueue(workqueue.DefaultItemBasedRateLimiter()) | ||
|
||
// when | ||
handler := IgnoringTimeChangesInToolchainConditions(acceptingHandler) | ||
handler.Update(context.TODO(), event.UpdateEvent{ObjectNew: updated, ObjectOld: orig}, queue) | ||
|
||
// then | ||
require.Zero(t, queue.Len()) | ||
}) | ||
|
||
t.Run("ignores last update time", func(t *testing.T) { | ||
// given | ||
updated := orig.DeepCopy() | ||
updated.Status.Conditions[0].LastUpdatedTime = &updated.Status.Conditions[0].LastTransitionTime | ||
|
||
queue := workqueue.NewRateLimitingQueue(workqueue.DefaultItemBasedRateLimiter()) | ||
|
||
// when | ||
handler := IgnoringTimeChangesInToolchainConditions(acceptingHandler) | ||
handler.Update(context.TODO(), event.UpdateEvent{ObjectNew: updated, ObjectOld: orig}, queue) | ||
|
||
// then | ||
require.Zero(t, queue.Len()) | ||
}) | ||
|
||
t.Run("doesn't ignore change in spec", func(t *testing.T) { | ||
// given | ||
updated := orig.DeepCopy() | ||
updated.Spec.SecretRef.Name = "other" | ||
|
||
queue := workqueue.NewRateLimitingQueue(workqueue.DefaultItemBasedRateLimiter()) | ||
|
||
// when | ||
handler := IgnoringTimeChangesInToolchainConditions(acceptingHandler) | ||
handler.Update(context.TODO(), event.UpdateEvent{ObjectNew: updated, ObjectOld: orig}, queue) | ||
|
||
// then | ||
require.Positive(t, queue.Len()) | ||
item, _ := queue.Get() | ||
require.NotNil(t, item) | ||
req, ok := item.(reconcile.Request) | ||
require.True(t, ok) | ||
assert.Equal(t, "tc", req.Name) | ||
assert.Equal(t, "ns", req.Namespace) | ||
}) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.