Skip to content

Commit

Permalink
Handle stuck deletes better in OperatorPolicy
Browse files Browse the repository at this point in the history 
Refs:
 - https://issues.redhat.com/browse/ACM-9287

Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>
  • Loading branch information
@JustinKuli @openshift-merge-bot
JustinKuli authored and openshift-merge-bot[bot] committed Apr 12, 2024
1 parent c762412 commit 23bec42
Show file tree
Hide file tree
Showing 3 changed files with 273 additions and 15 deletions.
95 changes: 82 additions & 13 deletions controllers/operatorpolicy_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -632,13 +632,15 @@ func (r *OperatorPolicyReconciler) mustnothaveOpGroup(
return nil, changed, nil
}

foundOpGroupName := ""
var foundOpGroupName string
var foundOpGroup *unstructured.Unstructured

for _, opGroup := range foundOpGroups {
emptyNameMatch := desiredOpGroup.Name == "" && opGroup.GetGenerateName() == desiredOpGroup.GenerateName

if opGroup.GetName() == desiredOpGroup.Name || emptyNameMatch {
foundOpGroupName = opGroup.GetName()
foundOpGroup = opGroup.DeepCopy()

break
}
Expand Down Expand Up @@ -684,6 +686,11 @@ func (r *OperatorPolicyReconciler) mustnothaveOpGroup(
}
}

if foundOpGroup.GetDeletionTimestamp() != nil {
// No "early" condition because that would cause the status to flap
return nil, updateStatus(policy, deletingCond("OperatorGroup"), deletingObj(desiredOpGroup)), nil
}

earlyConds := []metav1.Condition{}

if changed {
Expand Down Expand Up @@ -872,6 +879,11 @@ func (r *OperatorPolicyReconciler) mustnothaveSubscription(
return foundSub, nil, changed, nil
}

if foundSub.GetDeletionTimestamp() != nil {
// No "early" condition because that would cause the status to flap
return foundSub, nil, updateStatus(policy, deletingCond("Subscription"), deletingObj(foundSub)), nil
}

earlyConds := []metav1.Condition{}

if changed {
Expand Down Expand Up @@ -1174,21 +1186,40 @@ func (r *OperatorPolicyReconciler) mustnothaveInstallPlan(
earlyConds = append(earlyConds, calculateComplianceCondition(policy))
}

deletedInstallPlans := make([]policyv1.RelatedObject, 0, len(ownedInstallPlans))
anyAlreadyDeleting := false

for i := range ownedInstallPlans {
if ownedInstallPlans[i].GetDeletionTimestamp() != nil {
anyAlreadyDeleting = true
relatedInstallPlans[i] = deletingObj(&ownedInstallPlans[i])

continue
}

err := r.Delete(ctx, &ownedInstallPlans[i])
if err != nil {
changed := updateStatus(policy, foundNotWantedCond("InstallPlan"), deletedInstallPlans...)
changed := updateStatus(policy, foundNotWantedCond("InstallPlan"), relatedInstallPlans...)

if anyAlreadyDeleting {
// reset the "early" conditions to avoid flapping.
earlyConds = []metav1.Condition{}
}

return earlyConds, changed, fmt.Errorf("error deleting the InstallPlan: %w", err)
}

ownedInstallPlans[i].SetGroupVersionKind(installPlanGVK) // Delete stripped this information
deletedInstallPlans = append(deletedInstallPlans, deletedObj(&ownedInstallPlans[i]))
relatedInstallPlans[i] = deletedObj(&ownedInstallPlans[i])
}

if anyAlreadyDeleting {
// reset the "early" conditions to avoid flapping.
earlyConds = []metav1.Condition{}

return earlyConds, updateStatus(policy, deletingCond("InstallPlan"), relatedInstallPlans...), nil
}

updateStatus(policy, deletedCond("InstallPlan"), deletedInstallPlans...)
updateStatus(policy, deletedCond("InstallPlan"), relatedInstallPlans...)

return earlyConds, true, nil
}
Expand Down Expand Up @@ -1285,21 +1316,40 @@ func (r *OperatorPolicyReconciler) mustnothaveCSV(
earlyConds = append(earlyConds, calculateComplianceCondition(policy))
}

deletedCSVs := make([]policyv1.RelatedObject, 0, len(csvList))
anyAlreadyDeleting := false

for i := range csvList {
if csvList[i].GetDeletionTimestamp() != nil {
anyAlreadyDeleting = true
relatedCSVs[i] = deletingObj(&csvList[i])

continue
}

err := r.Delete(ctx, &csvList[i])
if err != nil {
changed := updateStatus(policy, foundNotWantedCond("ClusterServiceVersion"), deletedCSVs...)
changed := updateStatus(policy, foundNotWantedCond("ClusterServiceVersion"), relatedCSVs...)

if anyAlreadyDeleting {
// reset the "early" conditions to avoid flapping
earlyConds = []metav1.Condition{}
}

return earlyConds, changed, fmt.Errorf("error deleting ClusterServiceVersion: %w", err)
}

csvList[i].SetGroupVersionKind(clusterServiceVersionGVK)
deletedCSVs = append(deletedCSVs, deletedObj(&csvList[i]))
relatedCSVs[i] = deletedObj(&csvList[i])
}

if anyAlreadyDeleting {
// reset the "early" conditions to avoid flapping
earlyConds = []metav1.Condition{}

return earlyConds, updateStatus(policy, deletingCond("ClusterServiceVersion"), relatedCSVs...), nil
}

updateStatus(policy, deletedCond("ClusterServiceVersion"), deletedCSVs...)
updateStatus(policy, deletedCond("ClusterServiceVersion"), relatedCSVs...)

return earlyConds, true, nil
}
Expand Down Expand Up @@ -1420,21 +1470,40 @@ func (r *OperatorPolicyReconciler) handleCRDs(
earlyConds = append(earlyConds, calculateComplianceCondition(policy))
}

deletedCRDs := make([]policyv1.RelatedObject, 0, len(crdList))
anyAlreadyDeleting := false

for i := range crdList {
if crdList[i].GetDeletionTimestamp() != nil {
anyAlreadyDeleting = true
relatedCRDs[i] = deletingObj(&crdList[i])

continue
}

err := r.Delete(ctx, &crdList[i])
if err != nil {
changed := updateStatus(policy, foundNotWantedCond("CustomResourceDefinition"), deletedCRDs...)
changed := updateStatus(policy, foundNotWantedCond("CustomResourceDefinition"), relatedCRDs...)

if anyAlreadyDeleting {
// reset the "early" conditions to avoid flapping.
earlyConds = []metav1.Condition{}
}

return earlyConds, changed, fmt.Errorf("error deleting the CRD: %w", err)
}

crdList[i].SetGroupVersionKind(customResourceDefinitionGVK)
deletedCRDs = append(deletedCRDs, deletedObj(&crdList[i]))
relatedCRDs[i] = deletedObj(&crdList[i])
}

if anyAlreadyDeleting {
// reset the "early" conditions to avoid flapping.
earlyConds = []metav1.Condition{}

return earlyConds, updateStatus(policy, deletingCond("CustomResourceDefinition"), relatedCRDs...), nil
}

updateStatus(policy, deletedCond("CustomResourceDefinition"), deletedCRDs...)
updateStatus(policy, deletedCond("CustomResourceDefinition"), relatedCRDs...)

return earlyConds, true, nil
}
Expand Down
19 changes: 19 additions & 0 deletions controllers/operatorpolicy_status.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -459,6 +459,17 @@ func deletedCond(kind string) metav1.Condition {
}
}

// deletingCond returns a NonCompliant condition, with a Reason like '____Deleting',
// and a Message like 'the ____ has a deletion timestamp'
func deletingCond(kind string) metav1.Condition {
return metav1.Condition{
Type: condType(kind),
Status: metav1.ConditionFalse,
Reason: kind + "Deleting",
Message: "the " + kind + " has a deletion timestamp",
}
}

// keptCond returns a Compliant condition, with a Reason like '____Kept',
// and a Message like 'the policy specifies to keep the ____'
func keptCond(kind string) metav1.Condition {
Expand Down Expand Up @@ -839,6 +850,14 @@ func deletedObj(obj client.Object) policyv1.RelatedObject {
}
}

func deletingObj(obj client.Object) policyv1.RelatedObject {
return policyv1.RelatedObject{
Object: policyv1.ObjectResourceFromObj(obj),
Compliant: string(policyv1.NonCompliant),
Reason: "The object is being deleted but has not been removed yet",
}
}

// matchedObj returns a Compliant RelatedObject with reason = 'Resource found as expected'
func matchedObj(obj client.Object) policyv1.RelatedObject {
return policyv1.RelatedObject{
Expand Down
174 changes: 172 additions & 2 deletions test/e2e/case38_install_operator_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1902,15 +1902,185 @@ var _ = Describe("Testing OperatorPolicy", Ordered, func() {
utils.GetWithTimeout(clientManagedDynamic, gvrCRD, "quayregistries.quay.redhat.com",
"", true, eventuallyTimeout)
})
It("Should remove things when enforced while set to Delete everything", func(ctx SpecContext) {
// Change the removal behaviors from Keep to Delete
It("Should report a special status when the resources are stuck", func(ctx SpecContext) {
By("Adding a finalizer to each of the resources")
pol, err := clientManagedDynamic.Resource(gvrOperatorPolicy).
Namespace(opPolTestNS).Get(ctx, opPolName, metav1.GetOptions{})
Expect(err).NotTo(HaveOccurred())
Expect(pol).NotTo(BeNil())

relatedObjects, found, err := unstructured.NestedSlice(pol.Object, "status", "relatedObjects")
Expect(err).NotTo(HaveOccurred())
Expect(found).To(BeTrue())

var opGroupName, installPlanName, csvName string
crdNames := make([]string, 0)

for _, relatedObject := range relatedObjects {
relatedObj, ok := relatedObject.(map[string]interface{})
Expect(ok).To(BeTrue())

objKind, found, err := unstructured.NestedString(relatedObj, "object", "kind")
Expect(err).NotTo(HaveOccurred())
Expect(found).To(BeTrue())

objName, found, err := unstructured.NestedString(relatedObj, "object", "metadata", "name")
Expect(err).NotTo(HaveOccurred())
Expect(found).To(BeTrue())

switch objKind {
case "OperatorGroup":
opGroupName = objName
case "Subscription":
// just do the finalizer; we already know the subscription name
case "ClusterServiceVersion":
csvName = objName
case "InstallPlan":
installPlanName = objName
case "CustomResourceDefinition":
crdNames = append(crdNames, objName)
default:
// skip adding / removing the finalizer for other types
continue
}

utils.Kubectl("patch", objKind, objName, "-n", opPolTestNS, "--type=json", "-p",
`[{"op": "add", "path": "/metadata/finalizers", "value": ["donutdelete"]}]`)
DeferCleanup(func() {
By("removing the finalizer from " + objKind + " " + objName)
utils.Kubectl("patch", objKind, objName, "-n", opPolTestNS, "--type=json", "-p",
`[{"op": "remove", "path": "/metadata/finalizers"}]`)
})
}

By("Setting the removal behaviors to Delete")
utils.Kubectl("patch", "operatorpolicy", opPolName, "-n", opPolTestNS, "--type=json", "-p",
`[{"op": "replace", "path": "/spec/removalBehavior/operatorGroups", "value": "Delete"},`+
`{"op": "replace", "path": "/spec/removalBehavior/subscriptions", "value": "Delete"},`+
`{"op": "replace", "path": "/spec/removalBehavior/clusterServiceVersions", "value": "Delete"},`+
`{"op": "replace", "path": "/spec/removalBehavior/installPlans", "value": "Delete"},`+
`{"op": "replace", "path": "/spec/removalBehavior/customResourceDefinitions", "value": "Delete"}]`)

check(
opPolName,
false,
[]policyv1.RelatedObject{{
Object: policyv1.ObjectResource{
Kind: "OperatorGroup",
APIVersion: "operators.coreos.com/v1",
Metadata: policyv1.ObjectMetadata{
Namespace: opPolTestNS,
Name: opGroupName,
},
},
Compliant: "NonCompliant",
Reason: "The object is being deleted but has not been removed yet",
}},
metav1.Condition{
Type: "OperatorGroupCompliant",
Status: metav1.ConditionFalse,
Reason: "OperatorGroupDeleting",
Message: "the OperatorGroup has a deletion timestamp",
},
`the OperatorGroup was deleted`,
)
check(
opPolName,
false,
[]policyv1.RelatedObject{{
Object: policyv1.ObjectResource{
Kind: "Subscription",
APIVersion: "operators.coreos.com/v1alpha1",
Metadata: policyv1.ObjectMetadata{
Namespace: opPolTestNS,
Name: "project-quay",
},
},
Compliant: "NonCompliant",
Reason: "The object is being deleted but has not been removed yet",
}},
metav1.Condition{
Type: "SubscriptionCompliant",
Status: metav1.ConditionFalse,
Reason: "SubscriptionDeleting",
Message: "the Subscription has a deletion timestamp",
},
`the Subscription was deleted`,
)
check(
opPolName,
false,
[]policyv1.RelatedObject{{
Object: policyv1.ObjectResource{
Kind: "InstallPlan",
APIVersion: "operators.coreos.com/v1alpha1",
Metadata: policyv1.ObjectMetadata{
Namespace: opPolTestNS,
Name: installPlanName,
},
},
Compliant: "NonCompliant",
Reason: "The object is being deleted but has not been removed yet",
}},
metav1.Condition{
Type: "InstallPlanCompliant",
Status: metav1.ConditionFalse,
Reason: "InstallPlanDeleting",
Message: "the InstallPlan has a deletion timestamp",
},
`the InstallPlan was deleted`,
)
check(
opPolName,
false,
[]policyv1.RelatedObject{{
Object: policyv1.ObjectResource{
Kind: "ClusterServiceVersion",
APIVersion: "operators.coreos.com/v1alpha1",
Metadata: policyv1.ObjectMetadata{
Namespace: opPolTestNS,
Name: csvName,
},
},
Compliant: "NonCompliant",
Reason: "The object is being deleted but has not been removed yet",
}},
metav1.Condition{
Type: "ClusterServiceVersionCompliant",
Status: metav1.ConditionFalse,
Reason: "ClusterServiceVersionDeleting",
Message: "the ClusterServiceVersion has a deletion timestamp",
},
`the ClusterServiceVersion was deleted`,
)
desiredCRDObjects := make([]policyv1.RelatedObject, 0)
for _, name := range crdNames {
desiredCRDObjects = append(desiredCRDObjects, policyv1.RelatedObject{
Object: policyv1.ObjectResource{
Kind: "CustomResourceDefinition",
APIVersion: "apiextensions.k8s.io/v1",
Metadata: policyv1.ObjectMetadata{
Name: name,
},
},
Compliant: "NonCompliant",
Reason: "The object is being deleted but has not been removed yet",
})
}
check(
opPolName,
false,
desiredCRDObjects,
metav1.Condition{
Type: "CustomResourceDefinitionCompliant",
Status: metav1.ConditionFalse,
Reason: "CustomResourceDefinitionDeleting",
Message: "the CustomResourceDefinition has a deletion timestamp",
},
`the CustomResourceDefinition was deleted`,
)
})
It("Should report things as gone after the finalizers are removed", func() {
By("Checking that certain (named) resources are not there, indicating the removal was completed")
utils.GetWithTimeout(clientManagedDynamic, gvrClusterServiceVersion, "quay-operator.v3.8.13",
opPolTestNS, false, eventuallyTimeout)
Expand Down

0 comments on commit 23bec42

Please sign in to comment.