From 4546b0e020e36ef40c1e0839950805ae071d9999 Mon Sep 17 00:00:00 2001 From: Akash Verenkar Date: Fri, 1 Mar 2019 17:40:14 -0800 Subject: [PATCH 1/7] Creating a default channel controller that will watch every channel and update its status in case the channel is not being watched by any controller This could happen if the end user creates a channel but doesn't install the provisioner. Issue#779: https://github.com/knative/eventing/issues/779 --- cmd/controller/main.go | 2 + pkg/apis/eventing/v1alpha1/channel_types.go | 21 +- .../eventing/v1alpha1/channel_types_test.go | 15 +- pkg/reconciler/v1alpha1/channel/channel.go | 142 ++++++++++ .../v1alpha1/channel/channel_test.go | 262 ++++++++++++++++++ 5 files changed, 438 insertions(+), 4 deletions(-) create mode 100644 pkg/reconciler/v1alpha1/channel/channel.go create mode 100644 pkg/reconciler/v1alpha1/channel/channel_test.go diff --git a/cmd/controller/main.go b/cmd/controller/main.go index 860ceabe399..2da155dbaa1 100644 --- a/cmd/controller/main.go +++ b/cmd/controller/main.go @@ -23,6 +23,7 @@ import ( "net/http" "time" + "github.com/knative/eventing/pkg/reconciler/v1alpha1/channel" "github.com/knative/eventing/pkg/reconciler/v1alpha1/subscription" "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/controller-runtime/pkg/controller" @@ -119,6 +120,7 @@ func main() { // manager run it. providers := []ProvideFunc{ subscription.ProvideController, + channel.ProvideController, } for _, provider := range providers { if _, err := provider(mgr); err != nil { diff --git a/pkg/apis/eventing/v1alpha1/channel_types.go b/pkg/apis/eventing/v1alpha1/channel_types.go index 0771b022ddd..0793b5f222a 100644 --- a/pkg/apis/eventing/v1alpha1/channel_types.go +++ b/pkg/apis/eventing/v1alpha1/channel_types.go @@ -77,7 +77,7 @@ type ChannelSpec struct { Subscribable *eventingduck.Subscribable `json:"subscribable,omitempty"` } -var chanCondSet = duckv1alpha1.NewLivingConditionSet(ChannelConditionProvisioned, ChannelConditionAddressable) +var chanCondSet = duckv1alpha1.NewLivingConditionSet(ChannelConditionProvisioned, ChannelConditionAddressable, ChannelConditionProvisionerInstalled) // ChannelStatus represents the current state of a Channel. type ChannelStatus struct { @@ -119,6 +119,10 @@ const ( // ChannelConditionAddressable has status true when this Channel meets // the Addressable contract and has a non-empty hostname. ChannelConditionAddressable duckv1alpha1.ConditionType = "Addressable" + + // ChannelConditionProvisionerFound has status true when the channel is being watched + // by the provisioner's channel controller (in other words, the provisioner is installed) + ChannelConditionProvisionerInstalled duckv1alpha1.ConditionType = "ProvisionerInstalled" ) // GetCondition returns the condition currently associated with the given type, or nil. @@ -139,11 +143,26 @@ func (cs *ChannelStatus) InitializeConditions() { // MarkProvisioned sets ChannelConditionProvisioned condition to True state. func (cs *ChannelStatus) MarkProvisioned() { chanCondSet.Manage(cs).MarkTrue(ChannelConditionProvisioned) + + // MarkProvisionedNotInstalled will be set by default controller and needs to be unset by individual controllers + // This is done so that each individual channel controller gets it for free. + cs.MarkProvisionerInstalled() } // MarkNotProvisioned sets ChannelConditionProvisioned condition to False state. func (cs *ChannelStatus) MarkNotProvisioned(reason, messageFormat string, messageA ...interface{}) { chanCondSet.Manage(cs).MarkFalse(ChannelConditionProvisioned, reason, messageFormat, messageA...) + // MarkProvisionedNotInstalled will be set by default controller and needs to be unset by individual controllers + // This is done so that each individual channel controller gets it for free. + cs.MarkProvisionerInstalled() +} + +func (cs *ChannelStatus) MarkProvisionerInstalled() { + chanCondSet.Manage(cs).MarkTrue(ChannelConditionProvisionerInstalled) +} + +func (cs *ChannelStatus) MarkProvisionerNotInstalled(reason, messageFormat string, messageA ...interface{}) { + chanCondSet.Manage(cs).MarkFalse(ChannelConditionProvisionerInstalled, reason, messageFormat, messageA...) } // SetAddress makes this Channel addressable by setting the hostname. It also diff --git a/pkg/apis/eventing/v1alpha1/channel_types_test.go b/pkg/apis/eventing/v1alpha1/channel_types_test.go index 2cbabce3696..8ff1619f897 100644 --- a/pkg/apis/eventing/v1alpha1/channel_types_test.go +++ b/pkg/apis/eventing/v1alpha1/channel_types_test.go @@ -101,6 +101,9 @@ func TestChannelInitializeConditions(t *testing.T) { }, { Type: ChannelConditionProvisioned, Status: corev1.ConditionUnknown, + }, { + Type: ChannelConditionProvisionerInstalled, + Status: corev1.ConditionUnknown, }, { Type: ChannelConditionReady, Status: corev1.ConditionUnknown, @@ -121,6 +124,9 @@ func TestChannelInitializeConditions(t *testing.T) { }, { Type: ChannelConditionProvisioned, Status: corev1.ConditionFalse, + }, { + Type: ChannelConditionProvisionerInstalled, + Status: corev1.ConditionUnknown, }, { Type: ChannelConditionReady, Status: corev1.ConditionUnknown, @@ -141,12 +147,15 @@ func TestChannelInitializeConditions(t *testing.T) { }, { Type: ChannelConditionProvisioned, Status: corev1.ConditionTrue, + }, { + Type: ChannelConditionProvisionerInstalled, + Status: corev1.ConditionUnknown, }, { Type: ChannelConditionReady, Status: corev1.ConditionUnknown, - }}}, - }, - } + }}, + }, + }} for _, test := range tests { t.Run(test.name, func(t *testing.T) { diff --git a/pkg/reconciler/v1alpha1/channel/channel.go b/pkg/reconciler/v1alpha1/channel/channel.go new file mode 100644 index 00000000000..8b301ea9941 --- /dev/null +++ b/pkg/reconciler/v1alpha1/channel/channel.go @@ -0,0 +1,142 @@ +/* +Copyright 2018 The Knative Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package channel + +import ( + "context" + + corev1 "k8s.io/api/core/v1" + "sigs.k8s.io/controller-runtime/pkg/event" + "sigs.k8s.io/controller-runtime/pkg/predicate" + + "k8s.io/client-go/rest" + "k8s.io/client-go/tools/record" + "sigs.k8s.io/controller-runtime/pkg/controller" + "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/manager" + "sigs.k8s.io/controller-runtime/pkg/source" + + "github.com/golang/glog" + "github.com/knative/eventing/pkg/apis/eventing/v1alpha1" + "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/client-go/dynamic" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/reconcile" +) + +const ( + // controllerAgentName is the string used by this controller to identify + // itself when creating events. + controllerAgentName = "channel-controller" + finalizerName = controllerAgentName + channelReconciled = "ChannelReconciled" + channelUpdateStatusFailed = "ChannelUpdateStatusFailed" +) + +type reconciler struct { + client client.Client + restConfig *rest.Config + dynamicClient dynamic.Interface + recorder record.EventRecorder +} + +// Verify the struct implements reconcile.Reconciler +var _ reconcile.Reconciler = &reconciler{} + +// ProvideController returns a Channel controller. +// This Channel controller is a default controller for channels of all provisioner kinds +func ProvideController(mgr manager.Manager) (controller.Controller, error) { + // Setup a new controller to Reconcile channel + c, err := controller.New(controllerAgentName, mgr, controller.Options{ + Reconciler: &reconciler{ + recorder: mgr.GetRecorder(controllerAgentName), + }, + }) + if err != nil { + return nil, err + } + + // Watch channel events and enqueue Subscription object key. + // This controller is no-op when Channels are deleted + if err := c.Watch( + &source.Kind{Type: &v1alpha1.Channel{}}, + &handler.EnqueueRequestForObject{}, + predicate.Funcs{ + DeleteFunc: func(event.DeleteEvent) bool { + return false + }, + }); err != nil { + return nil, err + } + + return c, nil +} + +// This detaul channel reconciler will check if the channel is being watched by provisioner's channel controller +// This will improve UX. See https://github.com/knative/eventing/issues/779 +func (r *reconciler) Reconcile(request reconcile.Request) (reconcile.Result, error) { + glog.Infof("Reconciling channel %v", request) + ch := &v1alpha1.Channel{} + + // Controller-runtime client Get() always deep copies the object. Hence no need to again deep copy it + err := r.client.Get(context.TODO(), request.NamespacedName, ch) + + if errors.IsNotFound(err) { + glog.Errorf("could not find channel %v\n", request) + return reconcile.Result{}, nil + } + + if err != nil { + glog.Errorf("could not fetch channel %v for %+v\n", err, request) + return reconcile.Result{}, err + } + + err = r.reconcile(ch) + + if err != nil { + glog.Warningf("Error reconciling channel: %v. Will retry.", err) + r.recorder.Eventf(ch, corev1.EventTypeWarning, channelUpdateStatusFailed, "Failed to update channel status: %v", request.NamespacedName) + return reconcile.Result{Requeue: true}, err + } + glog.Infof("Successfully reconciled channel %v", request.NamespacedName) + r.recorder.Eventf(ch, corev1.EventTypeNormal, channelReconciled, "Channel reconciled: %v", request.NamespacedName) + return reconcile.Result{Requeue: false}, nil +} + +func (r *reconciler) reconcile(ch *v1alpha1.Channel) error { + // TODO: Test what happens on delete + ch.Status.InitializeConditions() + + c := ch.Status.GetCondition(v1alpha1.ChannelConditionProvisionerInstalled) + + if c.IsUnknown() { + ch.Status.MarkProvisionerNotInstalled( + "Provisioner not found.", + "Specified provisioner [Name:%v Kind:%v] is not installed or not controlling the channel.", + ch.Spec.Provisioner.Name, + ch.Spec.Provisioner.Kind, + ) + err := r.client.Status().Update(context.TODO(), ch) + return err + } + return nil +} + +func (r *reconciler) InjectClient(c client.Client) error { + r.client = c + return nil +} diff --git a/pkg/reconciler/v1alpha1/channel/channel_test.go b/pkg/reconciler/v1alpha1/channel/channel_test.go new file mode 100644 index 00000000000..20dc3c67df1 --- /dev/null +++ b/pkg/reconciler/v1alpha1/channel/channel_test.go @@ -0,0 +1,262 @@ +package channel + +import ( + "context" + "errors" + "fmt" + "testing" + + eventingduck "github.com/knative/eventing/pkg/apis/duck/v1alpha1" + eventingv1alpha1 "github.com/knative/eventing/pkg/apis/eventing/v1alpha1" + controllertesting "github.com/knative/eventing/pkg/reconciler/testing" + duckv1alpha1 "github.com/knative/pkg/apis/duck/v1alpha1" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/kubernetes/scheme" + "k8s.io/client-go/rest" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/reconcile" +) + +const ( + testNamespace = "testnamespace" + testAPIVersion = "eventing.knative.dev/v1alpha1" + testCCPName = "TestProvisioner" + testCCPKind = "ClusterChannelProvisioner" +) + +var ( + events = map[string]corev1.Event{ + channelReconciled: {Reason: channelReconciled, Type: corev1.EventTypeNormal}, + channelUpdateStatusFailed: {Reason: channelUpdateStatusFailed, Type: corev1.EventTypeWarning}, + } +) + +func init() { + // Add types to scheme + eventingv1alpha1.AddToScheme(scheme.Scheme) + duckv1alpha1.AddToScheme(scheme.Scheme) +} + +func TestAllCases(t *testing.T) { + + testCases := []controllertesting.TestCase{ + { + Name: "No channels exist", + WantErr: false, + WantResult: reconcile.Result{}, + ReconcileKey: fmt.Sprintf("%v/%v", "chan-1", testNamespace), + }, + { + Name: "Orphaned channel", + WantErr: false, + WantResult: reconcile.Result{}, + ReconcileKey: fmt.Sprintf("%v/%v", testNamespace, "chan-1"), + InitialState: []runtime.Object{ + Channel("chan-1", testNamespace), + Channel("chan-2", testNamespace).WithProvInstalledStatus(corev1.ConditionTrue), + Channel("chan-3", testNamespace).WithProvInstalledStatus(corev1.ConditionFalse), + }, + WantPresent: []runtime.Object{ + Channel("chan-1", testNamespace).WithProvInstalledStatus(corev1.ConditionFalse), + Channel("chan-2", testNamespace).WithProvInstalledStatus(corev1.ConditionTrue), + Channel("chan-3", testNamespace).WithProvInstalledStatus(corev1.ConditionFalse), + }, + WantEvent: []corev1.Event{ + events[channelReconciled], + }, + }, + { + Name: "Non-oprphaned channel test 1", + WantErr: false, + WantResult: reconcile.Result{}, + ReconcileKey: fmt.Sprintf("%v/%v", testNamespace, "chan-2"), + InitialState: []runtime.Object{ + Channel("chan-1", testNamespace), + Channel("chan-2", testNamespace).WithProvInstalledStatus(corev1.ConditionTrue), + Channel("chan-3", testNamespace).WithProvInstalledStatus(corev1.ConditionFalse), + }, + WantPresent: []runtime.Object{ + Channel("chan-1", testNamespace), + Channel("chan-2", testNamespace).WithProvInstalledStatus(corev1.ConditionTrue), + Channel("chan-3", testNamespace).WithProvInstalledStatus(corev1.ConditionFalse), + }, + WantEvent: []corev1.Event{ + events[channelReconciled], + }, + }, + { + Name: "Non-oprphaned channel test 2", + WantErr: false, + WantResult: reconcile.Result{}, + ReconcileKey: fmt.Sprintf("%v/%v", testNamespace, "chan-3"), + InitialState: []runtime.Object{ + Channel("chan-1", testNamespace), + Channel("chan-2", testNamespace).WithProvInstalledStatus(corev1.ConditionTrue), + Channel("chan-3", testNamespace).WithProvInstalledStatus(corev1.ConditionFalse), + }, + WantPresent: []runtime.Object{ + Channel("chan-1", testNamespace), + Channel("chan-2", testNamespace).WithProvInstalledStatus(corev1.ConditionTrue), + Channel("chan-3", testNamespace).WithProvInstalledStatus(corev1.ConditionFalse), + }, + WantEvent: []corev1.Event{ + events[channelReconciled], + }, + }, + { + Name: "Fail orphaned channel status update", + WantErr: true, + WantErrMsg: "Update failed", + WantResult: reconcile.Result{Requeue: true}, + ReconcileKey: fmt.Sprintf("%v/%v", testNamespace, "chan-1"), + InitialState: []runtime.Object{ + Channel("chan-1", testNamespace), + Channel("chan-2", testNamespace).WithProvInstalledStatus(corev1.ConditionTrue), + Channel("chan-3", testNamespace).WithProvInstalledStatus(corev1.ConditionFalse), + }, + WantPresent: []runtime.Object{ + Channel("chan-1", testNamespace), + Channel("chan-2", testNamespace).WithProvInstalledStatus(corev1.ConditionTrue), + Channel("chan-3", testNamespace).WithProvInstalledStatus(corev1.ConditionFalse), + }, + WantEvent: []corev1.Event{ + events[channelUpdateStatusFailed], + }, + Mocks: controllertesting.Mocks{ + MockStatusUpdates: []controllertesting.MockStatusUpdate{failUpdate}, + }, + }, + } + for _, tc := range testCases { + c := tc.GetClient() + dc := tc.GetDynamicClient() + recorder := tc.GetEventRecorder() + + r := &reconciler{ + client: c, + dynamicClient: dc, + restConfig: &rest.Config{}, + recorder: recorder, + } + tc.IgnoreTimes = true + t.Run(tc.Name, tc.Runner(t, r, c, recorder)) + } +} + +func failUpdate(innerClient client.Client, ctx context.Context, obj runtime.Object) (controllertesting.MockHandled, error) { + return controllertesting.Handled, errors.New("Update failed") +} + +type ChannelBuilder struct { + *eventingv1alpha1.Channel +} + +// Verify the Builder implements Buildable +var _ controllertesting.Buildable = &ChannelBuilder{} + +func (s *ChannelBuilder) Build() runtime.Object { + return s.Channel +} + +func Channel(name string, namespace string) *ChannelBuilder { + channel := getTestChannelWithoutStatus(name, namespace) + return &ChannelBuilder{ + Channel: channel, + } +} + +func (cb *ChannelBuilder) WithProvInstalledStatus(provInstalledStatus corev1.ConditionStatus) *ChannelBuilder { + cb.Status = eventingv1alpha1.ChannelStatus{} + cb.Status.InitializeConditions() + + switch provInstalledStatus { + case corev1.ConditionTrue: + cb.Status.MarkProvisionerInstalled() + break + case corev1.ConditionFalse: + cb.Status.MarkProvisionerNotInstalled( + "Provisioner not found.", + "Specified provisioner [Name:%v Kind:%v] is not installed or not controlling the channel.", + testCCPName, + testCCPKind, + ) + break + } + return cb +} + +func getTestChannelWithoutStatus(name string, namespace string) *eventingv1alpha1.Channel { + ch := &eventingv1alpha1.Channel{} + ch.APIVersion = testAPIVersion + ch.Namespace = testNamespace + ch.Kind = "Channel" + ch.Name = name + ch.Namespace = namespace + ch.Spec = getTestChannelSpec() + return ch +} + +// func getTestChannelWithStatus(provInstalledStatus corev1.ConditionStatus) *eventingv1alpha1.Channel { +// ch := getTestChannelWithoutStatus() +// switch provInstalledStatus { +// case corev1.ConditionTrue: +// ch.Status = getChanStatusProvInstalled() +// break +// case corev1.ConditionFalse: +// ch.Status = getChanStatusProvNotInstalled() +// break +// case corev1.ConditionUnknown: +// ch.Status = getChanStatusProvUnknown() +// break +// } +// return ch +// } + +func getTestChannelSpec() eventingv1alpha1.ChannelSpec { + chSpec := eventingv1alpha1.ChannelSpec{ + Provisioner: &corev1.ObjectReference{}, + Subscribable: &eventingduck.Subscribable{ + Subscribers: []eventingduck.ChannelSubscriberSpec{ + getTestSubscriberSpec(), + getTestSubscriberSpec(), + }, + }, + } + chSpec.Provisioner.Name = testCCPName + chSpec.Provisioner.APIVersion = testAPIVersion + chSpec.Provisioner.Kind = testCCPKind + return chSpec +} + +func getTestSubscriberSpec() eventingduck.ChannelSubscriberSpec { + return eventingduck.ChannelSubscriberSpec{ + SubscriberURI: "TestSubscriberURI", + ReplyURI: "TestReplyURI", + } +} + +// func getChanStatusProvInstalled() eventingv1alpha1.ChannelStatus { +// noProvChStatus := eventingv1alpha1.ChannelStatus{} +// noProvChStatus.InitializeConditions() +// noProvChStatus.MarkProvisionerInstalled() +// return noProvChStatus +// } + +// func getChanStatusProvNotInstalled() eventingv1alpha1.ChannelStatus { +// noProvChStatus := eventingv1alpha1.ChannelStatus{} +// noProvChStatus.InitializeConditions() +// noProvChStatus.MarkProvisionerNotInstalled( +// "Provisioner not found.", +// "Specified provisioner [Name:%v Kind:%v] is not installed or not controlling the channel.", +// testCCPName, +// testCCPKind, +// ) +// return noProvChStatus +// } + +// func getChanStatusProvUnknown() eventingv1alpha1.ChannelStatus { +// noProvChStatus := eventingv1alpha1.ChannelStatus{} +// noProvChStatus.InitializeConditions() +// return noProvChStatus +// } From ba80a2ea9455d2e3f911abee57d03f28c9cdcc58 Mon Sep 17 00:00:00 2001 From: Akash Verenkar Date: Fri, 1 Mar 2019 17:52:28 -0800 Subject: [PATCH 2/7] Removed commetned lines --- pkg/reconciler/v1alpha1/channel/channel.go | 1 - .../v1alpha1/channel/channel_test.go | 41 ------------------- 2 files changed, 42 deletions(-) diff --git a/pkg/reconciler/v1alpha1/channel/channel.go b/pkg/reconciler/v1alpha1/channel/channel.go index 8b301ea9941..aabc6af458d 100644 --- a/pkg/reconciler/v1alpha1/channel/channel.go +++ b/pkg/reconciler/v1alpha1/channel/channel.go @@ -118,7 +118,6 @@ func (r *reconciler) Reconcile(request reconcile.Request) (reconcile.Result, err } func (r *reconciler) reconcile(ch *v1alpha1.Channel) error { - // TODO: Test what happens on delete ch.Status.InitializeConditions() c := ch.Status.GetCondition(v1alpha1.ChannelConditionProvisionerInstalled) diff --git a/pkg/reconciler/v1alpha1/channel/channel_test.go b/pkg/reconciler/v1alpha1/channel/channel_test.go index 20dc3c67df1..1934d76ba75 100644 --- a/pkg/reconciler/v1alpha1/channel/channel_test.go +++ b/pkg/reconciler/v1alpha1/channel/channel_test.go @@ -197,22 +197,6 @@ func getTestChannelWithoutStatus(name string, namespace string) *eventingv1alpha return ch } -// func getTestChannelWithStatus(provInstalledStatus corev1.ConditionStatus) *eventingv1alpha1.Channel { -// ch := getTestChannelWithoutStatus() -// switch provInstalledStatus { -// case corev1.ConditionTrue: -// ch.Status = getChanStatusProvInstalled() -// break -// case corev1.ConditionFalse: -// ch.Status = getChanStatusProvNotInstalled() -// break -// case corev1.ConditionUnknown: -// ch.Status = getChanStatusProvUnknown() -// break -// } -// return ch -// } - func getTestChannelSpec() eventingv1alpha1.ChannelSpec { chSpec := eventingv1alpha1.ChannelSpec{ Provisioner: &corev1.ObjectReference{}, @@ -235,28 +219,3 @@ func getTestSubscriberSpec() eventingduck.ChannelSubscriberSpec { ReplyURI: "TestReplyURI", } } - -// func getChanStatusProvInstalled() eventingv1alpha1.ChannelStatus { -// noProvChStatus := eventingv1alpha1.ChannelStatus{} -// noProvChStatus.InitializeConditions() -// noProvChStatus.MarkProvisionerInstalled() -// return noProvChStatus -// } - -// func getChanStatusProvNotInstalled() eventingv1alpha1.ChannelStatus { -// noProvChStatus := eventingv1alpha1.ChannelStatus{} -// noProvChStatus.InitializeConditions() -// noProvChStatus.MarkProvisionerNotInstalled( -// "Provisioner not found.", -// "Specified provisioner [Name:%v Kind:%v] is not installed or not controlling the channel.", -// testCCPName, -// testCCPKind, -// ) -// return noProvChStatus -// } - -// func getChanStatusProvUnknown() eventingv1alpha1.ChannelStatus { -// noProvChStatus := eventingv1alpha1.ChannelStatus{} -// noProvChStatus.InitializeConditions() -// return noProvChStatus -// } From 8813fc8f5e6d35f0935f7ae0e7ab861ca6d35ad8 Mon Sep 17 00:00:00 2001 From: Akash Verenkar Date: Fri, 1 Mar 2019 18:02:07 -0800 Subject: [PATCH 3/7] Fixed some typos --- pkg/reconciler/v1alpha1/channel/channel.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/reconciler/v1alpha1/channel/channel.go b/pkg/reconciler/v1alpha1/channel/channel.go index aabc6af458d..7e19c7725f1 100644 --- a/pkg/reconciler/v1alpha1/channel/channel.go +++ b/pkg/reconciler/v1alpha1/channel/channel.go @@ -70,7 +70,7 @@ func ProvideController(mgr manager.Manager) (controller.Controller, error) { return nil, err } - // Watch channel events and enqueue Subscription object key. + // Watch channel events // This controller is no-op when Channels are deleted if err := c.Watch( &source.Kind{Type: &v1alpha1.Channel{}}, From 9b790f5af0e9cf9829db4811809433ae480c1a9d Mon Sep 17 00:00:00 2001 From: Akash Verenkar Date: Mon, 4 Mar 2019 10:35:58 -0800 Subject: [PATCH 4/7] Changes based on comments on code review (pull request) --- pkg/apis/eventing/v1alpha1/channel_types.go | 6 ++++-- pkg/reconciler/v1alpha1/channel/channel.go | 9 ++++----- pkg/reconciler/v1alpha1/channel/channel_test.go | 16 ++++++++++++++++ 3 files changed, 24 insertions(+), 7 deletions(-) diff --git a/pkg/apis/eventing/v1alpha1/channel_types.go b/pkg/apis/eventing/v1alpha1/channel_types.go index 0793b5f222a..d7f6b16c2f4 100644 --- a/pkg/apis/eventing/v1alpha1/channel_types.go +++ b/pkg/apis/eventing/v1alpha1/channel_types.go @@ -144,7 +144,7 @@ func (cs *ChannelStatus) InitializeConditions() { func (cs *ChannelStatus) MarkProvisioned() { chanCondSet.Manage(cs).MarkTrue(ChannelConditionProvisioned) - // MarkProvisionedNotInstalled will be set by default controller and needs to be unset by individual controllers + // Channel-default-controller sets ChannelConditionProvisionerInstalled=False, and it needs to be set to True by individual controllers // This is done so that each individual channel controller gets it for free. cs.MarkProvisionerInstalled() } @@ -152,15 +152,17 @@ func (cs *ChannelStatus) MarkProvisioned() { // MarkNotProvisioned sets ChannelConditionProvisioned condition to False state. func (cs *ChannelStatus) MarkNotProvisioned(reason, messageFormat string, messageA ...interface{}) { chanCondSet.Manage(cs).MarkFalse(ChannelConditionProvisioned, reason, messageFormat, messageA...) - // MarkProvisionedNotInstalled will be set by default controller and needs to be unset by individual controllers + // Channel-default-controller sets ChannelConditionProvisionerInstalled=False, and it needs to be set to True by individual controllers // This is done so that each individual channel controller gets it for free. cs.MarkProvisionerInstalled() } +// MarkProvisionerInstalled sets ChannelConditionProvisionerInstalled condition to True state. func (cs *ChannelStatus) MarkProvisionerInstalled() { chanCondSet.Manage(cs).MarkTrue(ChannelConditionProvisionerInstalled) } +// MarkProvisionerNotInstalled sets ChannelConditionProvisionerInstalled condition to False state. func (cs *ChannelStatus) MarkProvisionerNotInstalled(reason, messageFormat string, messageA ...interface{}) { chanCondSet.Manage(cs).MarkFalse(ChannelConditionProvisionerInstalled, reason, messageFormat, messageA...) } diff --git a/pkg/reconciler/v1alpha1/channel/channel.go b/pkg/reconciler/v1alpha1/channel/channel.go index 7e19c7725f1..60f329e8e08 100644 --- a/pkg/reconciler/v1alpha1/channel/channel.go +++ b/pkg/reconciler/v1alpha1/channel/channel.go @@ -1,5 +1,5 @@ /* -Copyright 2018 The Knative Authors +Copyright 2019 The Knative Authors Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -41,8 +41,7 @@ import ( const ( // controllerAgentName is the string used by this controller to identify // itself when creating events. - controllerAgentName = "channel-controller" - finalizerName = controllerAgentName + controllerAgentName = "channel-default-controller" channelReconciled = "ChannelReconciled" channelUpdateStatusFailed = "ChannelUpdateStatusFailed" ) @@ -86,7 +85,7 @@ func ProvideController(mgr manager.Manager) (controller.Controller, error) { return c, nil } -// This detaul channel reconciler will check if the channel is being watched by provisioner's channel controller +// This defaul channel reconciler will check if the channel is being watched by provisioner's channel controller // This will improve UX. See https://github.com/knative/eventing/issues/779 func (r *reconciler) Reconcile(request reconcile.Request) (reconcile.Result, error) { glog.Infof("Reconciling channel %v", request) @@ -125,7 +124,7 @@ func (r *reconciler) reconcile(ch *v1alpha1.Channel) error { if c.IsUnknown() { ch.Status.MarkProvisionerNotInstalled( "Provisioner not found.", - "Specified provisioner [Name:%v Kind:%v] is not installed or not controlling the channel.", + "Specified provisioner [Name:%s Kind:%s] is not installed or not controlling the channel.", ch.Spec.Provisioner.Name, ch.Spec.Provisioner.Kind, ) diff --git a/pkg/reconciler/v1alpha1/channel/channel_test.go b/pkg/reconciler/v1alpha1/channel/channel_test.go index 1934d76ba75..b1065af38bb 100644 --- a/pkg/reconciler/v1alpha1/channel/channel_test.go +++ b/pkg/reconciler/v1alpha1/channel/channel_test.go @@ -1,3 +1,19 @@ +/* +Copyright 2019 The Knative Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + package channel import ( From fbbdcc0ac6047da98c6bf735c6bdbb48d50e684c Mon Sep 17 00:00:00 2001 From: Akash Verenkar Date: Tue, 5 Mar 2019 15:31:13 -0800 Subject: [PATCH 5/7] Moved the code to set ChannelConditionProvisionerInstalled=True to Initialize() function from MarkProvisioned() --- pkg/apis/eventing/v1alpha1/channel_types.go | 11 +++++------ pkg/apis/eventing/v1alpha1/channel_types_test.go | 7 ++++--- pkg/reconciler/v1alpha1/channel/channel.go | 9 +++++---- pkg/reconciler/v1alpha1/channel/channel_test.go | 1 - 4 files changed, 14 insertions(+), 14 deletions(-) diff --git a/pkg/apis/eventing/v1alpha1/channel_types.go b/pkg/apis/eventing/v1alpha1/channel_types.go index d7f6b16c2f4..26040c3d64b 100644 --- a/pkg/apis/eventing/v1alpha1/channel_types.go +++ b/pkg/apis/eventing/v1alpha1/channel_types.go @@ -138,23 +138,22 @@ func (cs *ChannelStatus) IsReady() bool { // InitializeConditions sets relevant unset conditions to Unknown state. func (cs *ChannelStatus) InitializeConditions() { chanCondSet.Manage(cs).InitializeConditions() + // Channel-default-controller sets ChannelConditionProvisionerInstalled=False, and it needs to be set to True by individual controllers + // This is done so that each individual channel controller gets it for free. + // It is also implied here that the channel-default-controller never calls InitializeConditions(), while individual channel controllers + // call InitializeConditions() as one of the first things in its reconcile loop. + cs.MarkProvisionerInstalled() } // MarkProvisioned sets ChannelConditionProvisioned condition to True state. func (cs *ChannelStatus) MarkProvisioned() { chanCondSet.Manage(cs).MarkTrue(ChannelConditionProvisioned) - // Channel-default-controller sets ChannelConditionProvisionerInstalled=False, and it needs to be set to True by individual controllers - // This is done so that each individual channel controller gets it for free. - cs.MarkProvisionerInstalled() } // MarkNotProvisioned sets ChannelConditionProvisioned condition to False state. func (cs *ChannelStatus) MarkNotProvisioned(reason, messageFormat string, messageA ...interface{}) { chanCondSet.Manage(cs).MarkFalse(ChannelConditionProvisioned, reason, messageFormat, messageA...) - // Channel-default-controller sets ChannelConditionProvisionerInstalled=False, and it needs to be set to True by individual controllers - // This is done so that each individual channel controller gets it for free. - cs.MarkProvisionerInstalled() } // MarkProvisionerInstalled sets ChannelConditionProvisionerInstalled condition to True state. diff --git a/pkg/apis/eventing/v1alpha1/channel_types_test.go b/pkg/apis/eventing/v1alpha1/channel_types_test.go index 8ff1619f897..1bed66647ec 100644 --- a/pkg/apis/eventing/v1alpha1/channel_types_test.go +++ b/pkg/apis/eventing/v1alpha1/channel_types_test.go @@ -103,7 +103,7 @@ func TestChannelInitializeConditions(t *testing.T) { Status: corev1.ConditionUnknown, }, { Type: ChannelConditionProvisionerInstalled, - Status: corev1.ConditionUnknown, + Status: corev1.ConditionTrue, }, { Type: ChannelConditionReady, Status: corev1.ConditionUnknown, @@ -126,7 +126,7 @@ func TestChannelInitializeConditions(t *testing.T) { Status: corev1.ConditionFalse, }, { Type: ChannelConditionProvisionerInstalled, - Status: corev1.ConditionUnknown, + Status: corev1.ConditionTrue, }, { Type: ChannelConditionReady, Status: corev1.ConditionUnknown, @@ -149,7 +149,7 @@ func TestChannelInitializeConditions(t *testing.T) { Status: corev1.ConditionTrue, }, { Type: ChannelConditionProvisionerInstalled, - Status: corev1.ConditionUnknown, + Status: corev1.ConditionTrue, }, { Type: ChannelConditionReady, Status: corev1.ConditionUnknown, @@ -187,6 +187,7 @@ func TestChannelIsReady(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { cs := &ChannelStatus{} + cs.InitializeConditions() if test.markProvisioned { cs.MarkProvisioned() } else { diff --git a/pkg/reconciler/v1alpha1/channel/channel.go b/pkg/reconciler/v1alpha1/channel/channel.go index 60f329e8e08..961505acc4f 100644 --- a/pkg/reconciler/v1alpha1/channel/channel.go +++ b/pkg/reconciler/v1alpha1/channel/channel.go @@ -85,7 +85,7 @@ func ProvideController(mgr manager.Manager) (controller.Controller, error) { return c, nil } -// This defaul channel reconciler will check if the channel is being watched by provisioner's channel controller +// Reconcile will check if the channel is being watched by provisioner's channel controller // This will improve UX. See https://github.com/knative/eventing/issues/779 func (r *reconciler) Reconcile(request reconcile.Request) (reconcile.Result, error) { glog.Infof("Reconciling channel %v", request) @@ -107,7 +107,7 @@ func (r *reconciler) Reconcile(request reconcile.Request) (reconcile.Result, err err = r.reconcile(ch) if err != nil { - glog.Warningf("Error reconciling channel: %v. Will retry.", err) + glog.Warningf("Error reconciling channel %s: %s. Will retry.", request, err) r.recorder.Eventf(ch, corev1.EventTypeWarning, channelUpdateStatusFailed, "Failed to update channel status: %v", request.NamespacedName) return reconcile.Result{Requeue: true}, err } @@ -117,8 +117,9 @@ func (r *reconciler) Reconcile(request reconcile.Request) (reconcile.Result, err } func (r *reconciler) reconcile(ch *v1alpha1.Channel) error { - ch.Status.InitializeConditions() - + // Do not Initialize() Status in channel-default-controller. It will set ChannelConditionProvisionerInstalled=True + // Directly call GetCondition(). If the Status was never initialized then GetCondition() will return nil and + // IsUnknown() will return true c := ch.Status.GetCondition(v1alpha1.ChannelConditionProvisionerInstalled) if c.IsUnknown() { diff --git a/pkg/reconciler/v1alpha1/channel/channel_test.go b/pkg/reconciler/v1alpha1/channel/channel_test.go index b1065af38bb..976207955d4 100644 --- a/pkg/reconciler/v1alpha1/channel/channel_test.go +++ b/pkg/reconciler/v1alpha1/channel/channel_test.go @@ -184,7 +184,6 @@ func Channel(name string, namespace string) *ChannelBuilder { func (cb *ChannelBuilder) WithProvInstalledStatus(provInstalledStatus corev1.ConditionStatus) *ChannelBuilder { cb.Status = eventingv1alpha1.ChannelStatus{} - cb.Status.InitializeConditions() switch provInstalledStatus { case corev1.ConditionTrue: From 671056b7263c83602cf182cd6857741bb7cc2761 Mon Sep 17 00:00:00 2001 From: Akash Verenkar Date: Tue, 5 Mar 2019 15:56:40 -0800 Subject: [PATCH 6/7] Changed all %v to %s when logging strings --- pkg/reconciler/v1alpha1/channel/channel.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/reconciler/v1alpha1/channel/channel.go b/pkg/reconciler/v1alpha1/channel/channel.go index 961505acc4f..c02fb49dbfd 100644 --- a/pkg/reconciler/v1alpha1/channel/channel.go +++ b/pkg/reconciler/v1alpha1/channel/channel.go @@ -88,19 +88,19 @@ func ProvideController(mgr manager.Manager) (controller.Controller, error) { // Reconcile will check if the channel is being watched by provisioner's channel controller // This will improve UX. See https://github.com/knative/eventing/issues/779 func (r *reconciler) Reconcile(request reconcile.Request) (reconcile.Result, error) { - glog.Infof("Reconciling channel %v", request) + glog.Infof("Reconciling channel %s", request) ch := &v1alpha1.Channel{} // Controller-runtime client Get() always deep copies the object. Hence no need to again deep copy it err := r.client.Get(context.TODO(), request.NamespacedName, ch) if errors.IsNotFound(err) { - glog.Errorf("could not find channel %v\n", request) + glog.Errorf("could not find channel %s\n", request) return reconcile.Result{}, nil } if err != nil { - glog.Errorf("could not fetch channel %v for %+v\n", err, request) + glog.Errorf("could not fetch channel %s: %s\n", request, err) return reconcile.Result{}, err } @@ -108,11 +108,11 @@ func (r *reconciler) Reconcile(request reconcile.Request) (reconcile.Result, err if err != nil { glog.Warningf("Error reconciling channel %s: %s. Will retry.", request, err) - r.recorder.Eventf(ch, corev1.EventTypeWarning, channelUpdateStatusFailed, "Failed to update channel status: %v", request.NamespacedName) + r.recorder.Eventf(ch, corev1.EventTypeWarning, channelUpdateStatusFailed, "Failed to update channel status: %s", request) return reconcile.Result{Requeue: true}, err } - glog.Infof("Successfully reconciled channel %v", request.NamespacedName) - r.recorder.Eventf(ch, corev1.EventTypeNormal, channelReconciled, "Channel reconciled: %v", request.NamespacedName) + glog.Infof("Successfully reconciled channel %s", request) + r.recorder.Eventf(ch, corev1.EventTypeNormal, channelReconciled, "Channel reconciled: %s", request) return reconcile.Result{Requeue: false}, nil } From 672091805281416d4ca0047657150ed20c346a45 Mon Sep 17 00:00:00 2001 From: Akash Verenkar Date: Wed, 6 Mar 2019 11:04:46 -0800 Subject: [PATCH 7/7] Updating some formatting issues called out in PR --- pkg/apis/eventing/v1alpha1/channel_types.go | 1 - pkg/reconciler/v1alpha1/channel/channel_test.go | 12 ++++-------- 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/pkg/apis/eventing/v1alpha1/channel_types.go b/pkg/apis/eventing/v1alpha1/channel_types.go index 26040c3d64b..c0fc003aa9a 100644 --- a/pkg/apis/eventing/v1alpha1/channel_types.go +++ b/pkg/apis/eventing/v1alpha1/channel_types.go @@ -148,7 +148,6 @@ func (cs *ChannelStatus) InitializeConditions() { // MarkProvisioned sets ChannelConditionProvisioned condition to True state. func (cs *ChannelStatus) MarkProvisioned() { chanCondSet.Manage(cs).MarkTrue(ChannelConditionProvisioned) - } // MarkNotProvisioned sets ChannelConditionProvisioned condition to False state. diff --git a/pkg/reconciler/v1alpha1/channel/channel_test.go b/pkg/reconciler/v1alpha1/channel/channel_test.go index 976207955d4..b4980d09076 100644 --- a/pkg/reconciler/v1alpha1/channel/channel_test.go +++ b/pkg/reconciler/v1alpha1/channel/channel_test.go @@ -62,8 +62,7 @@ func TestAllCases(t *testing.T) { WantErr: false, WantResult: reconcile.Result{}, ReconcileKey: fmt.Sprintf("%v/%v", "chan-1", testNamespace), - }, - { + }, { Name: "Orphaned channel", WantErr: false, WantResult: reconcile.Result{}, @@ -81,8 +80,7 @@ func TestAllCases(t *testing.T) { WantEvent: []corev1.Event{ events[channelReconciled], }, - }, - { + }, { Name: "Non-oprphaned channel test 1", WantErr: false, WantResult: reconcile.Result{}, @@ -100,8 +98,7 @@ func TestAllCases(t *testing.T) { WantEvent: []corev1.Event{ events[channelReconciled], }, - }, - { + }, { Name: "Non-oprphaned channel test 2", WantErr: false, WantResult: reconcile.Result{}, @@ -119,8 +116,7 @@ func TestAllCases(t *testing.T) { WantEvent: []corev1.Event{ events[channelReconciled], }, - }, - { + }, { Name: "Fail orphaned channel status update", WantErr: true, WantErrMsg: "Update failed",