Skip to content

docs/dev/operators: Reorder assigned runlevels (machine API to 30, etc.) #100

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
Merged
openshift-merge-robot merged 1 commit into from
Feb 8, 2019
Merged

docs/dev/operators: Reorder assigned runlevels (machine API to 30, etc.) #100

openshift-merge-robot merged 1 commit into from
Feb 8, 2019

Conversation

@wking
Copy link
Member

@wking wking commented Jan 24, 2019

Catching up with openshift/cluster-openshift-apiserver-operator#121, openshift/machine-config-operator#331, and operator-framework/operator-lifecycle-manager#678. I'm not personally clear on the reasoning; I'd have expected the OLM entries to belong at the end of the queue, since they seem less central than the OpenShift core operators.

The Machine API and autoapprover entries had been added to the end of the queue in dcab5e4 (#64), but I don't see any discussion of that placement there.

I'd also understood the 0000_ prefix to be a special case for where ordering is critical, with most operators landing without specific ordering after the key components had been addressed. I dunno how that squares with:

$ oc adm release extract --from=quay.io/openshift-release-dev/ocp-release:4.0.0-0.1 --to=/tmp/manifests
Extracted release payload from digest sha256:66cee7428ba0d3cb983bd2a437e576b2289e7fd5abafa70256200a5408b26644 created at 2019-01-15T04:04:22Z
$ ls /tmp/manifests | wc -l
191
$ ls /tmp/manifests | grep -v '^0000_'
image-references
release-metadata

it looks like all of our operators are setting the 0000_ prefix.

@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 24, 2019
@wking wking force-pushed the reorder-operators branch 3 times, most recently from 5e25ae3 to c02052c Compare January 24, 2019 11:09
@wking
docs/dev/operators: Reorder assigned runlevels (machine API to 30, etc.)
002a50c 
Catching up with openshift/cluster-machine-approver@67d2bbcb (Change
run level to 09, 2019-01-21, openshift/cluster-machine-approver#12),
openshift/cluster-openshift-apiserver-operator@5a15f36e (move
openshift apiserver later in CVO payload, 2019-01-21,
openshift/cluster-openshift-apiserver-operator#121) moving to 61,
openshift/cluster-openshift-controller-manager-operator@b823879a (move
openshift controller manager later in CVO payload, 2019-01-21,
openshift/cluster-openshift-controller-manager-operator#61) moving to
63, openshift/machine-config-operator@4dd8b917 (mco/install: update
mco run level to 30, 2019-01-21,
openshift/machine-config-operator#331), and
operator-framework/operator-lifecycle-manager@f0e86b8c (chore(deploy):
change 30 prefix to 50, 2019-01-21,
operator-framework/operator-lifecycle-manager#678).  I'm not
personally clear on the reasoning; I'd have expected the OLM entries
to belong at the end of the queue, since they seem less central than
the OpenShift core operators.  In [1], David Eads says that nothing
earlier than 70 should require OpenShift APIs.

The Machine API and autoapprover entries had been added to the end of
the queue in dcab5e4 (payload: Ignore files without yaml, yml, or
json extensions, 2018-12-01, openshift#64), but I don't see any discussion of
that placement there.

I'd also understood the 0000_ prefix to be a special case for where
ordering is critical, with most operators landing without specific
ordering after the key components had been addressed.  I dunno how
that squares with:

  $ oc adm release extract --from=quay.io/openshift-release-dev/ocp-release:4.0.0-0.1 --to=/tmp/manifests
  Extracted release payload from digest sha256:66cee7428ba0d3cb983bd2a437e576b2289e7fd5abafa70256200a5408b26644 created at 2019-01-15T04:04:22Z
  $ ls /tmp/manifests | wc -l
  191
  $ ls /tmp/manifests | grep -v '^0000_'
  image-references
  release-metadata

it looks like *all* of our operators are setting the 0000_ prefix.

[1]: openshift/cluster-openshift-controller-manager-operator#61 (comment)
@wking wking force-pushed the reorder-operators branch from c02052c to 002a50c Compare January 24, 2019 11:17
@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 24, 2019
@smarterclayton
Copy link
Contributor

smarterclayton commented Jan 25, 2019

Everyone should set prefix. That allows the 90’s to be loosely coupled (service monitors)

@wking
Copy link
Member Author

wking commented Jan 25, 2019

Wouldn't all unprefixed operators be loosely coupled? Why make monitoring especially late?

@smarterclayton
Copy link
Contributor

smarterclayton commented Jan 27, 2019

/test integration

@smarterclayton
Copy link
Contributor

smarterclayton commented Jan 27, 2019

the CVO has a linear flow. For people to have service monitor means they have to have steps that are after the service monitor CRD is created, which is created by CMO. But kube-apiserver wants to declare service monitors. So kube-apiserver needs to declare something that gets run after the CMO is known to have run.

Everyone who doesn't specify anything gets put under 0000_70_x. So you can not specify anything, but you still end up prefixed.

@wking
Copy link
Member Author

wking commented Jan 27, 2019

the CVO has a linear flow.

Until #88, right?

For people to have service monitor means they have to have steps that are after the service monitor CRD is created, which is created by CMO.

So we should have an explicit entry for the CMO?

@openshift-merge-robot
Copy link
Contributor

openshift-merge-robot commented Feb 6, 2019

/retest

1 similar comment
@openshift-merge-robot
Copy link
Contributor

openshift-merge-robot commented Feb 6, 2019

/retest

@smarterclayton
Copy link
Contributor

smarterclayton commented Feb 7, 2019

See my comments in #116

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 7, 2019
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Feb 7, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: smarterclayton, wking

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [smarterclayton,wking]

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

@smarterclayton
Copy link
Contributor

smarterclayton commented Feb 8, 2019

/retest

@openshift-merge-robot openshift-merge-robot merged commit 481aef6 into openshift:master Feb 8, 2019
@wking wking deleted the reorder-operators branch April 3, 2019 07:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@abhinavdahiya abhinavdahiya Awaiting requested review from abhinavdahiya

@crawford crawford Awaiting requested review from crawford

Assignees

@smarterclayton smarterclayton

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/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@wking @smarterclayton @openshift-merge-robot @openshift-ci-robot