Merge into trunk : bug724623 : Code : OpenStack Compute (nova)

Status:	Work in progress
Proposed branch:	lp://qastaging/~justin-fathomdb/nova/bug724623
Merge into:	lp://qastaging/~hudson-openstack/nova/trunk
Prerequisite:	lp://qastaging/~justin-fathomdb/nova/test-openstack-api-servers
Diff against target:	120 lines (+55/-44) (has conflicts) 2 files modified nova/db/sqlalchemy/api.py (+10/-0) nova/tests/integrated/test_servers.py (+45/-44) Text conflict in bin/nova-api Text conflict in nova/api/openstack/servers.py Text conflict in nova/compute/manager.py Text conflict in nova/service.py Text conflict in nova/tests/glance/stubs.py Text conflict in nova/tests/test_xenapi.py Text conflict in nova/tests/xenapi/stubs.py Text conflict in nova/virt/xenapi/vm_utils.py Text conflict in nova/virt/xenapi/vmops.py Text conflict in nova/virt/xenapi_conn.py Text conflict in plugins/xenserver/xenapi/etc/xapi.d/plugins/glance
To merge this branch:	bzr merge lp://qastaging/~justin-fathomdb/nova/bug724623
Related bugs:	Link a bug report

Reviewer	Date Requested	Status
Josh Kearney (community)	2011-03-08	Approve on 2011-03-09
Jay Pipes (community)	2011-03-08	Approve on 2011-03-09
Review via email: mp+52505@code.qastaging.launchpad.net

Revision history for this message

Jay Pipes (jaypipes) wrote on 2011-02-25: Posted in a previous version of this proposal

#

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

Revision history for this message

justinsb (justin-fathomdb) wrote on 2011-02-25: Posted in a previous version of this proposal

#

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.
>

Revision history for this message

Jay Pipes (jaypipes) wrote on 2011-02-25: Posted in a previous version of this proposal

#

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

On Thu, Feb 24, 2011 at 10:26 PM, justinsb <justin@fathomdb.com> 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

Revision history for this message

justinsb (justin-fathomdb) wrote on 2011-02-28: Posted in a previous version of this proposal

#

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

Revision history for this message

Jay Pipes (jaypipes) wrote on 2011-02-28: Posted in a previous version of this proposal

#

Download full text (4.1 KiB)

> 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...

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

Again, I think the solution is to file a bug, and work with/nag the bug-fixer to merge a fix in as short a time as possible.

> 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!

I feel you, man :) I'm just going to stick to my point on this one and listen to what others have to say. :) I'm in jury duty all this week (writing this on lunch break) so if I don't immediately respond to further comments, that's what is going on. Just a heads up.

-jay

Revision history for this message

justinsb (justin-fathomdb) wrote on 2011-02-28: Posted in a previous version of this proposal

#

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.

Revision history for this message

justinsb (justin-fathomdb) wrote on 2011-03-08:

#

Resubmitted because the previous discussion focused on known_bugs. Based on community input, I have stripped out the known_bugs code: the dragons are now invisible.

Reply

Revision history for this message

Todd Willey (xtoddx) wrote on 2011-03-09:

#

=== added file 'nova/known_bugs.py.THIS' ???

Reply

Revision history for this message

Josh Kearney (jk0) wrote on 2011-03-09:

#

I also have a question about known_bugs.py.THIS -- was '.THIS' left there intentionally, if so, what does it mean?

review: Needs Information

Reply

Revision history for this message

Jay Pipes (jaypipes) wrote on 2011-03-09:

#

When you see a file with a .THIS extension, what that means is that you did a merge, had a conflict, but failed to do a bzr resolve --all before committing and pushing. Normally, bzr catches this kind of thing, but one of two things may have happened:

* Justin may be using the git-bzr bridge, in which case the .THIS file may have inadvertently been bzr added during the commit/push
* Justin may have seen files ending in .OTHER, .BASE, and .THIS and deleted the .OTHER and .BASE files. bzr uses these 3 files to store what the conflict is. The .OTHER branch contains the file in the branch being merged, the .BASE contains the file in a branch that is an ancestor to both the current branch and the merge branch, and .THIS contains the file in the current branch.

Solution: bzr remove the file with .THIS extension and next time, use bzr resolve --all to resolve the conflicts *after* you've resolved the merge conflict in the file...

Hope this helps,

-jay

Reply

Revision history for this message

justinsb (justin-fathomdb) wrote on 2011-03-09:

#

I'm going to try to get known_bugs in any way I can :-)

Removed the file - thanks for pointing it out guys!

Reply

Revision history for this message

Josh Kearney (jk0) wrote on 2011-03-09:

#

> When you see a file with a .THIS extension, what that means is that you did a
> merge, had a conflict, but failed to do a bzr resolve --all before committing
> and pushing. Normally, bzr catches this kind of thing, but one of two things
> may have happened:
>
> * Justin may be using the git-bzr bridge, in which case the .THIS file may
> have inadvertently been bzr added during the commit/push
> * Justin may have seen files ending in .OTHER, .BASE, and .THIS and deleted
> the .OTHER and .BASE files. bzr uses these 3 files to store what the conflict
> is. The .OTHER branch contains the file in the branch being merged, the .BASE
> contains the file in a branch that is an ancestor to both the current branch
> and the merge branch, and .THIS contains the file in the current branch.
>
> Solution: bzr remove the file with .THIS extension and next time, use bzr
> resolve --all to resolve the conflicts *after* you've resolved the merge
> conflict in the file...
>
> Hope this helps,
>
> -jay

Ahah, thanks Jay!

Reply

Revision history for this message

Jay Pipes (jaypipes) wrote on 2011-03-09:

#

Good stuff.

review: Approve

Reply

Revision history for this message

Josh Kearney (jk0) wrote on 2011-03-09:

#

lgtm

review: Approve

Reply

Revision history for this message

OpenStack Infra (hudson-openstack) wrote on 2011-03-09:

#

The prerequisite lp:~justin-fathomdb/nova/test-openstack-api-servers has not yet been merged into lp:nova.

Reply

Revision history for this message

Jay Pipes (jaypipes) wrote on 2011-03-09:

#

setting to work in progress while we get the review on the pre-req done...

Reply

OpenStack Compute (nova)

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

Commit message

Description of the change

Unmerged revisions

Preview Diff

Subscribers