Merge lp:~rackspace-titan/nova/lp742204 into lp:~hudson-openstack/nova/trunk

[Brian Lamar]

Glance used to return None when a date field wasn't set, now it returns ''.
Glance used to return dates in format "%Y-%m-%dT%H:%M:%S", now it returns "%Y-%m-%dT%H:%M:%S.%f".

Fixed to allow for all cases.

[termie (termie)]

Probably something of a meta question, but why aren't we using timestamps (123567890) instead of timestamps formatted as strings (2006-10-20 13:42:21)? Seems like it would negate the need for some of this.

[Brian Lamar (blamar)]

ISO8601 allows for timezone and tons of other stuff that a standard UNIX integer timestamp can't account for. Not that we use any of that currently... :)

[Jay Pipes (jaypipes)]

31 +signatures: %(ISO_FORMATS)s""") % (locals()))

no need for the parens around locals() there. :)

Also, the whole thing in there about NOW_GLANCE_OLD_FORMAT and NOW_GLANCE_FORMAT seems strange. AFAIK, Glance hasn't changed the format of its datetime field values returned in the API...was this just a case of a Faked/Stubbed client returning incorrect data in the first place?

-jay

review: Needs Fixing

[Brian Lamar (blamar)]

> no need for the parens around locals() there. :)

So I understand that using a tuple isn't required here, but I like it for a couple, let's say, personal reasons so I have a question:

This doesn't violate PEP8 or HACKING, so without getting in to my reasons for liking this format, did I get the needs fixing because of this or because of the NOW_GLANCE_OLD_FORMAT vs. NOW_GLANCE_FORMAT?

I'm looking at that now by the way, just a second and I'll get back to you on that second part of your comment.

[Mark Washenberger (markwash)]

Jay,

When I first switched to converting timestamps to datetimes, people commented that it broke their glance, which was returning timestamps without milliseconds. Ed Leafe noticed it and Rick Harris noticed it enough to change it back in his images branch. So I think it is a real issue.

See https://bugs.launchpad.net/nova/+bug/741429 for Ed's comment.

[Jay Pipes (jaypipes)]

> > no need for the parens around locals() there. :)
>
> So I understand that using a tuple isn't required here, but I like it for a
> couple, let's say, personal reasons so I have a question:
>
> This doesn't violate PEP8 or HACKING, so without getting in to my reasons for
> liking this format, did I get the needs fixing because of this or because of
> the NOW_GLANCE_OLD_FORMAT vs. NOW_GLANCE_FORMAT?

because of the (locals()) thing :)

The other was merely me being curious as to whether something really did change in Glance recently :)

[Dan Prince (dan-prince)]

Looks good. Just tried it and it works great.

review: Approve

[Jay Pipes (jaypipes)]

> Jay,
>
> When I first switched to converting timestamps to datetimes, people commented
> that it broke their glance, which was returning timestamps without
> milliseconds. Ed Leafe noticed it and Rick Harris noticed it enough to change
> it back in his images branch. So I think it is a real issue.
>
> See https://bugs.launchpad.net/nova/+bug/741429 for Ed's comment.

OK, I just want to make sure that when you are saying "Glance" that you in fact mean the GlanceImageService in Nova, and not Glance itself. AFAIK, the output from Glance API itself has not changed anytime recently, but the GlanceImageService in Nova definitely did in Rick's branch.

It's important to be exact in describing these things so we don't go looking at the wrong thing to fix ;)

-jay

[Jay Pipes (jaypipes)]

Hey Dan, sorry I should have caught this earlier...

30 + raise ValueError(_("""%(timestamp)s does not follow any of the \
31 +signatures: %(ISO_FORMATS)s""") % locals())

No \ is needed there, nor is the """ needed, since the string is inside a parentheses.

Should be just this:

raise ValueError(_("%(timestamp)s does not follow any of the "
                   "signatures: %(ISO_FORMATS)s") % locals())

Cheers!
jay

fix that up and approved from me.

review: Needs Fixing

[Brian Lamar (blamar)]

Pushed string format fix. Thanks Jay!

[lp:~rackspace-titan/nova/lp742204]

903. By Brian Lamar on 2011-03-29

Switch string concat style.

[Jay Pipes (jaypipes)]

rock on.

review: Approve

[Rick Harris (rconradharris)]

Looks good, nice work guys.

I think supporting ONE-TRUE-FORMAT for datetime is a good goal, so a "FIXME: remove this old format after 6/1/2011" or something would be nice. Supporting two formats indefinitely just seems messy.

That said, this looks like a good stop-gap, nicely implemented and tested.

review: Approve

=== modified file 'nova/image/glance.py'
--- nova/image/glance.py	2011-03-24 21:50:27 +0000
+++ nova/image/glance.py	2011-03-29 16:12:13 +0000
@@ -220,7 +220,7 @@
     Returns image with known timestamp fields converted to datetime objects
     """
     for attr in ['created_at', 'updated_at', 'deleted_at']:
-        if image_meta.get(attr) is not None:
+        if image_meta.get(attr):
             image_meta[attr] = _parse_glance_iso8601_timestamp(
                 image_meta[attr])
     return image_meta
@@ -230,8 +230,13 @@
     """
     Parse a subset of iso8601 timestamps into datetime objects
     """
-    GLANCE_FMT = "%Y-%m-%dT%H:%M:%S"
-    ISO_FMT = "%Y-%m-%dT%H:%M:%S.%f"
-    # FIXME(sirp): Glance is not returning in ISO format, we should fix Glance
-    # to do so, and then switch to parsing it here
-    return datetime.datetime.strptime(timestamp, GLANCE_FMT)
+    iso_formats = ["%Y-%m-%dT%H:%M:%S.%f", "%Y-%m-%dT%H:%M:%S"]
+
+    for iso_format in iso_formats:
+        try:
+            return datetime.datetime.strptime(timestamp, iso_format)
+        except ValueError:
+            pass
+
+    raise ValueError(_("%(timestamp)s does not follow any of the "
+                       "signatures: %(ISO_FORMATS)s") % locals())

=== modified file 'nova/tests/image/test_glance.py'
--- nova/tests/image/test_glance.py	2011-03-24 21:13:55 +0000
+++ nova/tests/image/test_glance.py	2011-03-29 16:12:13 +0000
@@ -55,7 +55,8 @@
 
 
 class BaseGlanceTest(unittest.TestCase):
-    NOW_GLANCE_FORMAT = "2010-10-11T10:30:22"
+    NOW_GLANCE_OLD_FORMAT = "2010-10-11T10:30:22"
+    NOW_GLANCE_FORMAT = "2010-10-11T10:30:22.000000"
     NOW_DATETIME = datetime.datetime(2010, 10, 11, 10, 30, 22)
 
     def setUp(self):
@@ -74,6 +75,10 @@
         self.assertEqual(image_meta['updated_at'], None)
         self.assertEqual(image_meta['deleted_at'], None)
 
+    def assertDateTimesBlank(self, image_meta):
+        self.assertEqual(image_meta['updated_at'], '')
+        self.assertEqual(image_meta['deleted_at'], '')
+
 
 class TestGlanceImageServiceProperties(BaseGlanceTest):
     def test_show_passes_through_to_client(self):
@@ -108,38 +113,72 @@
         image_meta = self.service.show(self.context, 'image1')
         self.assertDateTimesEmpty(image_meta)
 
+    def test_show_handles_blank_datetimes(self):
+        self.client.images = self._make_blank_datetime_fixtures()
+        image_meta = self.service.show(self.context, 'image1')
+        self.assertDateTimesBlank(image_meta)
+
     def test_detail_handles_none_datetimes(self):
         self.client.images = self._make_none_datetime_fixtures()
         image_meta = self.service.detail(self.context)[0]
         self.assertDateTimesEmpty(image_meta)
 
+    def test_detail_handles_blank_datetimes(self):
+        self.client.images = self._make_blank_datetime_fixtures()
+        image_meta = self.service.detail(self.context)[0]
+        self.assertDateTimesBlank(image_meta)
+
     def test_get_handles_none_datetimes(self):
         self.client.images = self._make_none_datetime_fixtures()
         writer = NullWriter()
         image_meta = self.service.get(self.context, 'image1', writer)
         self.assertDateTimesEmpty(image_meta)
 
+    def test_get_handles_blank_datetimes(self):
+        self.client.images = self._make_blank_datetime_fixtures()
+        writer = NullWriter()
+        image_meta = self.service.get(self.context, 'image1', writer)
+        self.assertDateTimesBlank(image_meta)
+
     def test_show_makes_datetimes(self):
         self.client.images = self._make_datetime_fixtures()
         image_meta = self.service.show(self.context, 'image1')
         self.assertDateTimesFilled(image_meta)
+        image_meta = self.service.show(self.context, 'image2')
+        self.assertDateTimesFilled(image_meta)
 
     def test_detail_makes_datetimes(self):
         self.client.images = self._make_datetime_fixtures()
         image_meta = self.service.detail(self.context)[0]
         self.assertDateTimesFilled(image_meta)
+        image_meta = self.service.detail(self.context)[1]
+        self.assertDateTimesFilled(image_meta)
 
     def test_get_makes_datetimes(self):
         self.client.images = self._make_datetime_fixtures()
         writer = NullWriter()
         image_meta = self.service.get(self.context, 'image1', writer)
         self.assertDateTimesFilled(image_meta)
+        image_meta = self.service.get(self.context, 'image2', writer)
+        self.assertDateTimesFilled(image_meta)
 
     def _make_datetime_fixtures(self):
-        fixtures = {'image1': {'name': 'image1', 'is_public': True,
-                               'created_at': self.NOW_GLANCE_FORMAT,
-                               'updated_at': self.NOW_GLANCE_FORMAT,
-                               'deleted_at': self.NOW_GLANCE_FORMAT}}
+        fixtures = {
+            'image1': {
+                'name': 'image1',
+                'is_public': True,
+                'created_at': self.NOW_GLANCE_FORMAT,
+                'updated_at': self.NOW_GLANCE_FORMAT,
+                'deleted_at': self.NOW_GLANCE_FORMAT,
+            },
+            'image2': {
+                'name': 'image2',
+                'is_public': True,
+                'created_at': self.NOW_GLANCE_OLD_FORMAT,
+                'updated_at': self.NOW_GLANCE_OLD_FORMAT,
+                'deleted_at': self.NOW_GLANCE_OLD_FORMAT,
+            },
+        }
         return fixtures
 
     def _make_none_datetime_fixtures(self):
@@ -148,6 +187,12 @@
                                'deleted_at': None}}
         return fixtures
 
+    def _make_blank_datetime_fixtures(self):
+        fixtures = {'image1': {'name': 'image1', 'is_public': True,
+                               'updated_at': '',
+                               'deleted_at': ''}}
+        return fixtures
+
 
 class TestMutatorDateTimeTests(BaseGlanceTest):
     """Tests create(), update()"""