Reduce number of related InstallPlans

Previously, the operator policy controller would list all InstallPlans in the subscription namespace and filter down to ones it considered relevant. Among other things, it used OwnerReferences to do this filtering, but those are inconsistently applied by OLM. Now, it only looks at InstallPlans labelled specifically for the subscription in the policy, which seems to be much more reliably set and updated by OLM. Generally, only one InstallPlan will have the label, which makes it more possible to unambiguously assign a compliance to it based on its phase (previously, it was unclear what to assign to "historic" InstallPlans). Much of the controller logic still handles the possibility of there being multiple relevant InstallPlans, for robustness. Refs: - https://issues.redhat.com/browse/ACM-11025 Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com> (cherry picked from commit c636d7d)
stolostron · May 15, 2024 · f1f4286 · f1f4286
1 parent 4e70583
commit f1f4286
Show file tree

Hide file tree

Showing 3 changed files with 13 additions and 81 deletions.
diff --git a/controllers/operatorpolicy_controller.go b/controllers/operatorpolicy_controller.go
@@ -1176,50 +1176,27 @@ func (r *OperatorPolicyReconciler) handleInstallPlan(
 	}
 
 	watcher := opPolIdentifier(policy.Namespace, policy.Name)
+	selector := subLabelSelector(sub)
 
-	foundInstallPlans, err := r.DynamicWatcher.List(
-		watcher, installPlanGVK, sub.Namespace, labels.Everything())
+	installPlans, err := r.DynamicWatcher.List(watcher, installPlanGVK, sub.Namespace, selector)
 	if err != nil {
 		return false, fmt.Errorf("error listing InstallPlans: %w", err)
 	}
 
-	ownedInstallPlans := make([]unstructured.Unstructured, 0, len(foundInstallPlans))
-	selector := subLabelSelector(sub)
-
-	for _, installPlan := range foundInstallPlans {
-		// sometimes the OwnerReferences aren't correct, but the label should be
-		if selector.Matches(labels.Set(installPlan.GetLabels())) {
-			ownedInstallPlans = append(ownedInstallPlans, installPlan)
-
-			break
-		}
-
-		for _, owner := range installPlan.GetOwnerReferences() {
-			match := owner.Name == sub.Name &&
-				owner.Kind == subscriptionGVK.Kind &&
-				owner.APIVersion == subscriptionGVK.GroupVersion().String()
-			if match {
-				ownedInstallPlans = append(ownedInstallPlans, installPlan)
-
-				break
-			}
-		}
-	}
-
 	// InstallPlans are generally kept in order to provide a history of actions on the cluster, but
 	// they can be deleted without impacting the installed operator. So, not finding any should not
 	// be considered a reason for NonCompliance, regardless of musthave or mustnothave.
-	if len(ownedInstallPlans) == 0 {
+	if len(installPlans) == 0 {
 		return updateStatus(policy, noInstallPlansCond, noInstallPlansObj(sub.Namespace)), nil
 	}
 
 	if policy.Spec.ComplianceType.IsMustHave() {
-		changed, err := r.musthaveInstallPlan(ctx, policy, sub, ownedInstallPlans)
+		changed, err := r.musthaveInstallPlan(ctx, policy, sub, installPlans)
 
 		return changed, err
 	}
 
-	return r.mustnothaveInstallPlan(policy, ownedInstallPlans)
+	return r.mustnothaveInstallPlan(policy, installPlans)
 }
 
 func (r *OperatorPolicyReconciler) musthaveInstallPlan(
@@ -1233,7 +1210,6 @@ func (r *OperatorPolicyReconciler) musthaveInstallPlan(
 	ipsRequiringApproval := make([]unstructured.Unstructured, 0)
 	anyInstalling := false
 	currentPlanFailed := false
-	selector := subLabelSelector(sub)
 
 	// Construct the relevant relatedObjects, and collect any that might be considered for approval
 	for i, installPlan := range ownedInstallPlans {
@@ -1253,11 +1229,7 @@ func (r *OperatorPolicyReconciler) musthaveInstallPlan(
 		// consider some special phases
 		switch phase {
 		case string(operatorv1alpha1.InstallPlanPhaseRequiresApproval):
-			// only consider InstallPlans with this label for approval - this label is supposed to
-			// indicate the "current" InstallPlan for this subscription.
-			if selector.Matches(labels.Set(installPlan.GetLabels())) {
-				ipsRequiringApproval = append(ipsRequiringApproval, installPlan)
-			}
+			ipsRequiringApproval = append(ipsRequiringApproval, installPlan)
 		case string(operatorv1alpha1.InstallPlanPhaseInstalling):
 			anyInstalling = true
 		case string(operatorv1alpha1.InstallPlanFailed):

diff --git a/controllers/operatorpolicy_status.go b/controllers/operatorpolicy_status.go
@@ -1042,9 +1042,8 @@ func noInstallPlansObj(namespace string) policyv1.RelatedObject {
 }
 
 // existingInstallPlanObj returns a RelatedObject for the InstallPlan, with a reason
-// like 'The InstallPlan is ____' based on the phase. Usually the object will not
-// have a compliance associated with it, but if it requires approval or is actively
-// installing, then it will be NonCompliant.
+// like 'The InstallPlan is ____' based on the phase. When the InstallPlan is in phase
+// 'Complete', the object will be Compliant, otherwise it will be NonCompliant.
 func existingInstallPlanObj(ip client.Object, phase string) policyv1.RelatedObject {
 	relObj := policyv1.RelatedObject{
 		Object: policyv1.ObjectResourceFromObj(ip),
@@ -1064,8 +1063,9 @@ func existingInstallPlanObj(ip client.Object, phase string) policyv1.RelatedObje
 		// FUTURE: check policy.spec.statusConfig.upgradesAvailable to determine `compliant`.
 		// For now, assume it is set to 'NonCompliant'
 		relObj.Compliant = string(policyv1.NonCompliant)
-	case string(operatorv1alpha1.InstallPlanPhaseInstalling):
-		// if it's still installing, then it shouldn't be considered compliant yet.
+	case string(operatorv1alpha1.InstallPlanPhaseComplete):
+		relObj.Compliant = string(policyv1.Compliant)
+	default:
 		relObj.Compliant = string(policyv1.NonCompliant)
 	}
 

diff --git a/test/e2e/case38_install_operator_test.go b/test/e2e/case38_install_operator_test.go
@@ -1329,16 +1329,6 @@ var _ = Describe("Testing OperatorPolicy", Ordered, Label("supports-hosted"), fu
 				opPolName,
 				false,
 				[]policyv1.RelatedObject{{
-					Object: policyv1.ObjectResource{
-						Kind:       "InstallPlan",
-						APIVersion: "operators.coreos.com/v1alpha1",
-						Metadata: policyv1.ObjectMetadata{
-							Namespace: opPolTestNS,
-							Name:      firstInstallPlanName,
-						},
-					},
-					Reason: "The InstallPlan is Complete",
-				}, {
 					Object: policyv1.ObjectResource{
 						Kind:       "InstallPlan",
 						APIVersion: "operators.coreos.com/v1alpha1",
@@ -1376,16 +1366,6 @@ var _ = Describe("Testing OperatorPolicy", Ordered, Label("supports-hosted"), fu
 				opPolName,
 				false,
 				[]policyv1.RelatedObject{{
-					Object: policyv1.ObjectResource{
-						Kind:       "InstallPlan",
-						APIVersion: "operators.coreos.com/v1alpha1",
-						Metadata: policyv1.ObjectMetadata{
-							Namespace: opPolTestNS,
-							Name:      firstInstallPlanName,
-						},
-					},
-					Reason: "The InstallPlan is Complete",
-				}, {
 					Object: policyv1.ObjectResource{
 						Kind:       "InstallPlan",
 						APIVersion: "operators.coreos.com/v1alpha1",
@@ -1394,7 +1374,8 @@ var _ = Describe("Testing OperatorPolicy", Ordered, Label("supports-hosted"), fu
 							Name:      secondInstallPlanName,
 						},
 					},
-					Reason: "The InstallPlan is Complete",
+					Compliant: "Compliant",
+					Reason:    "The InstallPlan is Complete",
 				}},
 				metav1.Condition{
 					Type:    "InstallPlanCompliant",
@@ -1868,16 +1849,6 @@ var _ = Describe("Testing OperatorPolicy", Ordered, Label("supports-hosted"), fu
 					},
 					Compliant: "Compliant",
 					Reason:    "Resource found but will not be handled in mustnothave mode",
-				}, {
-					Object: policyv1.ObjectResource{
-						Kind:       "InstallPlan",
-						APIVersion: "operators.coreos.com/v1alpha1",
-						Metadata: policyv1.ObjectMetadata{
-							Namespace: opPolTestNS,
-						},
-					},
-					Compliant: "Compliant",
-					Reason:    "Resource found but will not be handled in mustnothave mode",
 				}},
 				metav1.Condition{
 					Type:    "InstallPlanCompliant",
@@ -2304,17 +2275,6 @@ var _ = Describe("Testing OperatorPolicy", Ordered, Label("supports-hosted"), fu
 					},
 					Compliant: "Compliant",
 					Reason:    "Resource found but will not be handled in mustnothave mode",
-				}, {
-					Object: policyv1.ObjectResource{
-						Kind:       "InstallPlan",
-						APIVersion: "operators.coreos.com/v1alpha1",
-						Metadata: policyv1.ObjectMetadata{
-							Name:      installPlanName,
-							Namespace: opPolTestNS,
-						},
-					},
-					Compliant: "Compliant",
-					Reason:    "Resource found but will not be handled in mustnothave mode",
 				}},
 				metav1.Condition{
 					Type:    "InstallPlanCompliant",