Code review comment for lp:~nttdata/nova/live-migration

Hi Kei! Improvement look good, thanks for the updates. Here are my round-two review notes:

> def mktmpfile(self, context):

It might be a good idea to rename these functions. Right now, the name
confirm_tmpfile contain implementation details, but doesn't provide a good
hint as to what it's used for.

Might be better as:

    create_shared_storage_test_file

Also, what happens if the destination isn't on shared storage? We've deposited
the test file, will that ever be cleaned up?

Perhaps in pseudo-code it should be something like:

    def mounted_on_same_shared_storage:

        create_shared_storage_test_file(dest)
        try:
            # Unlike confirm_tmpfile, this doesn't delete the test_file; that is left
            # to cleanup_test_file
            test_file_exists = check_shared_storage_test_file(source)
        finally:
            # Regardless of whether we find it, we always delete it
            cleanup_shared_storage_test_file(dest)

> ======================================================================
> ERROR: Failure: ImportError (No module named libvirt)
> ----------------------------------------------------------------------
> Traceback (most recent call last):
> File "/Library/Python/2.6/site-packages/nose-0.11.3-py2.6.egg/nose/loader.py", line 382, in loadTestsFromName
> addr.filename, addr.module)
> File "/Library/Python/2.6/site-packages/nose-0.11.3-py2.6.egg/nose/importer.py", line 39, in importFromPath
> return self.importFromDir(dir_path, fqname)
> File "/Library/Python/2.6/site-packages/nose-0.11.3-py2.6.egg/nose/importer.py", line 86, in importFromDir
> mod = load_module(part_fqname, fh, filename, desc)
> File "/Users/rick/Documents/code/openstack/nova/live-migration/nova/tests/test_virt.py", line 17, in <module>
> import libvirt
> ImportError: No module named libvirt
>
> ----------------------------------------------------------------------

Libvirt is required for the tests to run.

Since not everyone is going to have libvirt on their machine, we should probably use
the 'LazyImport pattern' here so that we only import libvirt if it's actually
going to be used-- in the case of unit-tests only the FakeLibvirt should be
used.

> 232 + for v in instance_ref['volumes']:

In general, it's better to use variable names with one letter-- it aids
readability and make it a little easier to 'grep' around the code. In this
case, 'volume' seems like the right choice. There are a few other instances
throughout the code where I think single-letter variable names should
proabably be expanded:

    + p = os.path.join(FLAGS.instances_path, filename)

On the other hand, it's fine (and idiomatic) for exception handler blocks to use `e` as the
variable for their exception. Like:

+ except exception.ProcessExecutionError, e:

> 697 +compute_services = Table('compute_services', meta,

'compute_services' sounds little too much like the 'compute worker service'
that we already have. This might be clearer if renamed

'compute_nodes' or 'compute_hosts'.

The 'compute_node' would represent the physical machine, while the
'compute-service' would represent the logical endpoint to communicate with it
(via Rabbit messages).

> 960 + o = oservice_ref['hypervisor_type']
> 961 + d = dservice_ref['hypervisor_type']

Would be better named as 'orig_hypervisor' and 'dest_hypervisor' respectively.

> 987 + def has_enough_resources(self, context, instance_ref, dest):
Small nit here. This method is raising if not-enough memory is present. Based
on how it's named, it looks like it should return a boolean, so, instead of
raising, it should return False.

Of course, you still want to have your check, so perhaps you could add a new
method, somdthing like:

def assert_compute_node_has_enough_resources(....):
   if not has_enough_resources():
       raise Exception('Whoa, not enough resources!! :)')

> 1093 + if 0 == len(instance_refs):

Yoda syntax :) http://jamiethompson.co.uk/web/2010/05/20/yoda-syntax-a-php-design-pattern-for-if-statements/

Better:

if len(instance_refs) == 0:

Even Better:

if not instance_refs:

> 1098 + for i in project_ids:

`i` would be better as `project_id`.