OpenStack Compute (nova)

Merge lp://qastaging/~cerberus/nova/disk_config into lp://qastaging/~hudson-openstack/nova/trunk

disk_config
Merge into trunk

Proposed by Matt Dietz on 2011-09-10

Status:	Rejected
Rejected by:	Brian Waldon on 2011-09-26
Proposed branch:	lp://qastaging/~cerberus/nova/disk_config
Merge into:	lp://qastaging/~hudson-openstack/nova/trunk
Diff against target:	466 lines (+362/-5) 8 files modified nova/api/openstack/contrib/diskconfig.py (+150/-0) nova/db/sqlalchemy/migrate_repo/versions/050_add_disk_config_to_instances.py (+39/-0) nova/db/sqlalchemy/models.py (+1/-0) nova/tests/api/openstack/contrib/test_diskconfig.py (+156/-0) nova/tests/api/openstack/test_extensions.py (+1/-0) nova/virt/xenapi/vm_utils.py (+4/-2) nova/virt/xenapi/vmops.py (+5/-1) plugins/xenserver/xenapi/etc/xapi.d/plugins/glance (+6/-2)
To merge this branch:	bzr merge lp://qastaging/~cerberus/nova/disk_config
Related bugs:	Link a bug report

Reviewer	Date Requested	Status
Brian Waldon (community)		Needs Fixing on 2011-09-20
Vish Ishaya (community)	2011-09-10	Approve on 2011-09-11
Review via email: mp+74892@code.qastaging.launchpad.net

Description of the change

Implements two extensions and a migration for tracking the management of a disk or image. The intent is to use this to indicate whether or not a disk is "managed." If the flag isn't set, or is set to false, the user is acknowledging he or she would like the ability to alter the file system, at the expense of certain actions on their behalf.

This patch merely implements setting and displaying the flag, and provides no backing functionality as mentioned above.

Revision history for this message

Vish Ishaya (vishvananda) wrote on 2011-09-11:

This seems completely resonable. I don't know if it is worth it to get into diablo without underlying functionality. Thoughts?

review: Approve

Revision history for this message

Matt Dietz (cerberus) wrote on 2011-09-12:

Vish: probably not. It doesn't exactly solve any problems at the moment.

Revision history for this message

Brian Waldon (bcwaldon) wrote on 2011-09-18:

Great work, Dietz. My only question is about the migration: should the new column be NOT_NULL and have a default of True? Just a question, not saying that's the right way to do it.

review: Needs Information

Revision history for this message

Matt Dietz (cerberus) wrote on 2011-09-20:

Brian: I was actually trying to set it up that way, but I looked at every other migration with boolean columns and they seem to do it the way I did. I'd like to change it if it was obvious

Revision history for this message

Brian Waldon (bcwaldon) wrote on 2011-09-20:

Okay, that's fine. You do need to bump your migration number again. I also checked out your code and merged trunk and the two of the tests you added were erroring:

Traceback (most recent call last):
File "/Users/brian.waldon/valhalla/nova/disk_config/nova/tests/api/openstack/contrib/test_diskconfig.py", line 127, in test_image_get_disk_config_no_image_fails
fakes.stub_out_glance(self.stubs, initial_fixtures=self.fixtures)
TypeError: stub_out_glance() got an unexpected keyword argument 'initial_fixtures'

review: Needs Fixing

Revision history for this message

Matt Dietz (cerberus) wrote on 2011-09-21:

Tests should pass now, Brian. Good catch

Revision history for this message

Rick Harris (rconradharris) wrote on 2011-09-21:

Looks good.

> 419 + metadata = {'x-image-meta-property-managed-disk':

To me, passing in headers that the Glance xapi plugin turns around and proxies over to Glance breaks the 'client' abstraction that the glance plugin provides.

(In an ideal world, the only thing that would craft Glance requests would be `glance.client`, but since that's not compatible with Python 2.4, the glance plugin must serve as a second, ad-hoc client.)

Keeping request building code isolated in these two places (the client and the glance plugin) should hopefully make it easier for Glance to change without affecting Nova.

As an alternative, I'd recommend passing in `managed_disk` as a param to the plugin, which will then craft the headers. This was done for the `os_type` field.

Revision history for this message

Brian Waldon (bcwaldon) wrote on 2011-09-21:

Tests passing now. I definitely agree with what Rick said, about the client abstraction and the managed_disk kwarg. Would you mind implementing that? Just say so if not, I will understand :)

lp://qastaging/~cerberus/nova/disk_config updated on 2011-09-23

1527. By Matt Dietz on 2011-09-23: Merge from trunk and update migration number again

Revision history for this message

Matt Dietz (cerberus) wrote on 2011-09-23:

Nope, this makes sense to me.

lp://qastaging/~cerberus/nova/disk_config updated on 2011-09-23

1528. By Matt Dietz on 2011-09-23: Updates for merge prop comments

Revision history for this message

Brian Waldon (bcwaldon) wrote on 2011-09-26:

moved to gerrit

Unmerged revisions

1528. By Matt Dietz on 2011-09-23: Updates for merge prop comments
1527. By Matt Dietz on 2011-09-23: Merge from trunk and update migration number again
1526. By Matt Dietz on 2011-09-21: Merge from trunk, updated failing tests and pep8
1525. By Matt Dietz on 2011-09-19: Merge from trunk and migration renumbering
1524. By Matt Dietz on 2011-09-12: Made the migration less silly
1523. By Matt Dietz on 2011-09-12: Changed the output format to look more like the rest of the API commands
1522. By Matt Dietz on 2011-09-10: All tests passing, and PEP8 fixes
1521. By Matt Dietz on 2011-09-10: Fixes for unit tests
1520. By Matt Dietz on 2011-09-10: Updates to the plugin to actually have it apply the metadata
1519. By Matt Dietz on 2011-09-09: Merge from trunk