[PATCH v3] Avoid timezone confusion

Veronika Kabatova vkabatov at redhat.com
Mon Feb 26 21:54:46 AEDT 2018


----- Original Message -----
> From: "Daniel Axtens" <dja at axtens.net>
> To: vkabatov at redhat.com, patchwork at lists.ozlabs.org
> Sent: Saturday, February 24, 2018 2:58:56 AM
> Subject: Re: [PATCH v3] Avoid timezone confusion
> 
> Hi Veronika,
> 
> I am testing this now and I think I'm happy with it.
> 
> However, I set up a container in UTC+11, and while watching the events
> api endpoint:
>  - I added some patches without this patch
>  - I applied this patch and migrated
>  - I added some more patches
>  - I noticed that the new events were no longer added to the front of
>  the events list; they were now 11 hours back.
> 
> This is, on reflection, a straight-forward consequence of moving from
> UTC+11 to UTC without attempting to migrate old data.
> 
> I still think migration is an incredibly difficult and error-prone waste
> of time, so I still don't want to go down that road. What I do want is a
> piece of documentation in the release notes to warn people that if you
> are in UTC+n, your events feed will be out of order for n hours.
> 

Yes, the "delayed" events are expected. Additional documentation about
it seems fair.

> I don't know of any users of the event feed at the moment, so I'm not
> worried about breaking anything: I just want a couple of sentences so
> that any sysadmins/users who are especially observant don't freak out
> that they're suddenly losing events.
> 

We tried it but the events API was very slow and unreliable so we
switched back to polling series. I believe Stephen worked on fixing
up the API so hopefully there will be a working solution merged soon :)

> You can do a respin of this, or just send a new patch on top of this and
> I will apply both at the same time.
> 

I'll send an additional patch with releasnote changes and cc you so
you can find it easily.

> Regards,
> Daniel
> 
> vkabatov at redhat.com writes:
> 
> > From: Veronika Kabatova <vkabatov at redhat.com>
> >
> > Patchwork saves patches, comments etc with UTC timezone and reports
> > this time when opening the patch details. However, internally generated
> > processes such as events are reported with the instance's local time.
> > There's nothing wrong with that and making PW timezone-aware would add
> > useless complexity, but in a world-wide collaboration a lot of confusion
> > may arise as the timezone is not reported at all. Instance's local time
> > might be very different from the local time of CI integrating with PW,
> > which is different from the local time of person dealing with it etc.
> >
> > Use UTC everywhere by default instead of UTC for sumbissions and local
> > timezone for internally generated events (which is not reported).
> >
> > Signed-off-by: Veronika Kabatova <vkabatov at redhat.com>
> > ---
> > Changes: Implement Daniel's idea about using datetime.datetime.utcnow
> > ---
> >  docs/api/rest.rst                                  |  3 +-
> >  patchwork/migrations/0024_timezone_unify.py        | 46
> >  ++++++++++++++++++++++
> >  patchwork/models.py                                | 12 +++---
> >  patchwork/notifications.py                         |  4 +-
> >  patchwork/signals.py                               |  2 +-
> >  patchwork/templates/patchwork/submission.html      |  4 +-
> >  patchwork/tests/test_checks.py                     | 10 ++---
> >  patchwork/tests/test_expiry.py                     |  8 ++--
> >  patchwork/tests/test_notifications.py              |  2 +-
> >  patchwork/tests/utils.py                           |  6 +--
> >  .../notes/unify-timezones-0f7022f0c2a371be.yaml    |  5 +++
> >  11 files changed, 77 insertions(+), 25 deletions(-)
> >  create mode 100644 patchwork/migrations/0024_timezone_unify.py
> >  create mode 100644
> >  releasenotes/notes/unify-timezones-0f7022f0c2a371be.yaml
> >
> > diff --git a/docs/api/rest.rst b/docs/api/rest.rst
> > index 3d7292e..d526b27 100644
> > --- a/docs/api/rest.rst
> > +++ b/docs/api/rest.rst
> > @@ -107,7 +107,8 @@ Schema
> >  ------
> >  
> >  Responses are returned as JSON. Blank fields are returned as ``null``,
> >  rather
> > -than being omitted. Timestamps use the ISO 8601 format::
> > +than being omitted. Timestamps use the ISO 8601 format, times are by
> > default
> > +in UTC::
> >  
> >      YYYY-MM-DDTHH:MM:SSZ
> >  
> > diff --git a/patchwork/migrations/0024_timezone_unify.py
> > b/patchwork/migrations/0024_timezone_unify.py
> > new file mode 100644
> > index 0000000..99f0642
> > --- /dev/null
> > +++ b/patchwork/migrations/0024_timezone_unify.py
> > @@ -0,0 +1,46 @@
> > +# -*- coding: utf-8 -*-
> > +# Generated by Django 1.10.8 on 2018-02-22 23:11
> > +from __future__ import unicode_literals
> > +
> > +import datetime
> > +from django.db import migrations, models
> > +
> > +
> > +class Migration(migrations.Migration):
> > +
> > +    dependencies = [
> > +        ('patchwork', '0023_submissiontag'),
> > +    ]
> > +
> > +    operations = [
> > +        migrations.AlterField(
> > +            model_name='check',
> > +            name='date',
> > +            field=models.DateTimeField(default=datetime.datetime.utcnow),
> > +        ),
> > +        migrations.AlterField(
> > +            model_name='comment',
> > +            name='date',
> > +            field=models.DateTimeField(default=datetime.datetime.utcnow),
> > +        ),
> > +        migrations.AlterField(
> > +            model_name='emailconfirmation',
> > +            name='date',
> > +            field=models.DateTimeField(default=datetime.datetime.utcnow),
> > +        ),
> > +        migrations.AlterField(
> > +            model_name='event',
> > +            name='date',
> > +            field=models.DateTimeField(default=datetime.datetime.utcnow,
> > help_text=b'The time this event was created.'),
> > +        ),
> > +        migrations.AlterField(
> > +            model_name='patchchangenotification',
> > +            name='last_modified',
> > +            field=models.DateTimeField(default=datetime.datetime.utcnow),
> > +        ),
> > +        migrations.AlterField(
> > +            model_name='submission',
> > +            name='date',
> > +            field=models.DateTimeField(default=datetime.datetime.utcnow),
> > +        ),
> > +    ]
> > diff --git a/patchwork/models.py b/patchwork/models.py
> > index a8bb015..c5a2059 100644
> > --- a/patchwork/models.py
> > +++ b/patchwork/models.py
> > @@ -308,7 +308,7 @@ class EmailMixin(models.Model):
> >      # email metadata
> >  
> >      msgid = models.CharField(max_length=255)
> > -    date = models.DateTimeField(default=datetime.datetime.now)
> > +    date = models.DateTimeField(default=datetime.datetime.utcnow)
> >      headers = models.TextField(blank=True)
> >  
> >      # content
> > @@ -828,7 +828,7 @@ class Check(models.Model):
> >  
> >      patch = models.ForeignKey(Patch, on_delete=models.CASCADE)
> >      user = models.ForeignKey(User, on_delete=models.CASCADE)
> > -    date = models.DateTimeField(default=datetime.datetime.now)
> > +    date = models.DateTimeField(default=datetime.datetime.utcnow)
> >  
> >      state = models.SmallIntegerField(
> >          choices=STATE_CHOICES, default=STATE_PENDING,
> > @@ -900,7 +900,7 @@ class Event(models.Model):
> >          db_index=True,
> >          help_text='The category of the event.')
> >      date = models.DateTimeField(
> > -        default=datetime.datetime.now,
> > +        default=datetime.datetime.utcnow,
> >          help_text='The time this event was created.')
> >  
> >      # event object
> > @@ -965,7 +965,7 @@ class EmailConfirmation(models.Model):
> >      email = models.CharField(max_length=200)
> >      user = models.ForeignKey(User, null=True, on_delete=models.CASCADE)
> >      key = HashField()
> > -    date = models.DateTimeField(default=datetime.datetime.now)
> > +    date = models.DateTimeField(default=datetime.datetime.utcnow)
> >      active = models.BooleanField(default=True)
> >  
> >      def deactivate(self):
> > @@ -973,7 +973,7 @@ class EmailConfirmation(models.Model):
> >          self.save()
> >  
> >      def is_valid(self):
> > -        return self.date + self.validity > datetime.datetime.now()
> > +        return self.date + self.validity > datetime.datetime.utcnow()
> >  
> >      def save(self, *args, **kwargs):
> >          limit = 1 << 32
> > @@ -999,5 +999,5 @@ class EmailOptout(models.Model):
> >  class PatchChangeNotification(models.Model):
> >      patch = models.OneToOneField(Patch, primary_key=True,
> >                                   on_delete=models.CASCADE)
> > -    last_modified = models.DateTimeField(default=datetime.datetime.now)
> > +    last_modified = models.DateTimeField(default=datetime.datetime.utcnow)
> >      orig_state = models.ForeignKey(State, on_delete=models.CASCADE)
> > diff --git a/patchwork/notifications.py b/patchwork/notifications.py
> > index 88e9662..a5f6423 100644
> > --- a/patchwork/notifications.py
> > +++ b/patchwork/notifications.py
> > @@ -35,7 +35,7 @@ from patchwork.models import PatchChangeNotification
> >  
> >  
> >  def send_notifications():
> > -    date_limit = datetime.datetime.now() - datetime.timedelta(
> > +    date_limit = datetime.datetime.utcnow() - datetime.timedelta(
> >          minutes=settings.NOTIFICATION_DELAY_MINUTES)
> >  
> >      # We delay sending notifications to a user if they have other
> > @@ -104,7 +104,7 @@ def expire_notifications():
> >      Users whose registration confirmation has expired are removed.
> >      """
> >      # expire any invalid confirmations
> > -    q = (Q(date__lt=datetime.datetime.now() - EmailConfirmation.validity)
> > |
> > +    q = (Q(date__lt=datetime.datetime.utcnow() -
> > EmailConfirmation.validity) |
> >           Q(active=False))
> >      EmailConfirmation.objects.filter(q).delete()
> >  
> > diff --git a/patchwork/signals.py b/patchwork/signals.py
> > index e5e7370..f7b4f54 100644
> > --- a/patchwork/signals.py
> > +++ b/patchwork/signals.py
> > @@ -65,7 +65,7 @@ def patch_change_callback(sender, instance, raw,
> > **kwargs):
> >          notification.delete()
> >          return
> >  
> > -    notification.last_modified = dt.now()
> > +    notification.last_modified = dt.utcnow()
> >      notification.save()
> >  
> >  
> > diff --git a/patchwork/templates/patchwork/submission.html
> > b/patchwork/templates/patchwork/submission.html
> > index 6ed20c3..e817713 100644
> > --- a/patchwork/templates/patchwork/submission.html
> > +++ b/patchwork/templates/patchwork/submission.html
> > @@ -255,7 +255,7 @@ function toggle_div(link_id, headers_id)
> >  <div class="comment">
> >  <div class="meta">
> >   <span>{{ submission.submitter|personify:project }}</span>
> > - <span class="pull-right">{{ submission.date }}</span>
> > + <span class="pull-right">{{ submission.date }} UTC</span>
> >  </div>
> >  <pre class="content">
> >  {{ submission|commentsyntax }}
> > @@ -271,7 +271,7 @@ function toggle_div(link_id, headers_id)
> >  <div class="comment">
> >  <div class="meta">
> >   <span>{{ item.submitter|personify:project }}</span>
> > - <span class="pull-right">{{ item.date }} | <a href="{% url
> > 'comment-redirect' comment_id=item.id %}"
> > + <span class="pull-right">{{ item.date }} UTC | <a href="{% url
> > 'comment-redirect' comment_id=item.id %}"
> >     >#{{ forloop.counter }}</a></span>
> >  </div>
> >  <pre class="content">
> > diff --git a/patchwork/tests/test_checks.py
> > b/patchwork/tests/test_checks.py
> > index c0487f3..797390c 100644
> > --- a/patchwork/tests/test_checks.py
> > +++ b/patchwork/tests/test_checks.py
> > @@ -86,12 +86,12 @@ class PatchChecksTest(TransactionTestCase):
> >          self.assertChecksEqual(self.patch, [check_a, check_b])
> >  
> >      def test_checks__duplicate_checks(self):
> > -        self._create_check(date=(dt.now() - timedelta(days=1)))
> > +        self._create_check(date=(dt.utcnow() - timedelta(days=1)))
> >          check = self._create_check()
> >          # this isn't a realistic scenario (dates shouldn't be set by user
> >          so
> >          # they will always increment), but it's useful to verify the
> >          removal
> >          # of older duplicates by the function
> > -        self._create_check(date=(dt.now() - timedelta(days=2)))
> > +        self._create_check(date=(dt.utcnow() - timedelta(days=2)))
> >          self.assertChecksEqual(self.patch, [check])
> >  
> >      def test_checks__nultiple_users(self):
> > @@ -107,7 +107,7 @@ class PatchChecksTest(TransactionTestCase):
> >          self.assertCheckCountEqual(self.patch, 1, {Check.STATE_SUCCESS:
> >          1})
> >  
> >      def test_check_count__multiple_checks(self):
> > -        self._create_check(date=(dt.now() - timedelta(days=1)))
> > +        self._create_check(date=(dt.utcnow() - timedelta(days=1)))
> >          self._create_check(context='new/test1')
> >          self.assertCheckCountEqual(self.patch, 2, {Check.STATE_SUCCESS:
> >          2})
> >  
> > @@ -117,14 +117,14 @@ class PatchChecksTest(TransactionTestCase):
> >          self.assertCheckCountEqual(self.patch, 2, {Check.STATE_SUCCESS:
> >          2})
> >  
> >      def test_check_count__duplicate_check_same_state(self):
> > -        self._create_check(date=(dt.now() - timedelta(days=1)))
> > +        self._create_check(date=(dt.utcnow() - timedelta(days=1)))
> >          self.assertCheckCountEqual(self.patch, 1, {Check.STATE_SUCCESS:
> >          1})
> >  
> >          self._create_check()
> >          self.assertCheckCountEqual(self.patch, 2, {Check.STATE_SUCCESS:
> >          1})
> >  
> >      def test_check_count__duplicate_check_new_state(self):
> > -        self._create_check(date=(dt.now() - timedelta(days=1)))
> > +        self._create_check(date=(dt.utcnow() - timedelta(days=1)))
> >          self.assertCheckCountEqual(self.patch, 1, {Check.STATE_SUCCESS:
> >          1})
> >  
> >          self._create_check(state=Check.STATE_FAIL)
> > diff --git a/patchwork/tests/test_expiry.py
> > b/patchwork/tests/test_expiry.py
> > index 054d156..ce308bc 100644
> > --- a/patchwork/tests/test_expiry.py
> > +++ b/patchwork/tests/test_expiry.py
> > @@ -46,7 +46,7 @@ class TestRegistrationExpiry(TestCase):
> >          return (user, conf)
> >  
> >      def test_old_registration_expiry(self):
> > -        date = ((datetime.datetime.now() - EmailConfirmation.validity) -
> > +        date = ((datetime.datetime.utcnow() - EmailConfirmation.validity)
> > -
> >                  datetime.timedelta(hours=1))
> >          user, conf = self.register(date)
> >  
> > @@ -57,7 +57,7 @@ class TestRegistrationExpiry(TestCase):
> >              EmailConfirmation.objects.filter(pk=conf.pk).exists())
> >  
> >      def test_recent_registration_expiry(self):
> > -        date = ((datetime.datetime.now() - EmailConfirmation.validity) +
> > +        date = ((datetime.datetime.utcnow() - EmailConfirmation.validity)
> > +
> >                  datetime.timedelta(hours=1))
> >          user, conf = self.register(date)
> >  
> > @@ -68,7 +68,7 @@ class TestRegistrationExpiry(TestCase):
> >              EmailConfirmation.objects.filter(pk=conf.pk).exists())
> >  
> >      def test_inactive_registration_expiry(self):
> > -        user, conf = self.register(datetime.datetime.now())
> > +        user, conf = self.register(datetime.datetime.utcnow())
> >  
> >          # confirm registration
> >          conf.user.is_active = True
> > @@ -87,7 +87,7 @@ class TestRegistrationExpiry(TestCase):
> >          submitter = patch.submitter
> >  
> >          # ... then starts registration...
> > -        date = ((datetime.datetime.now() - EmailConfirmation.validity) -
> > +        date = ((datetime.datetime.utcnow() - EmailConfirmation.validity)
> > -
> >                  datetime.timedelta(hours=1))
> >          user = create_user(link_person=False, email=submitter.email)
> >          user.is_active = False
> > diff --git a/patchwork/tests/test_notifications.py
> > b/patchwork/tests/test_notifications.py
> > index 6cd3200..6d902f8 100644
> > --- a/patchwork/tests/test_notifications.py
> > +++ b/patchwork/tests/test_notifications.py
> > @@ -120,7 +120,7 @@ class PatchNotificationEmailTest(TestCase):
> >          self.project = create_project(send_notifications=True)
> >  
> >      def _expire_notifications(self, **kwargs):
> > -        timestamp = datetime.datetime.now() - \
> > +        timestamp = datetime.datetime.utcnow() - \
> >              datetime.timedelta(minutes=settings.NOTIFICATION_DELAY_MINUTES
> >              + 1)
> >  
> >          qs = PatchChangeNotification.objects.all()
> > diff --git a/patchwork/tests/utils.py b/patchwork/tests/utils.py
> > index 004c2ca..12b6d9e 100644
> > --- a/patchwork/tests/utils.py
> > +++ b/patchwork/tests/utils.py
> > @@ -214,7 +214,7 @@ def create_check(**kwargs):
> >      values = {
> >          'patch': create_patch() if 'patch' not in kwargs else None,
> >          'user': create_user() if 'user' not in kwargs else None,
> > -        'date': dt.now(),
> > +        'date': dt.utcnow(),
> >          'state': Check.STATE_SUCCESS,
> >          'target_url': 'http://example.com/',
> >          'description': '',
> > @@ -229,7 +229,7 @@ def create_series(**kwargs):
> >      """Create 'Series' object."""
> >      values = {
> >          'project': create_project() if 'project' not in kwargs else None,
> > -        'date': dt.now(),
> > +        'date': dt.utcnow(),
> >          'submitter': create_person() if 'submitter' not in kwargs else
> >          None,
> >          'total': 1,
> >      }
> > @@ -275,7 +275,7 @@ def _create_submissions(create_func, count=1,
> > **kwargs):
> >          'submitter': create_person() if 'submitter' not in kwargs else
> >          None,
> >      }
> >      values.update(kwargs)
> > -    date = dt.now()
> > +    date = dt.utcnow()
> >  
> >      objects = []
> >      for i in range(0, count):
> > diff --git a/releasenotes/notes/unify-timezones-0f7022f0c2a371be.yaml
> > b/releasenotes/notes/unify-timezones-0f7022f0c2a371be.yaml
> > new file mode 100644
> > index 0000000..2513650
> > --- /dev/null
> > +++ b/releasenotes/notes/unify-timezones-0f7022f0c2a371be.yaml
> > @@ -0,0 +1,5 @@
> > +---
> > +other:
> > +  - |
> > +    Unify timezones used -- use UTC for both email submissions and
> > internal
> > +    events.
> > --
> > 2.13.6
> >
> > _______________________________________________
> > Patchwork mailing list
> > Patchwork at lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/patchwork
> 


More information about the Patchwork mailing list