Merge lp:~justin-fathomdb/nova/bug724623 into lp:~hudson-openstack/nova/trunk

[justinsb]

Fixes bug 724623: server metadata couldn't actually be created, because it can't be written to the DB. Also adds unit tests that test this functionality through the OpenStack API.

[Jay Pipes (jaypipes)]

I'm not a fan of the known_bugs.py file in the prerequsite branch. Would it be possible to just fix the bug and propose that for merging instead of having that large pre-req branch? Also, you've got print statements in the test case below..

review: Disapprove

[justinsb (justin-fathomdb)]

Hi Jay - this branch is at the end of a long dependency chain, so I didn't
expect anyone to get to it so fast!!

Certainly I could "back-port" the bugfix, but I don't think it's necessarily
a blocker for anyone so I'm not sure it's worth the effort (?)

However, the 'known_bugs' thing is more interesting.... that's my experiment
in whether this approach works for separating out bugs (as I find them) from
the bug fixes. In this case, I found a random bug while I was writing the
test case: "In the OS API, when creating a server, the metadata is not
returned". I wanted the test case to have the logic, but equally I can't
actually test it until the bug is fixed. I'm not sure it's a high priority
bug at the moment (it's more of a TODO than a bug!) Can you think of a
better approach? Do I dare open this up to mailing list?

I am guilty of putting print statements in my tests - must remember not to
do that!!

On Thu, Feb 24, 2011 at 6:07 PM, Jay Pipes <email address hidden> wrote:

> Review: Disapprove
> I'm not a fan of the known_bugs.py file in the prerequsite branch. Would it
> be possible to just fix the bug and propose that for merging instead of
> having that large pre-req branch? Also, you've got print statements in the
> test case below..
> --
> https://code.launchpad.net/~justin-fathomdb/nova/bug724623/+merge/51227
> You are the owner of lp:~justin-fathomdb/nova/bug724623.
>

[Jay Pipes (jaypipes)]

On Thu, Feb 24, 2011 at 10:26 PM, justinsb <email address hidden> wrote:
> Hi Jay - this branch is at the end of a long dependency chain, so I didn't
> expect anyone to get to it so fast!!

You can never tell which one will be reviewed first ;)

> Certainly I could "back-port" the bugfix, but I don't think it's necessarily
> a blocker for anyone so I'm not sure it's worth the effort (?)

Sure, understood.

> However, the 'known_bugs' thing is more interesting.... that's my experiment
> in whether this approach works for separating out bugs (as I find them) from
> the bug fixes.

That's what the LP bugs system is for. I'm really not in favour of
adding a file that tracks known bugs. A similar concept is a file that
disables certain tests on certain builds. It just never works in the
long run, as things always get out of sync. I think it's best to just
use the Launchpad bugs system to track your fixes and make sure that
LP is always as up to date as possible and linked to the branches you
make that fix the bugs. You've been doing a great job of this so far,
so I was a bit surprised to see you change tactics here :)

> In this case, I found a random bug while I was writing the
> test case: "In the OS API, when creating a server, the metadata is not
> returned".  I wanted the test case to have the logic, but equally I can't
> actually test it until the bug is fixed.  I'm not sure it's a high priority
> bug at the moment (it's more of a TODO than a bug!)  Can you think of a
> better approach?  Do I dare open this up to mailing list?

OK, so this happens quite a bit. This is the way I deal with it:

When you run into the bug:
* If it is a bug ONLY in code that you have just added in your branch,
then just fix the bug
* If it is a bug that is reproducible in trunk (i.e. not *just* the
topic branch you're working in):
  (1) File the bug in Launchpad
  (2) Do not fix the bug in your topic branch
  (3) If you want to assign yourself to the bug you just filed, then:
    a. Branch a bugfix branch from your local *trunk* branch (NOT your
topic branch)
    b. Add a test case to your bugfix branch that will trigger the bug
    c. Patch code to fix the bug and pass the test case
    d. Push to LP with --fixes=lp:XXXX where XXX is the bug number
    e. Propose for merging your bugfix branch into trunk

At this point, return to working on your original topic branch and
continue coding. If you *must* have the fix for the bug you just
reported, then do:

bzr shelve --all
bzr merge ../bugXXX && bzr commit -m "Merge fix for XXX"
bzr unshelve 1

If you don't absolutely have to have the fix for bug XXX in your topic
branch, don't merge it, since there's really no need to...

Hope this helps explain a little bit how I like to work on things that
can have dependencies between them. In git, it's a little easier,
since you can cherry-pick a bugfix revision from another branch
without having to shelve/merge/unshelve, but in Bazaar, the above is
the way to go about this all...

> I am guilty of putting print statements in my tests - must remember not to
> do that!!

I do it all the time, no worries :)

-jay

[justinsb (justin-fathomdb)]

Hi Jay,

Thanks for outlining the process, but I think this only applies when I'm fixing a bug at the same time / am able to fix it myself. The goal of the known_bugs file is to cope when I'm not fixing a bug at the same time. Some examples:

* When I'm doing some of my 'advanced' unit tests, I'd rather use fake_carrot than RabbitMQ, but there was an issue in it which turned out to be way beyond my knowledge of the queuing stuff to solve (Vish fixed it). Though Vish fixed it quickly, I had to fall back to using RabbitMQ while the fix was in-progress.

* The authentication for OpenStack is currently 'unusual', and not the username and password you might expect. By controlling that with a known_bugs flag, once this is fixed the flag can be removed and then the tests will continue to pass. I can't just fix this bug, because it has to go through "the process".

* Some of my unit tests bring up a WSGI server, but I haven't currently figured out how to shut it down cleanly, so I need to recycle the same WSGI instance across tests. If someone that knows WSGI fixes this, I'd like to shut it down cleanly etc. My unit test code could also be a head start for the bug fixer in terms of diagnosing the problem (once the flag is removed, they'll get the failing tests immediately)

I agree that known_bugs is a bit ugly, but I think it has some upsides in terms of keeping this information neatly in one place in the code. I definitely expect that we'd file bugs alongside adding known_bugs flags - this isn't intended to be an end-run around that. I'm very open to better alternatives!

Justin

[Jay Pipes (jaypipes)]

> Thanks for outlining the process, but I think this only applies when I'm
> fixing a bug at the same time / am able to fix it myself.

I said the opposite, actually:

* If it is a bug that is reproducible in trunk (i.e. not *just* the
topic branch you're working in):
  (1) File the bug in Launchpad
  (2) Do not fix the bug in your topic branch
  (3) If you want to assign yourself to the bug you just filed, then:
    a. Branch a bugfix branch from your local *trunk* branch (NOT your topic branch)
    b. Add a test case to your bugfix branch that will trigger the bug
    c. Patch code to fix the bug and pass the test case
    d. Push to LP with --fixes=lp:XXXX where XXX is the bug number
    e. Propose for merging your bugfix branch into trunk

> The goal of the
> known_bugs file is to cope when I'm not fixing a bug at the same time. Some
> examples:
>
> * When I'm doing some of my 'advanced' unit tests, I'd rather use fake_carrot
> than RabbitMQ, but there was an issue in it which turned out to be way beyond
> my knowledge of the queuing stuff to solve (Vish fixed it). Though Vish fixed
> it quickly, I had to fall back to using RabbitMQ while the fix was in-
> progress.

Then you should simply file a bug. There is no reason to have the known_bugs.py file IMO. That is what Launchpad and all of its tracking goodness is for...

> * The authentication for OpenStack is currently 'unusual', and not the
> username and password you might expect. By controlling that with a known_bugs
> flag, once this is fixed the flag can be removed and then the tests will
> continue to pass. I can't just fix this bug, because it has to go through
> "the process".

The known_bugs flag doesn't "control" anything. The above scenario just means that your work depends on a fix for a bug. If you don't want that to delay you, you have the option of either fixing the bug if possible or nagging someone to fix it. Disabling a test case that happens to fail when you change code behaviour to depend on a potential bug fix or community-decided change to, say, the API, is not an option. This only leads to long and painful lists of dependent branches that all come crumbling down when the bug is fixed in a way that you didn't foresee. Or at least, that's been my experience. Having a file that disables tests is just not a good future-proof way to "push through" the inconvenience of a bug fix holding up code you want to get merged. I agree it's a pain, but it's a necessary pain and I believe adding the known_bugs.py file makes the source tree vulnerable to bugs via procrastination since the test is disabled and "hidden" from impacting automated testing.

Believe me, Justin, I understand the frustration that comes with having to wait for bug fixes. I do. Based on my experience in other projects, I just feel this particular patch is not a workable solution. But, it's just my opinion. I'll respect the wishes of other contributors if they feel the known_bugs solution is one that should be promoted.

> * Some of my unit tests bring up a WSGI server, but I haven't currently
> figured out how to shut it down cleanly, so I need to recycle the same WSGI
> instance across tests. If som...

[justinsb (justin-fathomdb)]

Jay, I see and fully respect your view. If I find a bug, I should fix it or get it fixed!

I think though the choice isn't between fixing a bug and not fixing a bug, it's between writing a test and not writing a test, or writing a hacked test and writing a good test (with a hack controlled by the known_bugs flag). To take the easiest example, authentication is 'confused' right now, so right now I set the values all to be the same string, so that it doesn't matter what permutation is used. But that's a terrible unit test! The choice is whether isn't whether or not I fix it (I'd like to fix it...), the choice is whether I write a good unit test or a bad one.

To be clear - I'm not talking about disabling a test normally, I'm talking about degrading a test. So, authentication will step-back to using the same values for all 3. We'll use real RabbitMQ instead of fake_carrot. We'll share WSGI services instead of cleanly restarting them.

Of course, this might not be evident from the current branch, but that's definitely the intent.

Right now, if I find a bug, my natural inclination is to work around it, not fix it. I certainly am not inclined to write a unit test that fails, because by definition I can't get that merged. I should open a bug, and I should fix it, but I'm not always going to want to break my flow and jump into a completely different system.

Also, when we have dedicated QA people, I'd like them to write unit tests that fail even if they can't fix it.

But at the same time, I see your point of view. It's complicated (but I think it's a complex problem). It does let us avoid fixing things (but that's sort of the point, really). It's not obvious that I'm going to get the test right for the case when the bug is fixed.

Not an easy one... might be time to share this debate with the mailing list.

[lp:~justin-fathomdb/nova/bug724623]

729. By justinsb on 2011-03-08

Merged with upstream

730. By justinsb on 2011-03-09

Removed the bizarre/bazaar known_bugs.py.THIS

Unmerged revisions

730. By justinsb on 2011-03-09

Removed the bizarre/bazaar known_bugs.py.THIS

729. By justinsb on 2011-03-08

Merged with upstream

728. By justinsb on 2011-02-24

Merge with upstream

727. By justinsb on 2011-02-24

Merge with rate-limiting-suppression

726. By justinsb on 2011-02-24

Merged with testing branch

725. By justinsb on 2011-02-24

Remove the bug we're working on

724. By justinsb on 2011-02-24

Map dictionary to DB objects

723. By justinsb on 2011-02-24

Add integrated unit tests for server metadata. Also add KNOWN_BUGS list, so that we can test bugs separately from fixing them

722. By justinsb on 2011-02-24

Created tests for OpenStack compute API. Merged with metadata hotfix.

721. By justinsb on 2011-02-24

Re-remove the problematic sqlalchemy fix that somehow made it back into this branch

=== modified file 'nova/db/sqlalchemy/api.py'
--- nova/db/sqlalchemy/api.py	2011-03-08 03:12:29 +0000
+++ nova/db/sqlalchemy/api.py	2011-03-08 03:12:30 +0000
@@ -676,6 +676,16 @@
     context - request context object
     values - dict containing column values.
     """
+
+    metadata = values.get('metadata')
+    metadata_refs = []
+    if metadata:
+        for metadata_item in metadata:
+            metadata_ref = models.InstanceMetadata()
+            metadata_ref.update(metadata_item)
+            metadata_refs.append(metadata_ref)
+    values['metadata'] = metadata_refs
+
     instance_ref = models.Instance()
     instance_ref.update(values)
 

=== added file 'nova/known_bugs.py.THIS'
--- nova/known_bugs.py.THIS	1970-01-01 00:00:00 +0000
+++ nova/known_bugs.py.THIS	2011-03-08 03:12:30 +0000
@@ -0,0 +1,35 @@
+# vim: tabstop=4 shiftwidth=4 softtabstop=4
+
+# Copyright 2011 Justin Santa Barbara
+# All Rights Reserved.
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+"""
+This file contains flag variables that track known bugs.
+
+The idea is that when we find a bug, we would like to have a test case for it,
+but we aren't necessarily in a position to fix it immediately.  So, we record
+a marker in here: KNOWN_BUGS.append('server_create_does_not_return_metadata')
+Then we can write a test, but not fail the test, because it's a known bug.
+We should also open a bug at the same time.
+
+When the person comes to fix that bug, they remove the marker; the test fails;
+they fix the bug; they commit.
+
+This also makes it easy to track known bugs - they should all be in this file.
+"""
+
+KNOWN_BUGS = []
+
+# In the OS API, when creating a server, the metadata is not returned
+KNOWN_BUGS.append('server_create_does_not_return_metadata')

=== modified file 'nova/tests/integrated/test_servers.py'
--- nova/tests/integrated/test_servers.py	2011-03-08 03:12:29 +0000
+++ nova/tests/integrated/test_servers.py	2011-03-08 03:12:30 +0000
@@ -166,50 +166,51 @@
 
         return server
 
-# TODO(justinsb): Enable this unit test when the metadata bug is fixed
-#    def test_create_server_with_metadata(self):
-#        """Creates a server with metadata"""
-#
-#        # Build the server data gradually, checking errors along the way
-#        server = self._build_minimal_create_server_request()
-#
-#        for metadata_count in range(30):
-#            metadata = {}
-#            for i in range(metadata_count):
-#                metadata['key_%s' % i] = 'value_%s' % i
-#            server['metadata'] = metadata
-#
-#            post = {'server': server}
-#            created_server = self.api.post_server(post)
-#            LOG.debug("created_server: %s" % created_server)
-#            self.assertTrue(created_server['id'])
-#            created_server_id = created_server['id']
-#            # Reenable when bug fixed
-#            # self.assertEqual(metadata, created_server.get('metadata'))
-#
-#            # Check it's there
-#            found_server = self.api.get_server(created_server_id)
-#            self.assertEqual(created_server_id, found_server['id'])
-#            self.assertEqual(metadata, found_server.get('metadata'))
-#
-#            # The server should also be in the all-servers details list
-#            servers = self.api.get_servers(detail=True)
-#            server_map = dict((server['id'], server) for server in servers)
-#            found_server = server_map.get(created_server_id)
-#            self.assertTrue(found_server)
-#            # Details do include metadata
-#            self.assertEqual(metadata, found_server.get('metadata'))
-#
-#            # The server should also be in the all-servers summary list
-#            servers = self.api.get_servers(detail=False)
-#            server_map = dict((server['id'], server) for server in servers)
-#            found_server = server_map.get(created_server_id)
-#            self.assertTrue(found_server)
-#            # Summary should not include metadata
-#            self.assertFalse(found_server.get('metadata'))
-#
-#            # Cleanup
-#            self._delete_server(created_server_id)
+    def test_create_server_with_metadata(self):
+        """Creates a server with metadata"""
+
+        # Build the server data gradually, checking errors along the way
+        server = self._build_minimal_create_server_request()
+
+        for metadata_count in range(0, 30, 5):
+            metadata = {}
+            for i in range(metadata_count):
+                metadata['key_%s' % i] = 'value_%s' % i
+            server['metadata'] = metadata
+
+            post = {'server': server}
+            created_server = self.api.post_server(post)
+            LOG.debug("created_server: %s" % created_server)
+            self.assertTrue(created_server['id'])
+            created_server_id = created_server['id']
+            # Reenable when this is implemented
+            # self.assertEqual(metadata, created_server.get('metadata'))
+
+            # Check it's there
+            found_server = self.api.get_server(created_server_id)
+            self.assertEqual(created_server_id, found_server['id'])
+            self.assertEqual(metadata, found_server.get('metadata'))
+
+            # The server should also be in the all-servers details list
+            servers = self.api.get_servers(detail=True)
+            server_map = dict((server['id'], server) for server in servers)
+            found_server = server_map.get(created_server_id)
+            self.assertTrue(found_server)
+            LOG.debug("found server: %s" % found_server)
+            # Details do include metadata
+            self.assertEqual(metadata, found_server.get('metadata'))
+
+            # The server should also be in the all-servers summary list
+            servers = self.api.get_servers(detail=False)
+            server_map = dict((server['id'], server) for server in servers)
+            found_server = server_map.get(created_server_id)
+            self.assertTrue(found_server)
+            LOG.debug("found server: %s" % found_server)
+            # Summary should not include metadata
+            self.assertFalse(found_server.get('metadata'))
+
+            # Cleanup
+            self._delete_server(created_server_id)
 
 
 if __name__ == "__main__":