Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

List only installed Operators; some refactor #4202

Merged
merged 4 commits into from Nov 20, 2020
Merged

List only installed Operators; some refactor #4202

merged 4 commits into from Nov 20, 2020

Conversation

dharmit
Copy link
Member

@dharmit dharmit commented Nov 6, 2020

What type of PR is this?
/kind bug
/kind code-refactoring

What does does this PR do / why we need it:
This PR makes sure that odo lists only those Operators that are successfully installed. This success is determined on the basis of the Phase of Operator. If the phase evaluates to Succeeded, the Operator is considered available to use.

Which issue(s) this PR fixes:

Fixes #4155 & https://bugzilla.redhat.com/show_bug.cgi?id=1889961

PR acceptance criteria:

How to test changes / Special notes to the reviewer:
This can be better tested on a minikube setup.

$ minikube start
$ minikube addons enable olm

# make sure you're working on default namespace
$ kubectl apply -f https://gist.github.com/dharmit/89079ad3daa7df83b92cc39c499f670f/raw/ea8808fcc6ec876b1c7c1f0838295f21324f3baf/operators.yaml

# Check the PHASE column in output. Give it a few seconds or about a minute for etcd Operator to show up
$ kubectl get csv   
NAME                                  DISPLAY                    VERSION             REPLACES                              PHASE
etcdoperator.v0.9.4-clusterwide       etcd                       0.9.4-clusterwide                                         Succeeded
service-binding-operator.v0.1.1-364   Service Binding Operator   0.1.1-364           service-binding-operator.v0.1.1-354   Failed

# odo doesn't show service binding operator since it failed to install
$ odo catalog list services
Operators available in the cluster
NAME                                CRDs
etcdoperator.v0.9.4-clusterwide     EtcdCluster, EtcdBackup, EtcdRestore

@openshift-ci-robot openshift-ci-robot added kind/bug Categorizes issue or PR as related to a bug. kind/code-refactoring labels Nov 6, 2020
@dharmit
Copy link
Member Author

dharmit commented Nov 6, 2020

@mik-dass @girishramnani @adisky @kadel in this PR, I've made some refactor to:

  1. prevent passing around the client and creating it in the business logic when needed
  2. moved the function IsCSVSupported from utils to business logic
  3. filtering the Operators happens in business logic
  4. moved the call to client from CLI package to business logic since we discussed that cli level should not be talking directly with the client.

It's not a full refactor. I've only changed the bits that I needed to for this PR.

@prietyc123
Copy link
Contributor

@dharmit Shouldn't we add tests for the mentioned functionality in description on minikube.

@dharmit
Copy link
Member Author

dharmit commented Nov 9, 2020

Shouldn't we add tests for the mentioned functionality in description on minikube.

@prietyc123 yes we must. I'm inclined towards adding unit tests and not too much of integration tests. For that, I think, testing ListOperatorServices should suffice. It'll require me to mock the call to client.GetClusterServiceVersionList(). How do you suggest I approach this?

mik-dass
mik-dass reviewed Nov 9, 2020
View reviewed changes
pkg/catalog/catalog.go Outdated Show resolved Hide resolved
mik-dass
mik-dass approved these changes Nov 10, 2020
View reviewed changes
Copy link
Contributor

@mik-dass mik-dass left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Nov 10, 2020
@prietyc123
Copy link
Contributor

@dharmit I think some real failure are there on prow https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/openshift_odo/4202/pull-ci-openshift-odo-master-v4.5-integration-e2e/1326130949171187712#1:build-log.txt%3A809

@dharmit
Copy link
Member Author

dharmit commented Nov 11, 2020

@prietyc123 the failure looks weird. It's not able to evaluate the regex that we're using to fetch the name of etcd Operator. On my CRC setup, I faced the same error for master branch!

should be able to create, list and then delete EtcdCluster from its alm example [It]
    /home/dshah/go/src/github.com/openshift/odo/tests/integration/operatorhub/cmd_service_test.go:77

    No future change is possible.  Bailing out early after 0.121s.
    Running odo with args [odo service create /EtcdCluster --project xfvsmdtvyg]
    Expected
        <int>: 1
    to match exit code:
        <int>: 0

It's executing the command odo service create /EtcdCluster --project xfvsmdtvyg when it should be executing odo service create etcdoperator.v0.9.4-clusterwide/EtcdCluster --project xfvsmdtvyg instead. etcdoperator.v0.9.4-clusterwide is missing in the command it's executing in the tests.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Nov 11, 2020
@dharmit
Copy link
Member Author

dharmit commented Nov 11, 2020

@dharmit I think some real failure are there on prow https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/openshift_odo/4202/pull-ci-openshift-odo-master-v4.5-integration-e2e/1326130949171187712#1:build-log.txt%3A809

c4bec97 should hopefully fix the issue. It worked for me on CRC.

@kadel
Copy link
Member

kadel commented Nov 11, 2020

/approve

@openshift-ci-robot
Copy link
Collaborator

[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:

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 openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Nov 11, 2020
@kadel
Copy link
Member

kadel commented Nov 11, 2020

/retest

@dharmit
Copy link
Member Author

dharmit commented Nov 12, 2020

/retest

Seeing unrelated failures:

 [odo]  •  Syncing files to the component  ...
[odo] I1111 14:34:05.870070   14297 component.go:719] Copying files  to pod
[odo] I1111 14:34:05.870097   14297 sync.go:34] CopyFile arguments: localPath /tmp/679316355, dest /tmp/src/679316355, targetPath /tmp/src, copyFiles [], globalExps [/tmp/679316355/.git /tmp/679316355/target /tmp/679316355/.classpath /tmp/679316355/.project /tmp/679316355/.settings /tmp/679316355/.odo/odo-file-index.json]
[odo] I1111 14:34:05.870174   14297 occlient.go:3009] Executing command tar xf - -C /tmp/src
[odo] I1111 14:34:05.870306   14297 sync.go:75] makeTar arguments: srcPath: /tmp/679316355, destPath: /tmp/src/679316355, files: []
[odo] I1111 14:34:05.870328   14297 sync.go:121] recursiveTar arguments: srcBase: /tmp, srcFile: 679316355, destBase: /tmp/src, destFile: 
[odo] I1111 14:34:05.870336   14297 sync.go:127] Corrected destinations: base: /tmp/src file: 
[odo] I1111 14:34:05.870596   14297 sync.go:121] recursiveTar arguments: srcBase: /tmp, srcFile: 679316355/.gitignore, destBase: /tmp/src, destFile: .gitignore
[odo] I1111 14:34:05.870612   14297 sync.go:127] Corrected destinations: base: /tmp/src file: .gitignore
[odo] I1111 14:34:06.507073   14297 sync.go:121] recursiveTar arguments: srcBase: /tmp, srcFile: 679316355/.odo, destBase: /tmp/src, destFile: .odo
[odo] I1111 14:34:06.507108   14297 sync.go:127] Corrected destinations: base: /tmp/src file: .odo
[odo] I1111 14:34:06.507243   14297 sync.go:121] recursiveTar arguments: srcBase: /tmp, srcFile: 679316355/.odo/config.yaml, destBase: /tmp/src, destFile: .odo/config.yaml
[odo] I1111 14:34:06.507256   14297 sync.go:127] Corrected destinations: base: /tmp/src file: .odo/config.yaml
[odo] I1111 14:34:06.507482   14297 sync.go:121] recursiveTar arguments: srcBase: /tmp, srcFile: 679316355/config, destBase: /tmp/src, destFile: config
[odo] I1111 14:34:06.507500   14297 sync.go:127] Corrected destinations: base: /tmp/src file: config
[odo] I1111 14:34:06.507793   14297 sync.go:121] recursiveTar arguments: srcBase: /tmp, srcFile: 679316355/pom.xml, destBase: /tmp/src, destFile: pom.xml
[odo] I1111 14:34:06.507816   14297 sync.go:127] Corrected destinations: base: /tmp/src file: pom.xml
[odo] I1111 14:34:06.508392   14297 sync.go:121] recursiveTar arguments: srcBase: /tmp, srcFile: 679316355/src, destBase: /tmp/src, destFile: src
[odo] I1111 14:34:06.508424   14297 sync.go:127] Corrected destinations: base: /tmp/src file: src
[odo] I1111 14:34:06.508543   14297 sync.go:121] recursiveTar arguments: srcBase: /tmp, srcFile: 679316355/src/main, destBase: /tmp/src, destFile: src/main
[odo] I1111 14:34:06.508560   14297 sync.go:127] Corrected destinations: base: /tmp/src file: src/main
[odo] I1111 14:34:06.508622   14297 sync.go:121] recursiveTar arguments: srcBase: /tmp, srcFile: 679316355/src/main/java, destBase: /tmp/src, destFile: src/main/java
[odo] I1111 14:34:06.508635   14297 sync.go:127] Corrected destinations: base: /tmp/src file: src/main/java
[odo] I1111 14:34:06.508711   14297 sync.go:121] recursiveTar arguments: srcBase: /tmp, srcFile: 679316355/src/main/java/AnotherMessageProducer.java, destBase: /tmp/src, destFile: src/main/java/AnotherMessageProducer.java
[odo] I1111 14:34:06.508747   14297 sync.go:127] Corrected destinations: base: /tmp/src file: src/main/java/AnotherMessageProducer.java
[odo] I1111 14:34:06.509013   14297 sync.go:121] recursiveTar arguments: srcBase: /tmp, srcFile: 679316355/src/main/java/Application.java, destBase: /tmp/src, destFile: src/main/java/Application.java
[odo] I1111 14:34:06.509030   14297 sync.go:127] Corrected destinations: base: /tmp/src file: src/main/java/Application.java
[odo] I1111 14:34:06.509280   14297 sync.go:121] recursiveTar arguments: srcBase: /tmp, srcFile: 679316355/src/main/java/MessageProducer.java, destBase: /tmp/src, destFile: src/main/java/MessageProducer.java
[odo] I1111 14:34:06.509308   14297 sync.go:127] Corrected destinations: base: /tmp/src file: src/main/java/MessageProducer.java
[odo]  ✗  Syncing files to the component [931ms]
[odo]  ✗  Command 'tar xf - -C /tmp/src' in container failed.
[odo] 
[odo]  ✗  stdout: 
[odo] 
[odo]  ✗  stderr: 
[odo] 
[odo]  ✗  err: error while streaming command: command terminated with exit code 137
[odo] 
[odo]  ✗  command terminated with exit code 137
Deleting project: bjaddrjgff
Running odo with args [odo project delete bjaddrjgff -f]
[odo] I1111 14:34:07.503681   14477 util.go:736] HTTPGetRequest: https://raw.githubusercontent.com/openshift/odo/master/build/VERSION
[odo] I1111 14:34:07.503885   14477 util.go:757] Response will be cached in /tmp/odohttpcache for 1h0m0s
[odo] I1111 14:34:07.504130   14477 util.go:770] Cached response used.
[odo]  ⚠  Warning! Projects are deleted from the cluster asynchronously. Odo does its best to delete the project. Due to multi-tenant clusters, the project may still exist on a different node.
[odo] I1111 14:34:07.867946   14477 odo.go:72] Could not get the latest release information in time. Never mind, exiting gracefully :)
[odo]  ✓  Deleted project : bjaddrjgff
Setting current dir to: /go/src/github.com/openshift/odo/tests/integration
Deleting dir: /tmp/679316355
• Failure [94.769 seconds]
odo push command tests
/go/src/github.com/openshift/odo/tests/integration/cmd_push_test.go:15
  when push command is executed
  /go/src/github.com/openshift/odo/tests/integration/cmd_push_test.go:161
    should delete the files from the container if its removed locally [It]
[odo] I1111 14:48:16.166441   20161 merged_client_builder.go:122] Using in-cluster configuration
[odo] I1111 14:48:16.167953   20161 merged_client_builder.go:164] Using in-cluster namespace
[odo]  ✗  The project vzqcqevycl does not exist. Please check the list of projects using `odo project list`

@codecov
Copy link

codecov bot commented Nov 12, 2020 •
edited

Codecov Report

Merging #4202 (c4bec97) into master (730e007) will increase coverage by 0.31%.
The diff coverage is 5.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4202      +/-   ##
==========================================
+ Coverage   42.23%   42.54%   +0.31%     
==========================================
  Files         155      156       +1     
  Lines       13204    13267      +63     
==========================================
+ Hits         5577     5645      +68     
- Misses       7020     7021       +1     
+ Partials      607      601       -6
Impacted Files Coverage Δ
pkg/catalog/catalog.go 48.92% <0.00%> (-1.63%) ⬇️
pkg/odo/cli/service/delete.go 21.81% <0.00%> (ø)
pkg/odo/cli/service/list.go 0.00% <0.00%> (ø)
pkg/odo/util/cmdutils.go 1.04% <ø> (+0.04%) ⬆️
pkg/service/service.go 36.02% <0.00%> (-0.50%) ⬇️
pkg/odo/cli/service/create.go 9.87% <50.00%> (ø)
pkg/devfile/validate/generic/errors.go 9.09% <0.00%> (-10.91%) ⬇️
pkg/kclient/kclient.go 0.00% <0.00%> (-8.46%) ⬇️
pkg/kclient/secrets.go 76.74% <0.00%> (-7.88%) ⬇️
pkg/occlient/templates.go 85.48% <0.00%> (-1.19%) ⬇️
... and 17 more

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 730e007...c4bec97. Read the comment docs.

@mik-dass
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Nov 16, 2020
@dharmit
Copy link
Member Author

dharmit commented Nov 17, 2020 

Running odo with args [odo catalog list services]
[odo] I1116 13:53:02.863168   70535 util.go:736] HTTPGetRequest: https://raw.githubusercontent.com/openshift/odo/master/build/VERSION
[odo] I1116 13:53:02.863448   70535 util.go:757] Response will be cached in /tmp/odohttpcache for 1h0m0s
[odo] I1116 13:53:02.863759   70535 util.go:770] Cached response used.
[odo] I1116 13:53:03.045006   70535 operators.go:24] Fetching list of operators installed in cluster
[odo] no deployable services/operators found

Test fails in the JustBeforeEach section since odo can't find any Operators available in the namespace. Looks flake to me.

/test v4.5-integration-e2e

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

13 similar comments
@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-merge-robot openshift-merge-robot merged commit 6eb43d0 into redhat-developer:master Nov 20, 2020
@dharmit dharmit mentioned this pull request Nov 23, 2020
Merged
4 tasks
openshift-merge-robot pushed a commit that referenced this pull request Nov 24, 2020
@dharmit
Increase timeout for Operator Hub tests (#4248)
85e974f 
This increased timeout makes sure that `odo catalog list services` being
executed in `JustBeforeEach` gets enough time to check for existence of
two Operators that we're installing in CI to perform our checks.

We need to increase this timeout because it's taking a long time for
`odo catalog list services` to find "successfully installed" Operators
in the newly created namespace even if these Operators are installed at
cluster level, i.e., in the `openshift-operators` namespace.

The increase in the time taken by odo to find this info is mainly
attributed to #4202. I am observing
that even `kubectl get csv` is taking more than usual time to list the
Operators or report their phase as "Succeeded" but there's not enough
data to back that up. So, in the interim, this seems like best way to
make sure that CI doesn't keep failing on `JustBeforeEach`.
@dharmit dharmit mentioned this pull request Dec 10, 2020
Closed
@dharmit dharmit deleted the fix-4155 branch January 7, 2021 03:37
@dharmit dharmit mentioned this pull request Sep 7, 2021
Closed
@rm3l rm3l added the area/refactoring Issues or PRs related to code refactoring label Jun 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adisky adisky adisky left review comments

@mik-dass mik-dass mik-dass approved these changes

@girishramnani girishramnani Awaiting requested review from girishramnani

Assignees

@mik-dass mik-dass

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. area/refactoring Issues or PRs related to code refactoring kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. Required by Prow.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

odo should only list the successfully installed Operators
9 participants
@dharmit @prietyc123 @kadel @openshift-ci-robot @mik-dass @openshift-bot @adisky @rm3l @openshift-merge-robot