From aadb3045781c275c17278a0cd034ff66a55a326d Mon Sep 17 00:00:00 2001 From: Grant Rodgers Date: Wed, 1 Aug 2018 15:44:43 -0700 Subject: [PATCH 1/3] Only update the feed in one place When there are multiple places a feed can be updated, resourceVersion conflicts become more likely. This splits Reconcile into two functions: - an inner function that reconciles the state of a feed - an outer function that loads the feed, calls the inner function if the feed exists, then updates the feed. This separation of responsibilities reduces the likelihood that a feed will be updated more than once per reconcile. --- pkg/controller/feed/reconcile.go | 50 ++++++++++++-------------------- 1 file changed, 19 insertions(+), 31 deletions(-) diff --git a/pkg/controller/feed/reconcile.go b/pkg/controller/feed/reconcile.go index 73b3de4a6e2..265b049d800 100644 --- a/pkg/controller/feed/reconcile.go +++ b/pkg/controller/feed/reconcile.go @@ -56,6 +56,21 @@ func (r *reconciler) Reconcile(request reconcile.Request) (reconcile.Result, err return reconcile.Result{}, err } + if err = r.reconcile(feed); err != nil { + glog.Errorf("error reconciling Feed: %v", err) + } + + if updateErr := r.updateFeed(feed); updateErr != nil { + glog.Errorf("failed to update Feed: %v", updateErr) + return reconcile.Result{}, updateErr + } + return reconcile.Result{}, err +} + +// reconcile tries to converge the current state of the given Feed to the +// desired state. This function should not update the Feed; the calling method +// should do that. +func (r *reconciler) reconcile(feed *feedsv1alpha1.Feed) error { feed.Status.InitializeConditions() // Fetch the EventSource and EventType that is being asked for // and if they don't exist update the status to reflect this back @@ -79,14 +94,10 @@ func (r *reconciler) Reconcile(request reconcile.Request) (reconcile.Result, err Reason: t.Reason, Message: t.Message, }) - default: - return reconcile.Result{}, err } - if err := r.updateFeed(feed); err != nil { - glog.Errorf("failed to update Feed status: %v", err) - return reconcile.Result{}, err - } - return reconcile.Result{}, nil + // This is a terminal state, and we've noted it in the status, so return + // nil to signal that no further reconciling is required. + return nil } // TODO: Set the FeedConditionDependenciesSatisfied to true here? Or, after @@ -101,10 +112,6 @@ func (r *reconciler) Reconcile(request reconcile.Request) (reconcile.Result, err // to do this properly. // TODO: Add issue link here. can't look up right now, no wifi r.setEventTypeOwnerReference(feed) - if err := r.updateOwnerReferences(feed); err != nil { - glog.Errorf("failed to update Feed owner references: %v", err) - return reconcile.Result{}, err - } if feed.GetDeletionTimestamp() == nil { err = r.reconcileStartJob(feed, eventSource, eventType) @@ -117,12 +124,7 @@ func (r *reconciler) Reconcile(request reconcile.Request) (reconcile.Result, err glog.Errorf("error reconciling stop Job: %v", err) } } - - if updateErr := r.updateFeed(feed); updateErr != nil { - glog.Errorf("failed to update Feed status: %v", updateErr) - return reconcile.Result{}, updateErr - } - return reconcile.Result{}, err + return nil } func (r *reconciler) reconcileStartJob(feed *feedsv1alpha1.Feed, es *feedsv1alpha1.EventSource, et *feedsv1alpha1.EventType) error { @@ -247,20 +249,6 @@ func (r *reconciler) reconcileStopJob(feed *feedsv1alpha1.Feed, es *feedsv1alpha return nil } -func (r *reconciler) updateOwnerReferences(u *feedsv1alpha1.Feed) error { - feed := &feedsv1alpha1.Feed{} - err := r.client.Get(context.TODO(), client.ObjectKey{Namespace: u.Namespace, Name: u.Name}, feed) - if err != nil { - return err - } - - if !equality.Semantic.DeepEqual(feed.OwnerReferences, u.OwnerReferences) { - feed.SetOwnerReferences(u.ObjectMeta.OwnerReferences) - return r.client.Update(context.TODO(), feed) - } - return nil -} - func (r *reconciler) updateFinalizers(u *feedsv1alpha1.Feed) error { feed := &feedsv1alpha1.Feed{} err := r.client.Get(context.TODO(), client.ObjectKey{Namespace: u.Namespace, Name: u.Name}, feed) From 4945f2dec04f7465d9eee2d4f651102f263f6778 Mon Sep 17 00:00:00 2001 From: Grant Rodgers Date: Thu, 2 Aug 2018 14:08:36 -0700 Subject: [PATCH 2/3] Document Reconcile flow Reconcile error handling is subtle and perhaps nonintuitive, so make it clear to new contributors. --- pkg/controller/feed/reconcile.go | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/pkg/controller/feed/reconcile.go b/pkg/controller/feed/reconcile.go index 265b049d800..f9e318402e0 100644 --- a/pkg/controller/feed/reconcile.go +++ b/pkg/controller/feed/reconcile.go @@ -41,27 +41,45 @@ const ( finalizerName = controllerAgentName ) -// Reconcile Feed resources +// Reconcile compares the actual state of a Feed with the desired, and attempts +// to converge the two. It then updates the Status block of the Feed with +// its current state. +// If Reconcile returns a non-nil error, the request will be retried. func (r *reconciler) Reconcile(request reconcile.Request) (reconcile.Result, error) { feed := &feedsv1alpha1.Feed{} err := r.client.Get(context.TODO(), request.NamespacedName, feed) + // The feed may have been deleted since it was added to the workqueue. If so + // there's nothing to be done. if errors.IsNotFound(err) { glog.Errorf("could not find Feed %v\n", request) return reconcile.Result{}, nil } + // If the feed exists but could not be retrieved, then we should retry. if err != nil { glog.Errorf("could not fetch Feed %v for %+v\n", err, request) return reconcile.Result{}, err } + // Now that we know the feed exists, we can reconcile it. An error returned + // here means the reconcile did not complete and the Feed should be requeued + // for another attempt. + // A successful reconcile does not necessarily mean the feed is in the desired + // state, it means no more can be done for now. In this case the feed will + // not be reconciled again until the resync period or a watched resource + // changes. if err = r.reconcile(feed); err != nil { glog.Errorf("error reconciling Feed: %v", err) } + // Since the reconcile is a sequence of steps, earlier steps may complete + // successfully while later steps fail. The Feed is updated on failure to + // preserve any useful status or metadata changes the non-failing steps made. if updateErr := r.updateFeed(feed); updateErr != nil { glog.Errorf("failed to update Feed: %v", updateErr) + // An error here means the feed should be reconciled again, regardless of + // whether the reconcile was successful or not. return reconcile.Result{}, updateErr } return reconcile.Result{}, err From e0af3ea771900e34ae9002eacc69718121450b91 Mon Sep 17 00:00:00 2001 From: Grant Rodgers Date: Thu, 2 Aug 2018 14:12:10 -0700 Subject: [PATCH 3/3] Add a default case for feed source errors This default case is currently unreachable, but future changes may start returning different error types from getFeedSource, so this is defensive programming. --- pkg/controller/feed/reconcile.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/controller/feed/reconcile.go b/pkg/controller/feed/reconcile.go index f9e318402e0..714101fb668 100644 --- a/pkg/controller/feed/reconcile.go +++ b/pkg/controller/feed/reconcile.go @@ -112,6 +112,10 @@ func (r *reconciler) reconcile(feed *feedsv1alpha1.Feed) error { Reason: t.Reason, Message: t.Message, }) + default: + // This is unreachable unless getFeedSource is refactored. + // Assume this is a non-terminal state and return the error. + return fmt.Errorf("Error getting feed source: %v", err) } // This is a terminal state, and we've noted it in the status, so return // nil to signal that no further reconciling is required.