Extract cron schedule registration out of runServeCmd - #47562
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughRefactors cron schedule registration by extracting 312 lines of inline registration from cmd/fleet/serve.go into a new cmd/fleet/cron_registration.go module. Introduces cronSchedulesDeps dependency container and startCronSchedules orchestrator function that invokes domain-scoped registration helpers in order: cleanup/maintenance, vulnerabilities, worker integrations, MDM, premium features, and miscellaneous. Updates serve.go to call startCronSchedules with consolidated dependencies, removes unused imports, and starts tracing.StartSettingsPoller earlier in initialization. Adds table-driven unit tests for the vulnerabilityProcessingDisabled helper function validating config evaluation logic. Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/fleet/cron_registration.go (1)
62-68: 🏗️ Heavy liftAdd a regression test around the orchestrator order and gates.
startCronSchedulesis now the single entry point for 30+ conditional registrations, but this refactor still relies on manual verification that helper order and gating match the old inline flow. A small recorder aroundCronSchedules.StartCronSchedulewould make future edits here much safer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/fleet/cron_registration.go` around lines 62 - 68, Add a regression test that verifies the orchestrator order and gating in startCronSchedules by creating a test double for CronSchedules whose StartCronSchedule method records each registration call (name/identifier and any gate-relevant params), then call startCronSchedules with controlled deps toggling the various gates and assert that the recorded sequence matches the expected order of helper registrations (registerCleanupAndMaintenanceCrons, registerVulnerabilityCrons, registerWorkerCrons, registerMDMCrons, registerPremiumCrons, registerMiscCrons) and that gated registrations only occur when their deps permit; implement assertions for both full-enabled and selectively-disabled gate permutations to catch regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cmd/fleet/cron_registration.go`:
- Around line 62-68: Add a regression test that verifies the orchestrator order
and gating in startCronSchedules by creating a test double for CronSchedules
whose StartCronSchedule method records each registration call (name/identifier
and any gate-relevant params), then call startCronSchedules with controlled deps
toggling the various gates and assert that the recorded sequence matches the
expected order of helper registrations (registerCleanupAndMaintenanceCrons,
registerVulnerabilityCrons, registerWorkerCrons, registerMDMCrons,
registerPremiumCrons, registerMiscCrons) and that gated registrations only occur
when their deps permit; implement assertions for both full-enabled and
selectively-disabled gate permutations to catch regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d923fb08-f6dd-4200-975e-8eaceec6dea3
📥 Commits
Reviewing files that changed from the base of the PR and between 268c918 and fa0e5a0a7a46859eeb0f97898878f97b26728f0a.
📒 Files selected for processing (2)
cmd/fleet/cron_registration.gocmd/fleet/serve.go
17fb140 to
4216ea1
Compare
|
Hello @MagnusHJensen - I am slicing the cron related code out of serve.go - For context on the structure: the domain-grouped Would you be able to take a review, please? And let me know wdyt of the approach forward for other cron extractions. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #47562 +/- ##
=======================================
Coverage 67.21% 67.21%
=======================================
Files 3630 3631 +1
Lines 229526 229511 -15
Branches 11956 11956
=======================================
+ Hits 154267 154268 +1
+ Misses 61392 61377 -15
+ Partials 13867 13866 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Hi @raju249 Sorry I didn't get to this today, I'm hoping to review it tomorrow. |
|
Hi @raju249 There seems to be a merge conflict now, do you mind resolving that? Sorry for the wait. |
| if err := deps.cronSchedules.StartCronSchedule( | ||
| func() (fleet.CronSchedule, error) { | ||
| return cronUpgradeCodeSoftwareMigration(ctx, deps.instanceID, deps.ds, deps.softwareInstallStore, deps.logger) | ||
| }, | ||
| ); err != nil { | ||
| deps.initFatal(err, fmt.Sprintf("failed to register %s", fleet.CronUpgradeCodeSoftwareMigration)) | ||
| } | ||
| } |
There was a problem hiding this comment.
I was thinking since a lot of these are similar, could we make a helper/wrapper function?
smth like:
startCronSchedule := func(name fleet.CronScheduleName, fn func() (fleet.CronSchedule, error)) {
if err := deps.cronSchedules.StartCronSchedule(fn); err != nil {
deps.initFatal(err, fmt.Sprintf("failed to register %s", name))
}
}
startCronSchedule(fleet.CronUninstallSoftwareMigration, func() (fleet.CronSchedule, error) {
return cronUninstallSoftwareMigration(ctx, deps.instanceID, deps.ds, deps.softwareInstallStore, deps.logger)
})What do you think?
| } | ||
|
|
||
| if err := deps.cronSchedules.StartCronSchedule(func() (fleet.CronSchedule, error) { | ||
| commander := apple_mdm.NewMDMAppleCommander(deps.mdmStorage, deps.mdmPushService) |
There was a problem hiding this comment.
I think we might be able to use a shared MDMAppleCommander as part of the new cronSchedulesDeps, it uses values that both support concurrency, and it's the same storage and pushService passed to each, and the commander itself is stateless. So that could be a nice change while we are at it.
| if deps.config.Vulnerabilities.DisableSchedule { | ||
| deps.logger.InfoContext(ctx, "vulnerabilities schedule disabled via vulnerabilities.disable_schedule") | ||
| } | ||
| if deps.config.Vulnerabilities.CurrentInstanceChecks == "no" || deps.config.Vulnerabilities.CurrentInstanceChecks == "0" { |
There was a problem hiding this comment.
nit: Could we wrap this into a method on the VulnerabilitiesConfig something like IsDisabledByInstanceCheck that way we share the check logic in vulnerabilityProcessingDisabled and here. Which disallows drift in the future.
The DisableSchedule is fine to keep, as it's a single bool check.
| // one-shot VPP country backfill), recovery lock passwords, managed local | ||
| // account rotation, activities streaming, and the calendar schedule. | ||
| func registerPremiumCrons(ctx context.Context, deps cronSchedulesDeps) { | ||
| if deps.license.IsPremium() { |
There was a problem hiding this comment.
Since all in here requires premium, could we use a guard clause and early exit?
if !deps.license.IsPremium() {
return
}| } | ||
| } | ||
|
|
||
| if deps.license.IsPremium() && deps.config.Activity.EnableAuditLog { |
There was a problem hiding this comment.
With the guard clause this becomes just:
if deps.config.Activity.EnableAuditLog {
|
@raju249 Left a couple of small comments for improvements, but otherwise it looks good! |
Move the 33 StartCronSchedule registrations into a new cmd/fleet/cron_registration.go, grouped by domain (cleanup/maintenance, vulnerabilities, workers, MDM, premium, misc) and driven by a cronSchedulesDeps struct. runServeCmd builds the struct once and makes a single startCronSchedules call; registration order and arguments are preserved exactly. A register helper removes the repetitive StartCronSchedule + initFatal boilerplate, the (stateless) MDMAppleCommander is built once and shared via the deps struct, registerPremiumCrons uses an early-return guard, and the vuln current_instance_checks logic moves to VulnerabilitiesConfig.IsDisabledByInstanceCheck so the predicate and the disable logging share it. vulnerabilityProcessingDisabled is unit-tested. Refs fleetdm#33370
4216ea1 to
bd6e67a
Compare
|
Thanks for the review @MagnusHJensen I rebased and fixed the conflicts. Your suggestions made sense to me, addressed those by applying them directly in the changes. |
This premium schedule was added on main while the extraction PR was open; it was dropped when resolving the rebase conflict. Restore its registration in registerPremiumCrons, preserving its original position. Refs fleetdm#33370
Second incremental merge of upstream into merge/upstream-main. Only 3 conflicts (vs 1952 in the initial merge), all resolved: - cmd/fleet/serve.go: upstream extracted cron registration into cron_registration.go (fleetdm#47562); relocated the OpenFrame query_results TTL cleanup schedule into registerCleanupAndMaintenanceCrons. - charts/fleet/{Chart.yaml,values.yaml}: kept the fork's own release versioning (v6.8.4 / appVersion v4.81.2); an upstream sync must not auto-bump the fork's chart/app/image versions. go build ./... is green.
Extracts the `/api/` request timeout/body-size override middleware out of `runServeCmd` and into `apiTimeoutOverrideHandler` in a new `cmd/fleet/http_middleware.go`. Same pattern as the prior extractions on this issue (#44929, #45343, #45583, #46166, #46421, #46517, #46742, #46830, #46893, #47151, #47562). `runServeCmd` drops from ~1000 to ~900 lines, and `serve.go` from 1475 to 1373. The middleware is the `~100`-line `rootMux.HandleFunc("/api/", ...)` closure that applies per-route read/write deadline overrides for endpoints that legitimately run long — synchronous script runs, large software-installer and bootstrap-package uploads, the Android enterprise signup SSE stream, and large MDM profile batch operations — and, for package-upload routes, caps the request body and threads the configured max installer size through the request context. Behavior is preserved — the handler is moved verbatim and wired into `rootMux` via a single `apiTimeoutOverrideHandler(apiHandler, config, logger)` call, so the same routes get the same overrides and every request still falls through to `apiHandler.ServeHTTP`. The now-unused `scripts` and `installersize` imports drop out of `serve.go`. On test scope: `TestAPITimeoutOverrideHandler` verifies the real decision in this middleware — that package-upload paths thread the configured max installer size into the request context (and non-upload requests keep the default) — and that the wrapped API handler is always invoked. The deadline overrides themselves go through `http.ResponseController`, which a unit-test `ResponseRecorder` doesn't support (the handler logs and proceeds, as in production), so those are exercised by booting the server rather than asserted in a unit test. **Related issue:** Refs #33370 # Checklist for submitter - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually (verified via local server boot) - Changes file: not applicable — internal refactor with no user-visible behavior change <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Refactor** * Improved timeout handling for long-running operations across the API. Script execution, file uploads, Server-Sent Event streams, and batch operations now have optimized request timeouts and body size limits. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
) Adds an end-to-end boot test for `runServeCmd`, the main server entry point. This is the coverage milestone for #33370: `serve.go` goes from ~7% to ~64%, and `runServeCmd` itself from 0% to ~62%. The earlier PRs on this issue (#44929, #45343, #45583, #46166, #46421, #46517, #46742, #46830, #46893, #47151, #47562, #47891) extracted testable pieces out of `runServeCmd`, but the function itself stayed at 0% — it blocks on an OS signal and wires the entire server together, so the only way to cover it is to actually boot it. This PR does that. `TestRunServeCmd` (gated behind `MYSQL_TEST` + `REDIS_TEST`) boots the full server against a real migrated test MySQL and Redis, waits for `/healthz`, then cancels the command context to trigger a graceful shutdown. It covers two paths: - **Full boot with Apple MDM enabled** — a 32-byte server private key brings up the Apple MDM protocol services and the host-identity / conditional-access SCEP setup, so the boot exercises the MDM startup path as well as the core wiring, cron schedules, and HTTP server. - **Fail-fast on bad config** — an invalid Redis host-cache configuration (enabled with a non-positive TTL) aborts startup through `initFatal` and returns rather than serving, covering the Redis-init error path and the nil-pool guard. Beyond coverage, this doubles as a regression net for the ongoing `runServeCmd` slicing: a future change that breaks startup now fails this test instead of reaching a release. **One production change**, in `runServeCmd`'s shutdown `select`: it now also watches `cmd.Context().Done()`. This is inert in production — the root command runs via `Execute()` (not `ExecuteContext()`), so `cmd.Context()` is `context.Background()` and never cancels. Only the test runs the command with a cancelable context, which is how it shuts the server down without sending a real signal (a `SIGTERM` would kill the test binary). A couple of notes for reviewers: - The test uses `os.Setenv` (not `t.Setenv`) because the MySQL test helper marks the test parallel; the boot scenarios run as serial subtests so the process-global config env doesn't race. - `runServeCmd` registers metrics with the process-global Prometheus registry, which can only happen once per process, so there is a single full boot here; the error-path scenario fails before that registration. - The test DB is loaded from a schema dump that doesn't mark every data migration as applied, so the boot runs with `FLEET_UPGRADES_ALLOW_MISSING_MIGRATIONS=1`. It adds ~2s to the `cmd/fleet` (`main`) test bundle, which is well off the CI critical path. **Related issue:** Refs #33370 # Checklist for submitter - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually (verified locally: boots to /healthz, graceful shutdown, ~64% serve.go coverage) - Changes file: not applicable — internal test coverage with no user-visible behavior change <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved server shutdown handling to stop cleanly when the running command’s context is canceled, not only on OS signals. * Added stronger startup validation to fail fast for invalid Redis host-cache configuration (e.g., non-positive TTL). * **Tests** * Added an end-to-end test that boots the server against real MySQL/Redis, verifies graceful startup/shutdown, and confirms fast-fail behavior for misconfiguration. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Extracts the cron schedule registration out of
runServeCmdand into a newcmd/fleet/cron_registration.go. Same pattern as the prior extractions on this issue (#44929, #45343, #45583, #46166, #46421, #46517, #46742, #46830, #46893, #47151). This is the largest slice so far —runServeCmddrops from ~1300 to ~1000 lines, andserve.gofrom 1776 to 1472.The 33
StartCronScheduleregistrations move into onestartCronSchedulesentry point backed by acronSchedulesDepsstruct (the dependencies the closures previously captured fromrunServeCmd). Registration is grouped by domain:registerCleanupAndMaintenanceCrons— chart data collection, thecron_statscleanup goroutine, software migrations, frequent cleanups, cleanups-then-aggregation, query results cleanup, upcoming activities, usage statistics, batch activities.registerVulnerabilityCrons— the vulnerabilities schedule, or the remote-trigger proxy when processing is disabled on this instance.registerWorkerCrons— automations and worker integrations.registerMDMCrons— Apple MDM worker, DEP profile assigner, service discovery, the Apple/Windows/Android profile managers, the Android device reconciler, the Android policy migrations, and the APNs pusher.registerPremiumCrons— iPhone/iPad refetcher and reviver, maintained apps, VPP app version refresh (and the one-shot VPP country backfill), recovery lock passwords, managed local account rotation, activities streaming, and the calendar schedule.registerMiscCrons— host vitals label membership and the batch activity completion checker.Behavior is preserved — the schedules register in the same order with the same arguments, the same conditionals gate them (premium, audit log, env vars, software store presence), and the
configis threaded as a pointer so the&configandconfig.Calendarmutations inside the calendar closure keep their original semantics.cmd/fleet/cron.go(the schedule definitions) is intentionally untouched; only the wiring moved.One unit test added:
TestVulnerabilityProcessingDisabledcovers the vuln enable/disable predicate extracted intovulnerabilityProcessingDisabled, including the legacycurrent_instance_checks"0"value. The rest of the file is dependency-wiring relocation with no further decision logic to unit-test — those paths construct real schedules, so they stay covered by the existing suite and integration tests. The fullcmd/fleetsuite passes against MySQL + Redis, and a local server boot confirms the same 30 cron schedules start as before (verified against the "started cron schedules" log line).Related issue: Refs #33370
Checklist for submitter
Summary by CodeRabbit