Bug 1740332: OLM should resume operator install #1006

tkashem · 2019-08-22T18:40:36Z

OLM should automatically resume operator install when a user grants
proper permission(s).

Currently, a user has to manually delete the subscription and recreate
it in order to trigger a reinstall of the operator.

Do the following to trigger reinstall:

Add a new field 'AttenuatedServiceAccountRef' to status of
InstallPlan. We use this to refer to the ServiceAccount that will be
used to do attenuated scoped install of the operator.
Watch on Role(Binding), ServiceAccount resources. When these RBAC
resources are added/updated find the target InstallPlan object.
Update the status.phase of the InstallPlan object to Installing.
This will trigger a sync of the InstallPlan.

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1740332
Jira: https://jira.coreos.com/browse/OLM-1244

tkashem · 2019-08-22T20:06:50Z

/test e2e-aws-console-olm

jpeeler · 2019-08-22T20:49:08Z

I don't think approach will work (well) since ~~syncObject will constantly attempt reinstalling without any change in the cluster~~ edit: sorry, missed the kind switch. Since install plans aren't reconciled constantly by design, I'm thinking the best way to proceed is to watch newly created roles and see if they apply to existing install plans (or perhaps check the status of failed install plans?)

openshift-ci-robot · 2019-08-23T20:39:27Z

@tkashem: This pull request references Bugzilla bug 1740332, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Bug 1740332: OLM should resume operator install

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tkashem · 2019-08-26T12:50:00Z

/retest

ecordell

/lgtm

had a couple of nits but I don't think they should block

ecordell · 2019-08-26T12:59:06Z

pkg/lib/scoped/attenuator.go

-		return
-	}
-
+func (s *ClientAttenuator) AttenuateClientWithServiceAccount(reference *corev1.ObjectReference) (kubeclient operatorclient.ClientInterface, crclient versioned.Interface, err error) {


nit: the comment has a different name than the method

ecordell · 2019-08-26T12:59:25Z

pkg/controller/operators/catalog/operator.go

@@ -1075,6 +1097,8 @@ var _ installPlanTransitioner = &Operator{}
 func transitionInstallPlanState(log *logrus.Logger, transitioner installPlanTransitioner, in v1alpha1.InstallPlan, now metav1.Time) (*v1alpha1.InstallPlan, error) {
 	out := in.DeepCopy()

+


nit: extra spaces

ecordell · 2019-08-26T13:00:03Z

pkg/controller/operators/catalog/operator.go

+				return
+			}
+
+			logger.Infof("successfully attached attenuated ServiceAccount to status - %s", reference.Name)			


nit: could use a structured logging field for the reference.Name here

ecordell · 2019-08-26T13:02:41Z

pkg/controller/operators/catalog/installplan_sync.go

+
+		if updateErr := update(ip); updateErr != nil {
+			errs = append(errs, updateErr)
+			logger.Warnf("failed to kick off InstallPlan retry - %v", updateErr)


nit logger.WithError()

OLM should automatically resume operator install when a user grants proper permission(s). Currently, a user has to manually delete the subscription and recreate it in order to trigger a reinstall of the operator. Do the following to trigger reinstall: - Add a new field 'AttenuatedServiceAccountRef' to status of InstallPlan. We use this to refer to the ServiceAccount that will be used to do attenuated scoped install of the operator. - Watch on Role(Binding), ServiceAccount resources. When these RBAC resources are added/updated find the target InstallPlan object. - Update the status.phase of the InstallPlan object to Installing. This will trigger a sync of the InstallPlan. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1740332 Jira: https://jira.coreos.com/browse/OLM-1244

tkashem · 2019-08-26T14:52:46Z

/retest

jpeeler · 2019-08-26T17:36:43Z

/lgtm

openshift-ci-robot · 2019-08-26T17:36:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ecordell, jpeeler, tkashem

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:

~~OWNERS~~ [ecordell,jpeeler]

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 · 2019-08-26T17:37:50Z

@tkashem: All pull requests linked via external trackers have merged. Bugzilla bug 1740332 has been moved to the MODIFIED state.

In response to this:

Bug 1740332: OLM should resume operator install

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

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 Aug 22, 2019

openshift-ci-robot requested review from alecmerdler and njhale August 22, 2019 18:40

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 22, 2019

tkashem force-pushed the bz-1740332 branch from 7b7a009 to a173c3a Compare August 23, 2019 20:36

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 23, 2019

tkashem force-pushed the bz-1740332 branch from a173c3a to 31f45e6 Compare August 23, 2019 20:37

tkashem changed the title ~~[WIP] test - olm should resume operator install~~ Bug 1740332: OLM should resume operator install Aug 23, 2019

openshift-ci-robot added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Aug 23, 2019

tkashem force-pushed the bz-1740332 branch from 31f45e6 to f5f86b3 Compare August 23, 2019 21:04

ecordell approved these changes Aug 26, 2019

View reviewed changes

openshift-ci-robot assigned ecordell Aug 26, 2019

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 26, 2019

tkashem force-pushed the bz-1740332 branch from f5f86b3 to ff19541 Compare August 26, 2019 13:33

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 26, 2019

openshift-ci-robot assigned jpeeler Aug 26, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 26, 2019

openshift-merge-robot merged commit 3fe25c4 into operator-framework:master Aug 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1740332: OLM should resume operator install #1006

Bug 1740332: OLM should resume operator install #1006

tkashem commented Aug 22, 2019 •

edited

Loading

tkashem commented Aug 22, 2019

jpeeler commented Aug 22, 2019 •

edited

Loading

openshift-ci-robot commented Aug 23, 2019

tkashem commented Aug 26, 2019

ecordell left a comment

ecordell Aug 26, 2019

tkashem Aug 26, 2019

ecordell Aug 26, 2019

tkashem Aug 26, 2019

ecordell Aug 26, 2019

tkashem Aug 26, 2019

ecordell Aug 26, 2019

tkashem Aug 26, 2019

tkashem commented Aug 26, 2019

jpeeler commented Aug 26, 2019

openshift-ci-robot commented Aug 26, 2019

openshift-ci-robot commented Aug 26, 2019

		@@ -1075,6 +1097,8 @@ var _ installPlanTransitioner = &Operator{}
		func transitionInstallPlanState(log logrus.Logger, transitioner installPlanTransitioner, in v1alpha1.InstallPlan, now metav1.Time) (v1alpha1.InstallPlan, error) {
		out := in.DeepCopy()

Bug 1740332: OLM should resume operator install #1006

Bug 1740332: OLM should resume operator install #1006

Conversation

tkashem commented Aug 22, 2019 • edited Loading

tkashem commented Aug 22, 2019

jpeeler commented Aug 22, 2019 • edited Loading

openshift-ci-robot commented Aug 23, 2019

tkashem commented Aug 26, 2019

ecordell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkashem commented Aug 26, 2019

jpeeler commented Aug 26, 2019

openshift-ci-robot commented Aug 26, 2019

openshift-ci-robot commented Aug 26, 2019

tkashem commented Aug 22, 2019 •

edited

Loading

jpeeler commented Aug 22, 2019 •

edited

Loading