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

fix(olm): Fix CSVs api-servers battle for ownership of APIServices #690

Merged
merged 5 commits into from
Mar 8, 2019
Merged

fix(olm): Fix CSVs api-servers battle for ownership of APIServices #690

merged 5 commits into from
Mar 8, 2019

Conversation

dinhxuanvu
Copy link
Member

Currently, CSVs that install extension api-servers battle for
ownership of APIServices. New CSVs only adopt APIServices that have
OwnerReferences to CSVs in the new CSV's replaces chain.

Signed-off-by: Vu Dinh [email protected]

@dinhxuanvu dinhxuanvu changed the title fix(olm): Fix CSVs api-servers battle for ownership of APIServices [WIP] fix(olm): Fix CSVs api-servers battle for ownership of APIServices Jan 28, 2019
@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/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 28, 2019
@dinhxuanvu dinhxuanvu force-pushed the api-owner branch 3 times, most recently from d6bf7eb to 3918d17 Compare January 29, 2019 10:12
@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 Jan 29, 2019
@dinhxuanvu dinhxuanvu changed the title [WIP] fix(olm): Fix CSVs api-servers battle for ownership of APIServices fix(olm): Fix CSVs api-servers battle for ownership of APIServices Jan 29, 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 29, 2019
njhale
njhale reviewed Jan 29, 2019
View reviewed changes
Copy link
Member

@njhale njhale left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think we want a test to ensure that the installation of a second CSV that claims an APIService fails if there already exists a CSV that claims it (not replacing).

@dinhxuanvu dinhxuanvu force-pushed the api-owner branch 11 times, most recently from 400238e to 7ccc48b Compare February 1, 2019 01:48
@openshift-bot
Copy link
Contributor

/retest

1 similar comment
@openshift-merge-robot
Copy link
Collaborator

/retest

@dinhxuanvu dinhxuanvu force-pushed the api-owner branch 2 times, most recently from 9afeeac to 426bae3 Compare February 12, 2019 18:38
njhale
njhale reviewed Feb 13, 2019
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.

Sorry it took so long to get around to really reviewing this. This looks great and has a really thorough e2e test! Could you write one more that ensures we can't install a CSV that provides the same APIService in two separate namespaces?

I left some additional feedback on the APIServiceAdoptable function:

pkg/lib/ownerutil/util.go Outdated Show resolved Hide resolved
pkg/lib/ownerutil/util.go Outdated Show resolved Hide resolved
test/e2e/csv_e2e_test.go Outdated Show resolved Hide resolved
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 4, 2019
ecordell
ecordell approved these changes Mar 4, 2019
View reviewed changes
Copy link
Member

@ecordell ecordell 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 Mar 4, 2019
@dinhxuanvu
Copy link
Member Author

/retest

@dinhxuanvu
Copy link
Member Author

/test e2e-aws-olm

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 7, 2019
@dinhxuanvu
Copy link
Member Author

/test unit

@jpeeler jpeeler force-pushed the api-owner branch 2 times, most recently from 6fcc261 to f258712 Compare March 7, 2019 22:45
@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 7, 2019
dinhxuanvu and others added 5 commits March 8, 2019 10:21
@dinhxuanvu
fix(olm): Fix CSVs api-servers battle for ownership of APIServices
4121c47 
Currently, CSVs that install extension api-servers battle for
ownership of APIServices. New CSVs only adopt APIServices that have
OwnerReferences to CSVs in the new CSV's replaces chain.

Signed-off-by: Vu Dinh <[email protected]>
fix(util): fill in GVK info from InferGroupVersionKind
0d92c51 
The owner comparison check was comparing against empty values before. This also
adds some additional warnings just to be safe.
chore(olm): add debugging logging and handle potential list failure
69848a8
fix(olm): delete api services upon CSV deletion
79b314d
fix(test): fix labelselector pointer
ee5c004
@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 8, 2019
@jpeeler
Copy link

jpeeler commented Mar 8, 2019

I could have sworn that when I tested this before that deleting a namespace would cause the CSV deletion handler to not run. But after testing today, it appears that is not the case.

njhale
njhale approved these changes Mar 8, 2019
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.

Thank you @dinhxuanvu and @jpeeler for getting this working!
/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@openshift-merge-robot openshift-merge-robot merged commit 22543bd into operator-framework:master Mar 8, 2019
@ecordell ecordell added the kind/bug Categorizes issue or PR as related to a bug. label Mar 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpeeler jpeeler jpeeler left review comments

@njhale njhale njhale approved these changes

@ecordell ecordell ecordell approved these changes

Assignees

@ecordell ecordell

@njhale njhale

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

Successfully merging this pull request may close these issues.
7 participants
@dinhxuanvu @openshift-bot @openshift-merge-robot @ecordell @njhale @jpeeler @openshift-ci-robot