From 7d533b9e3015c446c7ac7928f51dfc0b309390c3 Mon Sep 17 00:00:00 2001 From: Justin Kulikauskas Date: Thu, 2 May 2024 14:51:34 -0400 Subject: [PATCH] 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 --- controllers/operatorpolicy_controller.go | 40 ++++----------------- controllers/operatorpolicy_status.go | 10 +++--- test/e2e/case38_install_operator_test.go | 44 ++---------------------- 3 files changed, 13 insertions(+), 81 deletions(-) diff --git a/controllers/operatorpolicy_controller.go b/controllers/operatorpolicy_controller.go index 720d550b..c8bd1f38 100644 --- a/controllers/operatorpolicy_controller.go +++ b/controllers/operatorpolicy_controller.go @@ -1165,50 +1165,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( @@ -1222,7 +1199,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 { @@ -1242,11 +1218,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 index f413390e..f19418d5 100644 --- a/controllers/operatorpolicy_status.go +++ b/controllers/operatorpolicy_status.go @@ -1005,9 +1005,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), @@ -1027,8 +1026,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 index 91b9b1a5..8052bb27 100644 --- a/test/e2e/case38_install_operator_test.go +++ b/test/e2e/case38_install_operator_test.go @@ -1274,16 +1274,6 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() { 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", @@ -1321,16 +1311,6 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() { 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", @@ -1339,7 +1319,8 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() { Name: secondInstallPlanName, }, }, - Reason: "The InstallPlan is Complete", + Compliant: "Compliant", + Reason: "The InstallPlan is Complete", }}, metav1.Condition{ Type: "InstallPlanCompliant", @@ -1802,16 +1783,6 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() { }, 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", @@ -2100,17 +2071,6 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() { }, 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",