feat: do not follower scheduling when dry run #296
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #296 +/- ##
==========================================
+ Coverage 31.42% 31.44% +0.02%
==========================================
Files 123 122 -1
Lines 14343 14346 +3
==========================================
+ Hits 4507 4511 +4
+ Misses 9423 9421 -2
- Partials 413 414 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| if fedObj.GetAnnotations()[common.EnableFollowerSchedulingAnnotation] != common.AnnotationValueTrue || | ||
| fedObj.GetAnnotations()[common.NoSchedulingAnnotation] == common.AnnotationValueTrue || util.SkipSync(fedObj) { |
There was a problem hiding this comment.
Is checking the DryRun annotation sufficient? Ideally follower controller shouldn't need to care about the federated object status.
There was a problem hiding this comment.
I think it needs. For example, assume the workload have been propagated to member clusters, users added the dry-run anno, at this time, dry run will not work, but because we cut off the relationship of leader and follower, follower will not propagate, this will cause the original workload to not work properly.
There was a problem hiding this comment.
Makes sense. But if this is the case, we probably shouldn't have written the dry-run results into placements, as they can easily be misused by other controllers. An annotation would be better. What do you think?
There was a problem hiding this comment.
I have tested several situations, and I think they are relatively stable. Placements now only have three controller: global-scheduler, follower-controller and ns-auto controller. 90% of all cases are global-scheduler. Another point is that we are not yet sure whether this restriction len(fo.status().clusters) == 0will be removed in the future. This is mainly because there is no such scenario yet. So, I think putting into placement is OK.
Poor12
force-pushed
the
no-follower-when-dryrun
branch
from
9c979e1 to
42e8371
Compare
| fedResource.RecordEvent("DryRunWorked", "Dry run worked for %s", fedResource.FederatedName()) | ||
| return worker.StatusAllOK | ||
| } | ||
| fedResource.RecordEvent("DryRunSkipped", "Dry run skipped because resource has been propagated") |
There was a problem hiding this comment.
Add an event to prompt the user that dryRun does not take effect.
Signed-off-by: shentiecheng <[email protected]>
Poor12
force-pushed
the
no-follower-when-dryrun
branch
from
42e8371 to
ec4b269
Compare
feat: do not follower scheduling when dry run
No description provided.