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

feat(resolver): take all subscriptions into account when resolving #638

Merged
merged 2 commits into from
Jan 9, 2019
Merged

feat(resolver): take all subscriptions into account when resolving #638

merged 2 commits into from
Jan 9, 2019

Conversation

ecordell
Copy link
Member

@ecordell ecordell commented Dec 19, 2018 •
edited
Loading

This is a major change to the resolution algorithm but is backwards compatible in the API:

  • the entire namespace of subscriptions and csvs is considered as a unit
  • a new generation of subscriptions / csvs is calculated based on available updates and api providers in the catalogs
  • if the new generation of subscriptions will apply succesfully, an installplan is generated
  • if the installplan is approved, the update goes ahead

This means that installplans can't be manually written (they need resolved steps to be useful). These changes should inform future versions of the API, but for now we're strictly backwards-compatible.

Things left to do:

  • Document and explain this behavior (barebones description in docs files)
  • Rewrite installplan e2e tests
  • Rewrite subscription unit tests
  • Write tests that validate resolving apiservers (only did CRDs for the current batch of tests)
  • Write tests that validate extra bundle objects get resolved (CRDs are part of the bundle now, but we should verify we can make other types - I think we may have errors from the InferGVK parts)
  • Remove inmem catalog code

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 19, 2018
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 30, 2018
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 2, 2019
@njhale
Copy link
Member

njhale commented Jan 3, 2019

@evan I don't think this is actually blocked on @alecmerdler removing inmem logic from packageserver. As I recall, packageserver has its own trimmed-down implementation of CatalogSource configmap parsing.

@ecordell ecordell force-pushed the new-resolver branch 2 times, most recently from f75aa5c to 55909e0 Compare January 3, 2019 21:36
njhale
njhale reviewed Jan 4, 2019
View reviewed changes
pkg/controller/operators/catalog/operator.go Show resolved Hide resolved
pkg/controller/operators/catalog/operator.go Outdated Show resolved Hide resolved
pkg/controller/operators/olm/operator.go
@@ -440,7 +440,10 @@ func (a *Operator) syncClusterServiceVersion(obj interface{}) (syncError error)
outCSV, syncError := a.transitionCSVState(*clusterServiceVersion)

// no changes in status, don't update
if outCSV.Status.Phase == clusterServiceVersion.Status.Phase && outCSV.Status.Reason == clusterServiceVersion.Status.Reason && outCSV.Status.Message == clusterServiceVersion.Status.Message {
if outCSV.Status.LastUpdateTime == clusterServiceVersion.Status.LastUpdateTime &&
Copy link
Member

Choose a reason for hiding this comment

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

👍

pkg/controller/registry/resolver/evolver.go Outdated Show resolved Hide resolved
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 6, 2019
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 6, 2019
@ecordell ecordell force-pushed the new-resolver branch 7 times, most recently from d2f9714 to 77df5d6 Compare January 8, 2019 03:15
@ecordell ecordell changed the title [WIP] feat(resolver): take all subscriptions into account when resolving feat(resolver): take all subscriptions into account when resolving Jan 8, 2019
@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 Jan 8, 2019
@ecordell
Copy link
Member Author

ecordell commented Jan 8, 2019

/test unit

@ecordell ecordell force-pushed the new-resolver branch 3 times, most recently from 8224b04 to 862dd89 Compare January 8, 2019 21:14
@ecordell
feat(resolver): take all subscriptions into account when resolving
b9f59b6 
this is a major change to the resolution algorithm but is backwards
compatible in the API

- the entire namespace of subscriptions is considered as a unit
- a new generation of subscriptions / csvs is calculated
- if the new generation of subscriptions will apply succesfully,
 an installplan is generated
- if the installplan is approved, the update goes ahead

this means that installplans can't be manually written (they need
resolved steps to be useful)
@ecordell ecordell force-pushed the new-resolver branch 2 times, most recently from aad7b88 to 1dca7d8 Compare January 8, 2019 23:31
@ecordell
Copy link
Member Author

ecordell commented Jan 8, 2019

/retest

@dinhxuanvu
Copy link
Member

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

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

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

ecordell commented Jan 9, 2019

/skip gitlab-ci/pipeline

@ecordell ecordell merged commit 5a4031b into operator-framework:master Jan 9, 2019
ecordell added a commit to ecordell/operator-lifecycle-manager that referenced this pull request Mar 8, 2019
@ecordell
Merge pull request operator-framework#638 from ecordell/new-resolver
842dc37 
feat(resolver): take all subscriptions into account when resolving
@njhale njhale added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@njhale njhale njhale left review comments

@alecmerdler alecmerdler Awaiting requested review from alecmerdler

Assignees

@dinhxuanvu dinhxuanvu

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ecordell @njhale @dinhxuanvu @openshift-ci-robot