From 1632cc1b4dab96a0f19cd5d8e3279fd4b5e41837 Mon Sep 17 00:00:00 2001 From: Grant Rodgers Date: Mon, 24 Sep 2018 14:02:20 -0700 Subject: [PATCH 1/6] Correct some comments and variable names --- pkg/apis/eventing/v1alpha1/subscription_types.go | 4 +++- .../eventing/v1alpha1/subscription_types_test.go | 12 ++++++------ 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/pkg/apis/eventing/v1alpha1/subscription_types.go b/pkg/apis/eventing/v1alpha1/subscription_types.go index 9c8dd8ca68e..e8539c325ce 100644 --- a/pkg/apis/eventing/v1alpha1/subscription_types.go +++ b/pkg/apis/eventing/v1alpha1/subscription_types.go @@ -157,6 +157,8 @@ type Callable struct { TargetURI *string `json:"targetURI,omitempty"` } +// ResultStrategy specifies the handling of the Callable's returned result. If +// no Callable is specified, the identity function is assumed. type ResultStrategy struct { // This object must fulfill the Sinkable contract. // @@ -188,7 +190,7 @@ const ( // SubscriptionConditionReady has status True when all subconditions below have been set to True. SubscriptionConditionReady = duckv1alpha1.ConditionReady - // SubscriptionReferencesResolved has status True when all the specified references have been successfully + // SubscriptionConditionReferencesResolved has status True when all the specified references have been successfully // resolved. SubscriptionConditionReferencesResolved duckv1alpha1.ConditionType = "Resolved" diff --git a/pkg/apis/eventing/v1alpha1/subscription_types_test.go b/pkg/apis/eventing/v1alpha1/subscription_types_test.go index f41df7c91f0..3478d6df041 100644 --- a/pkg/apis/eventing/v1alpha1/subscription_types_test.go +++ b/pkg/apis/eventing/v1alpha1/subscription_types_test.go @@ -42,12 +42,12 @@ var subscriptionConditionFromReady = duckv1alpha1.Condition{ func TestSubscriptionGetCondition(t *testing.T) { tests := []struct { name string - cs *SubscriptionStatus + ss *SubscriptionStatus condQuery duckv1alpha1.ConditionType want *duckv1alpha1.Condition }{{ name: "single condition", - cs: &SubscriptionStatus{ + ss: &SubscriptionStatus{ Conditions: []duckv1alpha1.Condition{ subscriptionConditionReady, }, @@ -56,7 +56,7 @@ func TestSubscriptionGetCondition(t *testing.T) { want: &subscriptionConditionReady, }, { name: "multiple conditions", - cs: &SubscriptionStatus{ + ss: &SubscriptionStatus{ Conditions: []duckv1alpha1.Condition{ subscriptionConditionReady, subscriptionConditionReferencesResolved, @@ -66,7 +66,7 @@ func TestSubscriptionGetCondition(t *testing.T) { want: &subscriptionConditionReferencesResolved, }, { name: "multiple conditions, condition true", - cs: &SubscriptionStatus{ + ss: &SubscriptionStatus{ Conditions: []duckv1alpha1.Condition{ subscriptionConditionReady, subscriptionConditionFromReady, @@ -76,7 +76,7 @@ func TestSubscriptionGetCondition(t *testing.T) { want: &subscriptionConditionFromReady, }, { name: "unknown condition", - cs: &SubscriptionStatus{ + ss: &SubscriptionStatus{ Conditions: []duckv1alpha1.Condition{ subscriptionConditionReady, subscriptionConditionReferencesResolved, @@ -88,7 +88,7 @@ func TestSubscriptionGetCondition(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - got := test.cs.GetCondition(test.condQuery) + got := test.ss.GetCondition(test.condQuery) if diff := cmp.Diff(test.want, got); diff != "" { t.Errorf("unexpected condition (-want, +got) = %v", diff) } From cacf9543fef839258a4958134e7325f8f1cfe54a Mon Sep 17 00:00:00 2001 From: Grant Rodgers Date: Mon, 24 Sep 2018 14:09:56 -0700 Subject: [PATCH 2/6] Add IsReady and Mark* funcs to SubscriptionStatus These make the controller slightly easier to read. Assuming that the Ready condition flip to true is intended to be a FromReady flip. --- .../eventing/v1alpha1/subscription_types.go | 24 +++- .../v1alpha1/subscription_types_test.go | 118 ++++++++++++++++++ .../eventing/subscription/reconcile.go | 15 +-- 3 files changed, 144 insertions(+), 13 deletions(-) diff --git a/pkg/apis/eventing/v1alpha1/subscription_types.go b/pkg/apis/eventing/v1alpha1/subscription_types.go index e8539c325ce..8a9d98247d4 100644 --- a/pkg/apis/eventing/v1alpha1/subscription_types.go +++ b/pkg/apis/eventing/v1alpha1/subscription_types.go @@ -172,7 +172,9 @@ type ResultStrategy struct { Target *corev1.ObjectReference `json:"target,omitempty"` } -var subCondSet = duckv1alpha1.NewLivingConditionSet() +// subCondSet is a condition set with Ready as the happy condition and +// ReferencesResolved and FromReady as the dependent conditions. +var subCondSet = duckv1alpha1.NewLivingConditionSet(SubscriptionConditionReferencesResolved, SubscriptionConditionFromReady) // SubscriptionStatus (computed) for a subscription type SubscriptionStatus struct { @@ -215,6 +217,26 @@ func (ss *SubscriptionStatus) GetConditions() duckv1alpha1.Conditions { return ss.Conditions } +// IsReady returns true if the resource is ready overall. +func (ss *SubscriptionStatus) IsReady() bool { + return subCondSet.Manage(ss).IsHappy() +} + +// InitializeConditions sets relevant unset conditions to Unknown state. +func (ss *SubscriptionStatus) InitializeConditions() { + subCondSet.Manage(ss).InitializeConditions() +} + +// MarkReferencesResolved sets the ReferencesResolved condition to True state. +func (ss *SubscriptionStatus) MarkReferencesResolved() { + subCondSet.Manage(ss).MarkTrue(SubscriptionConditionReferencesResolved) +} + +// MarkFromReady sets the FromReady condition to True state. +func (ss *SubscriptionStatus) MarkFromReady() { + subCondSet.Manage(ss).MarkTrue(SubscriptionConditionFromReady) +} + // SetConditions sets the Conditions array. This enables generic handling of // conditions by implementing the duckv1alpha1.Conditions interface. func (ss *SubscriptionStatus) SetConditions(conditions duckv1alpha1.Conditions) { diff --git a/pkg/apis/eventing/v1alpha1/subscription_types_test.go b/pkg/apis/eventing/v1alpha1/subscription_types_test.go index 3478d6df041..68feda15cab 100644 --- a/pkg/apis/eventing/v1alpha1/subscription_types_test.go +++ b/pkg/apis/eventing/v1alpha1/subscription_types_test.go @@ -20,6 +20,7 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" duckv1alpha1 "github.com/knative/pkg/apis/duck/v1alpha1" corev1 "k8s.io/api/core/v1" ) @@ -96,6 +97,123 @@ func TestSubscriptionGetCondition(t *testing.T) { } } +func TestSubscriptionInitializeConditions(t *testing.T) { + tests := []struct { + name string + ss *SubscriptionStatus + want *SubscriptionStatus + }{{ + name: "empty", + ss: &SubscriptionStatus{}, + want: &SubscriptionStatus{ + Conditions: []duckv1alpha1.Condition{{ + Type: SubscriptionConditionFromReady, + Status: corev1.ConditionUnknown, + }, { + Type: SubscriptionConditionReady, + Status: corev1.ConditionUnknown, + }, { + Type: SubscriptionConditionReferencesResolved, + Status: corev1.ConditionUnknown, + }}, + }, + }, { + name: "one false", + ss: &SubscriptionStatus{ + Conditions: []duckv1alpha1.Condition{{ + Type: SubscriptionConditionFromReady, + Status: corev1.ConditionFalse, + }}, + }, + want: &SubscriptionStatus{ + Conditions: []duckv1alpha1.Condition{{ + Type: SubscriptionConditionFromReady, + Status: corev1.ConditionFalse, + }, { + Type: SubscriptionConditionReady, + Status: corev1.ConditionUnknown, + }, { + Type: SubscriptionConditionReferencesResolved, + Status: corev1.ConditionUnknown, + }}, + }, + }, { + name: "one true", + ss: &SubscriptionStatus{ + Conditions: []duckv1alpha1.Condition{{ + Type: SubscriptionConditionReferencesResolved, + Status: corev1.ConditionTrue, + }}, + }, + want: &SubscriptionStatus{ + Conditions: []duckv1alpha1.Condition{{ + Type: SubscriptionConditionFromReady, + Status: corev1.ConditionUnknown, + }, { + Type: SubscriptionConditionReady, + Status: corev1.ConditionUnknown, + }, { + Type: SubscriptionConditionReferencesResolved, + Status: corev1.ConditionTrue, + }}, + }, + }} + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + test.ss.InitializeConditions() + ignore := cmpopts.IgnoreFields(duckv1alpha1.Condition{}, "LastTransitionTime") + if diff := cmp.Diff(test.want, test.ss, ignore); diff != "" { + t.Errorf("unexpected conditions (-want, +got) = %v", diff) + } + }) + } +} + +func TestSubscriptionIsReady(t *testing.T) { + tests := []struct { + name string + markResolved bool + markFromReady bool + wantReady bool + }{{ + name: "all happy", + markResolved: true, + markFromReady: true, + wantReady: true, + }, { + name: "one sad", + markResolved: false, + markFromReady: true, + wantReady: false, + }, { + name: "other sad", + markResolved: true, + markFromReady: false, + wantReady: false, + }, { + name: "both sad", + markResolved: false, + markFromReady: false, + wantReady: false, + }} + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + ss := &SubscriptionStatus{} + if test.markResolved { + ss.MarkReferencesResolved() + } + if test.markFromReady { + ss.MarkFromReady() + } + got := ss.IsReady() + if test.wantReady != got { + t.Errorf("unexpected readiness: want %v, got %v", test.wantReady, got) + } + }) + } +} + func TestSubscriptionSetConditions(t *testing.T) { c := &Subscription{ Status: SubscriptionStatus{}, diff --git a/pkg/controller/eventing/subscription/reconcile.go b/pkg/controller/eventing/subscription/reconcile.go index 93f61bed3c2..c1479fa92d0 100644 --- a/pkg/controller/eventing/subscription/reconcile.go +++ b/pkg/controller/eventing/subscription/reconcile.go @@ -78,9 +78,7 @@ func (r *reconciler) Reconcile(request reconcile.Request) (reconcile.Result, err } func (r *reconciler) reconcile(subscription *v1alpha1.Subscription) error { - // TODO: Should this just also set up a defer call for subscription.SetConditions. - // No time right now as I'm turning into a pumpking but seems reasonable. - conditions := []duckv1alpha1.Condition{} + subscription.Status.InitializeConditions() // See if the subscription has been deleted accessor, err := meta.Accessor(subscription) @@ -130,10 +128,7 @@ func (r *reconciler) reconcile(subscription *v1alpha1.Subscription) error { } // Everything that was supposed to be resolved was, so flip the status bit on that. - conditions = append(conditions, duckv1alpha1.Condition{ - Type: v1alpha1.SubscriptionConditionReferencesResolved, - Status: corev1.ConditionTrue, - }) + subscription.Status.MarkReferencesResolved() // Ok, now that we have the From and at least one of the Call/Result, let's reconcile // the From with this information. @@ -143,11 +138,7 @@ func (r *reconciler) reconcile(subscription *v1alpha1.Subscription) error { return err } // Everything went well, set the fact that subscriptions have been modified - conditions = append(conditions, duckv1alpha1.Condition{ - Type: duckv1alpha1.ConditionReady, - Status: corev1.ConditionTrue, - }) - subscription.Status.SetConditions(conditions) + subscription.Status.MarkFromReady() return nil } From 76539b3a6bd1b5f58fd0143c4d065f91953a2f5d Mon Sep 17 00:00:00 2001 From: Grant Rodgers Date: Mon, 24 Sep 2018 14:31:08 -0700 Subject: [PATCH 3/6] Remove GetSpecJSON Replaced with Generation duck type. --- .../eventing/v1alpha1/subscription_types.go | 11 +++---- .../v1alpha1/subscription_types_test.go | 29 ------------------- 2 files changed, 4 insertions(+), 36 deletions(-) diff --git a/pkg/apis/eventing/v1alpha1/subscription_types.go b/pkg/apis/eventing/v1alpha1/subscription_types.go index 8a9d98247d4..2cd93d5cecf 100644 --- a/pkg/apis/eventing/v1alpha1/subscription_types.go +++ b/pkg/apis/eventing/v1alpha1/subscription_types.go @@ -17,8 +17,6 @@ package v1alpha1 import ( - "encoding/json" - "github.com/knative/pkg/apis" "github.com/knative/pkg/apis/duck" duckv1alpha1 "github.com/knative/pkg/apis/duck/v1alpha1" @@ -55,6 +53,10 @@ var _ duckv1alpha1.ConditionsAccessor = (*SubscriptionStatus)(nil) // Check that Subscription implements the Conditions duck type. var _ = duck.VerifyType(&Subscription{}, &duckv1alpha1.Conditions{}) +// Check that the Subscription implements the Generation duck type. +var emptyGen duckv1alpha1.Generation +var _ = duck.VerifyType(&Subscription{}, &emptyGen) + // And it's Subscribable var _ = duck.VerifyType(&Subscription{}, &duckv1alpha1.Subscribable{}) @@ -201,11 +203,6 @@ const ( SubscriptionConditionFromReady duckv1alpha1.ConditionType = "FromReady" ) -// GetSpecJSON returns spec as json -func (s *Subscription) GetSpecJSON() ([]byte, error) { - return json.Marshal(s.Spec) -} - // GetCondition returns the condition currently associated with the given type, or nil. func (ss *SubscriptionStatus) GetCondition(t duckv1alpha1.ConditionType) *duckv1alpha1.Condition { return subCondSet.Manage(ss).GetCondition(t) diff --git a/pkg/apis/eventing/v1alpha1/subscription_types_test.go b/pkg/apis/eventing/v1alpha1/subscription_types_test.go index 68feda15cab..e9c552a452d 100644 --- a/pkg/apis/eventing/v1alpha1/subscription_types_test.go +++ b/pkg/apis/eventing/v1alpha1/subscription_types_test.go @@ -225,32 +225,3 @@ func TestSubscriptionSetConditions(t *testing.T) { t.Errorf("unexpected conditions (-want, +got) = %v", diff) } } - -func TestSubscriptionGetSpecJSON(t *testing.T) { - targetURI := "http://example.com" - c := &Subscription{ - Spec: SubscriptionSpec{ - From: corev1.ObjectReference{ - Name: "foo", - }, - Call: &Callable{ - TargetURI: &targetURI, - }, - Result: &ResultStrategy{ - Target: &corev1.ObjectReference{ - Name: "result", - }, - }, - }, - } - - want := `{"from":{"name":"foo"},"call":{"targetURI":"http://example.com"},"result":{"target":{"name":"result"}}}` - got, err := c.GetSpecJSON() - if err != nil { - t.Fatalf("unexpected spec JSON error: %v", err) - } - - if diff := cmp.Diff(want, string(got)); diff != "" { - t.Errorf("unexpected spec JSON (-want, +got) = %v", diff) - } -} From d1742df95fcc8289db4cf30d3ffc747ca2744911 Mon Sep 17 00:00:00 2001 From: Grant Rodgers Date: Mon, 24 Sep 2018 15:33:02 -0700 Subject: [PATCH 4/6] Add IgnoreTimes option to controller table tests Time fields in conditions often need to be ignored because they rarely compare as equal. Setting IgnoreTimes true in the test case will cause diffs to ignore apis.VolatileTime values. --- pkg/controller/testing/table.go | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/pkg/controller/testing/table.go b/pkg/controller/testing/table.go index 5a85c1a7e39..96b369560ca 100644 --- a/pkg/controller/testing/table.go +++ b/pkg/controller/testing/table.go @@ -24,6 +24,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" + "github.com/knative/pkg/apis" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -77,6 +78,9 @@ type TestCase struct { // Fake dynamic objects Objects []runtime.Object + + // IgnoreTimes causes comparisons to ignore fields of type apis.VolatileTime. + IgnoreTimes bool } // Runner returns a testing func that can be passed to t.Run. @@ -203,9 +207,18 @@ func (tc *TestCase) VerifyWantPresent(c client.Client) error { } } - // Ignore TypeMeta, since the objects created by the controller won't have - // it - if diff := cmp.Diff(wp, o, cmpopts.IgnoreTypes(metav1.TypeMeta{})); diff != "" { + diffOpts := cmp.Options{ + // Ignore TypeMeta, since the objects created by the controller won't have + // it + cmpopts.IgnoreTypes(metav1.TypeMeta{}), + } + + if tc.IgnoreTimes { + // Ignore VolatileTime fields, since they rarely compare correctly. + diffOpts = append(diffOpts, cmpopts.IgnoreTypes(apis.VolatileTime{})) + } + + if diff := cmp.Diff(wp, o, diffOpts...); diff != "" { errs.errors = append(errs.errors, fmt.Errorf("Unexpected present %T %s/%s (-want +got):\n%v", wp, acc.GetNamespace(), acc.GetName(), diff)) } } From b79726d8c3d969a825d71da57c93655e932c0f5d Mon Sep 17 00:00:00 2001 From: Grant Rodgers Date: Mon, 24 Sep 2018 15:35:28 -0700 Subject: [PATCH 5/6] Update test to expect initialized condition All conditions are now defaulted to Unknown on first reconcile. --- .../eventing/subscription/reconcile_test.go | 26 +++++++++++-------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/pkg/controller/eventing/subscription/reconcile_test.go b/pkg/controller/eventing/subscription/reconcile_test.go index b883bf03d52..eb748e4f382 100644 --- a/pkg/controller/eventing/subscription/reconcile_test.go +++ b/pkg/controller/eventing/subscription/reconcile_test.go @@ -234,9 +234,10 @@ var testCases = []controllertesting.TestCase{ // TODO: Again this works on gke cluster, but I need to set // something else up here. later... // getNewSubscriptionWithReferencesResolvedStatus(), - getNewSubscription(), + getNewSubscriptionWithUnknownConditions(), }, - Scheme: scheme.Scheme, + IgnoreTimes: true, + Scheme: scheme.Scheme, Objects: []runtime.Object{ // Source channel &unstructured.Unstructured{ @@ -412,15 +413,6 @@ func getNewChannel(name string) *eventingv1alpha1.Channel { return channel } -func getNewSubscriptionWithReferencesResolvedStatus() *eventingv1alpha1.Subscription { - s := getNewSubscription() - s.Status.SetConditions([]duckv1alpha1.Condition{{ - Type: eventingv1alpha1.SubscriptionConditionReferencesResolved, - Status: corev1.ConditionTrue, - }}) - return s -} - func getNewSubscription() *eventingv1alpha1.Subscription { subscription := &eventingv1alpha1.Subscription{ TypeMeta: subscriptionType(), @@ -454,6 +446,18 @@ func getNewSubscription() *eventingv1alpha1.Subscription { return subscription } +func getNewSubscriptionWithUnknownConditions() *eventingv1alpha1.Subscription { + s := getNewSubscription() + s.Status.InitializeConditions() + return s +} + +func getNewSubscriptionWithReferencesResolvedStatus() *eventingv1alpha1.Subscription { + s := getNewSubscriptionWithUnknownConditions() + s.Status.MarkReferencesResolved() + return s +} + func channelType() metav1.TypeMeta { return metav1.TypeMeta{ APIVersion: eventingv1alpha1.SchemeGroupVersion.String(), From 31c4fc2dcca3f29e2017213d3a8375ed86c4019c Mon Sep 17 00:00:00 2001 From: Grant Rodgers Date: Mon, 24 Sep 2018 15:36:27 -0700 Subject: [PATCH 6/6] Correct vet issue in table test helper --- pkg/controller/testing/table.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/controller/testing/table.go b/pkg/controller/testing/table.go index 96b369560ca..af7195752d5 100644 --- a/pkg/controller/testing/table.go +++ b/pkg/controller/testing/table.go @@ -113,9 +113,8 @@ func (tc *TestCase) Runner(t *testing.T, r reconcile.Reconciler, c *MockClient) func (tc *TestCase) GetDynamicClient() dynamic.Interface { if tc.Scheme == nil { return dynamicfake.NewSimpleDynamicClient(runtime.NewScheme(), tc.Objects...) - } else { - return dynamicfake.NewSimpleDynamicClient(tc.Scheme, tc.Objects...) } + return dynamicfake.NewSimpleDynamicClient(tc.Scheme, tc.Objects...) } // GetClient returns the mockClient to use for this test case.