Emit fewer OperatorPolicy events #208
Conversation
JustinKuli
force-pushed
the
fewer-oppol-events
branch
from
14bc322
to
42f5199
Compare
| earlyComplianceEvents = make([]metav1.Condition, 0) | ||
|
|
||
| desiredSub, desiredOG, changed, err := r.buildResources(policy) | ||
| condChanged = condChanged || changed |
There was a problem hiding this comment.
| condChanged = condChanged || changed | |
| condChanged = changed |
| func (r *OperatorPolicyReconciler) buildResources(ctx context.Context, policy *policyv1beta1.OperatorPolicy) ( | ||
| *operatorv1alpha1.Subscription, *operatorv1.OperatorGroup, error, | ||
| func (r *OperatorPolicyReconciler) buildResources(policy *policyv1beta1.OperatorPolicy) ( | ||
| *operatorv1alpha1.Subscription, *operatorv1.OperatorGroup, bool, error, |
There was a problem hiding this comment.
Could you document the returned bool in the function doc comment?
| } | ||
|
|
||
| if conditionChanged { | ||
| conditionsToEmit = append(conditionsToEmit, calculateComplianceCondition(policy)) |
There was a problem hiding this comment.
Could you please add a comment indicating that this is the "final" compliance event to emit of the current policy state? It wasn't immediately obvious to me and that explains why conditionsToEmit is named as it is rather than earlyComplianceEvents.
| return reconcile.Result{}, utilerrors.NewAggregate(errs) | ||
| } | ||
|
|
||
| func (r *OperatorPolicyReconciler) handleResources(ctx context.Context, policy *policyv1beta1.OperatorPolicy) ( |
There was a problem hiding this comment.
Optional: Comment Missed? It would be good to me if this had a description
There was a problem hiding this comment.
Looks good to me, very clear to understand! Thanks!
Previously, every time the controller determined a piece of the status for an OperatorPolicy, it updated the policy status on the cluster, and emitted a compliance event. This made each condition's logic more self- contained, but leads to more API calls than necessary. Now, each `handle*` function returns whether the status in the cluster needs to be updated, and the controller will make a maximum of one status update pre reconcile. That status update will update all of the conditions and related objects at once. It will also send one compliance event at that point, reflecting the "final" state of the policy. Some `handle*` functions *should* emit multiple events: for example an enforced policy should report when something is missing, and then separately report if that resource was successfully created. So, some of the functions return "early" conditions, which the controller emits before the end of the reconcile. Compare this to "event batches" in ConfigurationPolicy. Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>
JustinKuli
dismissed stale reviews from yiraeChristineKim and mprahl
via
8021043
JustinKuli
force-pushed
the
fewer-oppol-events
branch
from
42f5199
to
8021043
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JustinKuli, mprahl, yiraeChristineKim 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 |
openshift-merge-bot
bot
merged commit 8afaf59
into
open-cluster-management-io:main
Previously, every time the controller determined a piece of the status for an OperatorPolicy, it updated the policy status on the cluster, and emitted a compliance event. This made each condition's logic more self- contained, but leads to more API calls than necessary.
Now, each
handle*function returns whether the status in the cluster needs to be updated, and the controller will make a maximum of one status update pre reconcile. That status update will update all of the conditions and related objects at once. It will also send one compliance event at that point, reflecting the "final" state of the policy.Some
handle*functions should emit multiple events: for example an enforced policy should report when something is missing, and then separately report if that resource was successfully created. So, some of the functions return "early" conditions, which the controller emits before the end of the reconcile. Compare this to "event batches" in ConfigurationPolicy.