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

Create operatorcondition for operator #1875

Merged
merged 2 commits into from Dec 4, 2020
Merged

Create operatorcondition for operator #1875

merged 2 commits into from Dec 4, 2020

Conversation

awgreene
Copy link
Member

Description of the change:

Motivation for the change:

Reviewer Checklist

  • Implementation matches the proposed design, or proposal is updated to match implementation
  • Sufficient unit test coverage
  • Sufficient end-to-end test coverage
  • Docs updated or added to /docs
  • Commit messages sensible and descriptive

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 20, 2020
@awgreene awgreene changed the title Create operatorcondition for operator WIP: Create operatorcondition for operator Nov 20, 2020
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 20, 2020
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 20, 2020
@awgreene awgreene force-pushed the create-operatorconditions-for-operator branch from 8b82a1a to e3cb5d1 Compare November 20, 2020 15:41
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 20, 2020
@awgreene awgreene force-pushed the create-operatorconditions-for-operator branch from e3cb5d1 to 3ba448c Compare November 20, 2020 16:04
njhale
njhale reviewed Nov 20, 2020
View reviewed changes
Copy link
Member

@njhale njhale left a comment

Choose a reason for hiding this comment

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

Nice work @awgreene!

Aside from some more tests, I do want us to think about what this looks like if we implement OperatorCondition generation as a net-new controller (in the style of the Operator API's controllers). I think we'll want to add features as new controllers when we can, so we can reduce the amount we need to rewrite as we move away from OLM's original controllers.

pkg/controller/registry/resolver/rbac.go Outdated Show resolved Hide resolved
pkg/controller/registry/resolver/steps.go Outdated Show resolved Hide resolved
pkg/controller/registry/resolver/rbac.go Outdated Show resolved Hide resolved
pkg/controller/registry/resolver/rbac.go Outdated Show resolved Hide resolved
pkg/controller/registry/resolver/steps.go Outdated Show resolved Hide resolved
pkg/lib/ownerutil/util.go Outdated
@@ -406,6 +406,12 @@ func InferGroupVersionKind(obj runtime.Object) error {
Version: apiextensionsv1beta1.SchemeGroupVersion.Version,
Kind: "CustomResourceDefinition",
})
case *operatorsv1.OperatorCondition:
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it's possible to get the GVK from a properly initialized runtime.Scheme.

pkg/lib/ownerutil/util.go Outdated Show resolved Hide resolved
test/e2e/subscription_e2e_test.go Outdated Show resolved Hide resolved
@awgreene
Copy link
Member Author

awgreene commented Nov 20, 2020 •
edited

Thanks for the detailed review @njhale

Aside from some more tests, I do want us to think about what this looks like if we implement OperatorCondition generation as a net-new controller (in the style of the Operator API's controllers). I think we'll want to add features as new controllers when we can, so we can reduce the amount we need to rewrite as we move away from OLM's original controllers.

Are you proposing that instead of generating the OperatorConditions and RBAC as part of the installplan we create a new controller that generates these resources itself when a CSV is created?

@njhale
Copy link
Member

njhale commented Nov 20, 2020 •
edited

@awgreene

Are you proposing that instead of generating the OperatorConditions and RBAC as part of the installplan we create a new controller that generates these resources itself when a CSV is created?

Exactly.

@awgreene awgreene force-pushed the create-operatorconditions-for-operator branch 2 times, most recently from 359d1ca to 6a2f13a Compare November 25, 2020 14:52
gallettilance
gallettilance reviewed Nov 25, 2020
View reviewed changes
Copy link
Member

@gallettilance gallettilance left a comment

Choose a reason for hiding this comment

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

looks great!

pkg/controller/operators/operatocondition_controller.go Outdated Show resolved Hide resolved
pkg/controller/operators/operatocondition_controller.go Outdated Show resolved Hide resolved
pkg/controller/operators/operatocondition_controller.go Outdated Show resolved Hide resolved
pkg/controller/operators/operatocondition_controller.go Outdated Show resolved Hide resolved
pkg/lib/ownerutil/util.go Outdated Show resolved Hide resolved
@awgreene awgreene force-pushed the create-operatorconditions-for-operator branch 4 times, most recently from 939f577 to a0c8076 Compare November 30, 2020 14:24
@awgreene awgreene force-pushed the create-operatorconditions-for-operator branch from a0c8076 to cbd0fd0 Compare November 30, 2020 20:34
benluddy
benluddy reviewed Nov 30, 2020
View reviewed changes
Copy link
Contributor

@benluddy benluddy left a comment

Choose a reason for hiding this comment

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

Nice work!

pkg/controller/operators/operatocondition_controller.go Outdated Show resolved Hide resolved
pkg/controller/operators/operatocondition_controller.go Outdated Show resolved Hide resolved
pkg/controller/operators/operatoconditiongenerator_controller.go Outdated Show resolved Hide resolved
pkg/controller/operators/operatoconditiongenerator_controller.go Outdated Show resolved Hide resolved
pkg/controller/operators/operatoconditiongenerator_controller.go Outdated Show resolved Hide resolved
pkg/controller/operators/operatocondition_controller.go Outdated Show resolved Hide resolved
pkg/controller/operators/operatocondition_controller.go Outdated Show resolved Hide resolved
pkg/controller/operators/operatocondition_controller.go Outdated Show resolved Hide resolved
pkg/controller/operators/operatoconditiongenerator_controller.go Outdated Show resolved Hide resolved
pkg/controller/operators/operatocondition_controller.go Outdated Show resolved Hide resolved
@awgreene awgreene force-pushed the create-operatorconditions-for-operator branch 3 times, most recently from ad97cf2 to 303f569 Compare December 1, 2020 15:13
@awgreene awgreene changed the title WIP: Create operatorcondition for operator Create operatorcondition for operator Dec 1, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 1, 2020
ecordell
ecordell reviewed Dec 1, 2020
View reviewed changes
pkg/controller/operators/operatorcondition_controller.go Outdated
func (r *OperatorConditionReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
// Set up a convenient log object so we don't have to type request over and over again
log := r.log.WithValues("request", req)
log.V(1).Info("reconciling operatorcondition")
Copy link
Member

Choose a reason for hiding this comment

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

probably don't want 1 log level for this (same comment for the rest of the level-1 logs in this file)

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'll bump it up a few levels.

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 have updated the logging levels in the latest version of this PR.

pkg/controller/operators/operatorcondition_controller.go Outdated Show resolved Hide resolved
pkg/controller/operators/operatorcondition_controller.go Outdated
return err
}
existingRoleBinding := &rbacv1.RoleBinding{}
err := r.Client.Get(context.TODO(), client.ObjectKey{Name: roleBinding.GetName(), Namespace: roleBinding.GetNamespace()}, existingRoleBinding)
Copy link
Member

Choose a reason for hiding this comment

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

The gets should hit the cache when possible instead of using the client

Copy link
Member Author

Choose a reason for hiding this comment

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

Controller runtime uses an inline cache, will likely need to enable it.

pkg/controller/operators/operatorcondition_controller.go Show resolved Hide resolved
pkg/controller/operators/operatorcondition_controller.go Outdated Show resolved Hide resolved
pkg/controller/operators/operatorconditiongenerator_controller.go Outdated Show resolved Hide resolved
pkg/controller/operators/operatorconditiongenerator_controller.go Outdated Show resolved Hide resolved
pkg/controller/operators/operatorconditiongenerator_controller.go Outdated Show resolved Hide resolved
pkg/controller/operators/operatorcondition_controller.go Show resolved Hide resolved
manifests/0000_50_olm_00-operatorconditions.crd.yaml Show resolved Hide resolved
@awgreene awgreene force-pushed the create-operatorconditions-for-operator branch 2 times, most recently from a8cf38d to d439679 Compare December 1, 2020 22:07
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 2, 2020
@awgreene
Copy link
Member Author

awgreene commented Dec 2, 2020

/retest

1 similar comment
@awgreene
Copy link
Member Author

awgreene commented Dec 2, 2020

/retest

@awgreene
Copy link
Member Author

awgreene commented Dec 2, 2020

/retest

Infra Issues...

dinhxuanvu
dinhxuanvu approved these changes Dec 2, 2020
View reviewed changes
Copy link
Member

@dinhxuanvu dinhxuanvu left a comment

Choose a reason for hiding this comment

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

/lgtm

@awgreene
Copy link
Member Author

awgreene commented Dec 2, 2020

/retest

3 similar comments
@awgreene
Copy link
Member Author

awgreene commented Dec 2, 2020

/retest

@awgreene
Copy link
Member Author

awgreene commented Dec 2, 2020

/retest

@awgreene
Copy link
Member Author

awgreene commented Dec 2, 2020

/retest

@awgreene
Copy link
Member Author

awgreene commented Dec 2, 2020

infra issues
/retest

@awgreene
Copy link
Member Author

awgreene commented Dec 2, 2020

/retest

2 similar comments
@awgreene
Copy link
Member Author

awgreene commented Dec 3, 2020

/retest

@awgreene
Copy link
Member Author

awgreene commented Dec 3, 2020

/retest

@openshift-ci-robot openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed lgtm Indicates that a PR is ready to be merged. labels Dec 3, 2020
@dinhxuanvu dinhxuanvu force-pushed the create-operatorconditions-for-operator branch from 98421bb to f571cd8 Compare December 3, 2020 06:14
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 3, 2020
dinhxuanvu
dinhxuanvu approved these changes Dec 3, 2020
View reviewed changes
Copy link
Member

@dinhxuanvu dinhxuanvu 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 Dec 3, 2020
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awgreene, dinhxuanvu

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

@jianzhangbjz
Copy link
Contributor

/retest

1 similar comment
@awgreene
Copy link
Member Author

awgreene commented Dec 3, 2020

/retest

@awgreene
Copy link
Member Author

awgreene commented Dec 3, 2020

Failed to acquire leases.

/retest

@awgreene
Copy link
Member Author

awgreene commented Dec 3, 2020

/retest

@openshift-bot
Copy link
Contributor

/retest

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

@awgreene
Copy link
Member Author

awgreene commented Dec 4, 2020

/retest

@openshift-merge-robot openshift-merge-robot merged commit 271771d into operator-framework:master Dec 4, 2020
@jianzhangbjz
Copy link
Contributor

/QE-approved

@erkanerol erkanerol mentioned this pull request Nov 9, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ecordell ecordell ecordell left review comments

@benluddy benluddy benluddy left review comments

@njhale njhale njhale left review comments

@kevinrizza kevinrizza kevinrizza left review comments

@gallettilance gallettilance gallettilance left review comments

@dinhxuanvu dinhxuanvu dinhxuanvu approved these changes

@ankitathomas ankitathomas Awaiting requested review from ankitathomas

Assignees

@kevinrizza kevinrizza

@dinhxuanvu dinhxuanvu

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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet