RHINENG-21568: Use our local systems table when generating playbooks#1038
Open
marleystipich2 wants to merge 4 commits intomasterfrom
Open
RHINENG-21568: Use our local systems table when generating playbooks#1038marleystipich2 wants to merge 4 commits intomasterfrom
marleystipich2 wants to merge 4 commits intomasterfrom
Conversation
Contributor
Reviewer's GuideThis PR changes playbook system resolution to prefer a local systems table before falling back to Inventory, adds a query helper for fetching system details from the local DB, re-exports the system storage helper for reuse, and updates seed data to include ansible_host for a test system. Sequence diagram for resolving systems using local DB with Inventory fallbacksequenceDiagram
participant GeneratorController as GeneratorController
participant RemediationsQueries as RemediationsQueries
participant DB as DB_systems_table
participant Inventory as InventoryConnector
participant WriteController as RemediationsWriteController
GeneratorController->>GeneratorController: resolveSystems(issues, strict)
GeneratorController->>GeneratorController: extract systemIds from issues
GeneratorController->>RemediationsQueries: getSystemDetailsForPlaybook(systemIds)
RemediationsQueries->>DB: SELECT id, hostname, ansible_hostname, display_name WHERE id IN systemIds
DB-->>RemediationsQueries: systems rows
RemediationsQueries-->>GeneratorController: systemsByIdFromDB
GeneratorController->>GeneratorController: compute missingIds (not in systemsByIdFromDB)
alt missingIds not empty
GeneratorController->>Inventory: getSystemDetailsBatch(missingIds, true)
Inventory-->>GeneratorController: inventoryData
GeneratorController->>WriteController: storeSystemDetails(inventoryData)
WriteController-->>GeneratorController: (async completion, errors logged if any)
GeneratorController->>GeneratorController: merge systems = systemsByIdFromDB + inventoryData
else no missingIds
GeneratorController->>GeneratorController: systems = systemsByIdFromDB
end
alt strict is false
GeneratorController->>GeneratorController: filter issue.systems to ids present in systems
GeneratorController->>GeneratorController: remove issues with no remaining systems
end
GeneratorController->>GeneratorController: map issue.systems to issue.hosts using systems
GeneratorController->>GeneratorController: throw error if any system id missing when strict is true
GeneratorController-->>GeneratorController: return issues with resolved hosts
Class diagram for updated system resolution and storage helpersclassDiagram
class GeneratorController {
+resolveSystems(issues, strict)
}
class RemediationsQueries {
+getPlaybookRunsWithDispatcherCounts(playbookRunIds)
+getSystemDetailsForPlaybook(systemIds)
}
class RemediationsWriteController {
+removeSystem(req, res)
+storeSystemDetails(systemDetails)
}
class SystemsModel {
+id: uuid
+hostname: string
+ansible_hostname: string
+display_name: string
}
class InventoryConnector {
+getSystemDetailsBatch(systemIds, bypassCache)
}
GeneratorController --> RemediationsQueries : uses getSystemDetailsForPlaybook
RemediationsQueries --> SystemsModel : queries
GeneratorController --> InventoryConnector : uses getSystemDetailsBatch
GeneratorController --> RemediationsWriteController : uses storeSystemDetails
RemediationsWriteController --> SystemsModel : persists systemDetails
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- When computing
missingIds, consider usingObject.prototype.hasOwnProperty.call(systems, id)instead ofid in systemsto avoid accidental matches on object prototype properties and keep the existence checks consistent with the rest ofresolveSystems. - In
getSystemDetailsForPlaybook, it may be safer to explicitly use an$incondition (e.g.{ id: { [Op.in]: systemIds } }) rather than relying on implicit array handling inwhere: { id: systemIds }, to make the query behavior clearer and more robust across Sequelize versions.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- When computing `missingIds`, consider using `Object.prototype.hasOwnProperty.call(systems, id)` instead of `id in systems` to avoid accidental matches on object prototype properties and keep the existence checks consistent with the rest of `resolveSystems`.
- In `getSystemDetailsForPlaybook`, it may be safer to explicitly use an `$in` condition (e.g. `{ id: { [Op.in]: systemIds } }`) rather than relying on implicit array handling in `where: { id: systemIds }`, to make the query behavior clearer and more robust across Sequelize versions.
## Individual Comments
### Comment 1
<location path="src/generator/generator.controller.js" line_range="147-148" />
<code_context>
+ trace.event(`Fetching ${missingIds.length} missing systems from Inventory...`);
+ // bypass cache as ansible_host may change so we want to grab the latest one
+ const inventoryData = await inventory.getSystemDetailsBatch(missingIds, true);
+ // Store Inventory systems our in our local systems table so we don't have to fetch from Inventory next time
+ storeSystemDetails(inventoryData).catch(err => log.warn({ err }, 'Failed to store system details'));
+ systems = { ...systems, ...inventoryData };
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Reconsider fire-and-forget `storeSystemDetails` error handling and visibility.
Because this call is fire-and-forget with only a warn-level log, persistent failures in `storeSystemDetails` could silently desync Inventory from the local DB. Consider either turning this into a proper background job with metrics/alerts, or at least enriching the log (e.g., system count, ID range) and increasing severity so repeated failures are visible operationally.
Suggested implementation:
```javascript
// bypass cache as ansible_host may change so we want to grab the latest one
const inventoryData = await inventory.getSystemDetailsBatch(missingIds, true);
// Store Inventory systems in our local systems table so we don't have to fetch from Inventory next time.
// Fire-and-forget, but with enriched logging so persistent failures are operationally visible.
storeSystemDetails(inventoryData).catch(err => {
const storedCount = inventoryData ? Object.keys(inventoryData).length : 0;
log.error(
{
err,
storedCount,
requestedSystemIds: missingIds
},
'Failed to store system details fetched from Inventory; local systems DB may be out of sync'
);
});
systems = { ...systems, ...inventoryData };
```
If your service already has metrics/alerting, consider:
1. Emitting a counter for failures (e.g., `metrics.increment('generator.store_system_details.failure', storedCount)` or equivalent) inside the `catch` block.
2. Optionally, surfacing a separate alert when the failure rate crosses a threshold over time.
These will need to follow whatever metrics/logging conventions and libraries exist elsewhere in the codebase.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Collaborator
Author
|
This PR focuses on using the local What I think we should handle in separate PRs/tickets:
|
Collaborator
Author
|
I realized we needed to handle the Inventory 404s correctly in the resolveSystems function in generator controller because we use the strict flags in there. If strict=true we're supposed to throw an UNKNOWN_SYSTEM error and if strict=false we're supposed to gracefully handle it and filter out the unknown systems. There are some changes in #1049 that helped were needed for this but I didn't want to combine the PRs into one.. So there's some overlap between that PR and this one. I'll rebase this one after we merge 1049. |
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
1 participant
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.
Summary by Sourcery
Use the local systems table as the primary source for system details when generating playbooks, falling back to Inventory only for missing systems and caching those results locally.
New Features:
Enhancements: