45635 reconciler#47070
Closed
getvictor wants to merge 5 commits into
Closed
Conversation
Pure refactor, no behavior change. First step for #45635 (Windows batched in-memory reconciler). - New server/mdm/reconcile package holds the include/exclude label handlers and the team+label applicability dispatcher. The Apple platform gate stays in the Apple wrapper since platform eligibility is platform-specific. - New platform-neutral fleet types (MDMProfileLabelRef, MDMProfileIncludeMode, MDMLabeledEntity); the Apple names are now type aliases so existing code and tests are unchanged. - BulkGetHostLabelMemberships moves from apple_mdm_batched.go to a neutral file; it was already platform-agnostic. - The existing Apple label-scenario tests keep covering the shared logic through the delegating wrappers; the shared package also gets its own handler/dispatcher tests.
Member
Author
|
@coderabbitai full review |
Member
Author
|
/agentic_review |
Contributor
✅ Action performedFull review finished. |
Code Review by Qodo
1. Cursor advances without work
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses #45635 by replacing the expensive Windows MDM “pending hosts” set-difference query with an indexed snapshot + in-memory diff approach, and introducing a shared, platform-neutral label/team applicability dispatcher used by both Apple and Windows reconcilers.
Changes:
- Reworked Windows profile reconciliation to page through enrolled Windows hosts via
GetWindowsProfileReconcileSnapshot, compute deltas in memory, and drain multiple windows per tick (bounded by a delivery cap and scan-time budget). - Introduced
server/mdm/reconcileas the shared dispatcher for team/include/exclude label semantics; refactored Apple to delegate to it. - Added/updated unit + property-based tests for cursor protocol, delta computation, and delivery-cap throttling.
Reviewed changes
Copilot reviewed 17 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/reconcile_windows_profiles_property_test.go | Updates PBT model to use snapshot-based pagination and new budget tunables. |
| server/service/microsoft_mdm.go | Implements snapshot-driven drain loop with delivery cap + scan budget; extracts legacy batch execution into helper. |
| server/service/microsoft_mdm_test.go | Updates mocks to provide snapshot inputs; adds delivery-cap throttling test. |
| server/mock/datastore_mock.go | Adds mock support for GetWindowsProfileReconcileSnapshot. |
| server/mdm/reconcile/reconcile.go | New platform-neutral dispatcher + include/exclude handlers. |
| server/mdm/reconcile/reconcile_test.go | Unit tests for shared dispatcher/handlers. |
| server/mdm/microsoft/reconcile.go | New in-memory desired-vs-current diff for Windows profiles. |
| server/mdm/microsoft/reconcile_test.go | Unit tests for Windows delta rules, team gating, label matrix. |
| server/mdm/apple/reconcile.go | Refactors Apple wrapper to call shared dispatcher after Apple platform gate. |
| server/mdm/apple/reconcile_test.go | Removes duplicated handler tests; adds Apple-wrapper threading assertions. |
| server/fleet/windows_mdm.go | Adds WindowsHostReconcileInfo / WindowsProfileForReconcile types used by snapshot + diff. |
| server/fleet/mdm_reconcile.go | Adds platform-neutral include-mode + label-ref primitives and MDMLabeledEntity. |
| server/fleet/datastore.go | Adds GetWindowsProfileReconcileSnapshot to datastore interface. |
| server/fleet/apple_mdm.go | Aliases Apple label/include types to platform-neutral primitives. |
| server/datastore/mysql/microsoft_mdm_batched.go | Implements snapshot loading in a read-only transaction for Windows reconciling. |
| server/datastore/mysql/mdm_label_memberships.go | Extracts reusable host↔label membership bulk lookup helper. |
| server/datastore/mysql/apple_mdm_batched.go | Removes duplicated label-membership helper now extracted to new file. |
| changes/45635-windows-batched-reconciler | Release note for Windows reconciler performance change. |
| changes/45635-shared-reconcile-primitives | Release note for shared reconcile primitives refactor. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
WalkthroughThis PR refactors MDM profile label-targeting rules into a platform-neutral shared package used by both Apple and Windows reconcilers, and replaces Windows MDM's fleet-wide query-based host finding with a snapshot-driven in-memory batch-processing model. The refactoring extracts include-all/any and exclude-any label gating logic into a shared dispatcher, aligns Apple types to those definitions, and implements Windows-specific reconciliation data structures and delta computation. Windows reconciliation shifts from querying pending hosts to iterating snapshots of enrolled-host batches, computing install/remove deltas in memory, and draining work within a per-tick delivery cap to reduce single-change latency and eliminate the host-discovery query bottleneck. Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@server/mdm/reconcile/reconcile_test.go`:
- Around line 23-30: The HasBrokenLabel method currently uses
append(e.includeLabels, e.excludeLabels...) which can mutate e.includeLabels;
change the implementation to avoid mutating state by iterating the two slices
separately (first loop over e.includeLabels, then loop over e.excludeLabels) and
return true if any LabelID == nil, otherwise return false; reference the
testEntity.HasBrokenLabel method and the includeLabels and excludeLabels fields
when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: faa47e95-b566-401c-a14e-7f4fae27d168
📒 Files selected for processing (19)
changes/45635-shared-reconcile-primitiveschanges/45635-windows-batched-reconcilerserver/datastore/mysql/apple_mdm_batched.goserver/datastore/mysql/mdm_label_memberships.goserver/datastore/mysql/microsoft_mdm_batched.goserver/fleet/apple_mdm.goserver/fleet/datastore.goserver/fleet/mdm_reconcile.goserver/fleet/windows_mdm.goserver/mdm/apple/reconcile.goserver/mdm/apple/reconcile_test.goserver/mdm/microsoft/reconcile.goserver/mdm/microsoft/reconcile_test.goserver/mdm/reconcile/reconcile.goserver/mdm/reconcile/reconcile_test.goserver/mock/datastore_mock.goserver/service/microsoft_mdm.goserver/service/microsoft_mdm_test.goserver/service/reconcile_windows_profiles_property_test.go
💤 Files with no reviewable changes (1)
- server/datastore/mysql/apple_mdm_batched.go
Comment on lines
+23
to
+30
| func (e *testEntity) HasBrokenLabel() bool { | ||
| for _, l := range append(e.includeLabels, e.excludeLabels...) { | ||
| if l.LabelID == nil { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid mutating includeLabels inside HasBrokenLabel.
append(e.includeLabels, e.excludeLabels...) can mutate e.includeLabels when capacity allows, which may leak state across assertions. Iterate both slices separately.
Suggested fix
func (e *testEntity) HasBrokenLabel() bool {
- for _, l := range append(e.includeLabels, e.excludeLabels...) {
+ for _, l := range e.includeLabels {
+ if l.LabelID == nil {
+ return true
+ }
+ }
+ for _, l := range e.excludeLabels {
if l.LabelID == nil {
return true
}
}
return false
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (e *testEntity) HasBrokenLabel() bool { | |
| for _, l := range append(e.includeLabels, e.excludeLabels...) { | |
| if l.LabelID == nil { | |
| return true | |
| } | |
| } | |
| return false | |
| } | |
| func (e *testEntity) HasBrokenLabel() bool { | |
| for _, l := range e.includeLabels { | |
| if l.LabelID == nil { | |
| return true | |
| } | |
| } | |
| for _, l := range e.excludeLabels { | |
| if l.LabelID == nil { | |
| return true | |
| } | |
| } | |
| return false | |
| } |
🤖 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/reconcile/reconcile_test.go` around lines 23 - 30, The
HasBrokenLabel method currently uses append(e.includeLabels, e.excludeLabels...)
which can mutate e.includeLabels; change the implementation to avoid mutating
state by iterating the two slices separately (first loop over e.includeLabels,
then loop over e.excludeLabels) and return true if any LabelID == nil, otherwise
return false; reference the testEntity.HasBrokenLabel method and the
includeLabels and excludeLabels fields when making the change.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #47070 +/- ##
==========================================
- Coverage 67.04% 67.00% -0.04%
==========================================
Files 2875 2881 +6
Lines 225133 225881 +748
Branches 11780 11780
==========================================
+ Hits 150931 151344 +413
- Misses 60530 60851 +321
- Partials 13672 13686 +14
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:
|
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
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.
Related issue: Resolves
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.Timeouts are implemented and retries are limited to avoid infinite loops
If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes
Testing
Added/updated automated tests
Where appropriate, automated tests simulate multiple hosts and test for host isolation (updates to one hosts's records do not affect another)
QA'd all new/changed functionality manually
For unreleased bug fixes in a release candidate, one of:
Database migrations
COLLATE utf8mb4_unicode_ci).New Fleet configuration settings
If you didn't check the box above, follow this checklist for GitOps-enabled settings:
fleetctl generate-gitopsfleetd/orbit/Fleet Desktop
runtime.GOOSis used as needed to isolate changesSummary by CodeRabbit
Performance Improvements
Platform Consistency