Fix: ServiceBinding resources are not deployed with odo deploy #6029

valaparthvi · 2022-08-17T11:57:20Z

Signed-off-by: Parthvi Vala pvala@redhat.com

What type of PR is this:
/kind bug

What does this PR do / why we need it:
ServiceBinding resources are now allowed to be deployed on the cluster with odo deploy.

Which issue(s) this PR fixes:

Fixes #5883

PR acceptance criteria:

Unit test
Integration test
Documentation

How to test changes / Special notes to the reviewer:
1.

odo init --devfile go --name my-go-app --starter go-starter

Copy the contents of the devfile.yaml as mentioned in ServiceBinding resources are not deployed with odo deploy #5883
Deploy the bindable kind instance as mentioned in ServiceBinding resources are not deployed with odo deploy #5883
Run odo deploy.

PODMAN_CMD=echo odo deploy

Note: Test case for odo dev without SBO is pending.

netlify · 2022-08-17T11:57:28Z

✅ Deploy Preview for odo-docusaurus-preview canceled.

Name	Link
🔨 Latest commit	`8fdafc5`
🔍 Latest deploy log	https://app.netlify.com/sites/odo-docusaurus-preview/deploys/63063c974ce490000aecc929

odo-robot · 2022-08-17T12:05:04Z

Unit Tests on commit a66b4ea finished successfully.
View logs: TXT HTML

odo-robot · 2022-08-17T12:14:38Z

Validate Tests on commit a66b4ea finished successfully.
View logs: TXT HTML

odo-robot · 2022-08-17T12:15:26Z

Kubernetes Tests on commit a66b4ea finished successfully.
View logs: TXT HTML

odo-robot · 2022-08-17T12:15:41Z

OpenShift Tests on commit a66b4ea finished successfully.
View logs: TXT HTML

odo-robot · 2022-08-17T13:08:19Z

Windows Tests (OCP) on commit ee0dae9 finished with errors.
View logs: TXT HTML

feloy · 2022-08-18T07:36:09Z

The change is small, but complexifies the logic of the code:

For odo dev, we are using PushLinks to create the bindings with or without operators
For odo deploy, we are directly applying the servicebinding resource

I think that making some refactoring around the functions PushLinks, pushDevfileKubernetesComponents, PushKubernetesResources and PushKubernetesResource would be helpful, so the code would be usable for odo dev and odo deploy in a nice way.

valaparthvi · 2022-08-23T10:49:20Z

@feloy There were a few complications with the refactoring when SBO is not installed. Since we're using library, it is required that the deployment in question be present before creating the secret. Because of this, we'll create the link after the component deployment is created if SBO is not present, otherwise it'll be created normally like any other K8s component.

With odo deploy, we don't really need to deal with the Without SBO scenario.

feloy · 2022-08-23T10:53:27Z

@feloy There were a few complications with the refactoring when SBO is not installed. Since we're using library, it is required that the deployment in question be present before creating the secret. Because of this, we'll create the link after the component deployment is created if SBO is not present, otherwise it'll be created normally like any other K8s component.

With odo deploy, we don't really need to deal with the Without SBO scenario.

@valaparthvi Did you try by creating all Kubernetes resources (servicebindings and all other resources) after the deployment?

valaparthvi · 2022-08-23T11:19:49Z

@feloy There were a few complications with the refactoring when SBO is not installed. Since we're using library, it is required that the deployment in question be present before creating the secret. Because of this, we'll create the link after the component deployment is created if SBO is not present, otherwise it'll be created normally like any other K8s component.
With odo deploy, we don't really need to deal with the Without SBO scenario.

@valaparthvi Did you try by creating all Kubernetes resources (servicebindings and all other resources) after the deployment?

Theoretically, I think that would work, but then we also talked about creating all the resources beforehand to avoid restarting the pod and speeding up the process. So I didn't go that way.

feloy · 2022-08-23T12:10:25Z

@feloy There were a few complications with the refactoring when SBO is not installed. Since we're using library, it is required that the deployment in question be present before creating the secret. Because of this, we'll create the link after the component deployment is created if SBO is not present, otherwise it'll be created normally like any other K8s component.
With odo deploy, we don't really need to deal with the Without SBO scenario.

@valaparthvi Did you try by creating all Kubernetes resources (servicebindings and all other resources) after the deployment?

Theoretically, I think that would work, but then we also talked about creating all the resources beforehand to avoid restarting the pod and speeding up the process. So I didn't go that way.

Thinking again about this, it's a matter of milliseconds. We would create Deployment and SBs very quickly, we are not 100% sure in which order they will be handled by controllers on the cluster.

And creating the SB before the Deployment can raise an error for the SBO because the Deployment does not exist when the SB is created, and I'm not sure how the error is recovered (either quickly or not).

valaparthvi · 2022-08-23T12:12:28Z

And creating the SB before the Deployment can raise an error for the SBO because the Deployment does not exist when the SB is created, and I'm not sure how the error is recovered (either quickly or not).

It seems to be working fine so far. Creating SB before Deployment has not been a problem so far.

valaparthvi · 2022-08-23T12:14:19Z

Thinking again about this, it's a matter of milliseconds. We would create Deployment and SBs very quickly, we are not 100% sure in which order they will be handled by controllers on the cluster.

Alright, let's go with creating K8s resources are the core deployment is created, it seems logical.

feloy · 2022-08-23T12:16:53Z

Thinking again about this, it's a matter of milliseconds. We would create Deployment and SBs very quickly, we are not 100% sure in which order they will be handled by controllers on the cluster.

Alright, let's go with creating K8s resources are the core deployment is created, it seems logical.

I'm trying to explore this direction, as it seems the most generic one, so we don't have to handle specific cases at high levels, and we can "hide" the fact that we are using the SBO library at the lowest possible level (in the kclient package).

valaparthvi · 2022-08-23T14:34:02Z

Thinking again about this, it's a matter of milliseconds. We would create Deployment and SBs very quickly, we are not 100% sure in which order they will be handled by controllers on the cluster.

Alright, let's go with creating K8s resources are the core deployment is created, it seems logical.

I'm trying to explore this direction, as it seems the most generic one, so we don't have to handle specific cases at high levels, and we can "hide" the fact that we are using the SBO library at the lowest possible level (in the kclient package).

I find it difficult to imagine handling this case in the kclient package.

feloy · 2022-08-23T14:36:55Z

Thinking again about this, it's a matter of milliseconds. We would create Deployment and SBs very quickly, we are not 100% sure in which order they will be handled by controllers on the cluster.

Alright, let's go with creating K8s resources are the core deployment is created, it seems logical.

I'm trying to explore this direction, as it seems the most generic one, so we don't have to handle specific cases at high levels, and we can "hide" the fact that we are using the SBO library at the lowest possible level (in the kclient package).

I find it difficult to imagine handling this case in the kclient package.

Or in the pkg/service package?

valaparthvi · 2022-08-23T14:49:34Z

Thinking again about this, it's a matter of milliseconds. We would create Deployment and SBs very quickly, we are not 100% sure in which order they will be handled by controllers on the cluster.

Alright, let's go with creating K8s resources are the core deployment is created, it seems logical.

I'm trying to explore this direction, as it seems the most generic one, so we don't have to handle specific cases at high levels, and we can "hide" the fact that we are using the SBO library at the lowest possible level (in the kclient package).

I find it difficult to imagine handling this case in the kclient package.

Or in the pkg/service package?

Yes, I was thinking that too.

Signed-off-by: Parthvi Vala <pvala@redhat.com>

feloy

LGTM

One question though

feloy · 2022-08-24T13:08:27Z

pkg/service/service.go

-	if err != nil {
-		return false, err
+	// If the component is of Kind: ServiceBinding, trying to run in Dev mode and SBO is not installed, run it without operator.
+	if isLinkResource(u.GetKind()) && mode == odolabels.ComponentDevMode && !sboSupported {


Why only for dev mode? Don't we want to use the library for odo deploy?

With odo deploy, I would expect user to know that they're supposed to install ServiceBindingOperator if they intend to use ServiceBinding, just like they would with any other Operator CRD.
Thinking about it this way, I think it makes sense to only check for odo dev, but if we want to provide consistency, having the same behavior for odo deploy makes sense.

OK.

I can see some tests failing about odo logs, probably not related, I'll restart the tests to be sure

No, they're failing locally as well. I am debugging them.

valaparthvi · 2022-08-24T13:58:19Z

The failures are real, I'm debugging them right now.

valaparthvi · 2022-08-24T14:39:48Z

pkg/devfile/adapters/kubernetes/component/adapter.go

@@ -132,6 +127,12 @@ func (a Adapter) Push(parameters adapters.PushParameters, componentStatus *watch
 		return fmt.Errorf("unable to create or update component: %w", err)
 	}

+	// Create all the K8s components defined in the devfile
+	k8sComponents, err := a.pushDevfileKubernetesComponents(labels, odolabels.ComponentDevMode)


Now that we're creating these resources after the core deployment, I think it also make senses to update the owner reference before they're created. WDYT @feloy ?

Yes you are right, you could set the owneerref when you're creating them

valaparthvi · 2022-08-24T15:24:09Z

OpenShift test failure is probably flake or it failed because of resource crunch. The test passes locally.

odo delete command tests
/go/odo_1/tests/integration/cmd_delete_test.go:13
  when deleting a component containing preStop event that is deployed with DEV [BeforeEach]
  /go/odo_1/tests/integration/cmd_delete_test.go:257
    should contain preStop events list
    /go/odo_1/tests/integration/cmd_delete_test.go:270
...
...
...
  Timed out after 180.001s.
  Expected
      <string>:   __
       /  \__     Developing using the nodejs Devfile
       \__/  \    Namespace: cmd-delete-test270afe
       /  \__/    odo version: v3.0.0-beta3
       \__/
      
      â†ª Deploying to the cluster in developer mode
       â€¢  Waiting for Kubernetes resources  ...
       âš   Pod is Pending
       âš   0/3 nodes are available: 3 Insufficient memory.
       âš   0/3 nodes are available: 3 Insufficient memory.
       âš   0/3 nodes are available: 3 Insufficient memory.
      
  to contain substring
      <string>: Press Ctrl+c to exit

feloy · 2022-08-24T15:43:23Z

Insufficient memory

This error is probably due to old pods from old tests running on the cluster. Could you connect to the Openshift cluster and check if there are old tests namespaces ? I have also seen tests pods in the default namespace this week, not sure where they were coming from.

sonarqubecloud · 2022-08-24T16:13:18Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

No Coverage information
0.3% Duplication

feloy · 2022-08-24T16:51:02Z

/lgtm
The code looks good
I'll try to make more tests before to approve

feloy · 2022-08-24T18:34:07Z

/approve

/override ci/prow/unit-and-validate-test
/override ci/prow/v4.10-integration-e2e
Infra errors
Tests pass on IBM Cloud

openshift-ci · 2022-08-24T18:34:17Z

@feloy: Overrode contexts on behalf of feloy: ci/prow/unit-and-validate-test, ci/prow/v4.10-integration-e2e

In response to this:

/approve

/override ci/prow/unit-and-validate-test
/override ci/prow/v4.10-integration-e2e
Infra errors
Tests pass on IBM Cloud

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.

openshift-ci · 2022-08-24T18:34:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: feloy

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

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

…t-developer#6029) * Fix: ServiceBinding resources are not deployed with odo deploy Signed-off-by: Parthvi Vala <pvala@redhat.com> * Fix test failures * Add owner reference before object creation Signed-off-by: Parthvi Vala <pvala@redhat.com>

openshift-ci bot added the kind/bug Categorizes issue or PR as related to a bug. label Aug 17, 2022

openshift-ci bot requested review from dharmit and feloy August 17, 2022 12:01

valaparthvi force-pushed the sb-bug branch from 3836e46 to 9e4e232 Compare August 23, 2022 10:39

valaparthvi force-pushed the sb-bug branch 3 times, most recently from 50dc2e0 to 919c53d Compare August 24, 2022 12:51

Fix: ServiceBinding resources are not deployed with odo deploy

ac1b934

Signed-off-by: Parthvi Vala <pvala@redhat.com>

valaparthvi force-pushed the sb-bug branch from 919c53d to ac1b934 Compare August 24, 2022 12:51

feloy reviewed Aug 24, 2022

View reviewed changes

feloy closed this Aug 24, 2022

feloy reopened this Aug 24, 2022

Fix test failures

c81a2bb

valaparthvi commented Aug 24, 2022

View reviewed changes

Add owner reference before object creation

8fdafc5

feloy closed this Aug 24, 2022

feloy reopened this Aug 24, 2022

openshift-ci bot assigned feloy Aug 24, 2022

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

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Aug 24, 2022

openshift-merge-robot merged commit 6d82a57 into redhat-developer:main Aug 24, 2022

rm3l mentioned this pull request Sep 12, 2022

odo dev not handling changes to the Devfile after removing a Kubernetes resource #6101

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: ServiceBinding resources are not deployed with odo deploy #6029

Fix: ServiceBinding resources are not deployed with odo deploy #6029

valaparthvi commented Aug 17, 2022 •

edited

Loading

netlify bot commented Aug 17, 2022 •

edited

Loading

odo-robot bot commented Aug 17, 2022 •

edited

Loading

odo-robot bot commented Aug 17, 2022 •

edited

Loading

odo-robot bot commented Aug 17, 2022 •

edited

Loading

odo-robot bot commented Aug 17, 2022 •

edited

Loading

odo-robot bot commented Aug 17, 2022 •

edited

Loading

feloy commented Aug 18, 2022

valaparthvi commented Aug 23, 2022

feloy commented Aug 23, 2022

valaparthvi commented Aug 23, 2022

feloy commented Aug 23, 2022 •

edited

Loading

valaparthvi commented Aug 23, 2022

valaparthvi commented Aug 23, 2022

feloy commented Aug 23, 2022 •

edited

Loading

valaparthvi commented Aug 23, 2022

feloy commented Aug 23, 2022

valaparthvi commented Aug 23, 2022

feloy left a comment

feloy Aug 24, 2022

valaparthvi Aug 24, 2022 •

edited

Loading

feloy Aug 24, 2022

valaparthvi Aug 24, 2022

valaparthvi commented Aug 24, 2022

valaparthvi Aug 24, 2022

feloy Aug 24, 2022

valaparthvi commented Aug 24, 2022

feloy commented Aug 24, 2022 •

edited

Loading

sonarqubecloud bot commented Aug 24, 2022

feloy commented Aug 24, 2022

feloy commented Aug 24, 2022

openshift-ci bot commented Aug 24, 2022

openshift-ci bot commented Aug 24, 2022

Fix: ServiceBinding resources are not deployed with odo deploy #6029

Fix: ServiceBinding resources are not deployed with odo deploy #6029

Conversation

valaparthvi commented Aug 17, 2022 • edited Loading

netlify bot commented Aug 17, 2022 • edited Loading

✅ Deploy Preview for odo-docusaurus-preview canceled.

odo-robot bot commented Aug 17, 2022 • edited Loading

odo-robot bot commented Aug 17, 2022 • edited Loading

odo-robot bot commented Aug 17, 2022 • edited Loading

odo-robot bot commented Aug 17, 2022 • edited Loading

odo-robot bot commented Aug 17, 2022 • edited Loading

feloy commented Aug 18, 2022

valaparthvi commented Aug 23, 2022

feloy commented Aug 23, 2022

valaparthvi commented Aug 23, 2022

feloy commented Aug 23, 2022 • edited Loading

valaparthvi commented Aug 23, 2022

valaparthvi commented Aug 23, 2022

feloy commented Aug 23, 2022 • edited Loading

valaparthvi commented Aug 23, 2022

feloy commented Aug 23, 2022

valaparthvi commented Aug 23, 2022

feloy left a comment

Choose a reason for hiding this comment

feloy Aug 24, 2022

Choose a reason for hiding this comment

valaparthvi Aug 24, 2022 • edited Loading

Choose a reason for hiding this comment

feloy Aug 24, 2022

Choose a reason for hiding this comment

valaparthvi Aug 24, 2022

Choose a reason for hiding this comment

valaparthvi commented Aug 24, 2022

valaparthvi Aug 24, 2022

Choose a reason for hiding this comment

feloy Aug 24, 2022

Choose a reason for hiding this comment

valaparthvi commented Aug 24, 2022

feloy commented Aug 24, 2022 • edited Loading

sonarqubecloud bot commented Aug 24, 2022

feloy commented Aug 24, 2022

feloy commented Aug 24, 2022

openshift-ci bot commented Aug 24, 2022

openshift-ci bot commented Aug 24, 2022

valaparthvi commented Aug 17, 2022 •

edited

Loading

netlify bot commented Aug 17, 2022 •

edited

Loading

odo-robot bot commented Aug 17, 2022 •

edited

Loading

odo-robot bot commented Aug 17, 2022 •

edited

Loading

odo-robot bot commented Aug 17, 2022 •

edited

Loading

odo-robot bot commented Aug 17, 2022 •

edited

Loading

odo-robot bot commented Aug 17, 2022 •

edited

Loading

feloy commented Aug 23, 2022 •

edited

Loading

feloy commented Aug 23, 2022 •

edited

Loading

valaparthvi Aug 24, 2022 •

edited

Loading

feloy commented Aug 24, 2022 •

edited

Loading