Add odo push to link/unlink tests #3837

dharmit · 2020-08-26T09:20:36Z

What type of PR is this?
I don't know. It's pretty sure not a bug! This PR is mainly opened to fix something that's working just fine on developer/QE systems but failing in CI.

What does does this PR do / why we need it:
This is being done for two reasons:

CI is likely slow to link/unlink as observed in
Consecutive service link/unlink doesn't work properly on CI #3834
link/unlink require "odo push" for underlying deployment

Which issue(s) this PR fixes:

Fixes #3834

PR acceptance criteria:

Unit test
Integration test
Documentation
I have read the test guidelines

How to test changes / Special notes to the reviewer:

This is being done for two reasons: 1) CI is likely slow to link/unlink as observed in #3834 2) link/unlink require "odo push" for underlying deployment

prietyc123 · 2020-08-26T09:40:36Z

tests/integration/operatorhub/cmd_service_test.go

@@ -433,11 +433,13 @@ spec:

 			stdOut := helper.CmdShouldPass("odo", "link", "EtcdCluster/example")
 			Expect(stdOut).To(ContainSubstring("Successfully created link between component"))
+			helper.CmdShouldPass("odo", "push")


From the logs https://prow.ci.openshift.org/view/gs/origin-ci-test/logs/periodic-ci-openshift-odo-master-v4.2-integration-e2e-periodic/1298409952288706560#1:build-log.txt%3A1119 I didn't see it failing over here. Things are breaking with the lines https://github.com/openshift/odo/pull/3837/files#diff-49f09fb5341717455c0421b3c592e685R449 . So I am not sure this will fix the exact issue.

You're right. But adding odo push at the place where you're suggesting it would be just a hack to make sure that we introduce some timeout between the two calls. Adding it where I have proposed in the PR right now makes sure that we're following the flow that a user would when using odo link/odo unlink commands and it still gives us the timeout we need to avoid the race condition from arising.

TBH I don't understand why we are bandaging it with odo push when its not necessary. I would say lets not complicate the test scenario, consider a simple example:

> odo link EtcdCluster/example > odo unlink EtcdCluster/example > odo link EtcdCluster/example //This is failing > odo push

Maybe your current changes will hide the issue but it won't fix the real issue. In future I am pretty sure somebody will hit the issue with the same scenario. Also frankly speaking as a end user I won't rely much on .odo/env/env.yaml for services state failure.

prietyc123 · 2020-08-26T10:43:21Z

tests/integration/operatorhub/cmd_service_test.go

@@ -433,11 +433,13 @@ spec:

 			stdOut := helper.CmdShouldPass("odo", "link", "EtcdCluster/example")
 			Expect(stdOut).To(ContainSubstring("Successfully created link between component"))
+			helper.CmdShouldPass("odo", "push")


TBH I don't understand why we are bandaging it with odo push when its not necessary. I would say lets not complicate the test scenario, consider a simple example:

> odo link EtcdCluster/example > odo unlink EtcdCluster/example > odo link EtcdCluster/example //This is failing > odo push

Maybe your current changes will hide the issue but it won't fix the real issue. In future I am pretty sure somebody will hit the issue with the same scenario. Also frankly speaking as a end user I won't rely much on .odo/env/env.yaml for services state failure.

dharmit · 2020-08-26T12:36:23Z

@prietyc123

TBH I don't understand why we are bandaging it with odo push when its not necessary.

I already explained why we're adding this in #3834 (comment).

Maybe your current changes will hide the issue but it won't fix the real issue.

The real issue has been pointed to by @amitkrout in #3834 (comment). It's a race condition showing up on CI. Maybe you can fix that and I'll close this PR in that case.

Also frankly speaking as a end user I won't rely much on .odo/env/env.yaml for services state failure.

We're relying on this file for information about the link. Not for the state of the service.

dharmit · 2020-08-26T12:46:22Z

Don't really understand why oc get csv would fail. Maybe a flake?
/test v4.5-integration-e2e

++ oc adm policy add-role-to-user edit developer
Warning: User 'developer' not found
clusterrole.rbac.authorization.k8s.io/edit added: "developer"
++ sh ./scripts/setup-operators.sh
+ CI_OPERATOR_HUB_PROJECT=ci-operator-hub-project
+ count=0
+ '[' 0 -lt 5 ']'
+ oc get csv -n openshift-operators
+ grep mongo
I0826 10:01:42.215739      87 request.go:621] Throttling request took 1.134171471s, request: GET:https://api.ci-op-17f0tnww-2f74e.origin-ci-int-aws.dev.rhcloud.com:6443/apis/batch/v1?timeout=32s
error: the server doesn't have a resource type "csv"
+ install_mongo_operator
+ oc create -f -

amitkrout · 2020-08-27T06:49:17Z

The real issue has been pointed to by @amitkrout in #3834 (comment). It's a race condition showing up on CI. Maybe you can fix that and I'll close this PR in that case.

The race condition i want to describe here is

For command odo link EtcdCluster/example i have not much idea what process it runs behind the scene but as a user i would expect the next command odo unlink EtcdCluster/example should not fail due to some pending status of the previous command. May be when link or unlink command complete but it still wait some process to complete under the hood.

@prietyc123 Last time i was using the the complete spec, instead you can scope out few things from there and focus the failure steps. For example you can please try these simple scenario locally.

odo unlink EtcdCluster/example
odo link EtcdCluster/example
odo link EtcdCluster/example

dharmit · 2020-08-27T08:22:08Z

For command odo link EtcdCluster/example i have not much idea what process it runs behind the scene

It creates a Service Bbinding Request. "Service Binding Request" (SBR) is an API provided by Service Binding Operator (SBO). Due to certain limitations, we had to introduce odo push as a part of the workflow for the linking to take place well. More info can be found in #2920.

May be when link or unlink command complete but it still wait some process to complete under the hood.

On the cluster side, link/unlink are incomplete unless odo push was executed. When working with Operator Hub, odo makes sure that there's clear messaging about requirement to do odo push:

$ odo link EtcdCluster/example
 ✓  Successfully created link between component "node-todo" and service "EtcdCluster/example"

To apply the link, please use `odo push`

$ odo unlink EtcdCluster/example                                                                                                                                                              
 ✓  Successfully unlinked component "node-todo" from service "EtcdCluster/example"                                                                                                            
                                                                                                                                                                                              
To apply the changes, please use `odo push`

amitkrout · 2020-08-27T11:59:41Z

/approve

openshift-ci-robot · 2020-08-27T11:59:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amitkrout

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:

~~tests/OWNERS~~ [amitkrout]

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

prietyc123 · 2020-08-27T12:00:41Z

Thanks for detailed information. Looks good to me.

/lgtm

openshift-bot · 2020-08-27T12:21:30Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-27T12:34:29Z

/retest

Please review the full test history for this PR and help us cut down flakes.

codecov · 2020-08-27T15:58:16Z

Codecov Report

Merging #3837 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3837   +/-   ##
=======================================
  Coverage   44.22%   44.22%           
=======================================
  Files         139      139           
  Lines       13420    13420           
=======================================
  Hits         5935     5935           
  Misses       6903     6903           
  Partials      582      582

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 552aad7...2323ab4. Read the comment docs.

girishramnani · 2020-08-27T20:02:23Z

/retest

openshift-bot · 2020-08-28T00:05:06Z

/retest

Please review the full test history for this PR and help us cut down flakes.

Add odo push to link/unlink tests

2323ab4

This is being done for two reasons: 1) CI is likely slow to link/unlink as observed in #3834 2) link/unlink require "odo push" for underlying deployment

openshift-ci-robot requested review from mik-dass and mohammedzee1000 August 26, 2020 09:20

prietyc123 suggested changes Aug 26, 2020

View reviewed changes

openshift-ci-robot assigned prietyc123 Aug 26, 2020

prietyc123 suggested changes Aug 26, 2020

View reviewed changes

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

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

openshift-merge-robot merged commit de77f8b into redhat-developer:master Aug 28, 2020

dharmit deleted the fix-3834 branch April 18, 2022 08:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add odo push to link/unlink tests #3837

Add odo push to link/unlink tests #3837

dharmit commented Aug 26, 2020

prietyc123 Aug 26, 2020

dharmit Aug 26, 2020

prietyc123 Aug 26, 2020

prietyc123 Aug 26, 2020

dharmit commented Aug 26, 2020

dharmit commented Aug 26, 2020

amitkrout commented Aug 27, 2020

dharmit commented Aug 27, 2020

amitkrout commented Aug 27, 2020

openshift-ci-robot commented Aug 27, 2020

prietyc123 commented Aug 27, 2020

openshift-bot commented Aug 27, 2020

openshift-bot commented Aug 27, 2020

codecov bot commented Aug 27, 2020 •

edited

Loading

girishramnani commented Aug 27, 2020

openshift-bot commented Aug 28, 2020

Add odo push to link/unlink tests #3837

Add odo push to link/unlink tests #3837

Conversation

dharmit commented Aug 26, 2020

prietyc123 Aug 26, 2020

Choose a reason for hiding this comment

dharmit Aug 26, 2020

Choose a reason for hiding this comment

prietyc123 Aug 26, 2020

Choose a reason for hiding this comment

prietyc123 Aug 26, 2020

Choose a reason for hiding this comment

dharmit commented Aug 26, 2020

dharmit commented Aug 26, 2020

amitkrout commented Aug 27, 2020

dharmit commented Aug 27, 2020

amitkrout commented Aug 27, 2020

openshift-ci-robot commented Aug 27, 2020

prietyc123 commented Aug 27, 2020

openshift-bot commented Aug 27, 2020

openshift-bot commented Aug 27, 2020

codecov bot commented Aug 27, 2020 • edited Loading

Codecov Report

girishramnani commented Aug 27, 2020

openshift-bot commented Aug 28, 2020

codecov bot commented Aug 27, 2020 •

edited

Loading