CP: Optimize ReparseWindowsHostCertificates migration (#50228) - #50263
Draft
sharon-fdm wants to merge 2 commits into
Draft
CP: Optimize ReparseWindowsHostCertificates migration (#50228)#50263sharon-fdm wants to merge 2 commits into
sharon-fdm wants to merge 2 commits into
Conversation
The original migration re-joined the hosts table on every batch of 1000 cert IDs, causing ~14 minutes of sustained full DB load at 100K hosts. Instead, collect all Windows host IDs once upfront and then soft-delete certs in batches by host_id without the repeated join.
Inserts 1,000 Windows hosts x 20 certs (20K rows) and verifies the migration completes in under 30s. Measured at 445ms with the fix, vs the original which took ~14 minutes at 100K hosts.
Member
|
@coderabbitai full review |
Member
|
/agentic_review |
Contributor
✅ Action performedFull review finished. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## rc-minor-fleet-v4.90.0 #50263 +/- ##
=========================================================
Coverage ? 67.89%
=========================================================
Files ? 3890
Lines ? 248287
Branches ? 12966
=========================================================
Hits ? 168575
Misses ? 64496
Partials ? 15216
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:
|
getvictor
reviewed
Jul 30, 2026
| // The test uses 1,000 Windows hosts x 20 certs each = 20,000 cert rows. | ||
| // This is enough to exercise the batching logic (hostBatchSize=500) while | ||
| // staying fast enough for CI (~seconds, not minutes). | ||
| func TestUp_20260723181402_Performance(t *testing.T) { |
Member
There was a problem hiding this comment.
This bench test doesn't need to be checked in, right?
Code Review by Qodo
1. Flaky perf test in CI
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+73
to
+74
| affected, _ := result.RowsAffected() | ||
| for range affected { |
There was a problem hiding this comment.
1. Rowsaffected error ignored 🐞 Bug ◔ Observability
softDeleteWindowsHostCertsForReparse ignores the error from result.RowsAffected(), so if the driver can’t return an affected-row count the migration will still succeed but progress reporting can be incorrect because increment() won’t be called for updated rows. This regresses the prior deterministic per-row increment behavior used by incrementalMigrationStep to compute progress percentages.
Agent Prompt
### Issue description
`RowsAffected()` errors are currently ignored when driving `increment()` calls. If the affected-row count cannot be obtained (or is unknown), progress output becomes misleading.
### Issue Context
`incrementalMigrationStep` computes and logs progress based on how many times `increment()` is invoked.
### Fix Focus Areas
- server/datastore/mysql/migrations/tables/20260723181402_ReparseWindowsHostCertificates.go[62-76]
- server/datastore/mysql/migrations/tables/migration.go[43-92]
### Suggested fix
- Capture and handle `RowsAffected()` error:
- `affected, err := result.RowsAffected()`
- If `err != nil` (or `affected < 0`), fall back to a deterministic count for that host batch (e.g., `SELECT COUNT(*) FROM host_certificates WHERE host_id IN (?) AND origin='osquery' AND deleted_at IS NULL` before the UPDATE) and use that count to call `increment()`.
- Otherwise, loop `affected` times and call `increment()`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+80
to
+105
| // Run the migration and time it. | ||
| start := time.Now() | ||
| applyNext(t, db) | ||
| elapsed := time.Since(start) | ||
| t.Logf("Migration completed in %s for %d Windows hosts x %d certs = %d cert rows", | ||
| elapsed, numWindowsHosts, certsPerHost, numWindowsHosts*certsPerHost) | ||
|
|
||
| // Verify: all Windows osquery certs are soft-deleted. | ||
| var windowsLive int | ||
| require.NoError(t, db.Get(&windowsLive, ` | ||
| SELECT COUNT(*) FROM host_certificates hc | ||
| JOIN hosts h ON h.id = hc.host_id | ||
| WHERE h.platform = 'windows' AND hc.origin = 'osquery' AND hc.deleted_at IS NULL`)) | ||
| require.Equal(t, 0, windowsLive, "all Windows osquery certs should be soft-deleted") | ||
|
|
||
| // Verify: all macOS certs are untouched. | ||
| var macLive int | ||
| require.NoError(t, db.Get(&macLive, ` | ||
| SELECT COUNT(*) FROM host_certificates hc | ||
| JOIN hosts h ON h.id = hc.host_id | ||
| WHERE h.platform = 'darwin' AND hc.origin = 'osquery' AND hc.deleted_at IS NULL`)) | ||
| require.Equal(t, numMacHosts*certsPerHost, macLive, "macOS certs should be untouched") | ||
|
|
||
| // At 1,000 hosts x 20 certs, the migration should complete in under 30 seconds. | ||
| // The original implementation took proportionally ~14 minutes at 100K hosts. | ||
| require.Less(t, elapsed, 30*time.Second, "migration took too long, possible performance regression") |
There was a problem hiding this comment.
2. Flaky perf test in ci 🐞 Bug ☼ Reliability
TestUp_20260723181402_Performance seeds ~25k rows using thousands of single-row INSERTs and enforces a fixed <30s wall-clock threshold for applyNext(), which can slow down CI and intermittently fail on slower/busy MySQL runners even when the migration is correct. Because recent migration tests run by default, this test will execute in CI for the current release window.
Agent Prompt
### Issue description
A performance-focused test with heavy DB seeding and a hard wall-clock threshold runs as a normal `go test`, increasing CI time and risking environment-dependent failures.
### Issue Context
Migration tests run by default for recent migrations (<60 days old), so this test will execute in standard CI.
### Fix Focus Areas
- server/datastore/mysql/migrations/tables/20260723181402_ReparseWindowsHostCertificates_bench_test.go[20-106]
- server/datastore/mysql/migrations/tables/migration_test.go[92-106]
### Suggested fix options (pick one)
1) Convert to a benchmark (`func Benchmark...`) and run it only under `go test -bench` with `-run '^$'`.
2) Keep it as a test but gate it behind an opt-in env var (e.g., `if os.Getenv("FLEET_RUN_PERF_TESTS") == "" { t.Skip(...) }`) and/or `testing.Short()`.
3) If keeping it in the default suite, remove the strict wall-clock assertion and/or significantly reduce seeding cost (multi-row INSERTs / batch inserts) to minimize CI impact.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
WalkthroughThe Windows host certificate re-parse migration now collects Windows host IDs once and soft-deletes matching 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
server/datastore/mysql/migrations/tables/20260723181402_ReparseWindowsHostCertificates_bench_test.go (2)
50-72: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider batching cert inserts for faster test setup.
24,000 single-row
INSERTs add setup overhead on top of the ~445ms migration being measured. A multi-rowINSERT ... VALUES (...), (...), ...or wrapping the loop in an explicit transaction would reduce test runtime, though not essential since the test already passes comfortably under the 30s budget.🤖 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/migrations/tables/20260723181402_ReparseWindowsHostCertificates_bench_test.go` around lines 50 - 72, The insertCerts helper performs one database INSERT per certificate, adding unnecessary benchmark setup overhead. Update insertCerts to batch certificate rows into multi-row INSERT statements or execute the loop within an explicit transaction, while preserving the existing certificate values and coverage for both Windows and macOS hosts.
50-101: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest doesn't exercise the
origin/deleted_atscoping it's meant to validate.All seeded Windows certs are
origin='osquery'and live (deleted_at IS NULL), so the assertions at lines 93 and 101 would still pass even if the migration'sorigin = 'osquery' AND deleted_at IS NULLfilter were accidentally dropped. Consider seeding a non-osquery-origin cert (e.g., MDM-origin) and an already-soft-deleted cert on a Windows host, then asserting both remain untouched, to actually pin down the filter's scoping.As per path instructions, ensure SQL queries intended to scope a specific set of rows (here: only live osquery Windows certs) are backed by tests that would fail if that scoping regressed.
🤖 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/migrations/tables/20260723181402_ReparseWindowsHostCertificates_bench_test.go` around lines 50 - 101, Expand the certificate seeding in insertCerts or a nearby helper to include Windows-host rows with a non-osquery origin and with deleted_at already set, while retaining the live osquery rows. Update the post-migration assertions to verify those scoped rows remain unchanged, including counts or targeted queries for MDM-origin and pre-deleted certificates, so the test fails if the migration processes anything beyond live osquery certificates.Source: Path instructions
🤖 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/migrations/tables/20260723181402_ReparseWindowsHostCertificates_bench_test.go`:
- Around line 50-72: The insertCerts helper performs one database INSERT per
certificate, adding unnecessary benchmark setup overhead. Update insertCerts to
batch certificate rows into multi-row INSERT statements or execute the loop
within an explicit transaction, while preserving the existing certificate values
and coverage for both Windows and macOS hosts.
- Around line 50-101: Expand the certificate seeding in insertCerts or a nearby
helper to include Windows-host rows with a non-osquery origin and with
deleted_at already set, while retaining the live osquery rows. Update the
post-migration assertions to verify those scoped rows remain unchanged,
including counts or targeted queries for MDM-origin and pre-deleted
certificates, so the test fails if the migration processes anything beyond live
osquery certificates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8787f336-a020-4b6d-b410-67d4f9255f6d
📒 Files selected for processing (3)
changes/50228-fix-reparse-certs-migrationserver/datastore/mysql/migrations/tables/20260723181402_ReparseWindowsHostCertificates.goserver/datastore/mysql/migrations/tables/20260723181402_ReparseWindowsHostCertificates_bench_test.go
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
2 participants
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: #50228
Cherry-pick of #50262 into the 4.90 RC branch.
Checklist for submitter
changes/.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
For unreleased bug fixes in a release candidate, one of:
Summary
The
ReparseWindowsHostCertificatesmigration caused ~14 minutes of sustained full DB load (CPU 89.75%, threads 2x vCPU count) during the 4.89 to 4.90 upgrade at 100K hosts.Root cause: Each batch of 1,000 cert IDs re-joined the
hoststable to filter byplatform = 'windows'.Fix: Collect Windows host IDs once upfront, then soft-delete certs in batches by
host_idwithout re-joining.Local benchmark: 1,000 Windows hosts x 20 certs (20K rows) completes in 445ms (was proportionally ~14 min at 100K).
Test plan
TestUp_20260723181402passes (correctness)TestUp_20260723181402_Performancepasses in 445ms (performance)Summary by CodeRabbit
Bug Fixes
Tests