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

Reduce number of related InstallPlans #239

Merged
merged 1 commit into from
May 15, 2024
Merged

Reduce number of related InstallPlans #239

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 6 additions & 34 deletions controllers/operatorpolicy_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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 {
Expand All @@ -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):
Expand Down
10 changes: 5 additions & 5 deletions controllers/operatorpolicy_status.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand All @@ -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)
}

Expand Down
44 changes: 2 additions & 42 deletions test/e2e/case38_install_operator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand All @@ -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",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down