Merge lp:~ewanmellor/nova/lp654025 into lp:~hudson-openstack/nova/trunk

Proposed by Ewan Mellor
Status: Merged
Approved by: Rick Clark
Approved revision: 317
Merged at revision: 341
Proposed branch: lp:~ewanmellor/nova/lp654025
Merge into: lp:~hudson-openstack/nova/trunk
Diff against target: 53 lines (+16/-4)
3 files modified
bin/nova-manage (+7/-2)
nova/auth/manager.py (+4/-1)
nova/db/api.py (+5/-1)
To merge this branch: bzr merge lp:~ewanmellor/nova/lp654025
Reviewer Review Type Date Requested Status
Jay Pipes (community) Approve
Soren Hansen (community) Disapprove
Eric Day (community) Approve
Review via email: mp+37391@code.launchpad.net

Commit message

Catch exception.NotFound when getting project VPN data.

To post a comment you must log in.
Revision history for this message
Ewan Mellor (ewanmellor) wrote :

Also, I took the opportunity to increase the column width for ip:port from 12 to 20, since 12 isn't enough to hold an IP address and a port. I forgot to mention that in the commit message, apologies.

Revision history for this message
Eric Day (eday) wrote :

lgtm

review: Approve
Revision history for this message
Soren Hansen (soren) wrote :

NAK.

See my comments on https://bugs.launchpad.net/nova/+bug/654025

If not having a VPN set is actually valid, there should be a way to create projects without them. As it is, .create_project creates a network, so in a sense, our data model does not support networkless projects. Ignoring discrepancies in our data model in the only UI we have is not a good idea, IMO.

review: Disapprove
Revision history for this message
Jay Pipes (jaypipes) wrote :

I agree with Soren that just ignoring the fact that an old project does not have a network associated with it in the only UI that Nova has is not likely the best idea.

I feel the best idea would be to list the project as needing an upgrade, and then code in some nova-manage project upgrade command that associates a network with the project. We could use this pattern to handle all manners of schema upgrades that will eventually be needed for Nova...

-jay

review: Disapprove
Revision history for this message
Ewan Mellor (ewanmellor) wrote :

On Mon, Oct 04, 2010 at 07:41:23PM +0100, Jay Pipes wrote:

> Review: Disapprove
> I agree with Soren that just ignoring the fact that an old project does not have a network associated with it in the only UI that Nova has is not likely the best idea.
>
> I feel the best idea would be to list the project as needing an upgrade, and then code in some nova-manage project upgrade command that associates a network with the project. We could use this pattern to handle all manners of schema upgrades that will eventually be needed for Nova...

I don't agree with the assessment of this patch. I'm not ignoring the missing
data -- I'm printing out that it is missing.

Once again, there is no way that we as developers will keep a schema upgrade
tool perfectly up-to-date with every possible "before" and "after" schema
in a rapidly churning project. The best that we can hope for is that the
schema upgrade works from release to release. That means that we need our
UI to work without crashing when data aren't present, otherwise we're
all going to spend our lives chasing down backtraces.

If you're sure that this is a violation of our datamodel, then the UI should
flag it as an error. I'm fine with that. A backtrace is not an appropriate
user interface for this.

Revision history for this message
Jay Pipes (jaypipes) wrote :

> On Mon, Oct 04, 2010 at 07:41:23PM +0100, Jay Pipes wrote:
>
> > Review: Disapprove
> > I agree with Soren that just ignoring the fact that an old project does not
> have a network associated with it in the only UI that Nova has is not likely
> the best idea.
> >
> > I feel the best idea would be to list the project as needing an upgrade, and
> then code in some nova-manage project upgrade command that associates a
> network with the project. We could use this pattern to handle all manners of
> schema upgrades that will eventually be needed for Nova...
>
> I don't agree with the assessment of this patch. I'm not ignoring the missing
> data -- I'm printing out that it is missing.
>
> Once again, there is no way that we as developers will keep a schema upgrade
> tool perfectly up-to-date with every possible "before" and "after" schema
> in a rapidly churning project. The best that we can hope for is that the
> schema upgrade works from release to release. That means that we need our
> UI to work without crashing when data aren't present, otherwise we're
> all going to spend our lives chasing down backtraces.
>
> If you're sure that this is a violation of our datamodel, then the UI should
> flag it as an error. I'm fine with that. A backtrace is not an appropriate
> user interface for this.

Ewan, please read my response before disagreeing... :)

I said:

"I feel the best idea would be to list the project as needing an upgrade, and
then code in some nova-manage project upgrade command that associates a
network with the project."

Where does that say a backtrace should be output?

-jay

Revision history for this message
Ewan Mellor (ewanmellor) wrote :

On Tue, Oct 05, 2010 at 06:39:44PM +0100, Jay Pipes wrote:

> > On Mon, Oct 04, 2010 at 07:41:23PM +0100, Jay Pipes wrote:
> >
> > > Review: Disapprove
> > > I agree with Soren that just ignoring the fact that an old project does not
> > have a network associated with it in the only UI that Nova has is not likely
> > the best idea.
> > >
> > > I feel the best idea would be to list the project as needing an upgrade, and
> > then code in some nova-manage project upgrade command that associates a
> > network with the project. We could use this pattern to handle all manners of
> > schema upgrades that will eventually be needed for Nova...
> >
> > I don't agree with the assessment of this patch. I'm not ignoring the missing
> > data -- I'm printing out that it is missing.
> >
> > Once again, there is no way that we as developers will keep a schema upgrade
> > tool perfectly up-to-date with every possible "before" and "after" schema
> > in a rapidly churning project. The best that we can hope for is that the
> > schema upgrade works from release to release. That means that we need our
> > UI to work without crashing when data aren't present, otherwise we're
> > all going to spend our lives chasing down backtraces.
> >
> > If you're sure that this is a violation of our datamodel, then the UI should
> > flag it as an error. I'm fine with that. A backtrace is not an appropriate
> > user interface for this.
>
> Ewan, please read my response before disagreeing... :)
>
> I said:
>
> "I feel the best idea would be to list the project as needing an upgrade, and
> then code in some nova-manage project upgrade command that associates a
> network with the project."
>
> Where does that say a backtrace should be output?

OK, I agree that you didn't say that a backtrace should be output ;-)
However, you did reject my patch, which is fixing the backtrace. I feel that
we should fix the most egregious problem immediately, even if we want to
discuss a wider concept of "project requires upgrade".

And while you didn't say that a backtrace should be output, Soren's view
seemed to be (and I say *seemed*, I don't want to put too many words in his
mouth) was that this shouldn't have happened, and we shouldn't try too hard
to work around things that shouldn't have happened. In this case, I disagree.
When it adds lots of complexity, I agree, but not when it's adding nothing
more than catching an exception near the top level.

Revision history for this message
Jay Pipes (jaypipes) wrote :

:) ok, points taken. I'll approve, since you are correct that the patch does indeed fix the original bug. And we can put off for the summit a discussion on how to gracefully handle schema upgrades in the future.

Soren, the ball's in your court :)

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bin/nova-manage'
2--- bin/nova-manage 2010-09-29 18:12:02 +0000
3+++ bin/nova-manage 2010-10-03 12:19:40 +0000
4@@ -88,11 +88,16 @@
5 def list(self):
6 """Print a listing of the VPNs for all projects."""
7 print "%-12s\t" % 'project',
8- print "%-12s\t" % 'ip:port',
9+ print "%-20s\t" % 'ip:port',
10 print "%s" % 'state'
11 for project in self.manager.get_projects():
12 print "%-12s\t" % project.name,
13- print "%s:%s\t" % (project.vpn_ip, project.vpn_port),
14+
15+ try:
16+ s = "%s:%s" % (project.vpn_ip, project.vpn_port)
17+ except exception.NotFound:
18+ s = "None"
19+ print "%-20s\t" % s,
20
21 vpn = self._vpn_for(project.id)
22 if vpn:
23
24=== modified file 'nova/auth/manager.py'
25--- nova/auth/manager.py 2010-09-25 03:32:00 +0000
26+++ nova/auth/manager.py 2010-10-03 12:19:40 +0000
27@@ -653,7 +653,10 @@
28 zippy.writestr(FLAGS.credential_key_file, private_key)
29 zippy.writestr(FLAGS.credential_cert_file, signed_cert)
30
31- (vpn_ip, vpn_port) = self.get_project_vpn_data(project)
32+ try:
33+ (vpn_ip, vpn_port) = self.get_project_vpn_data(project)
34+ except exception.NotFound:
35+ vpn_ip = None
36 if vpn_ip:
37 configfile = open(FLAGS.vpn_client_template, "r")
38 s = string.Template(configfile.read())
39
40=== modified file 'nova/db/api.py'
41--- nova/db/api.py 2010-10-01 09:28:31 +0000
42+++ nova/db/api.py 2010-10-03 12:19:40 +0000
43@@ -432,7 +432,11 @@
44
45
46 def project_get_network(context, project_id):
47- """Return the network associated with the project."""
48+ """Return the network associated with the project.
49+
50+ Raises NotFound if no such network can be found.
51+
52+ """
53 return IMPL.project_get_network(context, project_id)
54
55