Improve upon (currently dangerous) .from_schedule code #269

hartwork · 2019-06-15T17:59:44Z

For your consideration 😄

SolarSchedule.Meta.unique_together guarantes that we run into MultipleObjectsReturned with SolarSchedule. Current value: ('event', 'latitude', 'longitude')

…nstraints

liquidpele · 2019-06-16T02:20:56Z

django_celery_beat/models.py

-        except MultipleObjectsReturned:
-            cls.objects.filter(**spec).delete()
-            return cls(**spec)
+        return cls.objects.get_or_create(**spec)[0]


This can still raise MultipleObjectsReturned, why is not handling that better?

There is a uniqueness constraint covering all fields that go into spec so the database should not be able to contain more than a single match at any time.

I see your point, but even the previous logic seems terrible tbh.

As an example, lets say I have 2 different schedules for 2 different tasks, one running every sunrise and one running every sunset (just using solarschedule as the example but this applies to any of them). Now, lets say that my boss comes over and says all tasks running at sunset should be changed to sunrise for only the next week for whatever business reason. Instead of modifying a single schedule, I now have to go through and modify potentially hundreds of different periodic tasks to change their schedule... and then I have to remember which ones I changed so I can change them back later.

It seems like we should be allowing duplicate schedules. I'm really curious why this limitation ever existed in the first place?

I'm digging into where all from_schedule is used to see if we can sort this out some, feel free to do the same.

Now, lets say that my boss comes over and says all tasks running at sunset should be changed to sunrise for only the next week for whatever business reason. Instead of modifying a single schedule, I now have to go through and modify potentially hundreds of different periodic tasks to change their schedule... and then I have to remember which ones I changed so I can change them back later.

I was thinking of the same scenario earlier. I think this feature also has downsides: Once you have multiple schedules with the same values, you'll have a hard time telling them apart when creating new jobs.

Personally, I see the value in a unique constraint but also the pain with not supporting to change a schedule into something that already exists.

To fully support the scenario that you have in mind, it would take another layer of indirection, I think.

I'm digging into where all from_schedule is used to see if we can sort this out some, feel free to do the same.

There is a single place outside the test suite:

django-celery-beat/django_celery_beat/schedulers.py

Line 171 in db68a49

model_schedule = model_type.from_schedule(schedule)

Yea, I'm reading through the celery scheduler code those inherit from to work out the logic on what scenarios this call stack actually happens.

So, based on what I've looked through, the whole reason for all this is that when you use the django settings to set up the schedules, it deletes/inserts those into the DB. However, instead of having a flag for "inserted from django conf" or whatever, they compare it to what's already in the DB and treat the schedule items as if they are unique constraints. Still kind of playing around with this, but if that's the case then the deletes may be necessary for now, although there are probably several scenarios where things can get borked. Still playing around with this though.

liquidpele · 2019-06-16T02:22:50Z

django_celery_beat/models.py

@@ -138,6 +132,7 @@ class Meta:
        verbose_name = _('interval')
        verbose_name_plural = _('intervals')
        ordering = ['period', 'every']
+        unique_together = ('every', 'period')


What if I want two different jobs to run on the same interval? This would disallow that, that seems like an issue.

I think there is a mixup between jobs and schedules here: You assign the same "every 4 days" schedule to two distinct jobs and they both run at the time. That works before and after this pull request.

The current cls.objects.filter(**spec).delete() is effectively a unique constraint, except that it might cascade destroy data and does not prevent creation in the first place.

I think what the code needs is one of:

a) A uniqueness constraint for all schedules (as done in the pull request)

b) A uniqueness constraint for none of the schedules and no dangerous cls.objects.filter(**spec).delete() either

What do you think?

I think there is a mixup between jobs and schedules here: You assign the same "every 4 days" schedule to two distinct jobs and they both run at the time. That works before and after this pull request.

The current cls.objects.filter(**spec).delete() is effectively a unique constraint, except that it might cascade destroy data and does not prevent creation in the first place.

I think what the code needs is one of:

* a) A uniqueness constraint for _all_ schedules (as done in the pull request) * b) A uniqueness constraint for _none_ of the schedules and no dangerous `cls.objects.filter(**spec).delete()` either

What do you think?

just curious if we add UniqueContraint Check here would that be any useful?

Hi @auvipy I would need get back into the whole topic since it's been so long ago by now. I can do that work but I would need some "guarantee" that we'll actually get somewhere with this in the next few weeks to not invest in a dead end. (In case you're a friend of voice meetings for stuff like that, we can definitely do a Jitsi or Google Meet on the topic if you like, but no pressure, just one more option.)

Ok, I will release a new version tomorrow and do some more analysis to get back to you to reach a consensus

liquidpele · 2019-06-16T02:23:06Z

django_celery_beat/models.py

+            'every': every,
+            'period': period,
+        }
+        return cls.objects.get_or_create(**spec)[0]


Same question about MultipleObjectsReturned

liquidpele · 2019-06-16T02:23:23Z

django_celery_beat/models.py

@@ -174,6 +167,7 @@ class ClockedSchedule(models.Model):
    clocked_time = models.DateTimeField(
        verbose_name=_('Clock Time'),
        help_text=_('Run the task at clocked time'),
+        unique=True,


Why limit it like this? Having two jobs run at the same time is completely valid.

Please see #269 (comment)

liquidpele · 2019-06-16T02:23:37Z

django_celery_beat/models.py

-        except MultipleObjectsReturned:
-            cls.objects.filter(**spec).delete()
-            return cls(**spec)
+        return cls.objects.get_or_create(**spec)[0]


Same question about MultipleObjectsReturned

liquidpele · 2019-06-16T02:23:49Z

django_celery_beat/models.py

@@ -280,6 +268,8 @@ class Meta:
        verbose_name_plural = _('crontabs')
        ordering = ['month_of_year', 'day_of_month',
                    'day_of_week', 'hour', 'minute', 'timezone']
+        unique_together = ('minute', 'hour', 'day_of_week', 'day_of_month',
+                           'month_of_year', 'timezone')


Same issue about limiting by the schedule.

liquidpele · 2019-06-16T02:23:59Z

django_celery_beat/models.py

-        except MultipleObjectsReturned:
-            cls.objects.filter(**spec).delete()
-            return cls(**spec)
+        return cls.objects.get_or_create(**spec)[0]


Same question about MultipleObjectsReturned

hartwork · 2019-06-22T20:11:10Z

Any ideas how to proceed with #269 (comment) ?

hartwork · 2020-01-05T02:23:17Z

Any ideas how to proceed with #269 (comment) ?

Ping 🙏

django_celery_beat/models.py

izimobil · 2021-03-02T13:51:20Z

FWIW, here is my take on this:
(see also: #389 that fixes the very dangerous from_schedule implementation)

The idea of adding unique constraints is good, but there should be a warning in the changelog because it's not fully backwards compatible, existing code may try to create duplicate schedules (which currently works) and get a IntegrityError exception with the constraints added.

Furthermore, because existing databases can already contain duplicate schedules, the from_schedule methods should either be:

untouched if Do not blindly delete duplicate schedules #389 is merged
or modified to avoid deleting existing duplicates schedules if Do not blindly delete duplicate schedules #389 is discarded

hartwork · 2021-03-02T18:04:45Z

Closing as superseded by #389.

CC #322 as related.

auvipy · 2021-03-03T14:16:22Z

I might tackle this in the future if needed.

hartwork · 2021-03-09T00:47:33Z

I might tackle this in the future if needed.

@auvipy I see two unresolved topics that this pull request documents:

introducing uniqueness constraints and
potential to use get_or_create to simplify code and increase atomicity.

There is no way forward with the uniqueness constraint that doesn't…

require user intervention on setups with existing duplicates and
have downsides as well

…as explained by @liquidpele at https://github.com/celery/django-celery-beat/pull/269/files#r294091489 .

For get_or_create, a uniqueness constraint on the involved keys would be "needed" to not keep creating duplicate entries. So I see at least a soft dependency on the uniqueness constraint topic here.

With where we are today on master after #389 has been merged, this pull request titled "Improve upon (currently dangerous) .from_schedule code" is not the best way to keep those two potential todos in my opinion, plus I would like to make myself optional in this equation (which is in some conflict with me being the author of the pull request) and keep my own list of open pull request at https://github.com/pulls organized, so I'll close my pull request here again and ask you to create a new dedicated issue to address the topic of adding uniqueness constraints if that's still of interest. Thanks for your understanding 🙏

Best, Sebastian

models.py: Drop dead MultipleObjectsReturned code

c4ac2e2

SolarSchedule.Meta.unique_together guarantes that we run into MultipleObjectsReturned with SolarSchedule. Current value: ('event', 'latitude', 'longitude')

hartwork changed the title ~~Improve upon (currently dangourous) .from_schedule code~~ Improve upon (currently dangerous) .from_schedule code Jun 15, 2019

hartwork added 2 commits June 15, 2019 21:36

models.py: Turn risky MultipleObjectsReturned handling into unique co…

dac67d1

…nstraints

models.py: Leverage Django's get_or_create to simplify code

37d7605

liquidpele suggested changes Jun 16, 2019

View reviewed changes

liquidpele added the Changes Needed label Jun 16, 2019

auvipy requested changes Jan 5, 2020

View reviewed changes

django_celery_beat/models.py Show resolved Hide resolved

hartwork closed this Mar 2, 2021

auvipy mentioned this pull request Mar 3, 2021

The task celery.backend_cleanup will delete my tasks added before if have the same crontab time #322

Closed

auvipy reopened this Mar 3, 2021

auvipy self-assigned this Mar 3, 2021

hartwork closed this Mar 9, 2021

hartwork deleted the improve-from-schedule branch March 9, 2021 00:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve upon (currently dangerous) .from_schedule code #269

Improve upon (currently dangerous) .from_schedule code #269

hartwork commented Jun 15, 2019

liquidpele Jun 16, 2019

hartwork Jun 16, 2019

liquidpele Jun 16, 2019

liquidpele Jun 16, 2019

hartwork Jun 16, 2019

hartwork Jun 16, 2019

liquidpele Jun 16, 2019

liquidpele Jun 23, 2019

liquidpele Jun 16, 2019

hartwork Jun 16, 2019

auvipy Jan 18, 2021

hartwork Jan 18, 2021 •

edited

Loading

auvipy Jan 18, 2021

liquidpele Jun 16, 2019

liquidpele Jun 16, 2019

hartwork Jun 16, 2019

liquidpele Jun 16, 2019

liquidpele Jun 16, 2019

liquidpele Jun 16, 2019

hartwork commented Jun 22, 2019

hartwork commented Jan 5, 2020 •

edited

Loading

izimobil commented Mar 2, 2021 •

edited

Loading

hartwork commented Mar 2, 2021

auvipy commented Mar 3, 2021

hartwork commented Mar 9, 2021 •

edited

Loading

Improve upon (currently dangerous) .from_schedule code #269

Improve upon (currently dangerous) .from_schedule code #269

Conversation

hartwork commented Jun 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hartwork Jan 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hartwork commented Jun 22, 2019

hartwork commented Jan 5, 2020 • edited Loading

izimobil commented Mar 2, 2021 • edited Loading

hartwork commented Mar 2, 2021

auvipy commented Mar 3, 2021

hartwork commented Mar 9, 2021 • edited Loading

hartwork Jan 18, 2021 •

edited

Loading

hartwork commented Jan 5, 2020 •

edited

Loading

izimobil commented Mar 2, 2021 •

edited

Loading

hartwork commented Mar 9, 2021 •

edited

Loading