fix: allow the rollout controller to sync apply strategy updates #965
Conversation
| // Process apply strategy updates (if any). | ||
| // | ||
| // Apply strategy changes will be immediately applied to all bindings that have not been | ||
| // marked for deletion yet. Note that even unscheduled bindings will receive this update; | ||
| // as apply strategy changes might have an effect on its Applied and Available status, and | ||
| // consequently on the rollout progress. | ||
| if err := r.processApplyStrategyUpdates(ctx, &crp, allBindings); err != nil { | ||
| klog.ErrorS(err, "Failed to process apply strategy updates", "clusterResourcePlacement", crpName) | ||
| return runtime.Result{}, err | ||
| } | ||
|
|
There was a problem hiding this comment.
should this be even before the waitForResourcesToCleanUp?
There was a problem hiding this comment.
a bigger issue is that user have no idea when their change on the applyStrategy is applied as currently we don't reflect that in the CRP status. We need to add another logic in the CRP (which already watch the bindings) to have the appliedStrategy.
There was a problem hiding this comment.
Currently, the applyStrategy is copied from CRP to the binding during the rolling process which is after this call. What happens if the applyStrategy changes between these two? Should we only change it at one place?
btw, the function is called createUpdateInfo()
There was a problem hiding this comment.
Hi Ryan! The call was placed after the waitForResourcesToCleanUp call as the wait method reports a few unexpected errors, which supposedly should trigger alerts and halt the whole system. I am not 100% sure what would be the best way to proceed in these cases so I put the apply strategy refresh after the wait call to be on the safer side.
Though from a correctness perspective it really does not matter I think.
There was a problem hiding this comment.
For the two-time update case, at this moment in the reconciliation loop both places read the apply strategy from the same CRP object (retrieved in the very beginning of the loop) so it should be fine (consequent changes will trigger subsequent reconciliations), but we should change it at one place for consistency reasons; I will make the edit.
There was a problem hiding this comment.
And for the status part, yeah, that's always a concern 😣 At least we need something that can tell the user if the reported info. are based on the current configuration; I could prepare that PR as well. (though it seems that the CRP status is growing bigger and bigger after iterations 😂)
Description of your changes
This PR fixes an issue where apply strategy updates will not be sync'd to binding objects.
I have:
make reviewableto ensure this PR is ready for review.How has this code been tested
Special notes for your reviewer