From 40f9d0b512920a568f18c2455a72e2352b5fdb07 Mon Sep 17 00:00:00 2001 From: AlanCoding Date: Mon, 20 May 2019 20:47:28 -0400 Subject: [PATCH] More precise handling of schedule computed fields no-ops Do not set a next_run value for disabled schedules Bail if no fields are changed Do not update related template if its fields did not change Change call pattern to schedule.update_computed_fields() in doing so, fix bug where template does not pick up schedule due to schedules next_run not being saved Handle the case (also a bug) where template was not updated when schedule was deleted --- awx/main/models/schedules.py | 53 +++++-- awx/main/models/unified_jobs.py | 12 +- awx/main/tasks.py | 7 +- .../tests/functional/models/test_schedule.py | 142 ++++++++++++++++-- 4 files changed, 185 insertions(+), 29 deletions(-) diff --git a/awx/main/models/schedules.py b/awx/main/models/schedules.py index d78b78cb41..d4a5f12914 100644 --- a/awx/main/models/schedules.py +++ b/awx/main/models/schedules.py @@ -227,15 +227,23 @@ class Schedule(PrimordialModel, LaunchTimeConfig): job_kwargs['_eager_fields'] = {'launch_type': 'scheduled', 'schedule': self} return job_kwargs - def update_computed_fields(self): - future_rs = Schedule.rrulestr(self.rrule) - next_run_actual = future_rs.after(now()) + def update_computed_fields_no_save(self): + affects_fields = ['next_run', 'dtstart', 'dtend'] + starting_values = {} + for field_name in affects_fields: + starting_values[field_name] = getattr(self, field_name) - if next_run_actual is not None: - if not datetime_exists(next_run_actual): - # skip imaginary dates, like 2:30 on DST boundaries - next_run_actual = future_rs.after(next_run_actual) - next_run_actual = next_run_actual.astimezone(pytz.utc) + future_rs = Schedule.rrulestr(self.rrule) + + if self.enabled: + next_run_actual = future_rs.after(now()) + if next_run_actual is not None: + if not datetime_exists(next_run_actual): + # skip imaginary dates, like 2:30 on DST boundaries + next_run_actual = future_rs.after(next_run_actual) + next_run_actual = next_run_actual.astimezone(pytz.utc) + else: + next_run_actual = None self.next_run = next_run_actual try: @@ -248,11 +256,38 @@ class Schedule(PrimordialModel, LaunchTimeConfig): self.dtend = future_rs[-1].astimezone(pytz.utc) except IndexError: self.dtend = None + + changed = any(getattr(self, field_name) != starting_values[field_name] for field_name in affects_fields) + return changed + + def update_computed_fields(self): + changed = self.update_computed_fields_no_save() + if not changed: + return emit_channel_notification('schedules-changed', dict(id=self.id, group_name='schedules')) + # Must save self here before calling unified_job_template computed fields + # in order for that method to be correct + # by adding modified to update fields, we avoid updating modified time + super(Schedule, self).save(update_fields=['next_run', 'dtstart', 'dtend', 'modified']) with ignore_inventory_computed_fields(): self.unified_job_template.update_computed_fields() def save(self, *args, **kwargs): - self.update_computed_fields() self.rrule = Schedule.coerce_naive_until(self.rrule) + changed = self.update_computed_fields_no_save() + if changed and 'update_fields' in kwargs: + for field_name in ['next_run', 'dtstart', 'dtend']: + if field_name not in kwargs['update_fields']: + kwargs['update_fields'].append(field_name) super(Schedule, self).save(*args, **kwargs) + if changed: + with ignore_inventory_computed_fields(): + self.unified_job_template.update_computed_fields() + + def delete(self, *args, **kwargs): + ujt = self.unified_job_template + r = super(Schedule, self).delete(*args, **kwargs) + if ujt: + with ignore_inventory_computed_fields(): + ujt.update_computed_fields() + return r diff --git a/awx/main/models/unified_jobs.py b/awx/main/models/unified_jobs.py index 40ebf155cb..22cbc09149 100644 --- a/awx/main/models/unified_jobs.py +++ b/awx/main/models/unified_jobs.py @@ -253,11 +253,13 @@ class UnifiedJobTemplate(PolymorphicModel, CommonModelNameNotUnique, Notificatio return self.last_job_run def update_computed_fields(self): - Schedule = self._meta.get_field('schedules').related_model - related_schedules = Schedule.objects.filter(enabled=True, unified_job_template=self, next_run__isnull=False).order_by('-next_run') - if related_schedules.exists(): - self.next_schedule = related_schedules[0] - self.next_job_run = related_schedules[0].next_run + related_schedules = self.schedules.filter(enabled=True, next_run__isnull=False).order_by('-next_run') + new_next_schedule = related_schedules.first() + if new_next_schedule: + if new_next_schedule.pk == self.next_schedule_id and new_next_schedule.next_run == self.next_job_run: + return # no-op, common for infrequent schedules + self.next_schedule = new_next_schedule + self.next_job_run = new_next_schedule.next_run self.save(update_fields=['next_schedule', 'next_job_run']) def save(self, *args, **kwargs): diff --git a/awx/main/tasks.py b/awx/main/tasks.py index e1f0f77964..9c4f4d251e 100644 --- a/awx/main/tasks.py +++ b/awx/main/tasks.py @@ -107,9 +107,6 @@ def dispatch_startup(): for sch in Schedule.objects.all(): try: sch.update_computed_fields() - from awx.main.signals import disable_activity_stream - with disable_activity_stream(): - sch.save() except Exception: logger.exception("Failed to rebuild schedule {}.".format(sch)) @@ -496,7 +493,7 @@ def awx_periodic_scheduler(): old_schedules = Schedule.objects.enabled().before(last_run) for schedule in old_schedules: - schedule.save() + schedule.update_computed_fields() schedules = Schedule.objects.enabled().between(last_run, run_now) invalid_license = False @@ -507,7 +504,7 @@ def awx_periodic_scheduler(): for schedule in schedules: template = schedule.unified_job_template - schedule.save() # To update next_run timestamp. + schedule.update_computed_fields() # To update next_run timestamp. if template.cache_timeout_blocked: logger.warn("Cache timeout is in the future, bypassing schedule for template %s" % str(template.id)) continue diff --git a/awx/main/tests/functional/models/test_schedule.py b/awx/main/tests/functional/models/test_schedule.py index 9d76789470..aee73e50eb 100644 --- a/awx/main/tests/functional/models/test_schedule.py +++ b/awx/main/tests/functional/models/test_schedule.py @@ -1,4 +1,5 @@ from datetime import datetime +from contextlib import contextmanager from django.utils.timezone import now from django.db.utils import IntegrityError @@ -6,7 +7,7 @@ from unittest import mock import pytest import pytz -from awx.main.models import JobTemplate, Schedule +from awx.main.models import JobTemplate, Schedule, ActivityStream from crum import impersonate @@ -22,19 +23,140 @@ def job_template(inventory, project): @pytest.mark.django_db -def test_computed_fields_modified_by_retained(job_template, admin_user): - with impersonate(admin_user): +class TestComputedFields: + + # expired in 2015, so next_run should not be populated + dead_rrule = "DTSTART;TZID=UTC:20140520T190000 RRULE:FREQ=YEARLY;INTERVAL=1;BYMONTH=1;BYMONTHDAY=1;UNTIL=20150530T000000Z" + continuing_rrule = "DTSTART;TZID=UTC:20140520T190000 RRULE:FREQ=YEARLY;INTERVAL=1;BYMONTH=1;BYMONTHDAY=1" + + @property + def distant_rrule(self): + # this rule should produce a next_run, but it should not overlap with test run time + this_year = now().year + return "DTSTART;TZID=UTC:{}0520T190000 RRULE:FREQ=YEARLY;INTERVAL=1;BYMONTH=1;BYMONTHDAY=1;UNTIL={}0530T000000Z".format( + this_year + 1, this_year + 2 + ) + + @contextmanager + def assert_no_unwanted_stuff(self, schedule, act_stream=True, sch_assert=True): + """These changes are not wanted for any computed fields update + of schedules, so we make the assertions for all of the tests here + """ + original_sch_modified = schedule.modified + original_sch_modified_by = schedule.modified_by + original_ujt_modified = schedule.unified_job_template.modified + original_ujt_modified_by = schedule.unified_job_template.modified_by + original_AS_entries = ActivityStream.objects.count() + yield None + if sch_assert: + schedule.refresh_from_db() + assert schedule.modified == original_sch_modified + assert schedule.modified_by == original_sch_modified_by + # a related schedule update should not change JT modified time + schedule.unified_job_template.refresh_from_db() + assert schedule.unified_job_template.modified == original_ujt_modified + assert schedule.unified_job_template.modified_by == original_ujt_modified_by + if act_stream: + assert ActivityStream.objects.count() == original_AS_entries, ( + ActivityStream.objects.order_by('-timestamp').first().changes + ) + + def test_computed_fields_modified_by_retained(self, job_template, admin_user): + with impersonate(admin_user): + s = Schedule.objects.create( + name='Some Schedule', + rrule='DTSTART:20300112T210000Z RRULE:FREQ=DAILY;INTERVAL=1', + unified_job_template=job_template + ) + assert s.created_by == admin_user + with self.assert_no_unwanted_stuff(s): + s.update_computed_fields() # modification done by system here + s.save() + assert s.modified_by == admin_user + + def test_computed_fields_no_op(self, job_template): s = Schedule.objects.create( name='Some Schedule', - rrule='DTSTART:20300112T210000Z RRULE:FREQ=DAILY;INTERVAL=1', + rrule=self.dead_rrule, + unified_job_template=job_template, + enabled=True + ) + with self.assert_no_unwanted_stuff(s): + assert s.next_run is None + assert s.dtend is not None + prior_dtend = s.dtend + s.update_computed_fields() + assert s.next_run is None + assert s.dtend == prior_dtend + + def test_computed_fields_time_change(self, job_template): + s = Schedule.objects.create( + name='Some Schedule', + rrule=self.continuing_rrule, + unified_job_template=job_template, + enabled=True + ) + with self.assert_no_unwanted_stuff(s): + # force update of next_run, as if schedule re-calculation had not happened + # since this time + old_next_run = datetime(2009, 3, 13, tzinfo=pytz.utc) + Schedule.objects.filter(pk=s.pk).update(next_run=old_next_run) + s.next_run = old_next_run + prior_modified = s.modified + s.update_computed_fields() + assert s.next_run != old_next_run + assert s.modified == prior_modified + + def test_computed_fields_turning_on(self, job_template): + s = Schedule.objects.create( + name='Some Schedule', + rrule=self.distant_rrule, + unified_job_template=job_template, + enabled=False + ) + # we expect 1 activity stream entry for changing enabled field + with self.assert_no_unwanted_stuff(s, act_stream=False): + assert s.next_run is None + assert job_template.next_schedule is None + s.enabled = True + s.save(update_fields=['enabled']) + assert s.next_run is not None + assert job_template.next_schedule == s + + def test_computed_fields_turning_on_via_rrule(self, job_template): + s = Schedule.objects.create( + name='Some Schedule', + rrule=self.dead_rrule, unified_job_template=job_template ) - s.refresh_from_db() - assert s.created_by == admin_user - assert s.modified_by == admin_user - s.update_computed_fields() - s.save() - assert s.modified_by == admin_user + with self.assert_no_unwanted_stuff(s, act_stream=False): + assert s.next_run is None + assert job_template.next_schedule is None + s.rrule = self.distant_rrule + s.update_computed_fields() + assert s.next_run is not None + assert job_template.next_schedule == s + + def test_computed_fields_turning_off_by_deleting(self, job_template): + s1 = Schedule.objects.create( + name='first schedule', + rrule=self.distant_rrule, + unified_job_template=job_template + ) + s2 = Schedule.objects.create( + name='second schedule', + rrule=self.distant_rrule, + unified_job_template=job_template + ) + assert job_template.next_schedule in [s1, s2] + if job_template.next_schedule == s1: + expected_schedule = s2 + else: + expected_schedule = s1 + with self.assert_no_unwanted_stuff(expected_schedule, act_stream=False, sch_assert=False): + job_template.next_schedule.delete() + job_template.refresh_from_db() + assert job_template.next_schedule == expected_schedule @pytest.mark.django_db