Software ingestion fixes - #33399
Conversation
GitHub Actions
Inactive
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughReworks host software ingestion in MySQL: adds a two-phase flow with pre-insertion outside the main transaction, host-linking and rename handling inside the transaction, and OpenTelemetry tracing. Updates tests extensively to cover bundle ID renames, name truncation, browser extensions, checksum/id reuse, and pre-insert idempotency. Adds change notes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Host as Host
participant Svc as Service
participant DS as MySQL Datastore
participant DB as MySQL
rect rgb(240,248,255)
note over Svc,DS: Phase 1 — Pre-insert (outside main tx)
Host->>Svc: Report software inventory (apps, extensions, metadata)
Svc->>DS: UpdateHostSoftware(host_id, inventory)
DS->>DB: Pre-insert software_titles/software in batches
DB-->>DS: IDs for inserted/existing rows
end
rect rgb(245,245,245)
note over DS,DB: Phase 2 — Transactional host linking
DS->>DB: BEGIN
DS->>DB: Map checksums/bundle IDs to existing software/title IDs
DS->>DB: Update targeted bundle-ID name renames (where names differ)
DS->>DB: Link software to host (host_software inserts)
DS->>DB: Handle bundle-ID based linking for existing software
DB-->>DS: Commit results
DS->>DB: COMMIT
end
note over DS: OTel span: UpdateHostSoftware(host_id, software_count)
DS-->>Svc: Success/summary
Svc-->>Host: Acknowledgement
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 0
🧹 Nitpick comments (7)
server/datastore/mysql/software.go (1)
17-17: Consider importing as mysql_common for consistency.The alias
common_mysqlis unusual compared to typical Go naming conventions. Consider usingmysql_commonor justcommonto align with more conventional import naming.server/datastore/mysql/software_test.go (6)
1762-1771: Avoid shadowing err for clarity.Inside validateSoftware, replace the short declaration with assignment to avoid shadowing the outer err.
- err := ds.writer(ctx).GetContext( + err = ds.writer(ctx).GetContext( ctx, &titleID, `SELECT s.title_id FROM software s WHERE s.id = ?`, sw.ID, )
1892-1897: Make name_source assertion resilient to version changes.Asserting the exact "bundle_4.67" string is brittle. Prefer a prefix check.
- require.Equal(t, "bundle_4.67", nameSource, "Name source should indicate bundle ID match") + require.True(t, strings.HasPrefix(nameSource, "bundle_"), "Name source should indicate bundle ID match, got %q", nameSource)
1918-1948: Use a realistic source for bundle ID semantics.Using chrome_extensions with BundleIdentifier is atypical and could confuse future readers. apps better reflects bundle ID semantics.
- {Name: "MyApp", Version: "1.0", Source: "chrome_extensions", BundleIdentifier: "bundle_id1"}, - {Name: "MyApp", Version: "1.0", Source: "chrome_extensions", BundleIdentifier: "bundle_id2"}, + {Name: "MyApp", Version: "1.0", Source: "apps", BundleIdentifier: "bundle_id1"}, + {Name: "MyApp", Version: "1.0", Source: "apps", BundleIdentifier: "bundle_id2"},
8953-8962: Check crand.Read error.Don’t ignore crypto/rand errors, even if unlikely.
- _, _ = crand.Read(checksum) + if _, errRand := crand.Read(checksum); errRand != nil { + require.NoError(t, errRand) + }
9095-9101: Make name_source assertion version‑agnostic.Same concern as above: assert a prefix instead of a fixed string.
- require.Equal(t, "bundle_4.67", nameSource, "Name source should indicate bundle ID match") + require.True(t, strings.HasPrefix(nameSource, "bundle_"), "Name source should indicate bundle ID match, got %q", nameSource)
9138-9258: Use canonical browser-extension source names (e.g. "chrome_extensions")Repo search shows "chrome_extensions" is used across the codebase while "browser_plugins" only appears in schema and this test (server/datastore/mysql/software_test.go:9138–9258). Update the test to use the canonical source(s) (chrome_extensions / firefox_addons / safari_extensions) or confirm that using "browser_plugins" here is intentional.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
changes/29053-sw-names-on-ingest(1 hunks)changes/33298-software-ingestion(1 hunks)server/datastore/mysql/software.go(15 hunks)server/datastore/mysql/software_test.go(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
When reviewing SQL queries that are added or modified, ensure that appropriate filtering criteria are applied—especially when a query is intended to return data for a specific entity (e.g., a single host). Check for missing WHERE clauses or incorrect filtering that could lead to incorrect or non-deterministic results (e.g., returning the first row instead of the correct one). Flag any queries that may return unintended results due to lack of precise scoping.
Files:
server/datastore/mysql/software.goserver/datastore/mysql/software_test.go
🔇 Additional comments (28)
changes/29053-sw-names-on-ingest (1)
1-1: LGTM!The change note clearly explains the fix for the macOS software ingestion issue. The wording appropriately describes the prevention of duplicate software entries due to user renaming while maintaining focus on the end-user impact.
changes/33298-software-ingestion (1)
1-1: LGTM!The change note accurately describes the performance improvement achieved through pre-insertion of software/titles in smaller batches. This directly addresses the database lock time and performance degradation issues mentioned in the linked GitHub issue #33298.
server/datastore/mysql/software.go (21)
24-26: LGTM on OpenTelemetry integration!The OpenTelemetry imports are properly included and the implementation follows Go OpenTelemetry patterns where spans are created with
tracer.Start(ctx, "operation-name")and stored in context.Context.
38-40: LGTM on tracer initialization.The global tracer initialization follows OpenTelemetry Go best practices where
otel.Tracer()creates a tracer that can be used when a TracerProvider is set later. The no-op behavior when OTEL is disabled is also correctly noted.
52-54: Good addition of batch size control.The new
softwareInventoryInsertBatchSizevariable properly supports the pre-insertion optimization strategy. The smaller batch size (100 vs 1000) makes sense for reducing lock contention during the pre-insertion phase.
65-72: LGTM on span creation and attributes.The OpenTelemetry span creation follows proper patterns with
tracer.Start()and includes relevant attributes for observability. The host_id and software_count attributes will be valuable for monitoring software ingestion performance.
396-406: Excellent addition of two-phase software ingestion.The two-phase approach is well-designed:
- Phase 1: Pre-insert software inventory outside the main transaction to reduce lock contention
- Phase 2: Host-specific operations within transaction
The comment clearly explains the strategy and the idempotent nature due to
INSERT IGNORE. This directly addresses the performance issues mentioned in PR objectives.
417-423: Good documentation of the host linking phase.The comments clearly explain that the software inventory entries were already created in Phase 1, and this phase handles the host-specific linking. The separation of concerns is well-executed.
425-460: Well-implemented bundle ID handling.The bundle ID software linking and renaming logic is comprehensive:
- Handles existing software matching by bundle ID
- Builds software rename mappings for both inserted and existing software
- Calls
updateTargetedBundleIDsfor batch updates- Includes proper error handling
The approach addresses the macOS software name fix mentioned in the PR objectives.
483-524: Efficient batch processing for software renames.The
updateTargetedBundleIDsfunction implements proper batch processing:
- Uses
common_mysql.BatchProcessSimplefor efficient batching- Builds CASE statements for batch updates
- Sets
name_source = 'bundle_4.67'to track the rename source- Includes proper error handling
This efficiently handles the software name updates for bundle ID matches.
648-657: Good handling of bundle ID matches with existing software.The logic properly handles the case where incoming software has the same bundle ID as existing software but different name:
- Copies incoming software with existing software's ID
- Removes from
incomingChecksumToSoftwareto prevent duplicate processing- Maintains data integrity during the rename process
This directly addresses the macOS software ingestion issues.
704-708: Good handling of title mapping edge cases.The logic properly handles the case where multiple checksums can map to the same title (e.g., when names are truncated). The comment clarifies this edge case and the code iterates through all relevant checksums.
734-741: Proper handling of multiple checksums to single title.The implementation correctly maps all checksums that correspond to a single title, handling the edge case where software with different versions/checksums may have the same truncated name.
762-769: Consistent mapping pattern for bundle ID titles.The code follows the same pattern as the non-bundle ID case, properly mapping all checksums that correspond to each title. The consistency in approach is good.
823-837: Well-designed pre-insertion function signature.The
preInsertSoftwareInventoryfunction has a clear purpose and good parameter structure. The function name and documentation clearly indicate it's for Phase 1 of the two-phase approach.
853-857: Efficient batching for pre-insertion.Using
common_mysql.BatchProcessSimplewith the smallersoftwareInventoryInsertBatchSizeis appropriate for reducing lock contention during pre-insertion. The batching strategy aligns with the performance goals.
859-876: Good transaction handling for batches.Each batch runs in its own transaction via
ds.withRetryTxx, which is appropriate for the pre-insertion phase. This allows quick release of locks while maintaining consistency within each batch.
886-909: Efficient title deduplication.The title deduplication logic using a map with composite keys is well-designed:
- Prevents unnecessary duplicate INSERTs
- Uses struct keys for efficient comparison
- Handles both bundle ID and non-bundle ID cases
This optimization reduces database load during pre-insertion.
959-970: Proper handling of title ID retrieval.The title ID mapping logic correctly handles the relationship between checksums and titles:
- Maps titles back to their checksums using name/source/browser/bundle_identifier matching
- Handles the case where multiple checksums map to the same title
- Includes helpful comment about the edge case
The logic is sound and handles the complex mapping scenarios.
1014-1026: Good error logging for debugging.The logging of software without title IDs is helpful for debugging:
- Logs count and examples of problematic software
- Limits examples to avoid log spam
- Uses appropriate error level since this shouldn't happen normally
This will help identify any issues with the title mapping logic.
1039-1108: Well-implemented bundle ID software linking.The
linkExistingBundleIDSoftwarefunction properly handles edge cases:
- Verifies software still exists before linking (prevents orphaned references)
- Includes proper error checking for missing software IDs
- Uses
INSERT IGNOREfor idempotency- Logs warnings for missing software but continues processing
The defensive programming approach is appropriate for this scenario.
1110-1169: Robust implementation of host-software linking.The
linkSoftwareToHostfunction is well-designed:
- Verifies software exists before linking (defensive programming)
- Maps checksums to software for efficient lookup
- Uses
INSERT IGNOREfor idempotency- Logs warnings for missing software but continues
- Includes helpful comments explaining the purpose
This replaces
insertNewInstalledHostSoftwareDBwith the assumption that software inventory already exists from Phase 1.
1171-1175: Good defensive programming for empty input.The early return for empty checksums prevents unnecessary database queries and follows the fail-fast principle. This is a minor but good optimization.
server/datastore/mysql/software_test.go (5)
54-59: Nice coverage expansion for rename/ID reuse flows.The added cases exercise critical rename/ID‑reuse scenarios and browser extensions. Good additions.
97-98: Pre-insert idempotency test is valuable.Good to have a direct check on Phase 1 idempotency.
1705-1713: Local truncation helper is fine.Matches production behavior closely enough for tests.
1953-2032: Long-name truncation tests look solid.Good edge coverage around truncation and title_id consistency.
1857-1858: No action required — repogodirective is 1.25.1, so testing.T.Context() is supported.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #33399 +/- ##
==========================================
+ Coverage 64.05% 64.12% +0.06%
==========================================
Files 2059 2059
Lines 204990 205360 +370
Branches 6767 6767
==========================================
+ Hits 131316 131679 +363
+ Misses 63304 63300 -4
- Partials 10370 10381 +11
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:
|
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
sgress454
left a comment
sgress454
left a comment
There was a problem hiding this comment.
Couple of suggestions but otherwise as far as I can tell it looks good. The design of splitting into two phases makes sense, and the incorporation of @ksykulev's revamped renaming logic looks correct. Question in Slack re: the new logic for dealing with multiple software items w/ the same checksum, but assuming we're good on that this looks 👍
Would definitely want to try a before and after run on load test to make sure there's not some gotcha that will cause software to disappear 😬 .
| ctx context.Context, | ||
| tx sqlx.ExtContext, | ||
| hostID uint, | ||
| softwareChecksums map[string]fleet.Software, |
There was a problem hiding this comment.
would love to take this opportunity to rename this to incomingSoftwareChecksums
| const numberOfArgsPerSoftwareTitles = 5 | ||
| titlesValues := strings.TrimSuffix(strings.Repeat("(?,?,?,?,?),", len(uniqueTitlesToInsert)), ",") | ||
| titlesStmt := fmt.Sprintf("INSERT IGNORE INTO software_titles (name, source, browser, bundle_identifier, is_kernel) VALUES %s", titlesValues) | ||
| titlesArgs := make([]any, 0, len(uniqueTitlesToInsert)*numberOfArgsPerSoftwareTitles) | ||
|
|
||
| for _, title := range uniqueTitlesToInsert { | ||
| titlesArgs = append(titlesArgs, title.Name, title.Source, title.Browser, title.BundleIdentifier, title.IsKernel) | ||
| } | ||
|
|
||
| if _, err := tx.ExecContext(ctx, titlesStmt, titlesArgs...); err != nil { | ||
| return ctxerr.Wrap(ctx, err, "pre-insert software_titles") | ||
| } |
There was a problem hiding this comment.
Perhaps we can use sqlx.NamedExec here to simplify this a bit? There's places where we have no choice but to build these giant strings of ?s but it'd be good to avoid it where we can.
<!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Resolves #29053 Resolves #33298 For reference, the diffs for merging Konstantin's changes into my original PR are here: #33390 # Checklist for submitter - [x] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. ## Testing - [x] Added/updated automated tests - [x] Where appropriate, [automated tests simulate multiple hosts and test for host isolation](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/reference/patterns-backend.md#unit-testing) (updates to one hosts's records do not affect another) - [x] QA'd all new/changed functionality manually <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - Bug Fixes - Fixed duplicate macOS software entries caused by users renaming apps, ensuring accurate, consolidated inventory. - Documentation - Documented improved software ingestion performance by pre-inserting data in smaller batches to reduce database lock times during host check-ins. <!-- end of auto-generated comment: release notes by coderabbit.ai --> (cherry picked from commit 1ae9597)
<!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Resolves #29053 Resolves #33298 For reference, the diffs for merging Konstantin's changes into my original PR are here: #33390 # Checklist for submitter - [x] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. ## Testing - [x] Added/updated automated tests - [x] Where appropriate, [automated tests simulate multiple hosts and test for host isolation](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/reference/patterns-backend.md#unit-testing) (updates to one hosts's records do not affect another) - [x] QA'd all new/changed functionality manually <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - Bug Fixes - Fixed duplicate macOS software entries caused by users renaming apps, ensuring accurate, consolidated inventory. - Documentation - Documented improved software ingestion performance by pre-inserting data in smaller batches to reduce database lock times during host check-ins. <!-- end of auto-generated comment: release notes by coderabbit.ai --> (cherry picked from commit 1ae9597)
|
It could be worth testing if this resolves #28584 as well. |
<!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Resolves #29053 Resolves #33298 For reference, the diffs for merging Konstantin's changes into my original PR are here: #33390 # Checklist for submitter - [x] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. ## Testing - [x] Added/updated automated tests - [x] Where appropriate, [automated tests simulate multiple hosts and test for host isolation](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/reference/patterns-backend.md#unit-testing) (updates to one hosts's records do not affect another) - [x] QA'd all new/changed functionality manually <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - Bug Fixes - Fixed duplicate macOS software entries caused by users renaming apps, ensuring accurate, consolidated inventory. - Documentation - Documented improved software ingestion performance by pre-inserting data in smaller batches to reduce database lock times during host check-ins. <!-- end of auto-generated comment: release notes by coderabbit.ai --> (cherry picked from commit 1ae9597)
<!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Resolves #29053 Resolves #33298 For reference, the diffs for merging Konstantin's changes into my original PR are here: #33390 # Checklist for submitter - [x] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. ## Testing - [x] Added/updated automated tests - [x] Where appropriate, [automated tests simulate multiple hosts and test for host isolation](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/reference/patterns-backend.md#unit-testing) (updates to one hosts's records do not affect another) - [x] QA'd all new/changed functionality manually <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - Bug Fixes - Fixed duplicate macOS software entries caused by users renaming apps, ensuring accurate, consolidated inventory. - Documentation - Documented improved software ingestion performance by pre-inserting data in smaller batches to reduce database lock times during host check-ins. <!-- end of auto-generated comment: release notes by coderabbit.ai --> (cherry picked from commit 1ae9597)
This reverts commit 36525f0.
5 participants
Related issue:
Resolves #29053
Resolves #33298
For reference, the diffs for merging Konstantin's changes into my original PR are here: #33390
Checklist for submitter
changes/,orbit/changes/oree/fleetd-chrome/changes.Testing
Summary by CodeRabbit
Bug Fixes
Documentation