Populate HostMetric.used_in_inventories with actual inventory counts#16355
Open
tll3r wants to merge 1 commit intoansible:develfrom
Open
Populate HostMetric.used_in_inventories with actual inventory counts#16355tll3r wants to merge 1 commit intoansible:develfrom
tll3r wants to merge 1 commit intoansible:develfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds a periodic task, a management command, configuration entries, and tests to compute and store per-host distinct-inventory counts on HostMetric records and record the task's last-run timestamp and interval. Changes
Sequence DiagramsequenceDiagram
participant Scheduler as Scheduler / Cron
participant CLI as Management Command
participant Task as HostMetricInventoryCountTask
participant DB as Database (HostMetric, Host, Inventory)
Scheduler->>CLI: Trigger scheduled task
CLI->>Task: invoke update_counts()
Task->>DB: SELECT COUNT(DISTINCT inventory_id) per hostname (Subquery/OuterRef)
DB-->>Task: per-host inventory counts
Task->>DB: UPDATE HostMetric.used_in_inventories (COALESCE counts, 0)
Task->>DB: UPDATE config HOST_METRIC_INVENTORY_COUNTS_LAST_TS
Task-->>CLI: return updated count
CLI-->>Scheduler: output summary
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 the current code and only fix it if needed.
Inline comments:
In `@awx/main/tasks/host_metrics.py`:
- Around line 40-53: Wrap the threshold check and the call to
HostMetricInventoryCountTask.update_counts in a Postgres advisory lock so only
one worker can perform the check+update sequence: acquire the advisory lock at
the start of update_host_metric_inventory_counts (before calling
is_run_threshold_reached), re-check the threshold inside the lock, then call
HostMetricInventoryCountTask.update_counts and release the lock; use the
existing DB utilities or a pg_advisory_lock wrapper used elsewhere in the
project (or implement a short-timeout/non-blocking advisory lock) so concurrent
workers skip execution if they cannot obtain the lock.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dcb3ca11-33ff-4e0a-8ac2-f0466d61cfd0
📒 Files selected for processing (5)
awx/main/conf.pyawx/main/management/commands/update_host_metric_inventory_counts.pyawx/main/tasks/host_metrics.pyawx/main/tests/functional/commands/test_update_host_metric_inventory_counts.pyawx/settings/defaults.py
tll3r
force-pushed
the
fix/populate-used-in-inventories
branch
from
3139993 to
61c4740
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@awx/main/tasks/host_metrics.py`:
- Around line 308-315: The current correlated-subquery approach in
inventory_count_subquery combined with
HostMetric.objects.update(used_in_inventories=...) forces a full-table rewrite;
instead compute the new counts in a single grouped query (use
Host.objects.values('name').annotate(new_cnt=Count('inventory_id',
distinct=True)) to get a mapping of hostname -> new_cnt), then query HostMetric
for hostnames where used_in_inventories != new_cnt
(HostMetric.objects.filter(hostname__in=..., used_in_inventories__ne=...)) and
apply updates only to those rows using bulk_update (or an efficient per-row
update loop) to set used_in_inventories to the computed value; finally set
settings.HOST_METRIC_INVENTORY_COUNTS_LAST_TS = now() only after performing the
selective updates.
- Around line 47-50: The current guard using is_run_threshold_reached with
getattr(settings, 'HOST_METRIC_INVENTORY_COUNTS_INTERVAL', 7) * 86400 makes the
HostMetric used_in_inventories refresh only once a week; update this to match
the dispatcher freshness (e.g., default to 4 hours) or remove the threshold
entirely. Modify the call in host_metrics.py that references
is_run_threshold_reached and the settings keys
HOST_METRIC_INVENTORY_COUNTS_LAST_TS and HOST_METRIC_INVENTORY_COUNTS_INTERVAL
so the default interval is 4*3600 (or delegate to the dispatcher interval
config) or eliminate the is_run_threshold_reached check so used_in_inventories
on HostMetric is recalculated every dispatcher run. Ensure any change keeps the
same settings names and semantics so tests and callers (HostMetric updates)
continue to work.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2875a846-0423-4009-afe9-4a4119d2399b
📒 Files selected for processing (5)
awx/main/conf.pyawx/main/management/commands/update_host_metric_inventory_counts.pyawx/main/tasks/host_metrics.pyawx/main/tests/functional/commands/test_update_host_metric_inventory_counts.pyawx/settings/defaults.py
🚧 Files skipped from review as they are similar to previous changes (3)
- awx/main/conf.py
- awx/settings/defaults.py
- awx/main/management/commands/update_host_metric_inventory_counts.py
Comment on lines
+47
to
+50
| if is_run_threshold_reached( | ||
| getattr(settings, 'HOST_METRIC_INVENTORY_COUNTS_LAST_TS', None), | ||
| getattr(settings, 'HOST_METRIC_INVENTORY_COUNTS_INTERVAL', 7) * 86400, | ||
| ): |
There was a problem hiding this comment.
Defaulting this task to a 7-day threshold leaves the new field stale.
Line 49 throttles the only automatic refresh to once a week, so any HostMetric created or any inventory membership change after that run can leave used_in_inventories null/outdated for days even though the dispatcher wakes up every 4 hours. Please align this guard with the intended freshness of the field, or drop the extra threshold for this task.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@awx/main/tasks/host_metrics.py` around lines 47 - 50, The current guard using
is_run_threshold_reached with getattr(settings,
'HOST_METRIC_INVENTORY_COUNTS_INTERVAL', 7) * 86400 makes the HostMetric
used_in_inventories refresh only once a week; update this to match the
dispatcher freshness (e.g., default to 4 hours) or remove the threshold
entirely. Modify the call in host_metrics.py that references
is_run_threshold_reached and the settings keys
HOST_METRIC_INVENTORY_COUNTS_LAST_TS and HOST_METRIC_INVENTORY_COUNTS_INTERVAL
so the default interval is 4*3600 (or delegate to the dispatcher interval
config) or eliminate the is_run_threshold_reached check so used_in_inventories
on HostMetric is recalculated every dispatcher run. Ensure any change keeps the
same settings names and semantics so tests and callers (HostMetric updates)
continue to work.
The used_in_inventories field on HostMetric was introduced in PR ansible#13560 but the backend logic to populate it was never implemented, leaving the field permanently null since AAP 2.4. This adds: - HostMetricInventoryCountTask: counts distinct inventories per hostname by correlating Host.name with HostMetric.hostname - update_host_metric_inventory_counts: weekly scheduled task that invokes the count update (controlled via HOST_METRIC_INVENTORY_COUNTS_INTERVAL) - DISPATCHER_SCHEDULE entry so the task runs automatically every 4 hours (with an internal 7-day threshold guard matching the existing pattern) - HOST_METRIC_INVENTORY_COUNTS_LAST_TS registered in conf for persistence - awx-manage update_host_metric_inventory_counts: management command for on-demand execution - Functional tests covering zero, single, multiple inventories, mixed hosts, idempotency, and inventory membership changes Resolves the incomplete feature tracked in AWX PRs ansible#13560, ansible#13782, ansible#13999. Made-with: Cursor
tll3r
force-pushed
the
fix/populate-used-in-inventories
branch
from
61c4740 to
1749ace
Compare
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
The
used_in_inventoriesfield onHostMetricwas introduced in PR #13560 (Feb 2023) but the backend logic to populate it was never implemented, leaving the field permanentlynullsince AAP 2.4. The UI column was subsequently removed in PR #13782 with the note "Revert this commit once the backend is ready."This PR adds the missing backend:
HostMetricInventoryCountTask: counts distinct inventories per hostname by correlatingHost.namewithHostMetric.hostnamevia a single bulkUPDATEwith a correlated subqueryupdate_host_metric_inventory_counts: scheduled task (dispatched every 4 hours, with an internal 7-day run-threshold guard matching the existinghost_metric_summary_monthlypattern)HOST_METRIC_INVENTORY_COUNTS_LAST_TS: persisted via the conf registry so the run-threshold survives restartsawx-manage update_host_metric_inventory_counts: management command for on-demand executionRelated Issues
HostMetricmodel withused_in_inventoriescolumnhost_metric_summary_monthly(does not populateused_in_inventories)Test plan
awx-manage update_host_metric_inventory_countspopulates the field correctlyGET /api/v2/host_metrics/?order_by=-used_in_inventoriesreturns correct non-null valuespytest awx/main/tests/functional/commands/test_update_host_metric_inventory_counts.pyMade with Cursor
Summary by CodeRabbit