-
Notifications
You must be signed in to change notification settings - Fork 3.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
remove computed inventory fields from Host and Group #5448
Changes from all commits
568606d
107d2da
2bc6521
ec1c2a8
1220847
773b976
44c0eb8
be68a19
0f0d9ba
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -98,21 +98,14 @@ | |||||
'total_hosts', | ||||||
'hosts_with_active_failures', | ||||||
'total_groups', | ||||||
'groups_with_active_failures', | ||||||
'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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We deprecated these fields recently. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I can't imagine that we do
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. |
||||||
'total_hosts', | ||||||
'hosts_with_active_failures', | ||||||
'total_groups', | ||||||
'groups_with_active_failures', | ||||||
'has_inventory_sources'), | ||||||
'host': DEFAULT_SUMMARY_FIELDS, | ||||||
'group': DEFAULT_SUMMARY_FIELDS, | ||||||
'project': DEFAULT_SUMMARY_FIELDS + ('status', 'scm_type'), | ||||||
'source_project': DEFAULT_SUMMARY_FIELDS + ('status', 'scm_type'), | ||||||
'project_update': DEFAULT_SUMMARY_FIELDS + ('status', 'failed',), | ||||||
|
@@ -1549,20 +1542,15 @@ class InventorySerializer(BaseSerializerWithVariables): | |||||
'admin', 'adhoc', | ||||||
{'copy': 'organization.inventory_admin'} | ||||||
] | ||||||
groups_with_active_failures = serializers.IntegerField( | ||||||
read_only=True, | ||||||
min_value=0, | ||||||
help_text=_('This field has been deprecated and will be removed in a future release') | ||||||
) | ||||||
|
||||||
|
||||||
class Meta: | ||||||
model = Inventory | ||||||
fields = ('*', 'organization', 'kind', 'host_filter', 'variables', 'has_active_failures', | ||||||
'total_hosts', 'hosts_with_active_failures', 'total_groups', | ||||||
'groups_with_active_failures', 'has_inventory_sources', | ||||||
'total_inventory_sources', 'inventory_sources_with_failures', | ||||||
'insights_credential', 'pending_deletion',) | ||||||
'has_inventory_sources', 'total_inventory_sources', | ||||||
'inventory_sources_with_failures', 'insights_credential', | ||||||
'pending_deletion',) | ||||||
|
||||||
def get_related(self, obj): | ||||||
res = super(InventorySerializer, self).get_related(obj) | ||||||
|
@@ -1644,6 +1632,9 @@ class HostSerializer(BaseSerializerWithVariables): | |||||
show_capabilities = ['edit', 'delete'] | ||||||
capabilities_prefetch = ['inventory.admin'] | ||||||
|
||||||
has_active_failures = serializers.SerializerMethodField() | ||||||
has_inventory_sources = serializers.SerializerMethodField() | ||||||
|
||||||
class Meta: | ||||||
model = Host | ||||||
fields = ('*', 'inventory', 'enabled', 'instance_id', 'variables', | ||||||
|
@@ -1757,6 +1748,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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||
) | ||||||
|
||||||
def get_has_inventory_sources(self, obj): | ||||||
return obj.inventory_sources.exists() | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will probably incur an additional query. However, I see it as a valid candidate for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, I checked, and this does add more queries, so I added it to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||
|
||||||
|
||||||
class AnsibleFactsSerializer(BaseSerializer): | ||||||
class Meta: | ||||||
|
@@ -1769,17 +1768,10 @@ def to_representation(self, obj): | |||||
class GroupSerializer(BaseSerializerWithVariables): | ||||||
show_capabilities = ['copy', 'edit', 'delete'] | ||||||
capabilities_prefetch = ['inventory.admin', 'inventory.adhoc'] | ||||||
groups_with_active_failures = serializers.IntegerField( | ||||||
read_only=True, | ||||||
min_value=0, | ||||||
help_text=_('This field has been deprecated and will be removed in a future release') | ||||||
) | ||||||
|
||||||
class Meta: | ||||||
model = Group | ||||||
fields = ('*', 'inventory', 'variables', 'has_active_failures', | ||||||
'total_hosts', 'hosts_with_active_failures', 'total_groups', | ||||||
'groups_with_active_failures', 'has_inventory_sources') | ||||||
fields = ('*', 'inventory', 'variables') | ||||||
|
||||||
def build_relational_field(self, field_name, relation_info): | ||||||
field_class, field_kwargs = super(GroupSerializer, self).build_relational_field(field_name, relation_info) | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -204,20 +204,15 @@ 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This makes group-based failure tracking no longer exist/supported at There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, that's my impression, too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
models.Group.objects.filter(hosts_with_active_failures__gt=0) | models.Group.objects.filter(groups_with_active_failures__gt=0) | ||
).count() | ||
groups_inventory_failed = models.Group.objects.filter(inventory_sources__last_job_failed=True).count() | ||
data['groups'] = {'url': reverse('api:group_list', request=request), | ||
'failures_url': reverse('api:group_list', request=request) + "?has_active_failures=True", | ||
'total': user_groups.count(), | ||
'job_failed': groups_job_failed, | ||
'inventory_failed': groups_inventory_failed} | ||
|
||
user_hosts = get_user_queryset(request.user, models.Host) | ||
user_hosts_failed = user_hosts.filter(has_active_failures=True) | ||
user_hosts_failed = user_hosts.filter(last_job_host_summary__failed=True) | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doing this filter, I get different answers in my own server compared to doing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
'total': user_hosts.count(), | ||
'failed': user_hosts_failed.count()} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,52 @@ | ||
# -*- coding: utf-8 -*- | ||
# Generated by Django 1.11.16 on 2019-02-21 17:35 | ||
from __future__ import unicode_literals | ||
|
||
from django.db import migrations, models | ||
|
||
|
||
class Migration(migrations.Migration): | ||
|
||
dependencies = [ | ||
('main', '0102_v370_unifiedjob_canceled'), | ||
] | ||
|
||
operations = [ | ||
migrations.RemoveField( | ||
model_name='group', | ||
name='groups_with_active_failures', | ||
), | ||
migrations.RemoveField( | ||
model_name='group', | ||
name='has_active_failures', | ||
), | ||
migrations.RemoveField( | ||
model_name='group', | ||
name='has_inventory_sources', | ||
), | ||
migrations.RemoveField( | ||
model_name='group', | ||
name='hosts_with_active_failures', | ||
), | ||
migrations.RemoveField( | ||
model_name='group', | ||
name='total_groups', | ||
), | ||
migrations.RemoveField( | ||
model_name='group', | ||
name='total_hosts', | ||
), | ||
migrations.RemoveField( | ||
model_name='host', | ||
name='has_active_failures', | ||
), | ||
migrations.RemoveField( | ||
model_name='host', | ||
name='has_inventory_sources', | ||
), | ||
migrations.AlterField( | ||
model_name='jobhostsummary', | ||
name='failed', | ||
field=models.BooleanField(db_index=True, default=False, editable=False), | ||
), | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the performance differential when handling this in the list views?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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:
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:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Next, I made a bunch of inventory sources, and a ton of
JobHostSummary
objects associated with every host: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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: I added some more prefetching to speed this up a bit, and it cut the query count in half:
https://github.com/ansible/awx/pull/5448/files#r362574316