Bug 1767004: defer provided api update in operator groups #1114

jpeeler · 2019-11-07T18:19:18Z

This is the case of a bug where the CSV status wasn't updated due to an
invalid CSV. The cause for this was twofold:

The CSV and OperatorGroup reconcile loops were undoing each others
changes continously, giving no chance for the CSV to get synced beyond
provided api conflict detection.
Due to the way the provided APIs are produced differently for
operator groups (uses GVKSTringToProvidedAPISet) and OLM (uses
NewOperatorFromV1Alpha1CSV) when an invalid CSV is in use, the resolver
attempts to continue to instruct that an operator group annotation
update is needed beyond the point of that being true.

The changes are to ensure that OLM does not get stuck attempting to
update the operator group annotations when they are no longer needed
combined with ensuring that the operator group sync loop does not try to
prematurely dismiss a provided api before the CSV has a chance to check
the requirements and produce status.

In order to accomplish "premature detection", providedAPIsFromCSVs was
modified to not only return the APIs but also the CSV that provides a
given API.

Description of the change:

Motivation for the change:

Reviewer Checklist

Implementation matches the proposed design, or proposal is updated to match implementation
Sufficient unit test coverage
Sufficient end-to-end test coverage
Docs updated or added to /docs
Commit messages sensible and descriptive

openshift-ci-robot · 2019-11-07T18:19:23Z

@jpeeler: This pull request references Bugzilla bug 1767004, 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 1767004: defer provided api update in operator groups

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.

This is the case of a bug where the CSV status wasn't updated due to an invalid CSV. The cause for this was twofold: 1) The CSV and OperatorGroup reconcile loops were undoing each others changes continously, giving no chance for the CSV to get synced beyond provided api conflict detection. 2) Due to the way the provided APIs are produced differently for operator groups (uses GVKSTringToProvidedAPISet) and OLM (uses NewOperatorFromV1Alpha1CSV) when an invalid CSV is in use, the resolver attempts to continue to instruct that an operator group annotation update is needed beyond the point of that being true. The changes are to ensure that OLM does not get stuck attempting to update the operator group annotations when they are no longer needed combined with ensuring that the operator group sync loop does not try to prematurely dismiss a provided api before the CSV has a chance to check the requirements and produce status. In order to accomplish "premature detection", providedAPIsFromCSVs was modified to not only return the APIs but also the CSV that provides a given API.

jpeeler · 2019-11-07T22:08:04Z

/retest

jpeeler · 2019-11-07T22:17:51Z

/retest
I think this PR is good to go, but CI seems to be broken right now.

jpeeler · 2019-11-08T01:20:18Z

/retest

dinhxuanvu · 2019-11-08T06:05:13Z

/retest

dinhxuanvu · 2019-11-08T06:55:26Z

/retest

njhale

@jpeeler Thank you for getting on this so quickly! I had a few comments, but other than those, this PR really has me thinking about how/if we can simplify the provided API reconciliation. It would be nice if only OperatorGroup syncing handled provided API annotation updates, WDYT?

pkg/controller/operators/olm/operator_test.go

test/e2e/csv_e2e_test.go

njhale · 2019-11-13T04:25:20Z

pkg/controller/operators/olm/operatorgroup.go

 	// Prune providedAPIs annotation if the cluster has fewer providedAPIs (handles CSV deletion)
-	if intersection := groupProvidedAPIs.Intersection(providedAPIsFromCSVs); len(intersection) < len(groupProvidedAPIs) {
+	//if intersection := groupProvidedAPIs.Intersection(providedAPIsFromCSVs); len(intersection) < len(groupProvidedAPIs) {
+	if len(intersection) < len(groupProvidedAPIs) {


This should only happen when the OperatorGroup has provided APIs that no longer exist in the namespace. Could you explain how this condition is met otherwise?

This was changed mainly because proviedAPIsFromCSVs no longer is an APISet, so calculating the intersection isn't possible. But with an invalid api version specified in a CSV, it is possible that the operator group has apis that were never on the cluster (which is part of the problem here). I can remove the commented out line I left in there if you want.

But with an invalid api version specified in a CSV, it is possible that the operator group has apis that were never on the cluster (which is part of the problem here)

What does "never on the cluster" mean? IIRC, the point here is to remove APIs that no CSV in the OperatorGroup's namespace claims to provide. I would rather have false positives and cleanup more often

I can remove the commented out line I left in there if you want.

I think we want to avoid commented code piling up, but we don't need to cause another round of CI over it.

also see: #1114 (comment)

Remember that this bug is dealing with an invalid api due to botched syntax. Due to the botched syntax as shown in the bug, the CRD version could never have possibly existed.

The problem with cleaning up more often is there are two sync loops that end up fighting each other with a constant stream of updates, which prevents validation from ever occurring and therefore not getting reported back to the user.

jpeeler · 2019-11-13T20:38:09Z

@njhale In general, it seems that having any loops updating resources other than the one the loop was started for leads to bad contention. But operator groups and CSVs are coupled in a way that makes the problem particularly bad.

njhale · 2019-11-14T05:18:24Z

pkg/controller/operators/olm/operatorgroup.go

@@ -230,20 +237,40 @@ func (a *Operator) providedAPIsFromCSVs(group *v1.OperatorGroup, logger *logrus.
 			logger.WithError(err).Warn("could not create OperatorSurface from csv")
 			continue
 		}
-		providedAPIsFromCSVs = providedAPIsFromCSVs.Union(operatorSurface.ProvidedAPIs().StripPlural())
+		for providedAPI := range operatorSurface.ProvidedAPIs().StripPlural() {
+			providedAPIsFromCSVs[providedAPI] = csv


Why lug the CSV around when you can filter out the ones you don't want here?

I don't think I want to filter out any CSVs. The CSVs are returned so that in pruneProvidedAPIs while iterating over the provided apis, the CSV phase for a given api can be checked to ensure it has progressed to the point of validation (past phase pending).

hmm, I think they are roughly isomorphic, but this isn't a big deal.

njhale

I just have one last comment I wanted to get your thoughts on before we merge this: #1114 (comment)

njhale · 2019-11-15T14:44:30Z

/lgtm

openshift-ci-robot · 2019-11-15T14:44:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jpeeler, 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:

~~OWNERS~~ [jpeeler,njhale]

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-11-15T16:32:51Z

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

In response to this:

Bug 1767004: defer provided api update in operator groups

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 bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Nov 7, 2019

openshift-ci-robot requested review from ecordell and njhale November 7, 2019 18:19

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 7, 2019

jpeeler force-pushed the fix-status-invalid-csv branch from d58682e to 8004312 Compare November 7, 2019 20:16

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 Nov 7, 2019

jpeeler force-pushed the fix-status-invalid-csv branch from 8004312 to ad49491 Compare November 7, 2019 21:56

njhale reviewed Nov 13, 2019

View reviewed changes

njhale reviewed Nov 14, 2019

View reviewed changes

openshift-ci-robot assigned njhale Nov 15, 2019

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

openshift-merge-robot merged commit 76754c9 into operator-framework:master Nov 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1767004: defer provided api update in operator groups #1114

Bug 1767004: defer provided api update in operator groups #1114

jpeeler commented Nov 7, 2019

openshift-ci-robot commented Nov 7, 2019

jpeeler commented Nov 7, 2019

jpeeler commented Nov 7, 2019

jpeeler commented Nov 8, 2019

dinhxuanvu commented Nov 8, 2019

dinhxuanvu commented Nov 8, 2019

njhale left a comment

njhale Nov 13, 2019

jpeeler Nov 13, 2019

njhale Nov 14, 2019

njhale Nov 14, 2019

njhale Nov 14, 2019

jpeeler Nov 15, 2019

jpeeler commented Nov 13, 2019 •

edited

njhale Nov 14, 2019

jpeeler Nov 15, 2019

njhale Nov 15, 2019

njhale left a comment

njhale commented Nov 15, 2019

openshift-ci-robot commented Nov 15, 2019

openshift-ci-robot commented Nov 15, 2019

Bug 1767004: defer provided api update in operator groups #1114

Bug 1767004: defer provided api update in operator groups #1114

Conversation

jpeeler commented Nov 7, 2019

openshift-ci-robot commented Nov 7, 2019

jpeeler commented Nov 7, 2019

jpeeler commented Nov 7, 2019

jpeeler commented Nov 8, 2019

dinhxuanvu commented Nov 8, 2019

dinhxuanvu commented Nov 8, 2019

njhale 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

jpeeler commented Nov 13, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njhale left a comment

Choose a reason for hiding this comment

njhale commented Nov 15, 2019

openshift-ci-robot commented Nov 15, 2019

openshift-ci-robot commented Nov 15, 2019

jpeeler commented Nov 13, 2019 •

edited