diff --git a/awx/api/filters.py b/awx/api/filters.py index 4493cdc6..1c4a6060 100644 --- a/awx/api/filters.py +++ b/awx/api/filters.py @@ -45,8 +45,8 @@ def hosts_without_latest_summary(missing): class HostFieldLookupBackend(FieldLookupBackend): """ Resolves Host.last_job and Host.last_job_host_summary against the newest - JobHostSummary for each host. The columns of the same name are denormalized - caches that are no longer written, so a plain lookup matches nothing. + JobHostSummary for each host. These are serializer-derived fields with no + backing column on Host, so a plain lookup would be rejected as unknown. """ def value_to_python(self, model, lookup, value): diff --git a/awx/api/serializers.py b/awx/api/serializers.py index 8facaf49..bbc2d96f 100644 --- a/awx/api/serializers.py +++ b/awx/api/serializers.py @@ -179,8 +179,8 @@ 'workflow_approval': DEFAULT_SUMMARY_FIELDS + ('timeout', 'status'), 'schedule': DEFAULT_SUMMARY_FIELDS + ('next_run',), 'unified_job_template': DEFAULT_SUMMARY_FIELDS + ('unified_job_type',), - # last_job and last_job_host_summary are derived from JobHostSummary in HostSerializer, - # not from the stale FK fields on Host. + 'last_job': DEFAULT_SUMMARY_FIELDS + ('finished', 'status', 'failed', 'license_error', 'canceled_on'), + 'last_job_host_summary': DEFAULT_SUMMARY_FIELDS + ('failed',), 'last_update': DEFAULT_SUMMARY_FIELDS + ('status', 'failed', 'license_error'), 'current_update': DEFAULT_SUMMARY_FIELDS + ('status', 'failed', 'license_error'), 'current_job': DEFAULT_SUMMARY_FIELDS + ('status', 'failed', 'license_error'), @@ -2008,6 +2008,8 @@ class HostSerializer(BaseSerializerWithVariables): has_active_failures = serializers.SerializerMethodField() has_inventory_sources = serializers.SerializerMethodField() + last_job = serializers.SerializerMethodField() + last_job_host_summary = serializers.SerializerMethodField() class Meta: model = Host @@ -2024,7 +2026,7 @@ class Meta: 'last_job_host_summary', 'ansible_facts_modified', ) - read_only_fields = ('last_job', 'last_job_host_summary', 'ansible_facts_modified') + read_only_fields = ('ansible_facts_modified',) def build_relational_field(self, field_name, relation_info): field_class, field_kwargs = super(HostSerializer, self).build_relational_field(field_name, relation_info) @@ -2166,13 +2168,16 @@ def to_representation(self, obj): return ret if 'inventory' in ret and not obj.inventory: ret['inventory'] = None - last_summary = obj.latest_summary - if 'last_job' in ret: - ret['last_job'] = last_summary.job_id if last_summary else None - if 'last_job_host_summary' in ret: - ret['last_job_host_summary'] = last_summary.pk if last_summary else None return ret + def get_last_job(self, obj): + last_summary = obj.latest_summary + return last_summary.job_id if last_summary else None + + def get_last_job_host_summary(self, obj): + last_summary = obj.latest_summary + return last_summary.pk if last_summary else None + def get_has_active_failures(self, obj): last_summary = obj.latest_summary return bool(last_summary and last_summary.failed) diff --git a/awx/main/management/commands/cleanup_jobs.py b/awx/main/management/commands/cleanup_jobs.py index 80c8e5b6..9bfd4ee1 100644 --- a/awx/main/management/commands/cleanup_jobs.py +++ b/awx/main/management/commands/cleanup_jobs.py @@ -54,12 +54,11 @@ def dt_to_partition_name(tbl_name, dt): def _pre_delete_job_host_summaries(job_pks, logger=None): - """Pre-delete JobHostSummary rows and clear Host FK references in batches. + """Pre-delete JobHostSummary rows in batches. - Django's cascade collector materializes all JHS IDs into a single - UPDATE ... IN (...) to SET_NULL on Host.last_job_host_summary. - With many jobs x hosts this exceeds PostgreSQL's 1GB alloc limit. - Doing it in chunks with raw SQL avoids that. + Deleting via raw SQL in chunks avoids Django's cascade collector + materializing all JHS IDs into memory at once, which can exceed + PostgreSQL's 1GB alloc limit when many jobs x hosts are involved. """ if not job_pks: return @@ -69,13 +68,6 @@ def _pre_delete_job_host_summaries(job_pks, logger=None): for i in range(0, len(job_pks), JHS_CHUNK_SIZE): chunk = list(job_pks[i : i + JHS_CHUNK_SIZE]) - cursor.execute( - "UPDATE main_host SET last_job_host_summary_id = NULL" - " WHERE last_job_host_summary_id IN" - " (SELECT id FROM main_jobhostsummary WHERE job_id = ANY(%s))", - [chunk], - ) - cursor.execute( "DELETE FROM main_jobhostsummary WHERE job_id = ANY(%s)", [chunk], diff --git a/awx/main/migrations/0211_remove_host_last_job_fields.py b/awx/main/migrations/0211_remove_host_last_job_fields.py new file mode 100644 index 00000000..12f9bb67 --- /dev/null +++ b/awx/main/migrations/0211_remove_host_last_job_fields.py @@ -0,0 +1,18 @@ +from django.db import migrations + + +class Migration(migrations.Migration): + dependencies = [ + ('main', '0210_notification_templates_changed'), + ] + + operations = [ + migrations.RemoveField( + model_name='host', + name='last_job', + ), + migrations.RemoveField( + model_name='host', + name='last_job_host_summary', + ), + ] diff --git a/awx/main/models/events.py b/awx/main/models/events.py index 3c4dd3e8..6ba5c2d8 100644 --- a/awx/main/models/events.py +++ b/awx/main/models/events.py @@ -583,9 +583,6 @@ def _update_host_summary_from_stats(self, hostnames): JobHostSummary.objects.bulk_create(summaries.values()) - # last_job and last_job_host_summary are now derived via - # JobHostSummary.latest_for_host / latest_job_for_host - # Create/update Host Metrics self._update_host_metrics(updated_hosts_list) diff --git a/awx/main/models/inventory.py b/awx/main/models/inventory.py index 22fe0feb..8aad26df 100644 --- a/awx/main/models/inventory.py +++ b/awx/main/models/inventory.py @@ -562,23 +562,6 @@ class Meta: help_text=_('Host variables in JSON or YAML format.'), ) ) - last_job = models.ForeignKey( - 'Job', - related_name='hosts_as_last_job+', - null=True, - default=None, - editable=False, - on_delete=models.SET_NULL, - ) - last_job_host_summary = models.ForeignKey( - 'JobHostSummary', - related_name='hosts_as_last_job_summary+', - blank=True, - null=True, - default=None, - editable=False, - on_delete=models.SET_NULL, - ) inventory_sources = models.ManyToManyField( 'InventorySource', related_name='hosts', diff --git a/awx/main/models/jobs.py b/awx/main/models/jobs.py index d17ace16..c542e36e 100644 --- a/awx/main/models/jobs.py +++ b/awx/main/models/jobs.py @@ -1237,22 +1237,6 @@ def __str__(self): self.skipped, ) - @classmethod - def latest_for_host(cls, host_id): - """Return the most recent JobHostSummary for a given host, or None.""" - return cls.objects.filter(host_id=host_id).order_by('-id').first() - - @classmethod - def latest_job_for_host(cls, host_id): - """Return the Job from the most recent JobHostSummary for a host, or None.""" - summary = cls.latest_for_host(host_id) - if summary: - try: - return summary.job - except cls.job.field.related_model.DoesNotExist: - return None - return None - def get_absolute_url(self, request=None): return reverse('api:job_host_summary_detail', kwargs={'pk': self.pk}, request=request) diff --git a/awx/main/signals.py b/awx/main/signals.py index afb77bfd..f34cfb73 100644 --- a/awx/main/signals.py +++ b/awx/main/signals.py @@ -277,11 +277,6 @@ def migrate_children_from_deleted_group_to_parent_groups(sender, **kwargs): pass -# Host.last_job and Host.last_job_host_summary are now derived from -# JobHostSummary.latest_for_host / latest_job_for_host. -# No signal handlers needed to maintain these denormalized FKs. - - # Set via ActivityStreamRegistrar to record activity stream events diff --git a/awx/main/tests/functional/api/test_inventory.py b/awx/main/tests/functional/api/test_inventory.py index 71a48383..25b1fd42 100644 --- a/awx/main/tests/functional/api/test_inventory.py +++ b/awx/main/tests/functional/api/test_inventory.py @@ -4,6 +4,7 @@ from unittest import mock from django.core.exceptions import ValidationError +from django.utils.timezone import now from awx.api.versioning import reverse @@ -259,6 +260,26 @@ def test_create_inventory_smart_inventory_sources(post, get, inventory, admin_us assert jdata['count'] == 0 +@pytest.mark.django_db +def test_inventory_source_summary_fields_include_last_job(get, inventory_source, admin_user): + """The UI's source list Status column reads summary_fields.last_job; it must + survive perf changes to host summaries (see SUMMARIZABLE_FK_FIELDS).""" + update = inventory_source.create_unified_job() + update.status = 'successful' + update.finished = now() + update.save() + + url = reverse('api:inventory_inventory_sources_list', kwargs={'pk': inventory_source.inventory.pk}) + resp = get(url, admin_user, expect=200) + source_data = resp.data['results'][0] + + assert 'last_job' in source_data['summary_fields'] + last_job = source_data['summary_fields']['last_job'] + assert last_job['id'] == update.id + assert last_job['status'] == 'successful' + assert last_job['finished'] == update.finished + + @pytest.mark.django_db def test_urlencode_host_filter(post, admin_user, organization): """ diff --git a/awx/main/tests/functional/commands/test_cleanup_jobs_postgres.py b/awx/main/tests/functional/commands/test_cleanup_jobs_postgres.py index 906cb9dd..b43757aa 100644 --- a/awx/main/tests/functional/commands/test_cleanup_jobs_postgres.py +++ b/awx/main/tests/functional/commands/test_cleanup_jobs_postgres.py @@ -39,18 +39,14 @@ def _old_job_with_hosts(inventory, name, host_count): hosts = [] for i in range(host_count): host = Host.objects.create(name='%s-host-%d' % (name, i), inventory=inventory) - summary = JobHostSummary.objects.create(job=job, host=host, host_name=host.name) - host.last_job_host_summary = summary - host.last_job = job - host.save() + JobHostSummary.objects.create(job=job, host=host, host_name=host.name) hosts.append(host) return job, hosts @pytest.mark.django_db -def test_cleanup_jobs_clears_host_summary_references(inventory): - """The raw UPDATE has to null Host.last_job_host_summary before the DELETE, - otherwise the foreign key blocks it.""" +def test_cleanup_jobs_deletes_host_summaries(inventory): + """The raw DELETE has to remove the summary rows before the job goes.""" job, hosts = _old_job_with_hosts(inventory, 'pg-clears', 3) assert JobHostSummary.objects.filter(job=job).count() == 3 @@ -62,7 +58,7 @@ def test_cleanup_jobs_clears_host_summary_references(inventory): assert JobHostSummary.objects.filter(job_id=job.pk).count() == 0 for host in hosts: host.refresh_from_db() - assert host.last_job_host_summary_id is None + assert host.latest_summary is None @pytest.mark.django_db @@ -81,15 +77,13 @@ def test_cleanup_jobs_leaves_recent_jobs_and_their_summaries(inventory): recent = Job.objects.create(name='pg-recent', inventory=inventory, status='successful') host = Host.objects.create(name='pg-recent-host', inventory=inventory) summary = JobHostSummary.objects.create(job=recent, host=host, host_name=host.name) - host.last_job_host_summary = summary - host.save() _command().cleanup_jobs() assert Job.objects.filter(pk=recent.pk).exists() assert JobHostSummary.objects.filter(pk=summary.pk).exists() host.refresh_from_db() - assert host.last_job_host_summary_id == summary.pk + assert host.latest_summary.pk == summary.pk @pytest.mark.django_db @@ -108,4 +102,3 @@ def test_pre_delete_job_host_summaries_spans_chunks(inventory): assert skipped == 0 assert deleted == len(jobs) assert JobHostSummary.objects.filter(job__in=jobs).count() == 0 - assert not Host.objects.filter(last_job_host_summary__isnull=False, inventory=inventory).exists() diff --git a/awx/main/tests/functional/models/test_events.py b/awx/main/tests/functional/models/test_events.py index 96f04c87..c5adb4fc 100644 --- a/awx/main/tests/functional/models/test_events.py +++ b/awx/main/tests/functional/models/test_events.py @@ -71,7 +71,7 @@ def test_host_summary_generation(self): assert s.skipped == 0 for host in Host.objects.all(): - latest_summary = JobHostSummary.latest_for_host(host.id) + latest_summary = host.latest_summary assert latest_summary is not None assert latest_summary.job_id == self.job.id assert latest_summary.host == host @@ -106,13 +106,12 @@ def test_host_summary_generation_with_limit(self): # be related to the appropriate Host) assert JobHostSummary.objects.count() == 1 for h in Host.objects.all(): - latest_summary = JobHostSummary.latest_for_host(h.id) + latest_summary = h.latest_summary if h.name == 'Host 1': assert latest_summary is not None assert latest_summary.job_id == self.job.id assert latest_summary.id == JobHostSummary.objects.first().id else: - # all other hosts in the inventory should have no summary assert latest_summary is None def test_host_metrics_insert(self): diff --git a/awx/main/tests/functional/models/test_host_summary_fields.py b/awx/main/tests/functional/models/test_host_summary_fields.py index 951c579e..a87d8644 100644 --- a/awx/main/tests/functional/models/test_host_summary_fields.py +++ b/awx/main/tests/functional/models/test_host_summary_fields.py @@ -48,10 +48,6 @@ def _setup_host_with_job(self, status='canceled'): ).save() summary = JobHostSummary.objects.filter(host=host, job=job).first() - host.last_job = job - host.last_job_host_summary = summary - host.save(update_fields=['last_job', 'last_job_host_summary']) - host.refresh_from_db() return host, job, summary diff --git a/awx/main/tests/unit/commands/test_cleanup_jobs.py b/awx/main/tests/unit/commands/test_cleanup_jobs.py index 5d6de36e..0a5fa14d 100644 --- a/awx/main/tests/unit/commands/test_cleanup_jobs.py +++ b/awx/main/tests/unit/commands/test_cleanup_jobs.py @@ -23,13 +23,8 @@ def test_single_chunk(self): _pre_delete_job_host_summaries(job_pks) - assert mock_cursor.execute.call_count == 2 - update_call = mock_cursor.execute.call_args_list[0] - assert 'UPDATE main_host SET last_job_host_summary_id = NULL' in update_call[0][0] - assert 'ANY(%s)' in update_call[0][0] - assert update_call[0][1] == [[1, 2, 3]] - - delete_call = mock_cursor.execute.call_args_list[1] + assert mock_cursor.execute.call_count == 1 + delete_call = mock_cursor.execute.call_args_list[0] assert 'DELETE FROM main_jobhostsummary' in delete_call[0][0] assert 'ANY(%s)' in delete_call[0][0] assert delete_call[0][1] == [[1, 2, 3]] @@ -43,16 +38,16 @@ def test_multiple_chunks(self): _pre_delete_job_host_summaries(job_pks) - # 2 chunks x 2 SQL statements each = 4 execute calls - assert mock_cursor.execute.call_count == 4 + # 2 chunks x 1 DELETE each = 2 execute calls + assert mock_cursor.execute.call_count == 2 # First chunk should have JHS_CHUNK_SIZE items - first_update = mock_cursor.execute.call_args_list[0] - assert len(first_update[0][1][0]) == JHS_CHUNK_SIZE + first_delete = mock_cursor.execute.call_args_list[0] + assert len(first_delete[0][1][0]) == JHS_CHUNK_SIZE # Second chunk should have the remainder - second_update = mock_cursor.execute.call_args_list[2] - assert len(second_update[0][1][0]) == 499 + second_delete = mock_cursor.execute.call_args_list[1] + assert len(second_delete[0][1][0]) == 499 def test_sql_is_fully_static(self): """SQL strings contain no interpolated values — only ANY(%s) placeholders.""" @@ -82,21 +77,6 @@ def test_logger_called_per_chunk(self): logger.debug.assert_called_once() - def test_update_runs_before_delete(self): - """Host FK must be NULLed before JHS rows are deleted.""" - job_pks = [1] - with mock.patch('awx.main.management.commands.cleanup_jobs.connection') as mock_conn: - mock_cursor = mock.MagicMock() - mock_conn.cursor.return_value.__enter__ = mock.Mock(return_value=mock_cursor) - mock_conn.cursor.return_value.__exit__ = mock.Mock(return_value=False) - - _pre_delete_job_host_summaries(job_pks) - - first_sql = mock_cursor.execute.call_args_list[0][0][0] - second_sql = mock_cursor.execute.call_args_list[1][0][0] - assert 'UPDATE' in first_sql - assert 'DELETE' in second_sql - class TestDeleteMetaPreDelete: """Verify DeleteMeta.delete_jobs() calls _pre_delete_job_host_summaries correctly."""