Merge pull request #3933 from AlanCoding/schedule_no_op

More precise handling of schedule computed fields no-ops

Reviewed-by: https://github.com/softwarefactory-project-zuul[bot]

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

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):

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

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