Repository navigation
feat: add optional durable workload restart windows - #1214
echupriyanov wants to merge 6 commits into
Conversation
| } | ||
| if !restartwindow.Supported(upgradeFuncs.ResourceType) { | ||
| return true, fmt.Errorf("restart windows do not support %s", upgradeFuncs.ResourceType) | ||
| } |
There was a problem hiding this comment.
When this gate returns an error, Reloader stops processing the whole event, not just this workload. The loop over the remaining workloads ends, the later workload types are never reached, and after 5 retries the event is dropped for good. So every other workload that uses the same ConfigMap or Secret silently misses its reload.
I tested this on a cluster with one ConfigMap shared by two Deployments and a StatefulSet:
- Feature flag off, one Deployment annotated → nothing reloaded.
- Feature flag on, annotation on a CronJob → Deployments reloaded, the StatefulSet never did, because CronJobs are processed before StatefulSets.
- Flag off with only a leftover
pendingannotation and no policy → nothing reloaded.
The third case makes rollback awkward. Once a workload holds pending state, turning the flag off stops these reloads until someone turns it back on or removes the annotation. Nothing in the logs points at the cause.
The docs describe this as "fails closed", but it closes for other workloads too. Could this skip the workload instead: return no error, log the reason, and raise a warning event on that object?
There was a problem hiding this comment.
Fixed in cf7cc5f. Static window configuration errors now skip only the affected workload, emit a Warning Event and skipped metric, and allow the remaining workloads and later kinds to continue. Pending-only state is not treated as policy, so disabling the feature restores normal reload behavior after policy removal. TestPerformActionContinuesAfterInvalidWindowWorkload covers two workloads through PerformAction.
| } | ||
| queue.Forget(target) | ||
| // Revisit closed windows at least once a minute for clock changes and policy edits. | ||
| if delay > time.Minute { |
There was a problem hiding this comment.
Each pending workload is re-checked every minute with a live API call, even when the window is hours away, and a blocked workload repeats its warning event each time. The controller already runs workload informers, so the cache could answer the "is the window open" check. The read just before the patch should stay live, because it carries the resourceVersion precondition
There was a problem hiding this comment.
Fixed in cf7cc5f. The scheduler now evaluates pending state and the window from the workload informer cache, then uses AddAfter for the exact next opening. It performs the live workload/source reads and policy revalidation only when execution is due. Repairable failures use the rate-limited queue, and repeated blocked Warning Events are deduplicated per target/reason.
| @@ -0,0 +1,86 @@ | |||
| // Copyright 2026 Doska. Licensed under the Apache License, Version 2.0. | |||
There was a problem hiding this comment.
This file, state.go and internal/pkg/controller/restart_window.go all carry
// Copyright 2026 Doska..
Could you confirm where this code came from?
There was a problem hiding this comment.
This code was authored by the Doska team, the company behind birbir.uz, specifically for this contribution. It was not copied from another source and is donated to the Reloader project under the repository's Apache-2.0 license. I removed the company-specific source headers in cf7cc5f to follow the repository convention.
| return time.Minute, err | ||
| } | ||
| if changed && recorder != nil { | ||
| recorder.Event(obj, corev1.EventTypeNormal, "WindowReloaded", "Applied pending configuration changes within restart window") |
There was a problem hiding this comment.
When the window opens and the rollout happens, only a Kubernetes event is written. reloader_reload_executed_total and the ALERT_ON_RELOAD webhook never fire. Anyone who watches a dashboard will see reloads drop to zero once they enable windows. Worth recording the same metrics and alerts as the normal path.
There was a problem hiding this comment.
Fixed in cf7cc5f. A deferred execution now records the existing reload and action success/failure metrics, and sends the configured ALERT_ON_RELOAD notification after a successful rollout. Controller recovery tests assert both the success metric and exactly one alert.
| CommandLineOptions = &ReloaderOptions{} | ||
| } | ||
| // Return an independent snapshot: event workers and the window scheduler run concurrently. | ||
| CommandLineOptions := &ReloaderOptions{} |
There was a problem hiding this comment.
This local variable has the same name as the exported CommandLineOptions, so it hides it. That global is now set once at startup, before the flags are parsed, and keeps default values from then on. Nothing in the repo reads it today, but it is in a public package, so anything outside could quietly get the wrong values. Better to remove it or keep it updated.
There was a problem hiding this comment.
Fixed in cf7cc5f while retaining the exported symbol for v1 compatibility. GetCommandLineOptions now returns an independent snapshot without shadowing the global, and startup calls RefreshCommandLineOptions after Cobra parsing and before workers start. A test verifies that parsed values are published.
| token := "" | ||
| if changed { | ||
| sum := sha256.Sum256([]byte(encode(applied))) | ||
| token = hex.EncodeToString(sum[:]) |
There was a problem hiding this comment.
The window path writes restart-hash but leaves the STAKATER_* env var at its old value, so the two paths disagree about the current hash. When a workload later leaves the window path, that stale value causes one restart that no config change asked for.
I measured this with a control. Two identical Deployments shared a ConfigMap. One went through a window rollout, then I removed its annotation. On the next Reloader restart with --reload-on-create --sync-after-restart, and with the ConfigMap untouched, the previously windowed Deployment restarted and the control did not.
The docs recommend those two flags for first-time enrollment, so operators are likely to run with them. Could the window path also refresh the env var when it writes its hash?
There was a problem hiding this comment.
Fixed in cf7cc5f. Window execution now selects the consuming container from the fresh workload and updates the configured normal strategy marker in the same strategic-merge patch as the restart token and pending state. The env-var strategy refreshes STAKATER_<NAME>_<TYPE>; the annotation strategy refreshes last-reloaded-from. Replay tests verify that the next identical event does not cause another rollout.
| name string | ||
| window, enabled, wantError bool | ||
| }{ | ||
| {"unannotated-default", false, false, false}, {"window-enabled", true, true, false}, {"window-disabled-blocks", true, false, true}, |
There was a problem hiding this comment.
The window-disabled-blocks case expects an error, so it encodes the design above. The test also calls upgradeResource directly, so the effect on other workloads stays invisible. A test that goes through PerformAction with two workloads would catch it
There was a problem hiding this comment.
Fixed in cf7cc5f. The handler suite now exercises PerformAction with an invalid window workload followed by a valid workload and verifies processing continues. It also covers pending state without policy after windows are disabled. State/controller tests cover marker synchronization, replay safety, metrics, alerting, and startup recovery.
|
Hi! Thank you for your contribution, it's a welcome change. We are in the process of sun-setting the v1 of the Reloader in favor of the version 2 branch currently available in the repository. This change is to modernize the codebase and it closes a lot of long-running bugs and issues. To reduce further scope-creep of the release of the v2, we have chosen to not accept any new features to the current v1 branch. You are welcome to point the PR towards the v2 branch, though keep in mind the feature will most likely need to be rewritten slightly to fit the updated codebase. Apologies for any inconvenience. |
|
Thank you for clarifying the v1 sunset and for pointing us to v2. I have completed the requested review fixes on this branch so the v1 implementation is internally consistent and remains useful to existing v1 users. We will hold off on opening or developing a v2 PR until this current PR has a clear resolution and we have incorporated any further maintainer feedback. After that, we intend to submit the feature as a separate v2-native contribution rather than retargeting this branch across the rewritten architecture, reusing the annotation contract and behavioral tests while integrating with v2's controller-runtime cache, workload abstraction, metrics, Events, and alerter. |
|
Added replay hardening in
Regression tests cover both request-time suppression and reconciliation of stale pending state. |
3 participants
Problem
Secret and ConfigMap updates can trigger workload rollouts during busy periods. For example, a renewed
application-tlsSecret may need to be picked up bysample-appduring a nightly maintenance window, without losing the request if the controller restarts before that window.Change
Add optional per-workload restart windows, disabled by default through
--enable-restart-windows/ Helmreloader.enableRestartWindows.reloader.stakater.com/restart-windowon workload metadata with an explicit timezone, five-field cron expressions, and durations.Initial support covers Deployments, unpartitioned RollingUpdate StatefulSets, and RollingUpdate DaemonSets. Windows govern rollout start, not completion or application health. Jobs, CronJobs, DeploymentConfigs, Argo Rollouts, CSI sources, and directly managed pods are outside this initial scope. No CRD is introduced.
Review follow-up
Commit
cf7cc5faddresses the requested correctness and operability changes:CommandLineOptionscompatibility snapshot is refreshed after Cobra parsing and no longer shadows itself;Validation
golangci-lint, static binary build,git diff --check, and Helm lint pass.go test -race -count=1 -timeout 180s ./internal/pkg/leadership).The maintainers have indicated that v1 is being sunset and new features should target
v2. This PR remains the complete v1 implementation for users who still need that line; a separate v2-native implementation is the appropriate upstream path rather than retargeting this branch across the rewritten architecture.