remove computed inventory fields from Host and Group #5448
Conversation
ryanpetrello
requested review from
matburt,
AlanCoding,
wenottingham,
jakemcdermott and
mabashian
| 'has_inventory_sources', | ||
| 'total_inventory_sources', | ||
| 'inventory_sources_with_failures', | ||
| 'organization_id', | ||
| 'kind', | ||
| 'insights_credential_id',), | ||
| 'host': DEFAULT_SUMMARY_FIELDS + ('has_active_failures', | ||
| 'has_inventory_sources'), | ||
| 'group': DEFAULT_SUMMARY_FIELDS + ('has_active_failures', |
There was a problem hiding this comment.
Group loses all of these - assuming we remove the green/red icons and don't care about API backwards compat at /api/v2/dashboard/, I can't see any tangible reason we spend our time calculating these.
We deprecated these fields recently.
There was a problem hiding this comment.
assuming we ... don't care about API backwards compat at /
api/v2/dashboard/
I can't imagine that we do
assuming we remove the green/red icons
That is a removal of user-facing functionality. There is very non-trivial meaning in those orbs, because the Ansible notion of membership includes indirect via children groups. At the same time, merely a red/green indicator is a pretty gross over-simplification. Anyway, the final call is within the UX wheelhouse.
| 'has_inventory_sources', | ||
| 'total_inventory_sources', | ||
| 'inventory_sources_with_failures', | ||
| 'organization_id', | ||
| 'kind', | ||
| 'insights_credential_id',), | ||
| 'host': DEFAULT_SUMMARY_FIELDS + ('has_active_failures', |
There was a problem hiding this comment.
Host loses all of these, and they're replaced w/ queries that generate the data from other sources:
https://github.com/ansible/awx/pull/5448/files#diff-a81324c523b41de7296fdd5ff9063d10R1748
There was a problem hiding this comment.
What's the performance differential when handling this in the list views?
There was a problem hiding this comment.
@matburt the prior case is going to be slightly faster on list views (it's just two boolean field lookups).
The after version has two new generated fields on the list view now:
def get_has_active_failures(self, obj):
return bool(
obj.last_job_host_summary and obj.last_job_host_summary.failed
)
def get_has_inventory_sources(self, obj):
return obj.inventory_sources.exists()Here's the /api/v2/hosts/ list view before I preloaded my database with a bunch of data. Note that I've set up an inventory w/ a bunch of hosts:
awx-dev=> SELECT COUNT(*) FROM main_host;
count
-------
22809
(1 row)
There was a problem hiding this comment.
Next, I made a bunch of inventory sources, and a ton of JobHostSummary objects associated with every host:
In [1]: i = Inventory.objects.first()
In [2]: for s in InventorySource.objects.all():
...: for h in Host.objects.all():
...: h.inventory_sources.add(s)
...:
In [24]: for i in range(MANY):
...: j = Job.objects.create()
...: for h in Host.objects.all():
...: j.job_host_summaries.get_or_create(host=h, host_name=h.name, defaults=host_stats)
...:
...:awx-dev=> SELECT COUNT(*) FROM main_jobhostsummary;
count
--------
633365
(1 row)
awx-dev=> SELECT COUNT(*) FROM main_host_inventory_sources;
count
-------
132610
(1 row)First pass on this shows that it's definitely slower, but not that much slower:
I'm going to keep running my data generator all day and see if I can get some of these tables into the millions and see if I notice any additional performance degradation.
I'll also see if I can sprinkle in some ORM prefetching to cut down on these queries a bit.
There was a problem hiding this comment.
Update: I added some more prefetching to speed this up a bit, and it cut the query count in half:
| @@ -204,20 +204,14 @@ def get(self, request, format=None): | |||
| 'failed': ec2_inventory_failed.count()} | |||
|
|
|||
| user_groups = get_user_queryset(request.user, models.Group) | |||
| groups_job_failed = ( | |||
There was a problem hiding this comment.
This makes group-based failure tracking no longer exist/supported at /api/v2/dashboard/.
There was a problem hiding this comment.
It feels like this was for the old inventory model when groups and inventory sources were tied together.
There was a problem hiding this comment.
Yep, that's my impression, too.
ryanpetrello
force-pushed
the
remove-computed-group-and-host-fields
branch
2 times, most recently
from
1a3fccd
to
2384ccf
Compare
|
Inventory failure tracking still works:
|
|
Host state and recent jobs tracking works:
|
|
Groups have no status indicators anymore:
|
|
Dashboard loads, and the top failure links work when clicked on:
|
ryanpetrello
force-pushed
the
remove-computed-group-and-host-fields
branch
from
2384ccf
to
2db4181
Compare
| @@ -1756,6 +1745,14 @@ def to_representation(self, obj): | |||
| ret['last_job_host_summary'] = None | |||
| return ret | |||
|
|
|||
| def get_has_active_failures(self, obj): | |||
| return bool( | |||
| obj.last_job_host_summary and obj.last_job_host_summary.failed | |||
There was a problem hiding this comment.
Oh, so I guess we're already fetching this information and showing it in summary_fields. And fetching it... and even more
Lines 908 to 909 in 90d38a5
| data['hosts'] = {'url': reverse('api:host_list', request=request), | ||
| 'failures_url': reverse('api:host_list', request=request) + "?has_active_failures=True", | ||
| 'failures_url': reverse('api:host_list', request=request) + "?last_job_host_summary__failed=True", |
There was a problem hiding this comment.
Doing this filter, I get different answers in my own server compared to doing /api/v2/hosts/?last_job__failed=true. Reason still unclear.
There was a problem hiding this comment.
Okay, this means that the host had failed tasks.
last job failed means that any host in the last job this host was a part of failed.
|
Build failed.
|
ryanpetrello
changed the title
| ) | ||
|
|
||
| def get_has_inventory_sources(self, obj): | ||
| return obj.inventory_sources.exists() |
There was a problem hiding this comment.
This will probably incur an additional query. However, I see it as a valid candidate for prefetch_related, because there will be a large amount of duplication in many cases.
There was a problem hiding this comment.
Yep, I checked, and this does add more queries, so I added it to prefetch_related and watched the query count go down.
There was a problem hiding this comment.
Thanks. That might exhaust the actionable comments I had. I will go back over the diff one last time.
There was a problem hiding this comment.
|
I was worried about this, but much lower concern after seeing this. I would still like to profile the queries from the hosts endpoints. |
I do think we should add an index to |
ryanpetrello
force-pushed
the
remove-computed-group-and-host-fields
branch
from
2db4181
to
0171f8a
Compare
|
Build failed.
|
ryanpetrello
force-pushed
the
remove-computed-group-and-host-fields
branch
from
0171f8a
to
057334a
Compare
|
Build failed.
|
ryanpetrello
force-pushed
the
remove-computed-group-and-host-fields
branch
from
057334a
to
1a53f82
Compare
|
Build succeeded.
|
|
Build succeeded.
|
jakemcdermott
force-pushed
the
remove-computed-group-and-host-fields
branch
from
af97959
to
c2d3357
Compare
|
Build succeeded.
|
ryanpetrello
force-pushed
the
remove-computed-group-and-host-fields
branch
from
c2d3357
to
dfffa87
Compare
|
Build failed.
|
|
Build succeeded.
|
Remove the status col from any group list that used the now-removed computed fields.
ryanpetrello
force-pushed
the
remove-computed-group-and-host-fields
branch
from
588d460
to
ed9222a
Compare
|
Build failed.
|
ryanpetrello
force-pushed
the
remove-computed-group-and-host-fields
branch
from
ed9222a
to
be68a19
Compare
|
Build succeeded.
|
|
Build succeeded.
|
|
Build failed (gate pipeline). For information on how to proceed, see
|
ryanpetrello
force-pushed
the
remove-computed-group-and-host-fields
branch
from
512244d
to
0f0d9ba
Compare
|
Build succeeded.
|
|
Build succeeded (gate pipeline).
|
Move only_transmit_kwargs calculation out of thread
see: https://github.com/ansible/tower/issues/2592