Fix ServiceBinding removal when service not exist #834

qibobo · 2021-01-07T09:07:58Z

I will add the UT and acceptance tests later

openshift-ci-robot · 2021-01-07T09:08:09Z

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.

codecov · 2021-01-07T09:09:33Z

Codecov Report

Merging #834 (2c48563) into master (ecbab0f) will not change coverage.
The diff coverage is 93.54%.

@@           Coverage Diff           @@
##           master     #834   +/-   ##
=======================================
  Coverage   61.47%   61.47%           
=======================================
  Files          25       25           
  Lines        1918     1918           
=======================================
  Hits         1179     1179           
  Misses        568      568           
  Partials      171      171

Impacted Files	Coverage Δ
controllers/reconciler.go	`75.00% <93.54%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ecbab0f...2c48563. Read the comment docs.

DhritiShikhar · 2021-01-07T11:47:11Z

/ok-to-test

DhritiShikhar · 2021-01-07T11:48:11Z

@qibobo You would have to signoff your commit with git commit --amend --signoff for DCO PR check to pass.

qibobo · 2021-01-13T01:46:52Z

@DhritiShikhar @isutton
Could you help review this PR?

pedjak · 2021-01-13T16:51:10Z

pkg/controller/servicebinding/reconciler.go

+		logger.Info("Executing unbinding steps...")
+		// here just build a servicebinder with necessary fields to do unbind
+		ensureDefaults(sbr.Spec.Application)
+		sb := &serviceBinder{


could we use buildServiceBinder() here? How is this instance different from the existing instance, i.e. why unbind cannot be performed?

@pedjak
In current reconcile process, to do the unbind, it needs to buildServiceContexts and buildBinding, and buildServiceBinder. When the backing service has been deleted and it tries to delete the SBR, buildServiceContexts fail and requeue, then fail and requeue..... The SBR can not be deleted.

The reason here I did not use the buildServiceBinder is that it needs the result buildServiceContexts and buildBinding, which will fall as I mentioned above. And for unbind, I think there is no need to buildServiceContexts and buildBinding because the results of them are not necessary to do the unbind.

@qibobo thanks for explanation. If I understand correctly, the issue here is that buildServiceContexts()is not aware of the fact that service binding is about to be deleted. However, it would be good that the change inside Reconcile() function is as small as possible. Hence, how about this: instead of creating servicebinder manually, how about that we use buildServiceBinder as before, but we creating empty service context if we recognize that deletion is in progress? Something like:

serviceCtxs, err := buildServiceContexts(...) if err != nil { if sbr.GetDeletionTimestamp() != nil { serviceCtxs = serviceContextList{} } else { //handle service not found error if k8serrors.IsNotFound(err) { ... }

I believe this is the only change we need to perform in the reconcile function.

qibobo · 2021-01-14T13:30:08Z

/retest

isutton · 2021-01-14T14:08:44Z

pkg/controller/servicebinding/reconciler.go

-			if err != nil {
-				logger.Error(err, "Failed to update SBR conditions", "sbr", sbr)
+		if sbr.GetDeletionTimestamp() != nil {
+			serviceCtxs = serviceContextList{}


Wouldn't make sense to execute buildServiceContexts only if the GetDeletionTimestamp() != nil check fails?

pedjak · 2021-01-20T18:39:36Z

pkg/controller/servicebinding/reconciler_test.go

@@ -648,3 +649,98 @@ func TestBindTwoSbrsWithSingleApplication(t *testing.T) {
 		require.Equal(t, sbrName2, dep.Spec.Template.Spec.Containers[0].EnvFrom[1].SecretRef.LocalObjectReference.Name)
 	})
 }
+
+// TestApplicationByName tests discovery of application by name
+func TestUnBind(t *testing.T) {


what is scenario we test here, can you reflect with the function name? If the scenario is the same as the new introduced acceptance test, we can then avoid introducing this test fully.

qibobo · 2021-01-28T09:18:40Z

@isutton @pedjak Fixed.

pedjak

lgtm, please squash and restructure a bit the commit message, something like:

Fix ServiceBinding removal when service not exist

qibobo · 2021-01-29T01:50:28Z

@pedjak
done

pedjak · 2021-01-29T07:24:43Z

/retest

pedjak · 2021-01-29T07:25:04Z

/lgtm
/approve

openshift-ci-robot · 2021-01-29T07:25:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pedjak

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~~ [pedjak]

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

Signed-off-by: qibobo <[email protected]>

qibobo · 2021-02-01T08:37:52Z

@pedjak
rebased and need lgtm again.

pedjak · 2021-02-01T08:46:02Z

/lgtm

added naming strategy and deprecated namePrefix and globalNamePrefix Fix acceptance tests fix etcd acceptance test Added flexible strategy naming docs Added doc to describe naming strategy remove service level naming strategy removed NamingStrategy field from service API revert back to default naming strategy Fix regression: previously create test namespace is used across test runs (redhat-developer#850) This fixes regression introduced in PR redhat-developer#848 - we should reuse created test namespace across multiple runs of acceptance tests Signed-off-by: Predrag Knezevic <[email protected]> Use k8s api condition type for status.conditions (redhat-developer#851) Condition type got available in 1.19 API. Hence, we can drop the usage of github.com/openshift/custom-resource-status library. Signed-off-by: Predrag Knezevic <[email protected]> Fix ServiceBinding removal when service not exist (redhat-developer#834) Signed-off-by: qibobo <[email protected]> Refactor Fix acceptance tests and revert back the docs go fmt Fix failures Fix acceptance test

added naming strategy and deprecated namePrefix and globalNamePrefix Fix acceptance tests fix etcd acceptance test Added flexible strategy naming docs Added doc to describe naming strategy remove service level naming strategy removed NamingStrategy field from service API revert back to default naming strategy Fix regression: previously create test namespace is used across test runs (#850) This fixes regression introduced in PR #848 - we should reuse created test namespace across multiple runs of acceptance tests Signed-off-by: Predrag Knezevic <[email protected]> Use k8s api condition type for status.conditions (#851) Condition type got available in 1.19 API. Hence, we can drop the usage of github.com/openshift/custom-resource-status library. Signed-off-by: Predrag Knezevic <[email protected]> Fix ServiceBinding removal when service not exist (#834) Signed-off-by: qibobo <[email protected]> Refactor Fix acceptance tests and revert back the docs go fmt Fix failures Fix acceptance test Signed-off-by: akashshinde <[email protected]>

openshift-ci-robot added the do-not-merge/work-in-progress label Jan 7, 2021

openshift-ci-robot requested review from isutton and pratikjagrut January 7, 2021 09:08

openshift-ci-robot added the needs-ok-to-test label Jan 7, 2021

baijum mentioned this pull request Jan 7, 2021

Update contributing guide with code of coduct #827

Merged

openshift-ci-robot added ok-to-test and removed needs-ok-to-test labels Jan 7, 2021

qibobo force-pushed the dev_qy_cannotdelete branch from 3bb3ede to 7eccb6b Compare January 8, 2021 08:18

qibobo changed the title ~~[WIP] fix issue SBR can not be deleted if backend service has been deleted #832~~ fix issue SBR can not be deleted if backend service has been deleted #832 Jan 8, 2021

openshift-ci-robot removed the do-not-merge/work-in-progress label Jan 8, 2021

qibobo force-pushed the dev_qy_cannotdelete branch 2 times, most recently from fc9167b to 6a9e18e Compare January 10, 2021 08:18

pedjak reviewed Jan 13, 2021

View reviewed changes

qibobo force-pushed the dev_qy_cannotdelete branch from 6a9e18e to 2b47941 Compare January 14, 2021 12:17

qibobo force-pushed the dev_qy_cannotdelete branch from 2b47941 to 62a1752 Compare January 14, 2021 13:37

isutton suggested changes Jan 14, 2021

View reviewed changes

isutton changed the title ~~fix issue SBR can not be deleted if backend service has been deleted #832~~ Fix issue where a ServiceBinding can not be deleted if the backend service has been previously deleted Jan 14, 2021

openshift-ci-robot added the needs-rebase label Jan 18, 2021

pedjak reviewed Jan 20, 2021

View reviewed changes

qibobo force-pushed the dev_qy_cannotdelete branch from 62a1752 to 0554fdb Compare January 28, 2021 03:24

openshift-ci-robot removed the needs-rebase label Jan 28, 2021

qibobo force-pushed the dev_qy_cannotdelete branch from 0554fdb to ae5978a Compare January 28, 2021 03:29

pedjak approved these changes Jan 28, 2021

View reviewed changes

qibobo force-pushed the dev_qy_cannotdelete branch 2 times, most recently from 45d086a to 2cecfb4 Compare January 28, 2021 14:37

pedjak changed the title ~~Fix issue where a ServiceBinding can not be deleted if the backend service has been previously deleted~~ Fix ServiceBinding removal when service not exist Jan 29, 2021

openshift-ci-robot assigned pedjak Jan 29, 2021

openshift-ci-robot added the lgtm label Jan 29, 2021

openshift-ci-robot added the approved label Jan 29, 2021

pedjak added this to the v0.5.0 milestone Jan 29, 2021

Fix ServiceBinding removal when service not exist

2c48563

Signed-off-by: qibobo <[email protected]>

qibobo force-pushed the dev_qy_cannotdelete branch from 2cecfb4 to 2c48563 Compare January 29, 2021 14:18

openshift-ci-robot removed the lgtm label Jan 29, 2021

openshift-ci-robot added the lgtm label Feb 1, 2021

openshift-merge-robot merged commit 0efd994 into redhat-developer:master Feb 1, 2021

Fix ServiceBinding removal when service not exist #834

Fix ServiceBinding removal when service not exist #834

Uh oh!

Conversation

qibobo commented Jan 7, 2021

Uh oh!

openshift-ci-robot commented Jan 7, 2021

Uh oh!

codecov bot commented Jan 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

DhritiShikhar commented Jan 7, 2021

Uh oh!

DhritiShikhar commented Jan 7, 2021

Uh oh!

qibobo commented Jan 13, 2021

Uh oh!

pedjak Jan 13, 2021

Choose a reason for hiding this comment

Uh oh!

qibobo Jan 14, 2021

Choose a reason for hiding this comment

Uh oh!

pedjak Jan 14, 2021

Choose a reason for hiding this comment

Uh oh!

qibobo commented Jan 14, 2021

Uh oh!

isutton Jan 14, 2021

Choose a reason for hiding this comment

Uh oh!

pedjak Jan 20, 2021

Choose a reason for hiding this comment

Uh oh!

qibobo commented Jan 28, 2021

Uh oh!

pedjak left a comment

Choose a reason for hiding this comment

Uh oh!

qibobo commented Jan 29, 2021

Uh oh!

pedjak commented Jan 29, 2021

Uh oh!

pedjak commented Jan 29, 2021

Uh oh!

openshift-ci-robot commented Jan 29, 2021

Uh oh!

qibobo commented Feb 1, 2021

Uh oh!

pedjak commented Feb 1, 2021

Uh oh!

Uh oh!

codecov bot commented Jan 7, 2021 •

edited

Loading