fix(installmodes): update support logic to match expected behavior #733

njhale · 2019-02-28T15:54:45Z

Change InstallMode behavior to match desired:

OwnNamespace: If supported, the operator can be a member of an OperatorGroup that selects its own namespace
SingleNamespace: If supported, the operator can be a member of an OperatorGroup that selects one namespace
MultiNamespace: If supported, the operator can be a member of an OperatorGroup that selects more than one namespace
AllNamespaces: If supported, the operator can be a member of an OperatorGroup that selects all namespaces (target namespace set is the empty string "")

Addresses ALM-927.

- Add installmode support e2e test - Remove old version of supports logic

njhale · 2019-02-28T15:55:35Z

/hold

Need to verify community-operator still InstallModes work with the new logic.

njhale · 2019-02-28T18:44:30Z

/retest

ecordell

/approve
/lgtm

njhale · 2019-02-28T22:07:19Z

/hold cancel

njhale · 2019-03-01T01:14:37Z

/retest

njhale · 2019-03-01T01:14:44Z

/hold

njhale · 2019-03-01T23:20:14Z

pkg/controller/operators/olm/operator.go

@@ -346,6 +346,22 @@ func (a *Operator) handleClusterServiceVersionDeletion(obj interface{}) {
 	defer func(csv v1alpha1.ClusterServiceVersion) {
 		logger.Debug("removing csv from queue set")
 		a.csvQueueSet.Remove(csv.GetName(), csv.GetNamespace())
+
+		// Requeue all OperatorGroups in the namespace


While I was working on this, I decided to make OperatorGroup requeuing on CSV deletion more robust.

- OperatorGroup sync bails out after successful namespace update detected - Check CSV status before re-transitioning to failed states due to OperatorGroup issues - Check CSV annotations before setting OperatorGroup annotations - Make OperatorGroup requeuing more aggressive on CSV deletion - Requeue OperatorGroup in OperatorGroup sync if CSV update fails and CSV still exists

njhale · 2019-03-01T23:24:17Z

pkg/controller/operators/olm/operator.go

+	// No target annotation
+	if !ok {
+		logger.Info("no olm.targetNamespaces annotation")
+		return nil


Before adding the return, we would always associate a missing olm.targetNamespaces annotation with NamespaceAll.

njhale · 2019-03-01T23:27:06Z

pkg/controller/operators/olm/operator.go

-			if err != nil {
-				logger.Error("error adding operatorgroup annotation")
-				syncError = err
+			if a.operatorGroupAnnotationsDiffer(&out.ObjectMeta, operatorGroup) {


Check for differences before updating the annotations.

njhale · 2019-03-01T23:30:14Z

pkg/controller/operators/olm/operator.go

-		logger.Warn(syncError.Error())
-		out.SetPhaseWithEvent(v1alpha1.CSVPhaseFailed, v1alpha1.CSVReasonTooManyOperatorGroups, syncError.Error(), now, a.recorder)
+		logger.WithError(syncError).Warn("csv failed to become an operatorgroup member")
+		if out.Status.Reason != v1alpha1.CSVReasonTooManyOperatorGroups {


Wrap set phase with status check to prevent unnecessary updates.

njhale · 2019-03-01T23:31:42Z

pkg/controller/operators/olm/operator_test.go

@@ -3576,6 +3576,10 @@ func TestSyncOperatorGroups(t *testing.T) {
 				}
 			}

+			// Sync again to catch provided API changes
+			err = op.syncOperatorGroups(tt.initial.operatorGroup)


Needed because OperatorGroup sync returns after each meaningful update and relies on the update event to trigger another sync.

njhale · 2019-03-02T00:50:00Z

pkg/controller/operators/olm/operatorgroup.go

+		}
+
+		// CSV requeue is handled by the succeeding sync
+		return nil


The status update will trigger another sync - no need to continue.

njhale · 2019-03-02T00:58:42Z

pkg/controller/operators/olm/operatorgroup.go

 		if csv.IsCopied() {
-			logger.Debug("csv is copied. not including in operatorgroup's provided api set")
+			logger.Debug("csv is copied. not updating annotations or including in operatorgroup's provided api set")


Moved to also ignore annotation checks for copied CSVs.

njhale · 2019-03-02T01:00:50Z

pkg/controller/operators/olm/operatorgroup.go

 	metav1.SetMetaDataAnnotation(obj, v1alpha2.OperatorGroupNamespaceAnnotationKey, op.GetNamespace())
 	metav1.SetMetaDataAnnotation(obj, v1alpha2.OperatorGroupAnnotationKey, op.GetName())
-	if addTargets {
+
+	if addTargets && op.Status.Namespaces != nil {


Guard against empty namespace list being evaluated as NamespaceAll.

njhale · 2019-03-04T19:20:55Z

/hold cancel

ecordell · 2019-03-04T22:57:17Z

pkg/controller/operators/olm/operatorgroup.go

 		metav1.SetMetaDataAnnotation(obj, v1alpha2.OperatorGroupTargetsAnnotationKey, strings.Join(op.Status.Namespaces, ","))
 	}
 }

+func (a *Operator) operatorGroupAnnotationsDiffer(obj *metav1.ObjectMeta, op *v1alpha2.OperatorGroup) bool {


njhale · 2019-03-05T19:35:52Z

/test all

njhale · 2019-03-06T20:34:43Z

/retest

ecordell · 2019-03-07T18:46:18Z

/lgtm

openshift-ci-robot · 2019-03-07T18:46:21Z

[APPROVALNOTIFIER] This PR is APPROVED

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

~~OWNERS~~ [ecordell,njhale]

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

fix(installmodes): update support logic to match expected behavior

njhale added 3 commits February 27, 2019 17:44

fix(installmodes): update support logic to match expected behavior

de0586d

test(operatorgroups): add installmode support e2e test

b851a57

- Add installmode support e2e test - Remove old version of supports logic

fix(csv): fix unsupported operatorgroup status update

e8b4b69

openshift-ci-robot requested review from alecmerdler and jpeeler February 28, 2019 15:54

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 28, 2019

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 28, 2019

openshift-ci-robot assigned ecordell Feb 28, 2019

ecordell approved these changes Feb 28, 2019

View reviewed changes

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

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 28, 2019

njhale commented Mar 1, 2019

View reviewed changes

njhale force-pushed the fix-installmodes branch from 1f78dce to d039748 Compare March 1, 2019 23:22

njhale commented Mar 1, 2019

View reviewed changes

njhale commented Mar 2, 2019

View reviewed changes

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 4, 2019

pmorie mentioned this pull request Mar 4, 2019

Remove SingleNamespace support from cluster-scoped CSV openshift/federation-v2-operator#44

Closed

ecordell reviewed Mar 4, 2019

View reviewed changes

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

openshift-merge-robot merged commit 8d22327 into operator-framework:master Mar 7, 2019

ecordell pushed a commit to ecordell/operator-lifecycle-manager that referenced this pull request Mar 8, 2019

Merge pull request operator-framework#733 from njhale/fix-installmodes

17000e3

fix(installmodes): update support logic to match expected behavior

njhale added the kind/bug Categorizes issue or PR as related to a bug. label Mar 19, 2019

ecordell removed kind/bug Categorizes issue or PR as related to a bug. labels Mar 19, 2019

njhale added the kind/bug Categorizes issue or PR as related to a bug. label Mar 19, 2019

ecordell added kind/bug Categorizes issue or PR as related to a bug. and removed kind/bug Categorizes issue or PR as related to a bug. labels Mar 19, 2019

njhale deleted the fix-installmodes branch September 30, 2019 21:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(installmodes): update support logic to match expected behavior #733

fix(installmodes): update support logic to match expected behavior #733

njhale commented Feb 28, 2019 •

edited

Loading

njhale commented Feb 28, 2019

njhale commented Feb 28, 2019

ecordell left a comment

njhale commented Feb 28, 2019

njhale commented Mar 1, 2019

njhale commented Mar 1, 2019

njhale Mar 1, 2019

njhale Mar 1, 2019

njhale Mar 1, 2019 •

edited

Loading

njhale Mar 1, 2019

njhale Mar 1, 2019

njhale Mar 2, 2019 •

edited

Loading

njhale Mar 2, 2019

njhale Mar 2, 2019 •

edited

Loading

njhale commented Mar 4, 2019

ecordell Mar 4, 2019

njhale commented Mar 5, 2019

njhale commented Mar 6, 2019

ecordell commented Mar 7, 2019

openshift-ci-robot commented Mar 7, 2019

fix(installmodes): update support logic to match expected behavior #733

fix(installmodes): update support logic to match expected behavior #733

Conversation

njhale commented Feb 28, 2019 • edited Loading

njhale commented Feb 28, 2019

njhale commented Feb 28, 2019

ecordell left a comment

Choose a reason for hiding this comment

njhale commented Feb 28, 2019

njhale commented Mar 1, 2019

njhale commented Mar 1, 2019

njhale Mar 1, 2019

Choose a reason for hiding this comment

njhale Mar 1, 2019

Choose a reason for hiding this comment

njhale Mar 1, 2019 • edited Loading

Choose a reason for hiding this comment

njhale Mar 1, 2019

Choose a reason for hiding this comment

njhale Mar 1, 2019

Choose a reason for hiding this comment

njhale Mar 2, 2019 • edited Loading

Choose a reason for hiding this comment

njhale Mar 2, 2019

Choose a reason for hiding this comment

njhale Mar 2, 2019 • edited Loading

Choose a reason for hiding this comment

njhale commented Mar 4, 2019

ecordell Mar 4, 2019

Choose a reason for hiding this comment

njhale commented Mar 5, 2019

njhale commented Mar 6, 2019

ecordell commented Mar 7, 2019

openshift-ci-robot commented Mar 7, 2019

njhale commented Feb 28, 2019 •

edited

Loading

njhale Mar 1, 2019 •

edited

Loading

njhale Mar 2, 2019 •

edited

Loading

njhale Mar 2, 2019 •

edited

Loading