Code review comment for lp:~vladimir.p/nova/vsa

Nicely done vladimir. I just see a couple of issues:

573 === added directory 'nova/CA/newcerts'
574 === removed directory 'nova/CA/newcerts'
575 === added file 'nova/CA/newcerts/.placeholder'
576 === removed file 'nova/CA/newcerts/.placeholder'
577 === added directory 'nova/CA/private'
578 === removed directory 'nova/CA/private'
579 === added file 'nova/CA/private/.placeholder'
580 === removed file 'nova/CA/private/.placeholder'
581 === added directory 'nova/CA/projects'
582 === removed directory 'nova/CA/projects'
583 === added file 'nova/CA/projects/.gitignore'
584 --- nova/CA/projects/.gitignore 1970-01-01 00:00:00 +0000
585 +++ nova/CA/projects/.gitignore 2011-08-26 18:16:26 +0000
586 @@ -0,0 +1,1 @@
587 +*
588
589 === removed file 'nova/CA/projects/.gitignore'
590 --- nova/CA/projects/.gitignore 2010-07-28 08:32:40 +0000
591 +++ nova/CA/projects/.gitignore 1970-01-01 00:00:00 +0000
592 @@ -1,1 +0,0 @@
593 -*
594
595 === added file 'nova/CA/projects/.placeholder'
596 === removed file 'nova/CA/projects/.placeholder'
597 === added directory 'nova/CA/reqs'
598 === removed directory 'nova/CA/reqs'
599 === added file 'nova/CA/reqs/.gitignore'
600 --- nova/CA/reqs/.gitignore 1970-01-01 00:00:00 +0000
601 +++ nova/CA/reqs/.gitignore 2011-08-26 18:16:26 +0000
602 @@ -0,0 +1,1 @@
603 +*
604
605 === removed file 'nova/CA/reqs/.gitignore'
606 --- nova/CA/reqs/.gitignore 2010-07-28 08:32:40 +0000
607 +++ nova/CA/reqs/.gitignore 1970-01-01 00:00:00 +0000
608 @@ -1,1 +0,0 @@
609 -*
610
611 === added file 'nova/CA/reqs/.placeholder'
612 === removed file 'nova/CA/reqs/.placeholder'
613 === added file 'nova/api/openstack/contrib/virtual_storage_arrays.py'

Some weird stuff going here. Might be nice to recreate the branch to get rid of these strange issues. i.e. start with trunk and just copy all of the new files in. and put it in one commit.

4006 + def create_volumes(self, context, request_spec, availability_zone):
4007 + LOG.info(_("create_volumes called with req=%(request_spec)s, "\
4008 + "availability_zone=%(availability_zone)s"), locals())
4009 +

This doesn't look like it is actually implemented? It seems to be just logging so perhaps remove it for now?

----

Great job separating out special cases from the code. It seems that there is no problem running without nova-vsa. I feel like we need some clear documentation that the vsa code is Experimental and is not a supported core component. I think a note that prints out when you start nova-vsa and a note in the vsa folder in docstrings would be good enough.

It would also be good to mention in that note that a specific image is needed to use the functionality exposed, and other storage providers are invited to make images. I also think it is very important for this to have a generic image implementation that is open source. Even if it is just a silly command line version.

In general I'm worried about adding something to the project that is specific to a vendor, but since you guys have put so much effort into doing this the right way, I'm willing to assume that you will continue to generalize and make it possible for others to use this code.

Rest of nova-core, does that seem ok?

As far as impact on the rest of the code, it passes smoketests and it only adds one isolated db table so it seems pretty safe.