Repository: redhat-developer/service-binding-operator
Pull request #166 (2019-09-03 12:40:52)

Object Updates as Last Reconciliation Step
Refactoring reconciliation process to only update objects as last step. Status of `ServiceBindingRequest` is kept as a local variable, and updated as the reconciliation process goes, and attempts to update object status will happen in case of error, or at the end of reconciliation.

This PR addresses errors on updating objects that have been modified, and considerably reduces the amount of API calls during reconciliation.

Furthermore, fixing panic triggered on `planner.go:66` due a `nil` variable returned, covering issue scenario with unit-test. The test is asserting the reconciliation processes will return error and cause re-queue in case of expected resources are not found.

openshift-ci-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by:
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **otaviof**
You can assign the PR to them by writing `/assign @otaviof` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=redhat-developer%2Fservice-binding-operator).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/redhat-developer/service-binding-operator/blob/master/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["otaviof"]} -->
openshift-ci-robot: @otaviof: The following test **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
ci/prow/unit | d7a43441ff2169f7ecb2cbe28ea716189208bac9 | [link](https://prow.svc.ci.openshift.org/view/gcs/origin-ci-test/pr-logs/pull/redhat-developer_service-binding-operator/166/pull-ci-redhat-developer-service-binding-operator-master-unit/458) | `/test unit`

[Full PR test history](https://prow.svc.ci.openshift.org/pr-history?org=redhat-developer&repo=service-binding-operator&pr=166). [Your PR dashboard](https://prow.svc.ci.openshift.org/pr?query=is:pr+state:open+author:otaviof). Please help us cut down on flakes by [linking to](https://github.com/kubernetes/community/blob/master/contributors/devel/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/redhat-developer/service-binding-operator/issues?q=is:issue+is:open) when you hit one in your PR.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
openshift-ci-robot: @otaviof: The following tests **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
ci/prow/unit | d7a43441ff2169f7ecb2cbe28ea716189208bac9 | [link](https://prow.svc.ci.openshift.org/view/gcs/origin-ci-test/pr-logs/pull/redhat-developer_service-binding-operator/166/pull-ci-redhat-developer-service-binding-operator-master-unit/458) | `/test unit`
ci/prow/e2e | d7a43441ff2169f7ecb2cbe28ea716189208bac9 | [link](https://prow.svc.ci.openshift.org/view/gcs/origin-ci-test/pr-logs/pull/redhat-developer_service-binding-operator/166/pull-ci-redhat-developer-service-binding-operator-master-e2e/503) | `/test e2e`

[Full PR test history](https://prow.svc.ci.openshift.org/pr-history?org=redhat-developer&repo=service-binding-operator&pr=166). [Your PR dashboard](https://prow.svc.ci.openshift.org/pr?query=is:pr+state:open+author:otaviof). Please help us cut down on flakes by [linking to](https://github.com/kubernetes/community/blob/master/contributors/devel/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/redhat-developer/service-binding-operator/issues?q=is:issue+is:open) when you hit one in your PR.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
sbose78: Otavio, if you could confirm that you've seen attempted stale updates on SBR, I'm good with this change.
codecov[bot]: # [Codecov](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/166?src=pr&el=h1) Report
> :exclamation: No coverage uploaded for pull request base (`master@9dd3794`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
> The diff coverage is `56.6%`.

[![Impacted file tree graph](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/166/graphs/tree.svg?width=650&token=CICBeVWlrC&height=150&src=pr)](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/166?src=pr&el=tree)

```diff
@@            Coverage Diff            @@
##             master     #166   +/-   ##
=========================================
  Coverage          ?   64.56%           
=========================================
  Files             ?        9           
  Lines             ?      666           
  Branches          ?        0           
=========================================
  Hits              ?      430           
  Misses            ?      171           
  Partials          ?       65
```


| [Impacted Files](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/166?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/controller/servicebindingrequest/common.go](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/166/diff?src=pr&el=tree#diff-cGtnL2NvbnRyb2xsZXIvc2VydmljZWJpbmRpbmdyZXF1ZXN0L2NvbW1vbi5nbw==) | `15.38% <ø> (ø)` | |
| [...kg/controller/servicebindingrequest/annotations.go](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/166/diff?src=pr&el=tree#diff-cGtnL2NvbnRyb2xsZXIvc2VydmljZWJpbmRpbmdyZXF1ZXN0L2Fubm90YXRpb25zLmdv) | `88.33% <ø> (ø)` | |
| [pkg/controller/servicebindingrequest/reconciler.go](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/166/diff?src=pr&el=tree#diff-cGtnL2NvbnRyb2xsZXIvc2VydmljZWJpbmRpbmdyZXF1ZXN0L3JlY29uY2lsZXIuZ28=) | `60.46% <56.6%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/166?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/166?src=pr&el=footer). Last update [9dd3794...78741c9](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/166?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

openshift-ci-robot: @otaviof: The following test **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
ci/prow/e2e | a9545942ea4c14fd62653874d7ba2d00fde91eda | [link](https://prow.svc.ci.openshift.org/view/gcs/origin-ci-test/pr-logs/pull/redhat-developer_service-binding-operator/166/pull-ci-redhat-developer-service-binding-operator-master-e2e/533) | `/test e2e`

[Full PR test history](https://prow.svc.ci.openshift.org/pr-history?org=redhat-developer&repo=service-binding-operator&pr=166). [Your PR dashboard](https://prow.svc.ci.openshift.org/pr?query=is:pr+state:open+author:otaviof). Please help us cut down on flakes by [linking to](https://github.com/kubernetes/community/blob/master/contributors/devel/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/redhat-developer/service-binding-operator/issues?q=is:issue+is:open) when you hit one in your PR.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
otaviof: > Otavio, if you could confirm that you've seen attempted stale updates on SBR, I'm good with this change.

Please consider latest interactions on this PR, it's fairly different than first attempt, but in line with our more recent conversations.
sbose78: /lgtm
sbose78: /approve
openshift-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/redhat-developer/service-binding-operator/pull/166#issuecomment-528880051" title="Approved">sbose78</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=redhat-developer%2Fservice-binding-operator).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/redhat-developer/service-binding-operator/blob/master/OWNERS)~~ [sbose78]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->

Patch:
diff --git a/Makefile b/Makefile
index a032939a..e20f22f2 100644
--- a/Makefile
+++ b/Makefile
@@ -106,6 +106,7 @@ OPERATOR_IMAGE ?= quay.io/${OPERATOR_GROUP}/${GO_PACKAGE_REPO_NAME}
 OPERATOR_TAG_SHORT ?= $(OPERATOR_VERSION)
 OPERATOR_TAG_LONG ?= $(OPERATOR_VERSION)-$(GIT_COMMIT_ID)
 OPERATOR_IMAGE_BUILDER ?= buildah
+OPERATOR_SDK_EXTRA_ARGS ?= "--debug"
 
 QUAY_TOKEN ?= ""
 
@@ -187,10 +188,10 @@ test-e2e: e2e-setup
 	$(info Running E2E test: $@)
 	$(Q)GO111MODULE=$(GO111MODULE) GOCACHE=$(GOCACHE) SERVICE_BINDING_OPERATOR_DISABLE_ELECTION=true \
 		operator-sdk --verbose test local ./test/e2e \
-			--debug \
 			--namespace $(TEST_NAMESPACE) \
 			--up-local \
-			--go-test-flags "-timeout=15m"
+			--go-test-flags "-timeout=15m" \
+			$(OPERATOR_SDK_EXTRA_ARGS)
 
 .PHONY: test-unit
 ## Runs the unit tests without code coverage
@@ -222,7 +223,10 @@ test-e2e-olm-ci:
 	$(Q)kubectl apply -f ./test/operator-hub/subscription.yaml
 	$(eval DEPLOYED_NAMESPACE := openshift-operators)
 	$(Q)./hack/check-crds.sh
-	$(Q)operator-sdk --verbose test local ./test/e2e --no-setup --go-test-flags "-timeout=15m"
+	$(Q)operator-sdk --verbose test local ./test/e2e \
+			--no-setup \
+			--go-test-flags "-timeout=15m" \
+			$(OPERATOR_SDK_EXTRA_ARGS)
 
 ## -- Build Go binary and OCI image targets --
 
diff --git a/pkg/controller/servicebindingrequest/annotations.go b/pkg/controller/servicebindingrequest/annotations.go
index af46e79d..7e82a653 100644
--- a/pkg/controller/servicebindingrequest/annotations.go
+++ b/pkg/controller/servicebindingrequest/annotations.go
@@ -1,8 +1,6 @@
 package servicebindingrequest
 
 import (
-	"context"
-
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
@@ -79,7 +77,6 @@ func GetSBRNamespacedNameFromObject(obj runtime.Object) (types.NamespacedName, e
 // SetSBRAnnotations update existing annotations to include operator's. The annotations added are
 // referring to a existing SBR namespaced name.
 func SetSBRAnnotations(
-	ctx context.Context,
 	client dynamic.Interface,
 	namespacedName types.NamespacedName,
 	objs []*unstructured.Unstructured,
diff --git a/pkg/controller/servicebindingrequest/binder.go b/pkg/controller/servicebindingrequest/binder.go
index 9bab9855..1276d126 100644
--- a/pkg/controller/servicebindingrequest/binder.go
+++ b/pkg/controller/servicebindingrequest/binder.go
@@ -3,16 +3,17 @@ package servicebindingrequest
 import (
 	"context"
 	"fmt"
-	"k8s.io/apimachinery/pkg/api/errors"
 	"time"
 
 	"github.com/go-logr/logr"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/dynamic"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	logf "sigs.k8s.io/controller-runtime/pkg/runtime/log"
@@ -54,7 +55,7 @@ func (b *Binder) search() (*unstructured.UnstructuredList, error) {
 	}
 	// Return fake NotFound error explicitly to ensure requeue when objList(^) is empty.
 	if len(objList.Items) == 0 {
-		return nil , errors.NewNotFound(
+		return nil, errors.NewNotFound(
 			gvr.GroupResource(),
 			b.sbr.Spec.ApplicationSelector.Resource,
 		)
@@ -265,11 +266,21 @@ func (b *Binder) update(objs *unstructured.UnstructuredList) ([]*unstructured.Un
 			}
 		}
 
-		logger.Info("Updating object in Kube...")
+		logger.Info("Updating object...")
 		if err := b.client.Update(b.ctx, updatedObj); err != nil {
 			return nil, err
 		}
 
+		logger.Info("Reading back updated object...")
+		// reading object back again, to comply with possible modifications
+		namespacedName := types.NamespacedName{
+			Namespace: updatedObj.GetNamespace(),
+			Name:      updatedObj.GetName(),
+		}
+		if err = b.client.Get(b.ctx, namespacedName, updatedObj); err != nil {
+			return nil, err
+		}
+
 		updatedObjs = append(updatedObjs, updatedObj)
 	}
 
diff --git a/pkg/controller/servicebindingrequest/common.go b/pkg/controller/servicebindingrequest/common.go
index cd853a2c..1a6cf9e3 100644
--- a/pkg/controller/servicebindingrequest/common.go
+++ b/pkg/controller/servicebindingrequest/common.go
@@ -12,6 +12,9 @@ var (
 	log = logf.ZapLogger(true).WithName("servicebindingrequest")
 )
 
+// ServiceBindingRequestResource the name of ServiceBindingRequest resource.
+const ServiceBindingRequestResource = "servicebindingrequests"
+
 // ServiceBindingRequestKind defines the name of the CRD kind.
 const ServiceBindingRequestKind = "ServiceBindingRequest"
 
diff --git a/pkg/controller/servicebindingrequest/olm.go b/pkg/controller/servicebindingrequest/olm.go
index 3b8f58e3..c4590d5d 100644
--- a/pkg/controller/servicebindingrequest/olm.go
+++ b/pkg/controller/servicebindingrequest/olm.go
@@ -1,6 +1,7 @@
 package servicebindingrequest
 
 import (
+	"fmt"
 	"strings"
 
 	"github.com/go-logr/logr"
@@ -138,7 +139,7 @@ func (o *OLM) SelectCRDByGVK(gvk schema.GroupVersionKind) (*olmv1alpha1.CRDDescr
 
 	if len(crdDescriptions) == 0 {
 		logger.Info("No CRD could be found for GVK.")
-		return nil, nil
+		return nil, fmt.Errorf("no crd could be found for gvk")
 	}
 	return crdDescriptions[0], nil
 }
diff --git a/pkg/controller/servicebindingrequest/reconciler.go b/pkg/controller/servicebindingrequest/reconciler.go
index 82b08a64..76cf791b 100644
--- a/pkg/controller/servicebindingrequest/reconciler.go
+++ b/pkg/controller/servicebindingrequest/reconciler.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
@@ -34,43 +35,88 @@ const (
 )
 
 // setSecretName update the CR status field to "in progress", and setting secret name.
-func (r *Reconciler) setSecretName(
-	ctx context.Context,
-	instance *v1alpha1.ServiceBindingRequest,
-	name string,
-) error {
-	instance.Status.BindingStatus = bindingInProgress
-	instance.Status.Secret = name
-	return r.client.Status().Update(ctx, instance)
-
+func (r *Reconciler) setSecretName(sbrStatus *v1alpha1.ServiceBindingRequestStatus, name string) {
+	sbrStatus.BindingStatus = bindingInProgress
+	sbrStatus.Secret = name
 }
 
 // setStatus update the CR status field.
-func (r *Reconciler) setStatus(
-	ctx context.Context,
-	instance *v1alpha1.ServiceBindingRequest,
-	status string,
-) error {
-	instance.Status.BindingStatus = status
-	return r.client.Status().Update(ctx, instance)
+func (r *Reconciler) setStatus(sbrStatus *v1alpha1.ServiceBindingRequestStatus, status string) {
+	sbrStatus.BindingStatus = status
 }
 
 // setApplicationObjects set the ApplicationObject status field, and also set the overall status as
 // success, since it was able to bind applications.
 func (r *Reconciler) setApplicationObjects(
-	ctx context.Context,
-	instance *v1alpha1.ServiceBindingRequest,
+	sbrStatus *v1alpha1.ServiceBindingRequestStatus,
 	objs []*unstructured.Unstructured,
-) error {
+) {
 	names := []string{}
 	for _, obj := range objs {
 		names = append(names, fmt.Sprintf("%s/%s", obj.GetNamespace(), obj.GetName()))
 	}
+	sbrStatus.BindingStatus = bindingSuccess
+	sbrStatus.ApplicationObjects = names
+}
+
+// getServiceBindingRequest retrieve the SBR object based on namespaced-name.
+func (r *Reconciler) getServiceBindingRequest(
+	namespacedName types.NamespacedName,
+) (*v1alpha1.ServiceBindingRequest, error) {
+	gr := v1alpha1.SchemeGroupVersion.WithResource(ServiceBindingRequestResource)
+	resourceClient := r.dynClient.Resource(gr).Namespace(namespacedName.Namespace)
+	u, err := resourceClient.Get(namespacedName.Name, metav1.GetOptions{})
+	if err != nil {
+		return nil, err
+	}
+	sbr := &v1alpha1.ServiceBindingRequest{}
+	err = runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, sbr)
+	if err != nil {
+		return nil, err
+	}
+	return sbr, nil
+}
+
+// updateStatusServiceBindingRequest update the status field of a ServiceBindingRequest.
+func (r *Reconciler) updateStatusServiceBindingRequest(
+	sbr *v1alpha1.ServiceBindingRequest,
+	sbrStatus *v1alpha1.ServiceBindingRequestStatus,
+) error {
+	// coping status over informed object
+	sbr.Status = *sbrStatus
+
+	// converting object into unstructured
+	data, err := runtime.DefaultUnstructuredConverter.ToUnstructured(sbr)
+	if err != nil {
+		return err
+	}
+	u := &unstructured.Unstructured{Object: data}
 
-	instance.Status.BindingStatus = bindingSuccess
-	instance.Status.ApplicationObjects = names
+	gr := v1alpha1.SchemeGroupVersion.WithResource(ServiceBindingRequestResource)
+	resourceClient := r.dynClient.Resource(gr).Namespace(sbr.GetNamespace())
+	_, err = resourceClient.UpdateStatus(u, metav1.UpdateOptions{})
+	return err
+}
 
-	return r.client.Status().Update(ctx, instance)
+// onError comprise the update of ServiceBindingRequest status to set error flag, and inspect
+// informed error to apply a different behavior for not-founds.
+func (r *Reconciler) onError(
+	err error,
+	sbr *v1alpha1.ServiceBindingRequest,
+	sbrStatus *v1alpha1.ServiceBindingRequestStatus,
+	objs []*unstructured.Unstructured,
+) (reconcile.Result, error) {
+	// settting overall status to failed
+	r.setStatus(sbrStatus, bindingFail)
+	//
+	if objs != nil {
+		r.setApplicationObjects(sbrStatus, objs)
+	}
+	errStatus := r.updateStatusServiceBindingRequest(sbr, sbrStatus)
+	if errStatus != nil {
+		return RequeueError(errStatus)
+	}
+	return RequeueOnNotFound(err, requeueAfter)
 }
 
 // Reconcile a ServiceBindingRequest by the following steps:
@@ -85,6 +131,7 @@ func (r *Reconciler) setApplicationObjects(
 func (r *Reconciler) Reconcile(request reconcile.Request) (reconcile.Result, error) {
 	ctx := context.TODO()
 	objectsToAnnotate := []*unstructured.Unstructured{}
+
 	logger := logf.Log.WithValues(
 		"Request.Namespace", request.Namespace,
 		"Request.Name", request.Name,
@@ -92,32 +139,28 @@ func (r *Reconciler) Reconcile(request reconcile.Request) (reconcile.Result, err
 	logger.Info("Reconciling ServiceBindingRequest...")
 
 	// fetch the ServiceBindingRequest instance
-	instance := &v1alpha1.ServiceBindingRequest{}
-	err := r.client.Get(ctx, request.NamespacedName, instance)
+	sbr, err := r.getServiceBindingRequest(request.NamespacedName)
 	if err != nil {
 		logger.Error(err, "On retrieving service-binding-request instance.")
 		return RequeueError(err)
 	}
 
-	logger = logger.WithValues("ServiceBindingRequest.Name", instance.Name)
-	logger.Info("Found service binding request to inspect")
+	// splitting instance from it's status
+	sbrStatus := sbr.Status
 
-	if err = r.setStatus(ctx, instance, bindingInProgress); err != nil {
-		logger.Error(err, "On updating service-binding-request status.")
-		return RequeueError(err)
-	}
+	logger = logger.WithValues("ServiceBindingRequest.Name", sbr.Name)
+	logger.Info("Found service binding request to inspect")
 
 	//
 	// Planing changes
 	//
 
 	logger.Info("Creating a plan based on OLM and CRD.")
-	planner := NewPlanner(ctx, r.dynClient, instance)
+	planner := NewPlanner(ctx, r.dynClient, sbr)
 	plan, err := planner.Plan()
 	if err != nil {
-		_ = r.setStatus(ctx, instance, bindingFail)
 		logger.Error(err, "On creating a plan to bind applications.")
-		return RequeueOnNotFound(err, requeueAfter)
+		return r.onError(err, sbr, &sbrStatus, nil)
 	}
 
 	// storing CR in objects to annotate
@@ -128,18 +171,14 @@ func (r *Reconciler) Reconcile(request reconcile.Request) (reconcile.Result, err
 	//
 
 	logger.Info("Retrieving data to create intermediate secret.")
-	retriever := NewRetriever(ctx, r.dynClient, plan, instance.Spec.EnvVarPrefix)
+	retriever := NewRetriever(r.dynClient, plan, sbr.Spec.EnvVarPrefix)
 	retrievedObjects, err := retriever.Retrieve()
 	if err != nil {
-		_ = r.setStatus(ctx, instance, bindingFail)
 		logger.Error(err, "On retrieving binding data.")
-		return RequeueOnNotFound(err, requeueAfter)
+		return r.onError(err, sbr, &sbrStatus, nil)
 	}
 
-	if err = r.setSecretName(ctx, instance, plan.Name); err != nil {
-		logger.Error(err, "On updating service-binding-request status.")
-		return RequeueError(err)
-	}
+	r.setSecretName(&sbrStatus, plan.Name)
 
 	// storing objects used in Retriever
 	objectsToAnnotate = append(objectsToAnnotate, retrievedObjects...)
@@ -149,17 +188,15 @@ func (r *Reconciler) Reconcile(request reconcile.Request) (reconcile.Result, err
 	//
 
 	logger.Info("Binding applications with intermediary secret.")
-	binder := NewBinder(ctx, r.client, r.dynClient, instance, retriever.volumeKeys)
+	binder := NewBinder(ctx, r.client, r.dynClient, sbr, retriever.volumeKeys)
 	updatedObjects, err := binder.Bind()
 	if err != nil {
-		_ = r.setStatus(ctx, instance, bindingFail)
 		logger.Error(err, "On binding application.")
-		return RequeueOnNotFound(err, requeueAfter)
-	} else if err = r.setApplicationObjects(ctx, instance, updatedObjects); err != nil {
-		logger.Error(err, "On updating application objects status field.")
-		return RequeueError(err)
+		return r.onError(err, sbr, &sbrStatus, updatedObjects)
 	}
 
+	// saving on status the list of objects that have been touched
+	r.setApplicationObjects(&sbrStatus, updatedObjects)
 	// storing objects used in Binder
 	objectsToAnnotate = append(objectsToAnnotate, updatedObjects...)
 
@@ -167,12 +204,14 @@ func (r *Reconciler) Reconcile(request reconcile.Request) (reconcile.Result, err
 	// Annotating objects related to binding
 	//
 
-	sbrNamespacedName := types.NamespacedName{
-		Namespace: instance.GetNamespace(),
-		Name:      instance.GetName(),
-	}
-	if err = SetSBRAnnotations(ctx, r.dynClient, sbrNamespacedName, objectsToAnnotate); err != nil {
+	if err = SetSBRAnnotations(r.dynClient, request.NamespacedName, objectsToAnnotate); err != nil {
 		logger.Error(err, "On setting annotations in related objects.")
+		return r.onError(err, sbr, &sbrStatus, updatedObjects)
+	}
+
+	// updating status of request instance
+	if err = r.updateStatusServiceBindingRequest(sbr, &sbrStatus); err != nil {
+		logger.Error(err, "On updating status of ServiceBindingRequest.")
 		return RequeueError(err)
 	}
 
diff --git a/pkg/controller/servicebindingrequest/retriever.go b/pkg/controller/servicebindingrequest/retriever.go
index 4ece10c0..39c06a10 100644
--- a/pkg/controller/servicebindingrequest/retriever.go
+++ b/pkg/controller/servicebindingrequest/retriever.go
@@ -1,7 +1,6 @@
 package servicebindingrequest
 
 import (
-	"context"
 	"encoding/base64"
 	"fmt"
 	"strings"
@@ -22,7 +21,6 @@ type Retriever struct {
 	logger        logr.Logger                  // logger instance
 	data          map[string][]byte            // data retrieved
 	objects       []*unstructured.Unstructured // list of objects employed
-	ctx           context.Context              // request context
 	client        dynamic.Interface            // Kubernetes API client
 	plan          *Plan                        // plan instance
 	volumeKeys    []string                     // list of keys found
@@ -294,17 +292,11 @@ func (r *Retriever) Retrieve() ([]*unstructured.Unstructured, error) {
 }
 
 // NewRetriever instantiate a new retriever instance.
-func NewRetriever(
-	ctx context.Context,
-	client dynamic.Interface,
-	plan *Plan,
-	bindingPrefix string,
-) *Retriever {
+func NewRetriever(client dynamic.Interface, plan *Plan, bindingPrefix string) *Retriever {
 	return &Retriever{
 		logger:        logf.Log.WithName("retriever"),
 		data:          make(map[string][]byte),
 		objects:       []*unstructured.Unstructured{},
-		ctx:           ctx,
 		client:        client,
 		plan:          plan,
 		volumeKeys:    []string{},


Test patch:
diff --git a/pkg/controller/servicebindingrequest/reconciler_test.go b/pkg/controller/servicebindingrequest/reconciler_test.go
index c373767f..35bc05f1 100644
--- a/pkg/controller/servicebindingrequest/reconciler_test.go
+++ b/pkg/controller/servicebindingrequest/reconciler_test.go
@@ -12,7 +12,6 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 	logf "sigs.k8s.io/controller-runtime/pkg/runtime/log"
 
-	"github.com/redhat-developer/service-binding-operator/pkg/apis/apps/v1alpha1"
 	"github.com/redhat-developer/service-binding-operator/test/mocks"
 )
 
@@ -35,6 +34,25 @@ func reconcileRequest() reconcile.Request {
 	}
 }
 
+func TestReconcilerReconcileError(t *testing.T) {
+	resourceRef := "test-using-secret"
+	matchLabels := map[string]string{
+		"connects-to": "database",
+		"environment": "reconciler",
+	}
+
+	f := mocks.NewFake(t, reconcilerNs)
+	f.AddMockedUnstructuredServiceBindingRequest(reconcilerName, resourceRef, matchLabels)
+
+	fakeClient := f.FakeClient()
+	fakeDynClient := f.FakeDynClient()
+	reconciler := &Reconciler{client: fakeClient, dynClient: fakeDynClient, scheme: f.S}
+
+	res, err := reconciler.Reconcile(reconcileRequest())
+	assert.Error(t, err)
+	assert.True(t, res.Requeue)
+}
+
 // TestReconcilerReconcileUsingSecret test the reconciliation process using a secret, expected to be
 // the regular approach.
 func TestReconcilerReconcileUsingSecret(t *testing.T) {
@@ -46,14 +64,15 @@ func TestReconcilerReconcileUsingSecret(t *testing.T) {
 	}
 
 	f := mocks.NewFake(t, reconcilerNs)
-	f.AddMockedServiceBindingRequest(reconcilerName, resourceRef, matchLabels)
+	f.AddMockedUnstructuredServiceBindingRequest(reconcilerName, resourceRef, matchLabels)
 	f.AddMockedUnstructuredCSV("cluster-service-version-list")
-	f.AddMockedDatabaseCR(resourceRef)
+	f.AddMockedUnstructuredDatabaseCR(resourceRef)
 	f.AddMockedUnstructuredDeployment(reconcilerName, matchLabels)
 	f.AddMockedSecret("db-credentials")
 
 	fakeClient := f.FakeClient()
-	reconciler := &Reconciler{client: fakeClient, dynClient: f.FakeDynClient(), scheme: f.S}
+	fakeDynClient := f.FakeDynClient()
+	reconciler := &Reconciler{client: fakeClient, dynClient: fakeDynClient, scheme: f.S}
 
 	t.Run("reconcile-using-secret", func(t *testing.T) {
 		res, err := reconciler.Reconcile(reconcileRequest())
@@ -70,8 +89,10 @@ func TestReconcilerReconcileUsingSecret(t *testing.T) {
 		assert.NotNil(t, containers[0].EnvFrom[0].SecretRef)
 		assert.Equal(t, reconcilerName, containers[0].EnvFrom[0].SecretRef.Name)
 
-		sbrOutput := v1alpha1.ServiceBindingRequest{}
-		require.Nil(t, fakeClient.Get(ctx, namespacedName, &sbrOutput))
+		namespacedName = types.NamespacedName{Namespace: reconcilerNs, Name: reconcilerName}
+		sbrOutput, err := reconciler.getServiceBindingRequest(namespacedName)
+		require.NoError(t, err)
+
 		require.Equal(t, "Success", sbrOutput.Status.BindingStatus)
 		require.Equal(t, reconcilerName, sbrOutput.Status.Secret)
 
@@ -90,9 +111,9 @@ func TestReconcilerReconcileUsingVolumes(t *testing.T) {
 	}
 
 	f := mocks.NewFake(t, reconcilerNs)
-	f.AddMockedServiceBindingRequest(reconcilerName, resourceRef, matchLabels)
+	f.AddMockedUnstructuredServiceBindingRequest(reconcilerName, resourceRef, matchLabels)
 	f.AddMockedUnstructuredCSVWithVolumeMount("cluster-service-version-list")
-	f.AddMockedDatabaseCR(resourceRef)
+	f.AddMockedUnstructuredDatabaseCR(resourceRef)
 	f.AddMockedUnstructuredDeployment(reconcilerName, matchLabels)
 	f.AddMockedSecret("db-credentials")
 
diff --git a/pkg/controller/servicebindingrequest/retriever_test.go b/pkg/controller/servicebindingrequest/retriever_test.go
index c15bdc94..d7cdeb34 100644
--- a/pkg/controller/servicebindingrequest/retriever_test.go
+++ b/pkg/controller/servicebindingrequest/retriever_test.go
@@ -1,7 +1,6 @@
 package servicebindingrequest
 
 import (
-	"context"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -30,7 +29,7 @@ func TestRetriever(t *testing.T) {
 
 	fakeDynClient := f.FakeDynClient()
 
-	retriever = NewRetriever(context.TODO(), fakeDynClient, plan, "SERVICE_BINDING")
+	retriever = NewRetriever(fakeDynClient, plan, "SERVICE_BINDING")
 	require.NotNil(t, retriever)
 
 	t.Run("retrive", func(t *testing.T) {
@@ -96,7 +95,7 @@ func TestRetriever(t *testing.T) {
 	})
 
 	t.Run("empty prefix", func(t *testing.T) {
-		retriever = NewRetriever(context.TODO(), fakeDynClient, plan, "")
+		retriever = NewRetriever(fakeDynClient, plan, "")
 		require.NotNil(t, retriever)
 		retriever.data = make(map[string][]byte)
 
@@ -127,7 +126,7 @@ func TestRetrieverWithNestedCRKey(t *testing.T) {
 
 	fakeDynClient := f.FakeDynClient()
 
-	retriever = NewRetriever(context.TODO(), fakeDynClient, plan, "SERVICE_BINDING")
+	retriever = NewRetriever(fakeDynClient, plan, "SERVICE_BINDING")
 	require.NotNil(t, retriever)
 
 	t.Run("Second level", func(t *testing.T) {
@@ -169,7 +168,7 @@ func TestRetrieverWithConfigMap(t *testing.T) {
 
 	fakeDynClient := f.FakeDynClient()
 
-	retriever = NewRetriever(context.TODO(), fakeDynClient, plan, "SERVICE_BINDING")
+	retriever = NewRetriever(fakeDynClient, plan, "SERVICE_BINDING")
 	require.NotNil(t, retriever)
 
 	t.Run("read", func(t *testing.T) {
diff --git a/test/mocks/fake.go b/test/mocks/fake.go
index b97c14a0..4feac59d 100644
--- a/test/mocks/fake.go
+++ b/test/mocks/fake.go
@@ -8,6 +8,7 @@ import (
 	olmv1alpha1 "github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha1"
 	"github.com/stretchr/testify/require"
 	appsv1 "k8s.io/api/apps/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/client-go/dynamic"
 	fakedynamic "k8s.io/client-go/dynamic/fake"
@@ -27,13 +28,29 @@ type Fake struct {
 }
 
 // AddMockedServiceBindingRequest add mocked object from ServiceBindingRequestMock.
-func (f *Fake) AddMockedServiceBindingRequest(name, ref string, matchLabels map[string]string) *v1alpha1.ServiceBindingRequest {
+func (f *Fake) AddMockedServiceBindingRequest(
+	name string,
+	ref string,
+	matchLabels map[string]string,
+) *v1alpha1.ServiceBindingRequest {
 	f.S.AddKnownTypes(v1alpha1.SchemeGroupVersion, &v1alpha1.ServiceBindingRequest{})
 	sbr := ServiceBindingRequestMock(f.ns, name, ref, matchLabels)
 	f.objs = append(f.objs, sbr)
 	return sbr
 }
 
+func (f *Fake) AddMockedUnstructuredServiceBindingRequest(
+	name string,
+	ref string,
+	matchLabels map[string]string,
+) *unstructured.Unstructured {
+	f.S.AddKnownTypes(v1alpha1.SchemeGroupVersion, &v1alpha1.ServiceBindingRequest{})
+	sbr, err := UnstructuredServiceBindingRequestMock(f.ns, name, ref, matchLabels)
+	require.Nil(f.t, err)
+	f.objs = append(f.objs, sbr)
+	return sbr
+}
+
 // AddMockedUnstructuredCSV add mocked unstructured CSV.
 func (f *Fake) AddMockedUnstructuredCSV(name string) {
 	require.Nil(f.t, olmv1alpha1.AddToScheme(f.S))
@@ -74,6 +91,13 @@ func (f *Fake) AddMockedDatabaseCR(ref string) {
 	f.objs = append(f.objs, DatabaseCRMock(f.ns, ref))
 }
 
+func (f *Fake) AddMockedUnstructuredDatabaseCR(ref string) {
+	require.Nil(f.t, pgapis.AddToScheme(f.S))
+	d, err := UnstructuredDatabaseCRMock(f.ns, ref)
+	require.Nil(f.t, err)
+	f.objs = append(f.objs, d)
+}
+
 // AddMockedUnstructuredDeployment add mocked object from UnstructuredDeploymentMock.
 func (f *Fake) AddMockedUnstructuredDeployment(name string, matchLabels map[string]string) {
 	require.Nil(f.t, appsv1.AddToScheme(f.S))
diff --git a/test/mocks/mocks.go b/test/mocks/mocks.go
index 38dad18c..6e07a6db 100644
--- a/test/mocks/mocks.go
+++ b/test/mocks/mocks.go
@@ -268,7 +268,10 @@ func ConfigMapMock(ns, name string) *corev1.ConfigMap {
 
 // ServiceBindingRequestMock return a binding-request mock of informed name and match labels.
 func ServiceBindingRequestMock(
-	ns, name, resourceRef string, matchLabels map[string]string,
+	ns string,
+	name string,
+	resourceRef string,
+	matchLabels map[string]string,
 ) *v1alpha1.ServiceBindingRequest {
 	return &v1alpha1.ServiceBindingRequest{
 		ObjectMeta: metav1.ObjectMeta{
@@ -293,6 +296,23 @@ func ServiceBindingRequestMock(
 	}
 }
 
+// UnstructuredServiceBindingRequestMock returns a unstructured version of SBR.
+func UnstructuredServiceBindingRequestMock(
+	ns string,
+	name string,
+	resourceRef string,
+	matchLabels map[string]string,
+) (*unstructured.Unstructured, error) {
+	sbr := ServiceBindingRequestMock(ns, name, resourceRef, matchLabels)
+	data, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&sbr)
+	if err != nil {
+		return nil, err
+	}
+	u := &ustrv1.Unstructured{Object: data}
+	u.SetGroupVersionKind(v1alpha1.SchemeGroupVersion.WithKind(OperatorKind))
+	return u, nil
+}
+
 // DeploymentListMock returns a list of DeploymentMock.
 func DeploymentListMock(ns, name string, matchLabels map[string]string) appsv1.DeploymentList {
 	return appsv1.DeploymentList{