Optimize IsHostConnectedToFleetMDM on the orbit check-in hot path (#44629)#48375
Conversation
…4629) GetOrbitConfig ran IsHostConnectedToFleetMDM, a 3-table JOIN, on every orbit check-in for every host (including Linux/ChromeOS that never use the result) one line before GetHostMDM. In load testing the Windows variant was the #1 query by volume. Fold the connected-to-Fleet check into GetHostMDM via a connected_to_fleet column that mirrors the existing IsHostConnectedToFleetMDM and hostMDMSelect conditions, and derive the value in GetOrbitConfig from the host_mdm data it already fetches. This removes the separate query from the hot path (2 queries to 1) with no semantic change. The SQL CASE short-circuits for non-MDM platforms, so Linux/ChromeOS pay nothing for the enrollment lookup. Claude-Session: https://claude.ai/code/session_018vexp83A6iFZ6V53HSJAaR
|
@coderabbitai review |
|
/review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
This PR optimizes Orbit’s /config check-in hot path by removing the standalone IsHostConnectedToFleetMDM query and instead deriving “connected to Fleet MDM” from the GetHostMDM query result via a new connected_to_fleet computed column.
Changes:
- Update
GetOrbitConfigto derive Fleet-MDM connectivity fromGetHostMDM(removing an extra hot-path query). - Extend
GetHostMDMto compute and return aconnected_to_fleetboolean using platform-specific enrollment conditions. - Update unit tests/mocks to cover and consume the new
HostMDM.ConnectedToFleetfield.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| server/service/orbit.go | Removes the extra connectivity query and derives connectivity from mdmInfo.ConnectedToFleet. |
| server/service/orbit_test.go | Updates service tests’ HostMDM mocks to include ConnectedToFleet. |
| server/fleet/hosts.go | Adds ConnectedToFleet to fleet.HostMDM (db-mapped, not serialized). |
| server/datastore/mysql/hosts.go | Computes connected_to_fleet inside the GetHostMDM SQL query via CASE + EXISTS, with a LEFT JOIN hosts. |
| server/datastore/mysql/mdm_test.go | Adds assertions that GetHostMDM().ConnectedToFleet matches IsHostConnectedToFleetMDM across enrollment states. |
| changes/44629-optimize-orbit-mdm-connection-check | User-visible changes entry (content excluded from review). |
Files excluded by content exclusion policy (1)
- changes/44629-optimize-orbit-mdm-connection-check
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
|
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 (5)
🚧 Files skipped from review as they are similar to previous changes (5)
WalkthroughGetHostMDM now returns a 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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}
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/service/orbit.go`:
- Around line 494-505: The Orbit config path is only treating sql.ErrNoRows as a
missing host_mdm row, but GetHostMDM now returns its own not-found error for
absent MDM data. Update the error handling in orbit.go around svc.ds.GetHostMDM
so the missing-row case from GetHostMDM is also treated as a non-error and
leaves mdmInfo nil, preserving the ConnectedToFleet=false behavior in the
OrbitConfig flow. Use the GetHostMDM call and the subsequent
isConnectedToFleetMDM derivation to locate the fix.
🪄 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: 941c4ae9-1ae4-41b5-8b42-24852b427fb7
📒 Files selected for processing (6)
changes/44629-optimize-orbit-mdm-connection-checkserver/datastore/mysql/hosts.goserver/datastore/mysql/mdm_test.goserver/fleet/hosts.goserver/service/orbit.goserver/service/orbit_test.go
- Comment why GetHostMDM uses LEFT JOIN hosts (orphaned host_mdm rows degrade connected_to_fleet to false instead of dropping the row). - Cover Android in the IsHostConnectedToFleetMDM / GetHostMDM parity test. Claude-Session: https://claude.ai/code/session_018vexp83A6iFZ6V53HSJAaR
AI reviewers (Copilot, CodeRabbit, Qodo) flagged the sql.ErrNoRows-only guard. The original errors.Is(sql.ErrNoRows) already tolerated GetHostMDM's wrapped NotFound error (NotFoundError.Is matches sql.ErrNoRows), so the hot path was not actually broken. Still, lead with the idiomatic fleet.IsNotFound check (matching the rest of orbit.go) while retaining the sql.ErrNoRows net so a bare no-rows error is also treated as 'no MDM data'. Claude-Session: https://claude.ai/code/session_018vexp83A6iFZ6V53HSJAaR
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #48375 +/- ##
========================================
Coverage 67.47% 67.47%
========================================
Files 3673 3673
Lines 232784 232896 +112
Branches 12256 12256
========================================
+ Hits 157079 157156 +77
- Misses 61606 61623 +17
- Partials 14099 14117 +18
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:
|
The real GetHostMDM never returns a bare sql.ErrNoRows; it wraps the missing-row case as a Fleet NotFound error. Drop the redundant errors.Is(sql.ErrNoRows) branch and make the orbit unit-test mocks return newNotFoundError() to match the datastore, so the guard is a single idiomatic fleet.IsNotFound check. Claude-Session: https://claude.ai/code/session_018vexp83A6iFZ6V53HSJAaR
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
ksykulev
left a comment
ksykulev
left a comment
There was a problem hiding this comment.
There is a non-blocking test question. But the code itself looks good.
| } | ||
| ds.GetHostMDMFunc = func(ctx context.Context, hostID uint) (*fleet.HostMDM, error) { | ||
| return nil, sql.ErrNoRows | ||
| return nil, newNotFoundError() |
There was a problem hiding this comment.
Is this testing the correct path? GetOrbitConfig no longer calls IsHostConnectedToFleetMDM right? So in essence "host not connected" and "host has MDM but not enrolled" branches now run the same code?
There was a problem hiding this comment.
@ksykulev This is fixing an existing testing/code quality issue. The GetHostMDM method does not return sql.ErrNoRows
There was a problem hiding this comment.
@ksykulev I will put up a new PR with some test fixes.
Related issue: Resolves #44629
This folds the connected-to-Fleet check into
GetHostMDMvia aconnected_to_fleetcolumn that mirrors the existingIsHostConnectedToFleetMDMandhostMDMSelectconditions, and derives the value inGetOrbitConfigfrom thehost_mdmdata it already fetches. Result: 2 queries → 1 on the orbit check-in hot path, with no semantic change.Checklist for submitter
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