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.

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

Creating csv, crd, and packages for clusterlogging and elasticsearch … #570

Conversation

jcantrill
Copy link
Collaborator

…operators

This PR supercedes #555 and includes the code review items

@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 13, 2018
@alecmerdler
Copy link
Member

/ok-to-test

@openshift-ci-robot openshift-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 13, 2018
@jcantrill jcantrill force-pushed the aggregated_logging_csv_edits branch from 3cf8a09 to 23bc3d6 Compare November 13, 2018 21:41
Copy link
Member

@alecmerdler alecmerdler left a comment

Choose a reason for hiding this comment

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

Found a couple YAML issues.

@ecordell
Copy link
Member

You can run make schema-check locally, which should help you find some of the formatting issues early

@jcantrill jcantrill force-pushed the aggregated_logging_csv_edits branch from 23bc3d6 to 4a463c9 Compare November 14, 2018 13:47
@operator-framework operator-framework deleted a comment from jcantrill Nov 14, 2018
@jcantrill jcantrill force-pushed the aggregated_logging_csv_edits branch from 4a463c9 to efe15c8 Compare November 14, 2018 17:46
@alecmerdler alecmerdler dismissed their stale review November 14, 2018 17:57

Changes addressed

Copy link
Member

@alecmerdler alecmerdler 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. label Nov 14, 2018
@jcantrill jcantrill force-pushed the aggregated_logging_csv_edits branch from efe15c8 to 09c9125 Compare November 14, 2018 20:57
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 14, 2018
@jcantrill jcantrill force-pushed the aggregated_logging_csv_edits branch from 09c9125 to 097f542 Compare November 15, 2018 13:02
Copy link
Member

@alecmerdler alecmerdler left a comment

Choose a reason for hiding this comment

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

Holding until we can confirm both Operators work out of the box from an OLM Subscription.
/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 16, 2018
@richm
Copy link

richm commented Dec 1, 2018

Holding until we can confirm both Operators work out of the box from an OLM Subscription.

How do we do that? If I have a running cluster with olm, something like this?

  • oc edit -n operator-lifecycle-manager cm/rh-operators
  • add the logging package definitions to packages
  • add the logging crds to customResourceDefinitions
  • add the logging csvs to clusterServiceVersions
    ?

@jcantrill jcantrill force-pushed the aggregated_logging_csv_edits branch 2 times, most recently from 76792fb to bc00a98 Compare December 7, 2018 16:44
@jcantrill
Copy link
Collaborator Author

Holding until we can confirm both Operators work out of the box from an OLM Subscription.

How do we do that? If I have a running cluster with olm, something like this?

  • oc edit -n operator-lifecycle-manager cm/rh-operators
  • add the logging package definitions to packages
  • add the logging crds to customResourceDefinitions
  • add the logging csvs to clusterServiceVersions
    ?

Not sure if it is functional yet on the cluster but with @ecordell help I was able to deploy and display in the console:

$ oc scale -nopenshift-cluster-version --replicas=0 rs cluster-version-operator-7c5757d89
$ make release
...
wrote /tmp/tmp.u3y4Vk7V1f/chart/olm/templates/0000_30_06-rh-operators.configmap.yaml
...
$ oc replace --force -f /tmp/tmp.ivzg0towEf/chart/olm/templates/0000_30_06-rh-operators.configmap.yaml

Wait for a few minutes...

$ oc get packagemanifest
NAME                   AGE
svcat                  4h
couchbase-enterprise   4h
etcd                   4h
federationv2           4h
prometheus             4h
descheduler            8m
elasticsearch          8m
dynatrace-monitoring   4h
mongodb-enterprise     4h
amq-streams            4h
clusterLogging         8m

@jcantrill jcantrill force-pushed the aggregated_logging_csv_edits branch 3 times, most recently from 1229d5f to 19f9a82 Compare December 10, 2018 17:56
@jcantrill
Copy link
Collaborator Author

@alecmerdler @ecordell I have successfully deployed my operators though not logging (unrelated to OLM). I'm satisfied this PR provides the opportunity to deploy cluster-logging. It works assuming the image defined in the package manifest can be pulled which I assume will be resolved by the release process. Only things I'm not sure if they are addressable by what changes we make here:

  • elasticsearch-operator should not be visible in the UI; there is no intention to allow users to deploy the EO without CLO
  • Deleting the CLO subscription does not appear to remove the EO clusterserviceversion or affiliated service, deployment, replicaset, pod. Should it?
  • Not sure how the failed jobs relate to these changes.

@alecmerdler
Copy link
Member

  1. Even if we use filter in the UI, the PackageManifest will still be visible using kubectl and creating a Subscription for the elasticsearch-operator will result in OLM deploying the Operator. It sounds like you should make 2 CSVs for the CLO: one that includes the elasticsearch-operator deployment and CRD, and one without.
  2. No, it should not.
  3. IDK either, OpenShift CI is a mystery to me

@ecordell
Copy link
Member

@jcantrill the e2e test is a flake (the installer failed to bring up the cluster). if you rebase, I would expect to see the unit test job pass, though

@jcantrill jcantrill force-pushed the aggregated_logging_csv_edits branch from 19f9a82 to fef0a6a Compare December 10, 2018 22:14
@jcantrill
Copy link
Collaborator Author

jcantrill commented Dec 10, 2018

The most recent push combines the operators into a single deployment for now. Rebased as well. cc @alecmerdler. Tested PR directly and functions as I would expect when the proper images are in the packages

@ewolinetz
Copy link
Contributor

openshift/cluster-logging-operator#55 removes our operator's prior dependency on a signer cert (privileged operation) being provided to us.

@ecordell
Copy link
Member

#617 merged and fixes make schema-check to actually be checking your manifests

@jcantrill jcantrill force-pushed the aggregated_logging_csv_edits branch 2 times, most recently from 8f370ec to f0fcf63 Compare December 13, 2018 14:57
@jcantrill jcantrill force-pushed the aggregated_logging_csv_edits branch from f0fcf63 to e4392b3 Compare December 14, 2018 14:51
@operator-framework operator-framework deleted a comment from jcantrill Dec 14, 2018
@operator-framework operator-framework deleted a comment from jcantrill Dec 14, 2018
@operator-framework operator-framework deleted a comment from ecordell Dec 14, 2018
@operator-framework operator-framework deleted a comment from richm Dec 14, 2018
@operator-framework operator-framework deleted a comment from richm Dec 14, 2018
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 14, 2018
@ecordell
Copy link
Member

/approve
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 14, 2018
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ecordell

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. label Dec 14, 2018
@openshift-merge-robot openshift-merge-robot merged commit c2e20f1 into operator-framework:master Dec 14, 2018
@jcantrill jcantrill deleted the aggregated_logging_csv_edits branch December 14, 2018 18:22
ecordell pushed a commit to ecordell/operator-lifecycle-manager that referenced this pull request Mar 8, 2019
…ogging_csv_edits

Creating csv, crd, and packages for clusterlogging and elasticsearch …
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants