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

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

Merged

Conversation

jpeeler
Copy link
Contributor

@jpeeler jpeeler commented Jul 24, 2019

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

@openshift-ci-robot 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
@openshift-ci-robot 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
@jpeeler
Copy link
Contributor Author

jpeeler commented Jul 26, 2019

/retest

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

@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 Outdated Show resolved Hide resolved
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
Copy link
Contributor Author

jpeeler commented Jul 26, 2019

/test e2e-aws-olm

@jpeeler
Copy link
Contributor Author

jpeeler commented Jul 26, 2019

/retest

3 similar comments
@jpeeler
Copy link
Contributor Author

jpeeler commented Jul 26, 2019

/retest

@jpeeler
Copy link
Contributor Author

jpeeler commented Jul 26, 2019

/retest

@jpeeler
Copy link
Contributor Author

jpeeler commented Jul 29, 2019

/retest

@ecordell
Copy link
Member

/lgtm

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

[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:

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

@ecordell 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 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
Copy link
Collaborator

@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 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 openshift-merge-robot merged commit 26e2cc3 into operator-framework:master Jul 31, 2019
@openshift-ci-robot
Copy link
Collaborator

@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
Copy link
Member

ecordell commented Aug 2, 2019

/cherry-pick release-4.1

@openshift-cherrypick-robot

@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.

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. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants