Bug 1714140: fix(catalog): re-install resources in existing installplan #965

jpeeler · 2019-07-24T20:59:42Z

This specifically fixes the scenario where a subscription is deleted and recreated in a namespace with an additional subscription. When the subscription of interest is deleted, the associated installplan remains due to the other subscription's owner reference. Because the existing installplan manifests match, no additional work is done. This is a problem if resources that were originally installed have been deleted.

Now after a subscription is recreated, the proper owner references are re-added onto the install plan as well as all the resources are checked that they are installed.

https://bugzilla.redhat.com/show_bug.cgi?id=1714140

pkg/controller/operators/catalog/operator.go

jpeeler · 2019-07-26T13:13:04Z

/retest

ecordell · 2019-07-26T12:45:17Z

pkg/controller/operators/catalog/operator.go

+			for _, step := range installPlan.Status.Plan {
+				step.Status = v1alpha1.StepStatusUnknown
+			}
+			_, err = o.client.OperatorsV1alpha1().InstallPlans(namespace).UpdateStatus(installPlan)


Do we need to update the status if no owners were added?

Yes, the steps need to all be set to not completed status.

@jpeeler could you please explain a little more:

Yes, the steps need to all be set to not completed status.

In the case where an InstallPlan already has all of the provided Subscriptions as owners, why would we set its steps states to unknown? Are we always trying to get InstallPlans to re-apply?

ensureInstallPlan only gets called when there's a subscription change, right? Either way, the reapplication only occurs for an existing install plan with matching manifests. Reapplying an install plan (via setting previously executed steps as unknown) is the only way to get an operator to reinstall unless the way owner references are done currently is changed, specifically with regard to other subscriptions.

ensureInstallPlan only gets called when there's a subscription change, right?

It gets called any time a Subscription is synced, which is on any change and also every 15m (whatever the resync interval is), right?

Ah, that makes sense. Thanks for explaining. I'm only asking because from a user of the ensureInstallPlan(s) I'd expect only the InstallPlans that have changed to be re-installed, but it may not be a big deal for all of them to attempt a re-install though.

@jpeeler I believe leaving this as you have it solves a separate bug on our backlog too, actually!

This will check/recreate any objects that are in the bundle, so if they get inadvertently deleted this will recreate them.

pkg/controller/operators/catalog/operator.go

This specifically fixes the scenario where a subscription is deleted and recreated in a namespace with an additional subscription. When the subscription of interest is deleted, the associated installplan remains due to the other subscription's owner reference. Because the existing installplan manifests match, no additional work is done. This is a problem if resources that were originally installed have been deleted. Now after a subscription is recreated, the proper owner references are re-added onto the install plan as well as all the resources are checked that they are installed.

Using the standard logger allows a unit test to set the desired log level via `log.SetLevel(log.DebugLevel)`.

jpeeler · 2019-07-26T16:21:09Z

/test e2e-aws-olm

jpeeler · 2019-07-26T17:49:05Z

/retest

jpeeler · 2019-07-26T19:11:50Z

/retest

jpeeler · 2019-07-26T23:31:59Z

/retest

jpeeler · 2019-07-29T15:03:13Z

/retest

ecordell · 2019-07-31T18:24:28Z

/lgtm

openshift-ci-robot · 2019-07-31T18:24:33Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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-07-31T18:25:34Z

@jpeeler: This pull request references a valid Bugzilla bug. 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 1714140: fix(catalog): re-install resources in existing installplan

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 · 2019-07-31T20:42:40Z

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

In response to this:

Bug 1714140: fix(catalog): re-install resources in existing installplan

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.

ecordell · 2019-08-02T13:31:13Z

/cherry-pick release-4.1

openshift-cherrypick-robot · 2019-08-02T13:31:21Z

@ecordell: #965 failed to apply on top of branch "release-4.1":

error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	pkg/controller/operators/catalog/operator.go
M	pkg/controller/operators/catalog/subscriptions_test.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/controller/operators/catalog/subscriptions_test.go
Auto-merging pkg/controller/operators/catalog/operator.go
CONFLICT (content): Merge conflict in pkg/controller/operators/catalog/operator.go
Patch failed at 0001 fix(catalog): re-install resources in existing installplan

In response to this:

/cherry-pick release-4.1

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 requested review from alecmerdler and njhale July 24, 2019 20:59

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 24, 2019

jpeeler force-pushed the existing-installplan branch from 3ff979e to e67d450 Compare July 24, 2019 21:01

ecordell reviewed Jul 24, 2019

View reviewed changes

pkg/controller/operators/catalog/operator.go Outdated Show resolved Hide resolved

jpeeler force-pushed the existing-installplan branch from e67d450 to d899fe5 Compare July 25, 2019 22:27

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 25, 2019

ecordell reviewed Jul 26, 2019

View reviewed changes

jpeeler added 2 commits July 26, 2019 11:10

fix(unit): allow logrus log level to be adjusted

4af534f

Using the standard logger allows a unit test to set the desired log level via `log.SetLevel(log.DebugLevel)`.

jpeeler force-pushed the existing-installplan branch from d899fe5 to 4af534f Compare July 26, 2019 15:12

openshift-ci-robot assigned ecordell Jul 31, 2019

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

ecordell changed the title ~~fix(catalog): re-install resources in existing installplan~~ Bug: 1714140 fix(catalog): re-install resources in existing installplan Jul 31, 2019

ecordell changed the title ~~Bug: 1714140 fix(catalog): re-install resources in existing installplan~~ Bug 1714140: fix(catalog): re-install resources in existing installplan Jul 31, 2019

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Jul 31, 2019

openshift-merge-robot merged commit 26e2cc3 into operator-framework:master Jul 31, 2019

jpeeler mentioned this pull request Aug 5, 2019

Bug 1740174: fix(catalog): re-install resources in existing installplan #977

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1714140: fix(catalog): re-install resources in existing installplan #965

Bug 1714140: fix(catalog): re-install resources in existing installplan #965

jpeeler commented Jul 24, 2019

jpeeler commented Jul 26, 2019

ecordell Jul 26, 2019

jpeeler Jul 26, 2019

njhale Jul 29, 2019

jpeeler Jul 30, 2019

ecordell Jul 31, 2019

njhale Jul 31, 2019

ecordell Jul 31, 2019

jpeeler commented Jul 26, 2019

jpeeler commented Jul 26, 2019

jpeeler commented Jul 26, 2019

jpeeler commented Jul 26, 2019

jpeeler commented Jul 29, 2019

ecordell commented Jul 31, 2019

openshift-ci-robot commented Jul 31, 2019

openshift-ci-robot commented Jul 31, 2019

openshift-ci-robot commented Jul 31, 2019

ecordell commented Aug 2, 2019

openshift-cherrypick-robot commented Aug 2, 2019

Bug 1714140: fix(catalog): re-install resources in existing installplan #965

Bug 1714140: fix(catalog): re-install resources in existing installplan #965

Conversation

jpeeler commented Jul 24, 2019

jpeeler commented Jul 26, 2019

ecordell Jul 26, 2019

Choose a reason for hiding this comment

jpeeler Jul 26, 2019

Choose a reason for hiding this comment

njhale Jul 29, 2019

Choose a reason for hiding this comment

jpeeler Jul 30, 2019

Choose a reason for hiding this comment

ecordell Jul 31, 2019

Choose a reason for hiding this comment

njhale Jul 31, 2019

Choose a reason for hiding this comment

ecordell Jul 31, 2019

Choose a reason for hiding this comment

jpeeler commented Jul 26, 2019

jpeeler commented Jul 26, 2019

jpeeler commented Jul 26, 2019

jpeeler commented Jul 26, 2019

jpeeler commented Jul 29, 2019

ecordell commented Jul 31, 2019

openshift-ci-robot commented Jul 31, 2019

openshift-ci-robot commented Jul 31, 2019

openshift-ci-robot commented Jul 31, 2019

ecordell commented Aug 2, 2019

openshift-cherrypick-robot commented Aug 2, 2019