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

Proposal for Operator Hub integration with odo #2498

Merged
merged 2 commits into from
Jan 17, 2020
Merged

Proposal for Operator Hub integration with odo #2498

merged 2 commits into from
Jan 17, 2020

Conversation

dharmit
Copy link
Member

@dharmit dharmit commented Jan 7, 2020 •
edited

What type of PR is this?

/kind design
/kind feature

What does does this PR do / why we need it:
Proposes a plan for Operator Hub integration with odo

Which issue(s) this PR fixes:
NA

How to test changes / Special notes to the reviewer:
Easier way to navigate the markdown: https://github.com/dharmit/odo/blob/operator-hub-proposal/docs/proposals/operator-hub.md

@openshift-ci-robot openshift-ci-robot added kind/design kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation labels Jan 7, 2020
kadel
kadel requested changes Jan 7, 2020
View reviewed changes
Copy link
Member

@kadel kadel left a comment

Choose a reason for hiding this comment

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

One big part that I'm missing there is actual service creation.
odo service create We don't have to define it right now, but there should be a section with at least TODO note as this is the most important part

docs/proposals/operator-hub.md
- [GitHub issues related to this task](#github-issues-related-to-this-task)

## Which Operators do we support
Operator Hub is not the only place to find/list Operators that could be
Copy link
Member

Choose a reason for hiding this comment

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

The more precise way would be to say that our goal is to use Operator Lifecycle Manager (OLM)

Copy link
Member Author

Choose a reason for hiding this comment

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

OLM itself is an operator that helps manage lifecycle of the Operators. It seemed like going into the implementation detail so I didn't mention much in this doc. I'll add more info about it. 👍

docs/proposals/operator-hub.md
installing “Service Binding Operator” (which helps bind applications with
Operator-backed services) as a one-off command.

As far as creating services from Service Catalog is concerned, we expect the
Copy link
Member

Choose a reason for hiding this comment

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

I would explicitly say that odo will just "instantiate" operands and that it is expected that someone with the cluster-admin will enable (install) operators first.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with @kadel an admin needs to do something outside of odo to make the operator/CRD available.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree with your concern @kadel & @sspeiche, but if you read the paragraph fully, you'll see that your concern is exactly what is proposed. 😉 There are more words because we have #2464 which talks about letting developers do "few" admin tasks but I don't agree with that notion/idea. Let me know if this is still unclear/vague/lengthy and I'll change it.

docs/proposals/operator-hub.md Outdated
of linking services to components without using the Operator.

## List the services/operators
Taking this from Tomas’s comment on the issue. Added column for available
Copy link
Member

Choose a reason for hiding this comment

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

it would be good to include a link to the mentioned comment #2461 (comment)

docs/proposals/operator-hub.md Outdated
$ odo services list
NAME PLANS PROVISIONER
mongodb-enterprise.v1.2.4 prod OperatorHub
mariadb-operator-0.1.3-6 ephemeral,persistent ServiceCatalog
Copy link
Member

Choose a reason for hiding this comment

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

The command lists service instances. Each service instance uses only one plan that was specified during the service creation

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this section is about listing the service instances or the services/operands created using Operators. I pasted the wrong output.

docs/proposals/operator-hub.md Outdated
At the moment, we use `odo service delete <service-name>` to delete the service
deployed in OpenShift cluster. We should be able to delete the service
deployed using Operator Hub with the same command.
Testing on Kubernetes
Copy link
Member

Choose a reason for hiding this comment

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

Is this supposed to be under the Delete the service headline?
It looks like ti might be copy/paste error.

@sspeiche
Copy link
Contributor

sspeiche commented Jan 7, 2020

I'm confused by this proposal, why are we putting admin tasks into the developer client?

Is this proposal linked to #2161 ?

@dharmit
Copy link
Member Author

dharmit commented Jan 8, 2020

One big part that I'm missing there is actual service creation.
odo service create We don't have to define it right now, but there should be a section with at least TODO note as this is the most important part

What an irony on my part! 😞 Thanks for this note. At this moment, we don't have a clear idea about how odo service create command will look for Operator Hub. But it could look something like:

$ odo service create <operator-name> <service-name> --crd <crd-name> -p parameter1=value1 -p parameter2=value2

Adding to TODO. Thanks for catching this!

@dharmit
Copy link
Member Author

dharmit commented Jan 8, 2020 •
edited

I'm confused by this proposal, why are we putting admin tasks into the developer client?

@sspeiche we surely don't want the developers to be concerned with admin tasks. Things like installing Operators and Operator Lifecycle Manager (for Kubernetes since it's pre-installed in OCP/crc) should be admin's concern. Can you let me know which part of the document feels like we're putting admin tasks into developer client?

Is this proposal linked to #2161 ?

Yes. I think we could very well close that issue once we have reached a quorum on this proposal and broken things down into actionable items. WDYT @kadel ?

@kadel
Copy link
Member

kadel commented Jan 16, 2020

/approve
I think that we can merge this. The basic idea is described well.
We should be adding more details as progress.

@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 Jan 16, 2020
@girishramnani
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 Jan 17, 2020
@openshift-bot
Copy link

/retest

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

7 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-merge-robot openshift-merge-robot merged commit 515dbb6 into redhat-developer:master Jan 17, 2020
@dharmit dharmit deleted the operator-hub-proposal branch September 9, 2020 08:51
@rm3l rm3l added estimated-size/L (20-40) Rough sizing for Epics. About 2 sprints of work for a person. and removed size/L labels Jun 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sspeiche sspeiche sspeiche left review comments

@kadel kadel kadel requested changes

@cdrage cdrage Awaiting requested review from cdrage

@mohammedzee1000 mohammedzee1000 Awaiting requested review from mohammedzee1000

Assignees

@kadel kadel

@girishramnani girishramnani

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. estimated-size/L (20-40) Rough sizing for Epics. About 2 sprints of work for a person. kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation 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.

None yet
8 participants
@dharmit @sspeiche @kadel @openshift-ci-robot @girishramnani @openshift-bot @rm3l @openshift-merge-robot