Add support for unlinking component from service #3651

dharmit · 2020-07-28T11:03:41Z

What type of PR is this?
/kind feature

What does this PR do / why we need it:
Adds support for unlinking a component from an Operator backed service with odo unlink command

Which issue(s) this PR fixes:

Fixes #3563

PR acceptance criteria:

Unit test
Integration test
Documentation

How to test changes / Special notes to the reviewer:

Install "Service Binding Operator" on the OpenShift cluster.

Create a nodejs component using https://github.com/akashshinde/node-todo. Use this devfile for things to work fine:

schemaVersion: 2.0.0
metadata:
  name: nodejs
  version: 1.0.0
projects:
  - name: nodejs-starter
    git:
      location: "https://github.com/odo-devfiles/nodejs-ex.git"
components:
  - container:
      name: runtime
      image: registry.access.redhat.com/ubi8/nodejs-12:1-36
      memoryLimit: 1024Mi
      mountSources: true
      endpoints:
        - name: http-8080
          targetPort: 8080
          configuration:
            protocol: tcp
            scheme: http
            type: terminal
commands:
  - exec:
      id: install
      component: runtime
      commandLine: npm install
      workingDir: ${CHE_PROJECTS_ROOT}/nodejs-starter
      group:
        kind: build
        isDefault: true
  - exec:
      id: run
      component: runtime
      commandLine: npm start
      workingDir: ${CHE_PROJECTS_ROOT}/nodejs-starter
      group:
        kind: run
        isDefault: true

Create a URL for it using odo url create. Hit the URL and see that it's showing a loading animation. That's because it needs to be connected to an EtcdCluster.
Create an EtcdCluster using odo service create etcdoperator.v0.9.4-clusterwide --crd EtcdCluster (exact command depends upon the etcd Operator you've installed in the namesapce).
Link the two by doing odo link EtcdCluster/example (follows the odo link <servicetype>/<servicename> syntax since it's perfectly valid to create different kinds of service with same name when working with Operator Hub. Refer Support link command for devfile #2920 (comment).
Check the contents of .odo/env/env.yaml.
Do an odo push to make sure that link is applied. We didn't require odo push when linking with Service Catalog backed services but require it for Operator backed service. Refer: Support link command for devfile #2920 (comment).
Now load the URL created in earlier step again. The app should now be ready to take items. Play around with this ToDo app to make sure things work.
Doing odo unlink EtcdCluster/example for the component would delete "ServiceBindingRequest" that binds the component with the Operator backed service and also delete the Link information from .odo/env/env.yaml.

Example shell flow:

$ git clone https://github.com/akashshinde/node-todo
$ cd node-todo

$ odo create nodejs node-todo
# now replace the devfile or make changes

$ odo push
$ odo url create --now
# hit the URL and check the "Loading" animation

# replace Operator name with what's on your cluster
$ odo  service create etcdoperator.v0.9.4-clusterwide --crd EtcdCluster
$ odo service list

$ odo link EtcdCluster/example
$ cat .odo/env/env.yaml
$ odo push
# hit the URL and play with the todo app

$ odo unlink EtcdCluster/example
$ cat .odo/env/env.yaml
$ odo push
# refresh the URL; you'll see the "Loading" animation again

codecov · 2020-07-28T11:21:08Z

Codecov Report

Merging #3651 into master will decrease coverage by 0.04%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #3651      +/-   ##
==========================================
- Coverage   44.27%   44.22%   -0.05%     
==========================================
  Files         139      139              
  Lines       13405    13420      +15     
==========================================
  Hits         5935     5935              
- Misses       6888     6903      +15     
  Partials      582      582

Impacted Files	Coverage Δ
pkg/envinfo/envinfo.go	`44.44% <0.00%> (-3.66%)`	⬇️
pkg/service/service.go	`36.23% <0.00%> (-0.22%)`	⬇️

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 865cfdd...598fefa. Read the comment docs.

kadel · 2020-07-30T12:44:19Z

/approve

openshift-ci-robot · 2020-07-30T12:44:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kadel

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

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

girishramnani · 2020-07-31T10:50:57Z

pkg/envinfo/envinfo.go

@@ -301,6 +301,17 @@ func (esi *EnvSpecificInfo) DeleteURL(parameter string) error {
 	return esi.writeToFile()
 }

+func (esi *EnvSpecificInfo) DeleteLink(parameter string) error {
+	for i, link := range *esi.componentSettings.Link {


you shouldn't change the Link while you are looping though it. better just do

index := -1 ... if link.Name == parameter { index = i break } ... if index != -1 { s := *esi.componentSettings.Link s = append(s[:index], s[index+1:]...) esi.componentSettings.Link = &s }

Doesn't *esi.componentSettings.Link need to be checked to make sure it's not == nil as well? Looks like a null pointer error waiting to happen.

@cdrage there is a check to validate that a link exists before we reach this function. Here's how it looks when executed:

$ cat .odo/env/env.yaml ComponentSettings: Name: node-todo Namespace: myproject AppName: app RunMode: run $ odo unlink EtcdCluster/example ✗ failed to unlink the service "EtcdCluster/example" since it's not linked with the component "node-todo"

girishramnani · 2020-07-31T10:51:35Z

pkg/odo/cli/component/common_link.go

 		}

 		o.serviceType, o.serviceName, err = svc.IsOperatorServiceNameValid(suppliedName)
 		if err != nil {
 			return err
 		}

+		if o.operationName == "unlink" {


would be better to make this a const string?

girishramnani · 2020-07-31T10:51:55Z

pkg/odo/cli/component/common_link.go

@@ -172,6 +177,19 @@ func (o *commonLinkOptions) validate(wait bool) (err error) {
 			return fmt.Errorf("Couldn't find service named %q. Refer %q to see list of running services", svcFullName, "odo service list")
 		}

+		if o.operationName == "unlink" {


same as above

girishramnani · 2020-07-31T10:52:31Z

pkg/odo/cli/component/common_link.go

@@ -228,6 +246,24 @@ func (o *commonLinkOptions) validate(wait bool) (err error) {

 func (o *commonLinkOptions) run() (err error) {
 	if experimental.IsExperimentalModeEnabled() {
+		if o.operationName == "unlink" {


are we actually using anything from the common_link code? seems quite disjoint to me?

Discussed this over the call so not touching this part.

girishramnani · 2020-07-31T10:53:01Z

pkg/odo/cli/component/common_link.go

+		if o.operationName == "unlink" {
+			sbrName := getSBRName(o.EnvSpecificInfo.GetName(), o.serviceType, o.serviceName)
+			svcFullName := getSvcFullName(sbrKind, sbrName)
+			err = svc.DeleteOperatorService(o.KClient, svcFullName)


should unlink actually delete the service?

girishramnani · 2020-07-31T10:54:04Z

pkg/odo/cli/component/common_link.go

+}
+
+// getSBRName creates a name to be used for creation/deletion of SBR during link/unlink operations
+func getSBRName(componentName, serviceType, serviceName string) string {


I am losing track of such helper functions, are we sure we are reusing them?

As discussed on call, I create a helper only when it's used at least twice.

girishramnani · 2020-07-31T10:55:57Z

tests/integration/operatorhub/cmd_service_test.go

+			stdOut = helper.CmdShouldFail("odo", "link", "EtcdCluster/example")
+			Expect(stdOut).To(ContainSubstring("already linked with the service"))
+
+			stdOut = helper.CmdShouldPass("odo", "unlink", "EtcdCluster/example")


can we check if the secrets were actually removed? or if the SBR was deleted?

I tried to do this by adding oc get sbr in the test. But it takes some time for the SBR to disappear from the output. I'll have to add a few seconds' timeout to be able to validate this.

kadel · 2020-08-03T08:27:11Z

/retest

girishramnani · 2020-08-04T14:19:24Z

unit test seems to be failing on golint

golangci-lint run ./... --timeout 5m
tests/integration/operatorhub/cmd_service_test.go:443:4: ineffectual assignment to `stdOut` (ineffassign)
			stdOut = helper.CmdShouldPass("oc", "get", "sbr")
			^

cdrage · 2020-08-06T19:31:28Z

pkg/envinfo/envinfo.go

@@ -301,6 +301,17 @@ func (esi *EnvSpecificInfo) DeleteURL(parameter string) error {
 	return esi.writeToFile()
 }

+func (esi *EnvSpecificInfo) DeleteLink(parameter string) error {
+	for i, link := range *esi.componentSettings.Link {


Doesn't *esi.componentSettings.Link need to be checked to make sure it's not == nil as well? Looks like a null pointer error waiting to happen.

cdrage · 2020-08-06T19:31:50Z

pkg/odo/cli/component/common_link.go

+				return fmt.Errorf("failed to unlink the service %q since it's not linked with the component %q", svcFullName, componentName)
+			}
+
+			//verify if the underlying service binding request actually exists


Add space to comment & capitalize :)

cdrage · 2020-08-06T19:31:55Z

pkg/odo/cli/component/common_link.go

+				return err
+			}
+			if !sbrExists {
+				// this could have happened if the service binding request was deleted outside odo workflow (eg: oc delete sbr/<sbr-name>)


cdrage · 2020-08-06T19:32:04Z

pkg/odo/cli/component/common_link.go

+
+			linked := isComponentLinked(sbrName, links)
+			if !linked {
+				// user's trying to unlink a service that's not linked with the component


Capitalize :)

cdrage · 2020-08-06T19:32:20Z

pkg/odo/cli/component/common_link.go

+				if err != nil {
+					return fmt.Errorf("component's link with %q has been deleted outside odo; unable to delete odo's state of the link", svcFullName)
+				}
+				return fmt.Errorf("component's link with %q has been deleted outside odo", svcFullName)


components

I think component's is just right here since we're talking about a specific component. ☕ 😉

cdrage · 2020-08-06T19:32:37Z

pkg/odo/cli/component/common_link.go

+	return strings.Join([]string{componentName, strings.ToLower(serviceType), serviceName}, "-")
+}
+
+func isComponentLinked(sbrName string, links []envinfo.EnvInfoLink) bool {


comments maybe?

girishramnani · 2020-08-24T09:58:43Z

/lgtm

dharmit · 2020-08-24T12:44:17Z

Looks like an unrelated failure:

 • Failure [25.563 seconds]
odo sub component command tests
/go/src/github.com/openshift/odo/tests/integration/cmd_cmp_sub_test.go:13
  odo component updating
  /go/src/github.com/openshift/odo/tests/integration/component.go:588
    should be able to create a git component and update it from local to git [It]
    /go/src/github.com/openshift/odo/tests/integration/component.go:597
    No future change is possible.  Bailing out early after 22.594s.
      
    Running odo with args [odo component push --context /tmp/521314230]
    Expected
        <int>: 1
    to match exit code:
        <int>: 0
    /go/src/github.com/openshift/odo/tests/helper/helper_run.go:34

/test v4.5-integration-e2e

girishramnani · 2020-08-25T07:59:55Z

/lgtm

dharmit requested review from mik-dass, girishramnani, cdrage and adisky July 29, 2020 07:06

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Jul 30, 2020

girishramnani reviewed Jul 31, 2020

View reviewed changes

cdrage suggested changes Aug 6, 2020

View reviewed changes

openshift-ci-robot assigned cdrage Aug 6, 2020

Add support for unlinking component from service

164ce4f

openshift-ci-robot assigned girishramnani Aug 24, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Aug 24, 2020

Changes based on PR feedback

598fefa

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Aug 24, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Aug 25, 2020

openshift-merge-robot merged commit 552aad7 into redhat-developer:master Aug 25, 2020

dharmit deleted the fix-3563 branch September 21, 2020 10:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for unlinking component from service #3651

Add support for unlinking component from service #3651

dharmit commented Jul 28, 2020

codecov bot commented Jul 28, 2020 •

edited

Loading

kadel commented Jul 30, 2020

openshift-ci-robot commented Jul 30, 2020

girishramnani Jul 31, 2020

cdrage Aug 6, 2020

dharmit Aug 24, 2020

girishramnani Jul 31, 2020

girishramnani Jul 31, 2020

girishramnani Jul 31, 2020

dharmit Jul 31, 2020

girishramnani Jul 31, 2020

girishramnani Jul 31, 2020

dharmit Jul 31, 2020

girishramnani Jul 31, 2020

dharmit Aug 24, 2020

kadel commented Aug 3, 2020

girishramnani commented Aug 4, 2020

cdrage Aug 6, 2020

cdrage Aug 6, 2020

cdrage Aug 6, 2020

cdrage Aug 6, 2020

cdrage Aug 6, 2020

dharmit Aug 24, 2020

cdrage Aug 6, 2020

girishramnani commented Aug 24, 2020

dharmit commented Aug 24, 2020

girishramnani commented Aug 25, 2020

Add support for unlinking component from service #3651

Add support for unlinking component from service #3651

Conversation

dharmit commented Jul 28, 2020

codecov bot commented Jul 28, 2020 • edited Loading

Codecov Report

kadel commented Jul 30, 2020

openshift-ci-robot commented Jul 30, 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

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

kadel commented Aug 3, 2020

girishramnani commented Aug 4, 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

Choose a reason for hiding this comment

girishramnani commented Aug 24, 2020

dharmit commented Aug 24, 2020

girishramnani commented Aug 25, 2020

codecov bot commented Jul 28, 2020 •

edited

Loading