Fixes schedule update reset active - #4209
Merged
Merged
Conversation
Signed-off-by: Xavi Garcia <xavi.garcia@suse.com>
Signed-off-by: Xavi Garcia <xavi.garcia@suse.com>
0xavi0
force-pushed
the
fix-schedule-update-reset-active
branch
from
October 9, 2025 09:15
686a16f to
b96fe35
Compare
0xavi0
marked this pull request as ready for review
October 9, 2025 09:35
weyfonk
reviewed
Oct 9, 2025
| k8sclient client.Client, | ||
| cluster, namespace string, | ||
| scheduledExpected, flaggedAsScheduledExpected bool) { | ||
| scheduledExpected, flaggedAsScheduledExpected, activeScheduleExpected bool) { |
Contributor
There was a problem hiding this comment.
nit: I think creating a struct containing these 3 booleans would make the calls above a bit easier to read.
Example:
type expected struct{
job bool
statusScheduled bool
statusActiveSchedule bool
}
... and, since these fields would be visible on the calling line, this function could be renamed to e.g. checkState
// before
checkClusterIsScheduled(scheduler, k8sclient, "test-cluster4", "default", true, true, false)
// after
checkState(scheduler, k8sclient, "test-cluster4", "default", expected{job:true, statusScheduled: true, statusActiveSchedule: true})
Contributor
Author
There was a problem hiding this comment.
changed as suggested
| cluster.Status.ActiveSchedule = false | ||
| } | ||
|
|
||
| // when this function is called is either because we're updating a |
Contributor
There was a problem hiding this comment.
Suggested change
| // when this function is called is either because we're updating a | |
| // This function is called either because we're updating a |
Contributor
Author
There was a problem hiding this comment.
changed as suggested
p-se
approved these changes
Oct 10, 2025
| @@ -413,10 +416,8 @@ func setClustersScheduled(ctx context.Context, c client.Client, clusters []strin | |||
| func updateScheduledClusters(ctx context.Context, scheduler quartz.Scheduler, c client.Client, clustersNew []string, clustersOld []string, namespace string) error { | |||
| // first look for clusters that are not scheduled yet and flag them as scheduled | |||
Contributor
There was a problem hiding this comment.
Is this comment still up-to-date?
Contributor
Author
There was a problem hiding this comment.
I've deleted the comment because now the code is obvious.
Signed-off-by: Xavi Garcia <xavi.garcia@suse.com>
0xavi0
force-pushed
the
fix-schedule-update-reset-active
branch
from
October 13, 2025 12:45
acd19b6 to
237c907
Compare
weyfonk
approved these changes
Oct 13, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There was a bug in the recently merged Scheduling PR.
The bug consists of applying a label to the Schedule that initially matches all clusters, and then, while the Schedule is active, changing the target so that it matches fewer clusters.
The clusters that remained as targets would stay in the ActiveSchedule state, which is incorrect, since modifying the Schedule should reset it and set its status to not active.
Follow-up to #4184.
Related to #3726.
Additional Information
Checklist
[ ] I have updated the documentation via a pull request in thefleet-docs repository.