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

Hi Rick,

Thanks for review!
I think I fixed all of your comments.

Additional changes are made at nova.compute.api.create and nova.image.s3.
It is not related to live-migration, but instances which has kernel and ramdisk cannot launch without this changes. I never change this file and I tested not only run_test.sh but also confirm instances were successfully migrated at real server before I raised merge request. So I completely have no idea when this changes are included...
Anyway, I think this change is necessary. Could you also please review it?

Kindly Regards,
Kei

-----Original Message-----
From: <email address hidden> [mailto:<email address hidden>] On Behalf Of Rick Harris
Sent: Friday, February 18, 2011 7:10 AM
To: <email address hidden>
Subject: Re: [Merge] lp:~nttdata/nova/live-migration into lp:nova

Review: Needs Fixing
Just a few nits :)

> + def describeresource(self, host):
> + def updateresource(self, host):

These should probably be `describe_resource` and `update_resource` respectively.

3083 +def mktmpfile(dir):
3084 + """create tmpfile under dir, and return filename."""
3085 + filename = datetime.datetime.utcnow().strftime('%Y%m%d%H%M%S')
3086 + fpath = os.path.join(dir, filename)
3087 + open(fpath, 'a+').write(fpath + '\n')
3088 + return fpath

It would probably be better to use the `tempfile` module in the Python stdlib.

3091 +def exists(filename):
3092 + """check file path existence."""
3093 + return os.path.exists(filename)
3094 +
3095 +
3096 +def remove(filename):
3097 + """remove file."""
3098 + return os.remove(filename)

These wrapper functions seem unnecessary, it would probably be better to just use os.path.exists and os.remove directly in the code.

If you need a stub-point for testing, you can stub out `os.path` and `os` directly.

+ LOG.info('post_live_migration() is started..')

Needs i18n _('post_live...') treatment.

533 + #services.create_column(services_vcpus)
534 + #services.create_column(services_memory_mb)
535 + #services.create_column(services_local_gb)
536 + #services.create_column(services_vcpus_used)
537 + #services.create_column(services_memory_mb_used)
538 + #services.create_column(services_local_gb_used)
539 + #services.create_column(services_hypervisor_type)
540 + #services.create_column(services_hypervisor_version)
541 + #services.create_column(services_cpu_info)

Was this left in by mistake?

902 + print 'manager.attrerr', e

Probably should be logging here, rather than printing to stdout.

--
https://code.launchpad.net/~nttdata/nova/live-migration/+merge/49699
Your team NTT DATA is subscribed to branch lp:~nttdata/nova/live-migration.