OpenStack Compute (nova)

Code review comment for lp:~yamahata/nova/boot-from-volume-0

boot-from-volume-0
Merge into trunk

Revision history for this message

Matt Dietz (cerberus) wrote on 2011-06-08:

First of all, great work on this!

I see a few failing tests:

======================================================================
ERROR: test_compute_can_update_available_resource (nova.tests.test_service.ServiceTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/cerberus/code/python/nova/boot-from-volume-0/nova/test.py", line 94, in tearDown
    self.mox.VerifyAll()
  File "/Users/cerberus/code/python/nova/boot-from-volume-0/.nova-venv/lib/python2.6/site-packages/mox.py", line 197, in VerifyAll
    mock_obj._Verify()
  File "/Users/cerberus/code/python/nova/boot-from-volume-0/.nova-venv/lib/python2.6/site-packages/mox.py", line 344, in _Verify
    raise ExpectedMethodCallsError(self._expected_calls_queue)
ExpectedMethodCallsError: Verify: Expected methods never called:
  0. __call__(new=<IgnoreArg>) -> None

======================================================================
ERROR: test_create (nova.tests.test_service.ServiceTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/cerberus/code/python/nova/boot-from-volume-0/nova/test.py", line 94, in tearDown
    self.mox.VerifyAll()
  File "/Users/cerberus/code/python/nova/boot-from-volume-0/.nova-venv/lib/python2.6/site-packages/mox.py", line 197, in VerifyAll
    mock_obj._Verify()
  File "/Users/cerberus/code/python/nova/boot-from-volume-0/.nova-venv/lib/python2.6/site-packages/mox.py", line 344, in _Verify
    raise ExpectedMethodCallsError(self._expected_calls_queue)
ExpectedMethodCallsError: Verify: Expected methods never called:
  0. __call__(new=<IgnoreArg>) -> None

----------------------------------------------------------------------

455 + try:
456 + bdms = self.db.block_device_mapping_get_all_by_instance(
457 + context, instance_id)
458 + except exception.NotFound:
459 + pass

I don't really like throwing away exceptions. It's an explicit exception, sure, but it could mask something important. Seems like this should at least log the error.

476 + assert ((bdm['snapshot_id'] is None) or
477 + (bdm['volume_id'] is not None))

It seems like it would be better to raise an explicit exception here, with a message describing exactly why this is a bad state.

Nits:

48 - if value == 'True':
49 + if value.lower() == 'true':
50 return True
51 - if value == 'False':
52 + if value.lower() == 'false':

I'd probably lower() this once and check it rather than paying to parse the string twice

489 + # TODO(yamahata)
493 + # TODO(yamahata)

What are you planning TODO here?

Typos:

239 + """Stop each instance in instace_id"""
245 + """Start each instance in instace_id"""

instace_id should be instance_id

Also, given that instance_id is singular, it should probably say something like:

"Start the instance denoted by instance_id"

389 + """Stop an instnace."""
408 + """Start an instnace."""

Should be "instance"

469 + # TODO(yamahata): creatning volume simulteneously

should be "creating volume simultaneously"

700 + except exception.NotFound:
701 + pass

Same as above: I'd prefer at least a log indicating that this happened

First of all, great work on this!

I see a few failing tests:

----------------------------------------------------------------------

455 +        try:
456 +            bdms = self.db.block_device_mapping_get_all_by_instance(
457 +                context, instance_id)
458 +        except exception.NotFound:
459 +            pass

I don't really like throwing away exceptions. It's an explicit exception, sure, but it could mask something important. Seems like this should at least log the error.

476 +                assert ((bdm['snapshot_id'] is None) or
477 +                        (bdm['volume_id'] is not None))

It seems like it would be better to raise an explicit exception here, with a message describing exactly why this is a bad state.

Nits:

48  -    if value == 'True':
49  +    if value.lower() == 'true':
50           return True
51  -    if value == 'False':
52  +    if value.lower() == 'false':

I'd probably lower() this once and check it rather than paying to parse the string twice

489 +                    # TODO(yamahata)
493 +                    # TODO(yamahata)

What are you planning TODO here?

Typos:

239 +        """Stop each instance in instace_id"""
245 +        """Start each instance in instace_id"""

instace_id should be instance_id

Also, given that instance_id is singular, it should probably say something like:

"Start the instance denoted by instance_id"

389 +        """Stop an instnace."""
408 +        """Start an instnace."""

Should be "instance"

469  +                    # TODO(yamahata): creatning volume simulteneously

should be "creating volume simultaneously"

700 +    except exception.NotFound:
701 +        pass

Same as above: I'd prefer at least a log indicating that this happened

review: Needs Fixing

« Back to merge proposal