Improve OperatorGroup removal logic #230
Conversation
JustinKuli
force-pushed
the
11022-11077-mnh-operatorgroup
branch
2 times, most recently
from
c625f0f
to
a90a129
Compare
|
|
||
| break | ||
| if len(foundSubscriptions) != 0 { |
There was a problem hiding this comment.
Since the subscription is handled after the operator group, I think this will be true and then the next reconcile this would be false. Perhaps filtering out the subscription if it should be deleted or rearrange the order in handleResources if it's must not have?
There was a problem hiding this comment.
Personally I prefer the simplicity of just expecting it to be handled in a future reconcile. Do you think there would be a big advantage to changing the order just for mustnothave? I specifically don't want to take an action expecting that the subscription will be removed, that just seems like asking for some unforeseen problem.
There was a problem hiding this comment.
Looking at it more, I don't think this is just an ordering problem: if the policy was only being informed, right now it would report the OperatorGroup as compliant, because it's being used by the (NonCompliant) Subscription. So... something more to think about and fix
| anyAlreadyDeleting := false | ||
| deletionErrs := make([]string, 0) | ||
|
|
||
| for _, opGroupToDelete := range toDelete { |
There was a problem hiding this comment.
After a discussion, we agreed that we should only allow the removal behavior of Keep and DeleteIfUnused for operator group.
There was a problem hiding this comment.
And, if there are multiple operator groups, just report it as noncompliant and don't try to adjust things.
In some clusters, a default OperatorGroup might be provided in a namespace for "global" operators. This OperatorGroup was not being correctly identified by the policy: in fact any pre-existing group that did not match the group specified by the policy, or the one that would be generated when the policy does not specify a group, was not reported correctly. Now, pre-existing "special" operator groups should be reported and handled correctly. If they are owned by another resource, they will be considered as "used" for the purposes of the DeleteIfUnused setting. This also removes the "regular" `Delete` option for OperatorGroups: that was too likely to cause confusion. Refs: - https://issues.redhat.com/browse/ACM-11022 - https://issues.redhat.com/browse/ACM-11077 Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>
JustinKuli
force-pushed
the
11022-11077-mnh-operatorgroup
branch
from
a90a129
to
06ba1e1
Compare
|
The force-push diff is probably very helpful for anyone that had already looked at the initial changed: https://github.com/open-cluster-management-io/config-policy-controller/compare/a90a129630b3ae4062e8127237ea38a8c5978354..06ba1e1020039bda973a98dd5a61714c4da52bb1 |
| // Missing OperatorGroup: report Compliance | ||
| changed := updateStatus(policy, missingNotWantedCond("OperatorGroup"), missingNotWantedObj(desiredOpGroup)) | ||
|
|
||
| return nil, changed, nil | ||
| } | ||
|
|
||
| var foundOpGroupName string | ||
| var foundOpGroup *unstructured.Unstructured | ||
| if len(allFoundOpGroups) > 1 { |
There was a problem hiding this comment.
Just to confirm, OperatorPolicy would never add an additional operator group if one already existed so there is never a time where this would prevent undoing an OperatorPolicy mistake by the user right? This would only happen if there was already multiple operator groups when the OperatorPolicy was defined or the must have OperatorPolicy was defined and then an additional operator group was added afterwards.
There was a problem hiding this comment.
If the controller sees an OperatorGroup already in the namespace, it will not create another one, even if the policy specifies a different OperatorGroup than the one that was found.
But, there is always the possibility of a race condition where something else creates an OperatorGroup between when this controller determines one is not yet present and when it submits its request to create one. But, since this controller does not have concurrent reconciles, it at least can not race with itself.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JustinKuli, mprahl 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:
Approvers can indicate their approval by writing |
|
/unhold I don't think that additional reviews are required. |
openshift-merge-bot
bot
merged commit 21143ac
into
open-cluster-management-io:main
In some clusters, a default OperatorGroup might be provided in a namespace for "global" operators. This OperatorGroup was not being correctly identified by the policy: in fact any pre-existing group that did not match the group specified by the policy, or the one that would be generated when the policy does not specify a group, was not reported correctly.
Now, pre-existing "special" operator groups should be reported and handled correctly. If they are owned by another resource, they will be considered as "used" for the purposes of the DeleteIfUnused setting.
Refs: