-
Notifications
You must be signed in to change notification settings - Fork 66
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Try to reduce the log verbosity of the SPC controller. (#1131)
* Try to reduce the log verbosity of the SPC controller. This is done by introducing a predicate for the watch that should ignore any changes to the TCs that are only caused by the changes in the timestamps of the conditions. Co-authored-by: Matous Jobanek <mjobanek@redhat.com>
- Loading branch information
1 parent
f35e54f
commit e2d2b30
Showing
5 changed files
with
259 additions
and
4 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| package predicate | ||
|
|
||
| import ( | ||
| toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" | ||
| "github.com/codeready-toolchain/toolchain-common/pkg/condition" | ||
| "sigs.k8s.io/controller-runtime/pkg/client" | ||
| "sigs.k8s.io/controller-runtime/pkg/event" | ||
| "sigs.k8s.io/controller-runtime/pkg/predicate" | ||
| ) | ||
|
|
||
| var _ predicate.Predicate = (*ToolchainConditionChanged)(nil) | ||
|
|
||
| type ( | ||
| GetToolchainConditions func(client.Object) []toolchainv1alpha1.Condition | ||
|
|
||
| // ToolchainConditionChanged is a predicate to check that some conditions changed in a toolchain object. | ||
| ToolchainConditionChanged struct { | ||
| // GetConditions is function used to obtain the conditions from an object. There is no generic way of | ||
| // doing that using the standard k8s api. | ||
| GetConditions GetToolchainConditions | ||
|
|
||
| // Type is the type of the condition to look at | ||
| Type toolchainv1alpha1.ConditionType | ||
| } | ||
| ) | ||
|
|
||
| // Create implements predicate.Predicate. | ||
| func (t *ToolchainConditionChanged) Create(event.CreateEvent) bool { | ||
| return true | ||
| } | ||
|
|
||
| // Delete implements predicate.Predicate. | ||
| func (t *ToolchainConditionChanged) Delete(event.DeleteEvent) bool { | ||
| return true | ||
| } | ||
|
|
||
| // Generic implements predicate.Predicate. | ||
| func (t *ToolchainConditionChanged) Generic(event.GenericEvent) bool { | ||
| return true | ||
| } | ||
|
|
||
| func (t *ToolchainConditionChanged) Update(evt event.UpdateEvent) bool { | ||
| oldConds := t.GetConditions(evt.ObjectOld) | ||
| newConds := t.GetConditions(evt.ObjectNew) | ||
|
|
||
| oldCond, oldOk := condition.FindConditionByType(oldConds, t.Type) | ||
| newCond, newOk := condition.FindConditionByType(newConds, t.Type) | ||
|
|
||
| if oldOk != newOk { | ||
| // the condition appeared or disappeared | ||
| return true | ||
| } | ||
|
|
||
| if !oldOk { | ||
| // neither the old nor the new version contains a condition of the required type | ||
| return false | ||
| } | ||
|
|
||
| // we're intentionally ignoring changes to *Time fields of the conditions. If nothing else changed, those do not represent a factual | ||
| // change in the condition. | ||
| return oldCond.Status != newCond.Status || oldCond.Reason != newCond.Reason || oldCond.Message != newCond.Message | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,172 @@ | ||
| package predicate | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "sigs.k8s.io/controller-runtime/pkg/client" | ||
| "sigs.k8s.io/controller-runtime/pkg/event" | ||
|
|
||
| toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" | ||
| "github.com/codeready-toolchain/toolchain-common/pkg/test" | ||
| "github.com/stretchr/testify/assert" | ||
| corev1 "k8s.io/api/core/v1" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| ) | ||
|
|
||
| func TestToolchainConditionChangedPredicate(t *testing.T) { | ||
| // given | ||
| blueprint := &toolchainv1alpha1.ToolchainCluster{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "tc", | ||
| Namespace: test.HostOperatorNs, | ||
| }, | ||
| Spec: toolchainv1alpha1.ToolchainClusterSpec{ | ||
| SecretRef: toolchainv1alpha1.LocalSecretReference{ | ||
| Name: "secret", | ||
| }, | ||
| }, | ||
| Status: toolchainv1alpha1.ToolchainClusterStatus{ | ||
| APIEndpoint: "https://endpoint", | ||
| OperatorNamespace: "member-op-ns", | ||
| Conditions: []toolchainv1alpha1.Condition{ | ||
| { | ||
| Type: toolchainv1alpha1.ConditionReady, | ||
| Status: corev1.ConditionTrue, | ||
| Reason: "because", | ||
| Message: "I CAN", | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| predicate := ToolchainConditionChanged{ | ||
| GetConditions: func(o client.Object) []toolchainv1alpha1.Condition { | ||
| return o.(*toolchainv1alpha1.ToolchainCluster).Status.Conditions | ||
| }, | ||
| Type: toolchainv1alpha1.ConditionReady, | ||
| } | ||
|
|
||
| test := func(t *testing.T, expectedResult bool, updateObjects func(old, new *toolchainv1alpha1.ToolchainCluster)) { | ||
| t.Helper() | ||
|
|
||
| // given | ||
| tcOld := blueprint.DeepCopy() | ||
| tcNew := blueprint.DeepCopy() | ||
|
|
||
| updateObjects(tcOld, tcNew) | ||
|
|
||
| ev := event.UpdateEvent{ | ||
| ObjectOld: tcOld, | ||
| ObjectNew: tcNew, | ||
| } | ||
|
|
||
| t.Run("update", func(t *testing.T) { | ||
| // when | ||
| result := predicate.Update(ev) | ||
|
|
||
| // then | ||
| assert.Equal(t, expectedResult, result) | ||
| }) | ||
|
|
||
| t.Run("create", func(t *testing.T) { | ||
| // when | ||
| result := predicate.Create(event.CreateEvent{Object: tcNew}) | ||
|
|
||
| // then | ||
| assert.True(t, result) | ||
| }) | ||
|
|
||
| t.Run("delete", func(t *testing.T) { | ||
| // when | ||
| result := predicate.Delete(event.DeleteEvent{Object: tcNew}) | ||
|
|
||
| // then | ||
| assert.True(t, result) | ||
| }) | ||
|
|
||
| t.Run("generic", func(t *testing.T) { | ||
| // when | ||
| result := predicate.Generic(event.GenericEvent{Object: tcNew}) | ||
|
|
||
| // then | ||
| assert.True(t, result) | ||
| }) | ||
| } | ||
|
|
||
| t.Run("ignores no change", func(t *testing.T) { | ||
| test(t, false, func(old, new *toolchainv1alpha1.ToolchainCluster) {}) | ||
| }) | ||
|
|
||
| t.Run("ignores changes in the object meta", func(t *testing.T) { | ||
| test(t, false, func(old, new *toolchainv1alpha1.ToolchainCluster) { | ||
| new.Annotations = map[string]string{"k": "v"} | ||
| }) | ||
| }) | ||
|
|
||
| t.Run("ignores changes in the spec", func(t *testing.T) { | ||
| test(t, false, func(old, new *toolchainv1alpha1.ToolchainCluster) { | ||
| new.Spec.SecretRef.Name = "other" | ||
| }) | ||
| }) | ||
|
|
||
| t.Run("ignores changes in the status outside of conditions", func(t *testing.T) { | ||
| test(t, false, func(old, new *toolchainv1alpha1.ToolchainCluster) { | ||
| new.Status.OperatorNamespace = "other" | ||
| }) | ||
| }) | ||
|
|
||
| t.Run("ignores change in other condition types", func(t *testing.T) { | ||
| test(t, false, func(old, new *toolchainv1alpha1.ToolchainCluster) { | ||
| new.Status.Conditions = append(new.Status.Conditions, toolchainv1alpha1.Condition{Type: toolchainv1alpha1.ConditionType("other")}) | ||
| }) | ||
| }) | ||
|
|
||
| t.Run("ignores changes in the last transition time", func(t *testing.T) { | ||
| test(t, false, func(old, new *toolchainv1alpha1.ToolchainCluster) { | ||
| new.Status.Conditions[0].LastTransitionTime = metav1.Now() | ||
| }) | ||
| }) | ||
|
|
||
| t.Run("ignores changes in the updated time", func(t *testing.T) { | ||
| test(t, false, func(old, new *toolchainv1alpha1.ToolchainCluster) { | ||
| time := metav1.Now() | ||
| new.Status.Conditions[0].LastUpdatedTime = &time | ||
| }) | ||
| }) | ||
|
|
||
| t.Run("detects when condition appears", func(t *testing.T) { | ||
| test(t, true, func(old, new *toolchainv1alpha1.ToolchainCluster) { | ||
| new.Status.Conditions = old.Status.Conditions | ||
| old.Status.Conditions = nil | ||
| }) | ||
| }) | ||
| t.Run("ignores missing conditions", func(t *testing.T) { | ||
| test(t, false, func(old, new *toolchainv1alpha1.ToolchainCluster) { | ||
| new.Status.Conditions = nil | ||
| old.Status.Conditions = nil | ||
| }) | ||
| }) | ||
| t.Run("detects change when condition disappears", func(t *testing.T) { | ||
| test(t, true, func(old, new *toolchainv1alpha1.ToolchainCluster) { | ||
| new.Status.Conditions = nil | ||
| }) | ||
| }) | ||
|
|
||
| t.Run("detects change in the status", func(t *testing.T) { | ||
| test(t, true, func(old, new *toolchainv1alpha1.ToolchainCluster) { | ||
| new.Status.Conditions[0].Status = corev1.ConditionFalse | ||
| }) | ||
| }) | ||
|
|
||
| t.Run("detects change in the reason", func(t *testing.T) { | ||
| test(t, true, func(old, new *toolchainv1alpha1.ToolchainCluster) { | ||
| new.Status.Conditions[0].Reason = "other" | ||
| }) | ||
| }) | ||
|
|
||
| t.Run("detects change in the message", func(t *testing.T) { | ||
| test(t, true, func(old, new *toolchainv1alpha1.ToolchainCluster) { | ||
| new.Status.Conditions[0].Message = "other" | ||
| }) | ||
| }) | ||
| } |