Optimize Apple profile reconciler approach by moving logic to code - #45573
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #45573 +/- ##
==========================================
+ Coverage 66.82% 66.88% +0.05%
==========================================
Files 2754 2762 +8
Lines 220192 222171 +1979
Branches 10878 10878
==========================================
+ Hits 147151 148601 +1450
- Misses 59747 60083 +336
- Partials 13294 13487 +193
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
… handlers
Optimizes the Apple profile reconciler path by scanning hosts in bounded
batches (default 5k per tick via a host_uuid cursor in Redis) and
computing desired state in Go using per-label-mode handlers instead of a
large MySQL UNION join.
Each tick:
1. ListAppleMDMHostsForReconcileBatch pulls the next batch of
Apple-enrolled host UUIDs by cursor (no profile-status check).
2. ListAppleProfilesForReconcile loads the full profile catalog and
label assignments once per tick.
3. BulkGetHostLabelMemberships and BulkGetHostMDMAppleProfilesByUUIDs
load the per-batch label memberships and current state.
4. computeAppleReconcileDeltas dispatches each (host, profile) pair
to one of four in-code handlers: no-labels, include-all,
include-any, exclude-any. Broken-label and dynamic-label-timing
semantics match the legacy SQL.
5. The downstream CA-throttle, user-enrollment, host-being-set-up
skip, BulkUpsertMDMAppleHostProfiles, and ProcessAndEnqueueProfiles
flow is reused unchanged.
Gated by FLEET_MDM_APPLE_BATCHED_RECONCILER=true so the legacy path
stays the default. The cursor is persisted in Redis (mysqlredis wrapper)
and resets when a full pass completes, mirroring the Windows reconciler
pattern. Includes unit tests for each handler and the delta computation.
https://claude.ai/code/session_01Vvy1keXRKZRzDbJQd7dzDn
Drop the FLEET_MDM_APPLE_BATCHED_RECONCILER toggle so loadtests don't need an env var to enable the batched path. The branch always runs ReconcileAppleProfilesBatched; the legacy ReconcileAppleProfiles function is still present for diffing/reference but no longer wired into the cron. https://claude.ai/code/session_01Vvy1keXRKZRzDbJQd7dzDn
Split AppleProfileForReconcile's single LabelMode + Labels into: - IncludeMode (None / All / Any) + IncludeLabels - ExcludeLabels (always "exclude any" semantic) The dispatcher composes the two gates: a profile applies iff the include gate passes (skipped when IncludeMode == None) AND the exclude gate passes (skipped when ExcludeLabels is empty). The existing per-gate handlers are unchanged in semantics; they now take []AppleProfileLabelRef directly so they're pure functions composable in any combination. Datastore loader partitions label rows by mcpl.exclude into the two slices and derives IncludeMode from the include-row require_all flag. Broken-label exemption from removal now considers labels in either slice. https://claude.ai/code/session_01Vvy1keXRKZRzDbJQd7dzDn
Surface where a tick stalls and which commands the enqueue path fails on so the cursor-stuck symptom can be traced to a specific step. New log lines (look for cron=mdm_apple_profile_manager): - batched reconcile: listed hosts - batched reconcile: loaded profiles - batched reconcile: computed deltas (with to_install / to_remove) - batched reconcile: before bulk upsert - batched reconcile: enqueue complete (succeeded / failed cmd counts) - batched reconcile: failed command UUID (per failed cmd, with err) - batched reconcile: tick errored; cursor not advanced (with err) - batched reconcile: cursor advanced / tick complete, cursor unchanged - batched reconcile: ProcessAndEnqueueProfiles returned error The cursor-advance deferred block was rewritten as a switch so the outcome (errored / advanced / unchanged) is always logged with the cursor values, making it easy to spot ticks that stall mid-pass. https://claude.ai/code/session_01Vvy1keXRKZRzDbJQd7dzDn
ListAppleProfilesForReconcile used COALESCE(lbl.created_at, '2000-01-01 00:00:00') with a string-literal default, which made MySQL coerce the result column to VARCHAR. The MySQL driver returns that as []uint8, and sql.NullTime.Scan can only accept time.Time or nil — producing: sql: Scan error on column index 4, name "label_created_at": unsupported Scan, storing driver.Value type []uint8 into type *time.Time This errored on every tick, so the reconciler never reached the deferred cursor advance — the cursor stayed pinned at whatever value a prior successful run had set it to, and no further work got done. Drop the COALESCE so the column stays TIMESTAMP. NULL → invalid NullTime → zero time.Time, which the exclude-any handler already treats as "no timing check" (matching the broken-label semantics). https://claude.ai/code/session_01Vvy1keXRKZRzDbJQd7dzDn
The apple_mdm worker's installProfilesForEnrollingHost previously ran
its own ad-hoc reconcile: list profiles to install via the legacy
4-way UNION SQL (h.uuid=? filter), build install targets, upsert, then
ProcessAndEnqueueProfiles. This duplicated label / team / platform
semantics that already exist in the batched cron path — risking drift
between what the cron decides should apply and what the worker
decides on enrollment.
Extract the in-memory pipeline into a per-host variant:
service.ReconcileAppleProfilesForHost(ctx, ds, commander, logger,
hostUUID, certProfilesLimit) ([]string, error)
It calls the new GetAppleMDMHostForReconcile datastore method, reuses
ListAppleProfilesForReconcile / BulkGetHostLabelMemberships /
BulkGetHostMDMAppleProfilesByUUIDs, runs the same
computeAppleReconcileDeltas + handlers as the cron, and returns the
list of enqueued cmd UUIDs so the worker can wait on them.
executeAppleReconcileBatch:
- Now returns ([]string, error). The cron path discards the cmd UUIDs.
- Accepts a nil redisKeyValue to skip the "host being set up" check.
The per-host path passes nil because by construction that host IS
being set up and we explicitly want to install its profiles now.
Worker integration uses a function-field hook
(ReconcileAppleProfilesForHostFn) wired in cron.go, avoiding an
import cycle (service already imports worker).
The DDM kick (DeclarativeManagement command) at the end of
installProfilesForEnrollingHost is preserved as-is.
https://claude.ai/code/session_01Vvy1keXRKZRzDbJQd7dzDn
Add ReconcileAppleDeclarationsBatched alongside ReconcileAppleProfilesBatched
and route the existing manage_apple_declarations cron job to it. The
declaration path is structured as a deliberate mirror of the profile
path so label-membership semantics are guaranteed not to drift between
the two reconcilers.
Shared layer (single source of truth — no parallel copy for DDM):
- new fleet.AppleLabeledEntity interface (GetTeamID, GetIncludeMode,
GetIncludeLabels, GetExcludeLabels, HasBrokenLabel)
- AppleProfileForReconcile and the new AppleDeclarationForReconcile
both implement it
- appleEntityAppliesToHost is the one dispatcher used by both
reconcilers; appleProfileHandler{IncludeAll,IncludeAny,ExcludeAny}
stay entity-agnostic (they already take []AppleProfileLabelRef)
- new TestAppleEntityAppliesToHost_DeclarationsShareSameDispatcher
test pins the contract: if a profile and a declaration with the
same label config ever produce different applies-to-host results,
that test breaks loudly
Declaration-specific additions (compute + execute, intentionally thin):
- ListAppleDeclarationsForReconcile loads decls + their labels via
the same mcpl join (apple_declaration_uuid)
- BulkGetHostMDMAppleDeclarationsByUUIDs returns current state per host
- BulkUpsertMDMAppleHostDeclarations writes diff'd rows (respects
per-row Status / OperationType, unlike the legacy helper which
forces a single status across all rows)
- GetMDMAppleDeclarationReconcileCursor / SetMDMAppleDeclarationReconcileCursor
persist a separate Redis cursor so profile and declaration passes
advance independently
- computeAppleDeclarationDeltas runs appleEntityAppliesToHost
against declarations, diffs against current rows on token + secrets +
operation_type/status, and produces (changedHostUUIDs, host_decl
rows to write). Sends one DeclarativeManagement command per tick
targeting only the changed hosts.
The legacy ReconcileAppleDeclarations (which calls the legacy 4-way
UNION SQL via MDMAppleBatchSetHostDeclarationState) is left in place for
reference / rollback but is no longer wired into the cron.
https://claude.ai/code/session_01Vvy1keXRKZRzDbJQd7dzDn
Tests construct AppleMDM workers without setting the new ReconcileAppleProfilesForHost field. The previous nil-check returned a hard error, breaking those tests. The cron-based reconciliation will pick up the host on its next pass anyway — the post-enrollment install is a setup-experience speedup, not a correctness requirement — so log a warning and skip instead of erroring. https://claude.ai/code/session_01Vvy1keXRKZRzDbJQd7dzDn
The per-host reconciler in service was reached via a function-field injection on the worker struct to avoid the service<->worker import cycle. That meant worker tests without injection silently skipped the install flow, which is the opposite of what we want — tests should exercise the real path. Move the shared compute + execute pipeline to server/mdm/apple: apple_mdm.EntityAppliesToHost — single dispatcher (profiles + decls) apple_mdm.HandlerIncludeAll/Any apple_mdm.HandlerExcludeAny apple_mdm.ComputeReconcileDeltas apple_mdm.ComputeDeclarationDeltas apple_mdm.IsBrokenProfile / IsBrokenDeclaration apple_mdm.ExecuteReconcileBatch apple_mdm.ReconcileProfilesForHost Now both the cron (service.ReconcileAppleProfilesBatched and ReconcileAppleDeclarationsBatched) and the worker (installProfilesForEnrollingHost) call directly into apple_mdm.X with no injection. The worker struct loses the ReconcileAppleProfilesForHost function field and gains a CertProfilesLimit int field (set by cron.go from config). Worker tests that don't mock anything now hit the real flow against the real datastore via mysqltest — same as before my refactor, just with the shared label/diff code path. Service files shrink from ~840 + ~290 lines to ~170 + ~165 lines. The handler / compute / drift-prevention tests move to server/mdm/apple/reconcile_test.go alongside the code they cover. https://claude.ai/code/session_01Vvy1keXRKZRzDbJQd7dzDn
- BulkGetHostLabelMemberships: replace manual rows.Close() management with sqlx.SelectContext into a typed slice. Cleaner, fewer error paths, fixes the sqlclosecheck complaints flagged by golangci-lint. - server/fleet/apple_mdm.go: gofmt the getter declarations on AppleProfileForReconcile / AppleDeclarationForReconcile. - server/mdm/apple/reconcile.go ExecuteReconcileBatch: add defensive pp != nil check after GetMatchingProfileInCurrentState to satisfy nilaway. The legacy code uses the same pattern but is grandfathered in the linter's incremental scope. https://claude.ai/code/session_01Vvy1keXRKZRzDbJQd7dzDn
…t path The legacy installProfilesForEnrollingHost explicitly called FilterOutUserScopedProfiles on the list before processing, deferring user-channel profile delivery to the cron reconciler. My refactor routed user-scoped profiles through ExecuteReconcileBatch which writes Status=NULL rows when the user channel isn't ready yet. That changed the post-enrollment host_mdm_apple_profiles row count (more rows, immediately after enrollment) and broke any test or poller that compares counts or waits for a specific row set. Add FilterOutUserScopedProfiles in ReconcileProfilesForHost so the worker per-host path matches legacy behavior: only device-scoped profiles get written by the worker, and the cron picks up the user- scoped ones via the isAwaitingUserEnrollment path as before. https://claude.ai/code/session_01Vvy1keXRKZRzDbJQd7dzDn
The name change makes the enrollment-specific behavior explicit: this path filters out user-scoped profiles (deferred to the cron's user- enrollment-aware path), skips the Redis "host being set up" check because by construction the host IS being set up, and is intended for the post-DEP / post-manual-enrollment worker tasks. Reads naturally as 'reconcile profiles for an enrolling host' rather than the ambiguous 'for host' which could be confused with a generic per-host reconciler. https://claude.ai/code/session_01Vvy1keXRKZRzDbJQd7dzDn
There was a problem hiding this comment.
Pull request overview
Refactors Apple MDM profile + DDM declaration reconciliation to compute desired state in Go (batched/cursor-based) instead of relying on large SQL unions, and reuses shared dispatcher logic across cron + post-enrollment worker to prevent semantic drift.
Changes:
- Added batched, cursor-based reconcilers for Apple profiles and declarations, with shared “entity applies to host” logic in
server/mdm/apple. - Updated cron and enrollment worker to use the new batched/shared reconciliation paths (including passing certificate profile throttling into the worker).
- Expanded datastore interface + MySQL/MySQL+Redis implementations to support batched host listing, bulk fetches, and cursor persistence.
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| server/worker/apple_mdm.go | Worker enrollment path now delegates profile reconciliation to shared apple_mdm logic and uses a configured cert-profile limit. |
| server/service/integration_vpp_install_test.go | Tightens test assertions to fail on unexpected MDM command types. |
| server/service/integration_mdm_test.go | Switches integration cron execution to batched reconcilers; acknowledges DeclarativeManagement commands in a loop. |
| server/service/apple_mdm_declarations_batched.go | Introduces batched/cursor-based DDM declaration reconciler entry point. |
| server/service/apple_mdm_batched.go | Introduces batched/cursor-based Apple profile reconciler entry point. |
| server/platform/mysql/testing_utils/testing_utils.go | Adjusts MySQL test config connection pool settings. |
| server/mock/datastore_mock.go | Extends datastore mock with new batched reconciliation methods/cursor methods. |
| server/mdm/apple/reconcile.go | Adds shared dispatcher + delta computation + batch execution pipeline used by cron and enrollment path. |
| server/mdm/apple/reconcile_test.go | Adds unit tests for dispatcher/handlers and delta computation behavior. |
| server/fleet/datastore.go | Extends the datastore interface with new reconciliation/bulk APIs and cursor APIs. |
| server/fleet/apple_mdm.go | Adds new Fleet types/interfaces used by the in-memory reconciler (hosts, labeled entities, include modes). |
| server/datastore/mysqlredis/apple_recon_cursor.go | Persists Apple profile reconcile cursor in Redis for batched paging. |
| server/datastore/mysqlredis/apple_declaration_recon_cursor.go | Persists Apple declaration reconcile cursor in Redis (separate key from profiles). |
| server/datastore/mysql/apple_mdm_batched.go | Implements MySQL queries and bulk operations needed by the batched reconcilers. |
| cmd/fleet/serve.go | Passes configured certificate profile limit into the Apple MDM worker schedule. |
| cmd/fleet/cron.go | Wires cron jobs to the new batched reconcilers and propagates cert-profile limit to worker. |
| changes/46153-optimize-apple-profile-reconciler | Release note entry for the reconciliation optimization. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
server/mdm/apple/reconcile_test.go (2)
218-290: ⚡ Quick winAdd one subtest that exercises a non-empty
profilesWithBrokenLabelmap.Right now this new argument is always
{}in this test file, so the explicit “broken-label map suppresses removal” path isn’t validated directly.Proposed test addition
func TestComputeReconcileDeltas(t *testing.T) { @@ t.Run("broken label profile is not removed", func(t *testing.T) { @@ }) + + t.Run("profile marked broken via map is not removed", func(t *testing.T) { + current := map[string][]*fleet.MDMAppleProfilePayload{ + "uuid-A": {{ + ProfileUUID: "aBrokenByMap", + HostUUID: "uuid-A", + Checksum: []byte("xxxx"), + OperationType: fleet.MDMOperationTypeInstall, + Status: new(fleet.MDMDeliveryVerified), + }}, + } + profilesWithBrokenLabel := map[string]struct{}{"aBrokenByMap": {}} + toInstall, toRemove := ComputeReconcileDeltas( + []*fleet.AppleHostReconcileInfo{hostA}, nil, current, profilesByTeam, profilesWithBrokenLabel, + ) + require.Empty(t, toRemove) + require.Len(t, toInstall, 1) + }) }🤖 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 `@server/mdm/apple/reconcile_test.go` around lines 218 - 290, Add a new subtest that passes a non-empty profilesWithBrokenLabel into ComputeReconcileDeltas to validate the "broken-label suppresses removal" path: create a map[string]struct{} with the ProfileUUID (e.g., "aBrokenLabel") as a key, call ComputeReconcileDeltas with that map instead of the empty profilesWithBrokenLabel used elsewhere, and assert that toRemove is empty (and toInstall unchanged) for the host containing the broken profile; reference the existing test setup and functions ComputeReconcileDeltas, profilesWithBrokenLabel, and the "broken label profile is not removed" case to place the new assertion.
310-350: ⚡ Quick winMirror the broken-label-map coverage for declarations.
declsWithBrokenLabelis also always passed as empty, so declaration broken-label suppression isn’t explicitly pinned by a test case.🤖 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 `@server/mdm/apple/reconcile_test.go` around lines 310 - 350, Add a test that exercises suppression of declaration diffs when a declaration is marked in declsWithBrokenLabel: update the existing Test (where ComputeDeclarationDeltas is called with hostA and declsByTeam) to include a case that populates declsWithBrokenLabel with the declaration UUID (e.g. "aDeclGlobal") and assert that ComputeDeclarationDeltas returns no changed entries and no rows; specifically target the ComputeDeclarationDeltas call that currently uses declsWithBrokenLabel (and the hostA/declsByTeam fixtures) so the broken-label suppression for declarations is explicitly covered.server/datastore/mysql/apple_mdm_batched.go (1)
512-573: 💤 Low valueConsider extracting shared label-processing logic.
The label-processing logic (include mode accumulation, mixed-mode detection, warning logs) is nearly identical to lines 230–295 for profiles. A shared helper accepting a generic "labeled entity" callback or interface could reduce ~60 lines of duplication and keep the two reconcilers in sync if the logic ever changes.
Not blocking—flagging for a future cleanup pass if desired.
🤖 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 `@server/datastore/mysql/apple_mdm_batched.go` around lines 512 - 573, The duplicated label-processing block (includeAccum, includeModes map, loop over labelRows, mixed-mode detection and logging) should be extracted into a shared helper to avoid duplication between the Apple MDM batched reconciler and the profiles reconciler; create a function (e.g., processLabelIncludes) that accepts the slice of labelRows plus callbacks or an interface methods to (1) append include/exclude label refs to the target entity, (2) set the final IncludeMode on the target, and (3) emit the warning via ds.logger; replace the inline logic in apple_mdm_batched.go (the includeAccum/includeModes usage and subsequent loop setting d.IncludeMode) with a call to this helper so both reconcilers (the code around includeAccum and the similar block in the profiles reconciler) call the same function.
🤖 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 `@server/datastore/mysql/apple_mdm_batched.go`:
- Around line 512-573: The duplicated label-processing block (includeAccum,
includeModes map, loop over labelRows, mixed-mode detection and logging) should
be extracted into a shared helper to avoid duplication between the Apple MDM
batched reconciler and the profiles reconciler; create a function (e.g.,
processLabelIncludes) that accepts the slice of labelRows plus callbacks or an
interface methods to (1) append include/exclude label refs to the target entity,
(2) set the final IncludeMode on the target, and (3) emit the warning via
ds.logger; replace the inline logic in apple_mdm_batched.go (the
includeAccum/includeModes usage and subsequent loop setting d.IncludeMode) with
a call to this helper so both reconcilers (the code around includeAccum and the
similar block in the profiles reconciler) call the same function.
In `@server/mdm/apple/reconcile_test.go`:
- Around line 218-290: Add a new subtest that passes a non-empty
profilesWithBrokenLabel into ComputeReconcileDeltas to validate the
"broken-label suppresses removal" path: create a map[string]struct{} with the
ProfileUUID (e.g., "aBrokenLabel") as a key, call ComputeReconcileDeltas with
that map instead of the empty profilesWithBrokenLabel used elsewhere, and assert
that toRemove is empty (and toInstall unchanged) for the host containing the
broken profile; reference the existing test setup and functions
ComputeReconcileDeltas, profilesWithBrokenLabel, and the "broken label profile
is not removed" case to place the new assertion.
- Around line 310-350: Add a test that exercises suppression of declaration
diffs when a declaration is marked in declsWithBrokenLabel: update the existing
Test (where ComputeDeclarationDeltas is called with hostA and declsByTeam) to
include a case that populates declsWithBrokenLabel with the declaration UUID
(e.g. "aDeclGlobal") and assert that ComputeDeclarationDeltas returns no changed
entries and no rows; specifically target the ComputeDeclarationDeltas call that
currently uses declsWithBrokenLabel (and the hostA/declsByTeam fixtures) so the
broken-label suppression for declarations is explicitly covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fdd6b2c7-7c1e-4fa2-a5bf-11f40ee97e08
📒 Files selected for processing (6)
server/datastore/mysql/apple_mdm_batched.goserver/mdm/apple/reconcile.goserver/mdm/apple/reconcile_test.goserver/service/apple_mdm_batched.goserver/service/apple_mdm_declarations_batched.goserver/service/integration_mdm_test.go
💤 Files with no reviewable changes (1)
- server/service/integration_mdm_test.go
| // row sets the mode and later disagreements mark it mixed. Exclude | ||
| // rows always go to ExcludeLabels and have a single "exclude any" | ||
| // semantic (their require_all column is ignored). A profile may | ||
| // carry both an include set and an exclude set. |
There was a problem hiding this comment.
I think if we can cover both stories here that would be awesome. One small note is we'll still have to do the Android and windows SQL on the other story, but this is great for Apple
|
@JordanMontgomery Ready for another review, or if you think it was in a good enough state, let me know and I'll begin some more heavy testing (running roughly through the test plan testing each scenario) Edit: I have started the test plan to verify the current PR functionality |
|
Got a script to run through some test cases put together here is the result: Profile delivery (mobileconfig)
DDM delivery (declarations)
Test plan coverage
|
JordanMontgomery
left a comment
There was a problem hiding this comment.
Overall this PR looks great to me especially with the recent transactional changes. Will be interested to hear how the final loadtest goes but I think this will be a massive improvement
| hostUUID string, | ||
| ) (*fleet.AppleHostReconcileInfo, error) { | ||
| const stmt = ` | ||
| SELECT |
There was a problem hiding this comment.
I think that makes sense, but this is fine too I don't think there's too much overhead either way
Related issue: Closes #46153
This PR is big, but I found it worth it to include in the same PR to keep the mental change context in one place.
This PR moves away from our previous version of a big SQL computing the desired state and label membership with big union branches. It does so by switching the model up completely, first:
It comes with some slight caveats, which is we now load a lot more data into memory (but before we could spike worse), so when loadtesting we watched CPU/Memory utilization, which never seemed to spike as the datasets are kept as small as possible.
Cleanup will come in a follow-up PR where we remove all the old code.
Checklist for submitter
If some of the following don't apply, delete the relevant line.
Changes file added for user-visible changes in
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Input data is properly validated,
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters.Testing
Summary by CodeRabbit