Create an overall `Ready` to indicate whether the SBR reconcile is completed successfully #665 #681

qibobo · 2020-09-21T15:03:29Z

According to the discussion in #665, we need to add a overall Ready condition to show what is the status of the ServiceBinding CR.

openshift-ci-robot · 2020-09-21T15:03:37Z

Hi @qibobo. Thanks for your PR.

I'm waiting for a redhat-developer member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

pedjak

acceptance tests need to updated to reflect and check this new flag.

qibobo · 2020-09-21T15:13:40Z

@isutton
Could you help review this PR?

qibobo · 2020-09-22T05:08:30Z

@pedjak done

isutton · 2020-09-24T08:14:12Z

/ok-to-test

qibobo · 2020-09-24T11:18:24Z

/retest

Avni-Sharma · 2020-09-24T13:15:18Z

pkg/controller/servicebinding/reconciler.go

@@ -31,6 +36,10 @@ const (
 	EmptyApplicationReason = "EmptyApplication"
 	// ApplicationNotFoundReason is used when the application is not found.
 	ApplicationNotFoundReason = "ApplicationNotFound"
+	// FailedToGatherServiceInformationReason is used when the it is failed to fetch backend service information.
+	FailedToGatherServiceInformationReason = "FailedToGatherServiceInformation"
+	// FailedToSaveSecretReason is used when the it is failed to save intermediate secret.


Do we need this ? Could not find any reference to this particular FailedToSaveReason in this PR.

pmacik · 2020-09-24T20:58:44Z

/test 4.5-acceptance

qibobo · 2020-09-25T14:26:45Z

@Avni-Sharma @isutton @pmacik @pedjak
The acceptance test is failed, but seems I did not modify the error part. Could you help confirm whether it is the acceptance test problem or the issue of my PR?

test/acceptance/features/steps/steps.py:225
      Assertion Failed: 
      Expected: not SBO jq result "{result}" should match "{json_value_regex}"
           but: was <None>

pedjak

This PR should introduce new condition, but should alte the logic around the existing conditions.

pedjak · 2020-09-23T18:04:55Z

pkg/controller/servicebinding/reconciler.go

 	conditionsv1 "github.com/openshift/custom-resource-status/conditions/v1"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"


this line can be removed - the import already exists in line 10.

pedjak · 2020-09-24T10:33:29Z

pkg/controller/servicebinding/reconciler.go

+			conditionsv1.Condition{
+				Type:    BindingReady,
+				Status:  corev1.ConditionFalse,
+				Reason:  EmptyServiceSelectorsReason,


I am not sure if we need to copy reasons here from other conditions. If we do that then we are back of the question if we really need to have other conditions.

BTW, we need to fill LastTransitionTime.

IMHO, In case when Ready=false we should look into other conditions if we want to know more details.

pedjak · 2020-09-25T14:37:41Z

pkg/controller/servicebinding/reconciler.go

+			conditionsv1.Condition{
+				Type:    InjectionReady,
+				Status:  corev1.ConditionFalse,
+				Reason:  FailedToGatherServiceInformationReason,
+				Message: err.Error(),
+			},
+			conditionsv1.Condition{
+				Type:    BindingReady,
+				Status:  corev1.ConditionFalse,
+				Reason:  FailedToGatherServiceInformationReason,
+				Message: err.Error(),
+			},


We did not even reach the state when we can say if something is injection ready or not, because we did not gather service data at all. Conditions must not exist at all. We should update them only if there is a change in status, and in that case we are going to update last transition time as well. In this PR, it looks like we have 3 conditions, but they all share value and reason. Why then not collapse it into single condition.

BTW, excellent read about conditions: https://dev.to/maelvls/what-the-heck-are-kubernetes-conditions-for-4je7

pedjak · 2020-09-25T14:39:32Z

pkg/controller/servicebinding/reconciler.go

+		_ = updateSBRConditions(r.dynClient, sbr,
+			conditionsv1.Condition{
+				Type:    CollectionReady,
+				Status:  corev1.ConditionFalse,
+				Reason:  FailedToGatherServiceInformationReason,
+				Message: err.Error(),
+			},
+			conditionsv1.Condition{
+				Type:    InjectionReady,
+				Status:  corev1.ConditionFalse,
+				Reason:  FailedToGatherServiceInformationReason,
+				Message: err.Error(),
+			},
+			conditionsv1.Condition{
+				Type:    BindingReady,
+				Status:  corev1.ConditionFalse,
+				Reason:  FailedToGatherServiceInformationReason,
+				Message: err.Error(),
+			})


same comments as above. Error in buildServiceContexts or buildBinding does not move the process into the same condition.

pedjak · 2020-09-25T14:40:31Z

pkg/controller/servicebinding/reconciler.go

+		_ = updateSBRConditions(r.dynClient, sbr,
+			conditionsv1.Condition{
+				Type:    CollectionReady,
+				Status:  corev1.ConditionFalse,
+				Reason:  FailedToGatherServiceInformationReason,
+				Message: err.Error(),
+			},
+			conditionsv1.Condition{
+				Type:    InjectionReady,
+				Status:  corev1.ConditionFalse,
+				Reason:  FailedToGatherServiceInformationReason,
+				Message: err.Error(),
+			},
+			conditionsv1.Condition{
+				Type:    BindingReady,
+				Status:  corev1.ConditionFalse,
+				Reason:  FailedToGatherServiceInformationReason,
+				Message: err.Error(),
+			},


same comment as above.

pedjak · 2020-09-25T14:41:47Z

pkg/controller/servicebinding/sbrcontroller.go

+func createFunc(logger *log.Log) func(event.CreateEvent) bool {
+	return func(e event.CreateEvent) bool {
+		return true
+	}
+}
+


why do we need this?

sbose78 · 2020-09-25T16:06:30Z

@qibobo Could you please add a description why this is needed :) ?

Avni-Sharma · 2020-09-28T14:41:46Z

pkg/controller/servicebinding/reconciler_test.go

@@ -257,6 +259,63 @@ func TestReconcilerReconcileUsingVolumes(t *testing.T) {
 	})
 }

+func TestServiceNotFound(t *testing.T) {
+	backingServiceResourceRef := "backingService1"
+	matchLabels := map[string]string{


Do we require matchLabels?

Avni-Sharma · 2020-09-28T14:44:16Z

pkg/controller/servicebinding/reconciler_test.go

+	}
+	f := mocks.NewFake(t, reconcilerNs)
+	f.AddMockedUnstructuredServiceBinding(reconcilerName, backingServiceResourceRef, "", deploymentsGVR, matchLabels)
+	f.AddMockedUnstructuredCSV("cluster-service-version-list")


CSV might not be necessary
depends whether we are checking via annotations on CRDs or via x-descriptors

Avni-Sharma · 2020-09-28T14:53:33Z

pkg/controller/servicebinding/servicebinder.go

@@ -273,14 +279,7 @@ func removeFinalizer(sbr *v1alpha1.ServiceBinding) {
 // handleApplicationError handles scenarios when:
 // 1. application not declared in the Service Binding
 // 2. application not found
-func (b *serviceBinder) handleApplicationError(reason string, applicationError error, sbrStatus *v1alpha1.ServiceBindingStatus) (reconcile.Result, error) {


handleApplicationError is for the reasons provided in the above comments. We can still keep this function's functionality. Any particular reason for modifying this?

@Avni-Sharma
Put the condition setting out of handleApplicationError function as there are two callers:
caller1:

service-binding-operator/pkg/controller/servicebinding/servicebinder.go

Line 327 in 2f561b7

if isApplicationEmpty(b.sbr.Spec.Application) {

caller2:

service-binding-operator/pkg/controller/servicebinding/servicebinder.go

Line 343 in 2f561b7

if errors.Is(err, errApplicationNotFound) {

caller1 is for application empty so InjectionReady is false and BindingReady is true.
caller2 is for application not found so both InjectionReady and BindingReady are false.
So here we need to put the condition setting out of this function.

Thanks. However, in order to improve readability of the code, and avoid copy/pasting conditionsv1.SetStatusCondition blocks around, how about to define InjectionReadyStatus(...), ReadyStatus, ... on ServiceBindingStatus struct and use them across the code? IMHO, it would improve readbility and reusability of the code. Also, Ready condition could even autoderived from other two conditions in that case.

qibobo · 2020-09-28T15:12:52Z

@qibobo Could you please add a description why this is needed :) ?

@sbose78 Done. Add the issue that for the discussion of this PR.

pkg/controller/servicebinding/reconciler.go

pedjak · 2020-10-01T09:50:32Z

pkg/controller/servicebinding/reconciler_test.go

+	// backingServiceResourceRef := "backingService1"
+	// matchLabels := map[string]string{
+	// 	"connects-to": "database",
+	// 	"environment": "reconciler",
+	// }
+	// f := mocks.NewFake(t, reconcilerNs)
+	// f.AddMockedUnstructuredServiceBinding(reconcilerName, backingServiceResourceRef, "", deploymentsGVR, nil)
+	// f.AddMockedUnstructuredCSV("cluster-service-version-list")
+	// f.AddMockedUnstructuredDatabaseCRD()
+	// f.AddMockedUnstructuredDeployment(reconcilerName, nil)


if not needed, please remove from the source code.

pedjak · 2020-10-01T09:52:02Z

pkg/controller/servicebinding/reconciler_test.go

+	applicationResourceRef := "applicationRef"
+	f := mocks.NewFake(t, reconcilerNs)
+	f.AddMockedUnstructuredServiceBindingWithoutService(reconcilerName, applicationResourceRef, deploymentsGVR)
+	// f.AddMockedUnstructuredDatabaseCR(backingServiceResourceRef)


remove if not needed anymore.

pedjak · 2020-10-01T11:16:52Z

pkg/controller/servicebinding/reconciler_test.go

+}
+
+// TestEmptyServiceSelector tests that Status is successfully updated when ServiceSelector is empty
+func TestEmptyServiceSelector(t *testing.T) {


please rename this function to reflect what the comment above says.

@pedjak
What name should it be instead of TestEmptyServiceSelector?

the comment line above has a few hints how the test function name could look like. It should contain the context (empty selector) and the consequence (status is successfully updated)

pedjak · 2020-10-01T11:18:13Z

pkg/controller/servicebinding/sbrcontroller.go

@@ -145,6 +145,7 @@ func buildGVKPredicate(logger *log.Log) predicate.Funcs {
 		UpdateFunc: updateFunc(logger),
 		DeleteFunc: func(e event.DeleteEvent) bool {
 			// evaluates to false if the object has been confirmed deleted
+			logger.Debug("Resource update event received", "GVK", e.Object.GetObjectKind(), "name", e.Meta.GetName())


do we really need this change in PR? BTW, the message is misleading, since we receive here delete event.

pkg/controller/servicebinding/servicebinder.go

pedjak · 2020-10-06T08:59:38Z

Does this PR include the work in #502 ?

qibobo · 2020-10-06T16:00:27Z

Does this PR include the work in #502 ?

@pedjak
Both this PR and 502 are not merged yet, so we can not include each other.

pedjak · 2020-10-06T16:37:24Z

Both this PR and 502 are not merged yet, so we can not include each other.

#502 is about handling situation when service is not found. Does your PR cover such situation?

qibobo · 2020-10-12T15:24:50Z

@pedjak @Avni-Sharma any more comments?

pkg/controller/servicebinding/reconciler.go

pkg/controller/servicebinding/servicebinder_test.go

Avni-Sharma · 2020-10-22T19:06:44Z

I also think that we can have acceptance tests for ready condition. For example https://github.com/redhat-developer/service-binding-operator/blob/master/test/acceptance/features/bindAppToService.feature#L70

qibobo · 2020-10-23T10:02:22Z

@Avni-Sharma
updated.

qibobo · 2020-10-23T10:03:33Z

I also think that we can have acceptance tests for ready condition. For example https://github.com/redhat-developer/service-binding-operator/blob/master/test/acceptance/features/bindAppToService.feature#L70

@Avni-Sharma I think they have been added.

qibobo · 2020-10-23T13:41:11Z

/retest

qibobo · 2020-10-28T07:50:00Z

/retest

Avni-Sharma · 2020-10-28T11:01:02Z

I also think that we can have acceptance tests for ready condition. For example https://github.com/redhat-developer/service-binding-operator/blob/master/test/acceptance/features/bindAppToService.feature#L70

@Avni-Sharma I think they have been added.

Oh , I meant specifically checking for Ready under conditions

…mpleted successfully redhat-developer#665

qibobo · 2020-11-03T02:58:26Z

@Avni-Sharma squashed the commits.

Avni-Sharma · 2020-11-03T15:25:53Z

@qibobo Please feel free to create an issue regarding #681 (comment) . You can perhaps take it up later

Avni-Sharma · 2020-11-03T15:44:06Z

/lgtm
/approve

openshift-ci-robot · 2020-11-03T15:44:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Avni-Sharma

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Avni-Sharma]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot requested review from otaviof and pmacik September 21, 2020 15:03

openshift-ci-robot added the needs-ok-to-test label Sep 21, 2020

pedjak suggested changes Sep 21, 2020

View reviewed changes

qibobo force-pushed the dev_qy_ready branch from 195b1f9 to be46321 Compare September 23, 2020 01:34

openshift-ci-robot added ok-to-test and removed needs-ok-to-test labels Sep 24, 2020

qibobo mentioned this pull request Sep 24, 2020

Community Meeting - September 24, 2020 #688

Closed

Avni-Sharma reviewed Sep 24, 2020

View reviewed changes

qibobo force-pushed the dev_qy_ready branch from 3d8d43d to 94100ab Compare September 25, 2020 14:30

pedjak suggested changes Sep 25, 2020

View reviewed changes

qibobo force-pushed the dev_qy_ready branch from 53a791f to 111b7dc Compare September 27, 2020 14:48

Avni-Sharma reviewed Sep 28, 2020

View reviewed changes

pedjak suggested changes Oct 1, 2020

View reviewed changes

openshift-ci-robot added the needs-rebase label Oct 4, 2020

pedjak mentioned this pull request Oct 6, 2020

[WIP] Update ServiceBindingRequest.Status.Conditions when service not found #502

Closed

qibobo force-pushed the dev_qy_ready branch 4 times, most recently from 73d99f5 to 6b45175 Compare October 10, 2020 05:33

cdlliuy mentioned this pull request Oct 15, 2020

Community Meeting - October 15, 2020 #721

Closed

Avni-Sharma reviewed Oct 19, 2020

View reviewed changes

pkg/controller/servicebinding/reconciler.go Show resolved Hide resolved

Avni-Sharma reviewed Oct 19, 2020

View reviewed changes

pkg/controller/servicebinding/reconciler.go Show resolved Hide resolved

pkg/controller/servicebinding/servicebinder_test.go Show resolved Hide resolved

qibobo mentioned this pull request Oct 22, 2020

Community Meeting - Oct 22, 2020 #736

Closed

qibobo force-pushed the dev_qy_ready branch from 7be30af to 9872ff1 Compare October 28, 2020 06:24

qibobo mentioned this pull request Oct 29, 2020

Community Meeting - Oct 29, 2020 #737

Closed

Create an overall Ready to indicate whether the SBR reconcile is co…

767b344

…mpleted successfully redhat-developer#665

qibobo force-pushed the dev_qy_ready branch from bdb0cd1 to 767b344 Compare November 3, 2020 02:57

openshift-ci-robot assigned Avni-Sharma Nov 3, 2020

openshift-ci-robot added the lgtm label Nov 3, 2020

openshift-ci-robot added the approved label Nov 3, 2020

openshift-merge-robot merged commit 66009c0 into redhat-developer:master Nov 3, 2020

qibobo mentioned this pull request Nov 4, 2020

Refactor ServiceBindingStatus to improve readability of the code #751

Closed

pmacik linked an issue Nov 5, 2020 that may be closed by this pull request

Create an overall Ready to indicate whether the SBR reconcile is completed successfully #665

Closed

pedjak mentioned this pull request Nov 9, 2020

Check ready status of SB created in acceptance test #755

Merged

Create an overall Ready to indicate whether the SBR reconcile is completed successfully #665 #681

Create an overall Ready to indicate whether the SBR reconcile is completed successfully #665 #681

Conversation

qibobo commented Sep 21, 2020 • edited Loading

openshift-ci-robot commented Sep 21, 2020

pedjak left a comment

Choose a reason for hiding this comment

qibobo commented Sep 21, 2020

qibobo commented Sep 22, 2020

isutton commented Sep 24, 2020

qibobo commented Sep 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmacik commented Sep 24, 2020

qibobo commented Sep 25, 2020

pedjak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbose78 commented Sep 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qibobo commented Sep 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedjak commented Oct 6, 2020

qibobo commented Oct 6, 2020

pedjak commented Oct 6, 2020

qibobo commented Oct 12, 2020

Avni-Sharma commented Oct 22, 2020

qibobo commented Oct 23, 2020

qibobo commented Oct 23, 2020

qibobo commented Oct 23, 2020

qibobo commented Oct 28, 2020

Avni-Sharma commented Oct 28, 2020

qibobo commented Nov 3, 2020

Avni-Sharma commented Nov 3, 2020

Avni-Sharma commented Nov 3, 2020

openshift-ci-robot commented Nov 3, 2020

Create an overall `Ready` to indicate whether the SBR reconcile is completed successfully #665 #681

Create an overall `Ready` to indicate whether the SBR reconcile is completed successfully #665 #681

qibobo commented Sep 21, 2020 •

edited

Loading