Merge lp:~rconradharris/nova/server_progress into lp:~hudson-openstack/nova/trunk
- server_progress
- Merge into trunk
Proposed by
Rick Harris
| Status: | Merged |
|---|---|
| Approved by: | Matt Dietz |
| Approved revision: | 1575 |
| Merged at revision: | 1613 |
| Proposed branch: | lp:~rconradharris/nova/server_progress |
| Merge into: | lp:~hudson-openstack/nova/trunk |
| Diff against target: |
1033 lines (+322/-110) 22 files modified
nova/api/openstack/views/servers.py (+5/-5) nova/compute/api.py (+6/-3) nova/compute/manager.py (+9/-6) nova/db/sqlalchemy/migrate_repo/versions/049_add_instances_progress.py (+43/-0) nova/db/sqlalchemy/models.py (+2/-0) nova/flags.py (+4/-0) nova/tests/api/openstack/contrib/test_createserverext.py (+4/-2) nova/tests/api/openstack/contrib/test_volumes.py (+2/-1) nova/tests/api/openstack/test_server_actions.py (+1/-0) nova/tests/api/openstack/test_servers.py (+10/-5) nova/tests/test_compute.py (+1/-1) nova/tests/test_virt_drivers.py (+2/-1) nova/tests/test_xenapi.py (+45/-31) nova/tests/xenapi/stubs.py (+5/-2) nova/utils.py (+14/-0) nova/virt/driver.py (+10/-6) nova/virt/fake.py (+3/-3) nova/virt/xenapi/vm_utils.py (+4/-0) nova/virt/xenapi/vmops.py (+126/-30) nova/virt/xenapi_conn.py (+14/-9) plugins/xenserver/xenapi/etc/xapi.d/plugins/glance (+5/-1) plugins/xenserver/xenapi/etc/xapi.d/plugins/migration (+7/-4) |
| To merge this branch: | bzr merge lp:~rconradharris/nova/server_progress |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Trey Morris (community) | Approve | ||
| Matt Dietz (community) | Approve | ||
|
Review via email:
|
|||
Commit message
Description of the change
This patch adds instance progress which is used by the OpenStack API to indicate how far along the current executing action is (BUILD/REBUILD, MIGRATION/RESIZE).
For the first cut, we decided to keep it simple and compute progress by counting discrete steps. This is not ideal since some steps, in particular, steps which involve transferring large amounts of data over the network, take *much* longer than others. A better approximation would account for the data-transferred to the destination host, since in most cases, this dominates the time spent.
In addition to adding progress, this patch:
- Allows resizes to use same host for source and destination which is useful for dev environments without a second host. This is enabled by the --allow_
- Fixes a bug in the glance and migration XenAPI plugins where the VHDs were being copied into the SR in the wrong order. Before the base-copy was copied first meaning it was possible for snapwatchd to see the base-copy before the dependent cow was present. It was treat the base_copy as an unreferenced parent, and GC it.
- Additional refactoring and cleanups.
To post a comment you must log in.
Revision history for this message
| Matt Dietz (cerberus) wrote : | # |
Revision history for this message
| Jay Pipes (jaypipes) wrote : | # |
> So this looks really good from an implementation perspective. Beyond that, I
> really don't like that this kind of functionality is part of the API spec,
> which is of course not your fault. The following is a diatribe against the
> idea, not the patch.
>
> Though the progress is done in a way that mostly keeps all the moving parts
> decoupled (since it's just a guess based on the number of expected steps
> rather than disk progress, as you mention,) I'm really not a fan of the false
> sense of security it will provide end users. Questions like "Why is my Linux
> instance at 25% for a quarter of the time a Windows instance is?" will pop up
> shortly. Moving to a model of disk progress has all sorts of other scary
> implications, like trying to track the copy progress through the XS plugin
> that is currently running (yuck) and I don't even know how you'd do it for the
> other hypervisors. It also opens up all kinds of synchronization issues when
> you start including zones and trying to bubble that data up to the API in a
> reasonably timely manner.
>
> Long story short, I think including progress counters is a very strong
> statement regarding the type of cloud you expect it to be, and in this case, I
> don't think synchronous features like progress are indicative of the sort of
> scale we expect Nova to achieve.
>
> If the community decides they want this, you can consider the above an
> "approve." I don't want to block it just because I might have a different idea
> of the Way Things Should Be™
+10
Revision history for this message
| Rick Harris (rconradharris) wrote : | # |
> Long story short, I think including progress counters is a very strong
> statement regarding the type of cloud you expect it to be, and in this case, I
> don't think synchronous features like progress are indicative of the sort of
> scale we expect Nova to achieve.
Nailed it.
Feels weird to argue against my own patch, but you're absolutely right.
If we get a consensus that this patch should be rejected, I'll go ahead and extract out the misc fixes I included in this patch so we don't lose that work.
Revision history for this message
| Naveed Massjouni (ironcamel) wrote : | # |
I do not see where you are testing the new server progress feature you have added. For example, I see no new assertions in the tests. Is it because this is difficult to test and should be tested at a higher level?
Also, it looks like you have only added the progress feature for xen. Is this correct?
Revision history for this message
| Rick Harris (rconradharris) wrote : | # |
> I do not see where you are testing the new server progress feature you have added. For example, I
> see no new assertions in the tests. Is it because this is difficult to test and should be tested at
> a higher level?
Automated functional testing for this didn't feel like it would be worth the additional effort. This was a judgement call, and feel free to disagree here, but it seemed like any tests for this would be slow, difficult to write, and not very useful.
The reason I think it would not be very useful is that, unlike some areas of the code where we have complex interactions, this is very straightforward. That's not to say this code can't break, it's just that the chances of that happening inadvertently are probably small relative to the pain of testing this.
> Also, it looks like you have only added the progress feature for xen. Is this correct?
Yeah adding it to XenServer for now since that's the hypervisor I'm most familiar with. At a high level the steps should be similar, but each virt driver may have a slightly different process for builds and migrations--and I'm not familiar enough with libvirt, etc to add those myself, without considerable additional research.
Revision history for this message
| Rick Harris (rconradharris) wrote : | # |
TL;DR: Agree that instance progress is a bad idea, we should merge this anyway and reevaluate our options at the Essex Design Summit.
This patch is catching a lot of heat, and for good reason. The current implementation, while simple, is not all that accurate; and creating accurate progress at scale may turn out to be either far too complex or outright impossible.
To those points, I'm in complete agreement and at the Essex design summit I will argue against including any kind of instance progress.
All that said, I still think we should merge this patch in the interim. The reasons are:
1. REQUIREMENT: We committed to supporting the Openstack API and progress is a required part of the spec. If we're going to abandon part of the spec for technical reasons, that's something we should hash out in a formal setting where we can have some good back-and-forth on this.
Until we decide to change course, though, I think we should continue our march towards 100% compatibility.
2. EASY-TO-REMOVE: When and if we decide to drop support for instance progress, removing the code should be a dead-simple; it's just a matter of removing the update_progress method and its callers, dropping the column, and having the OSAPI return to its original behavior of stubbing out progress to be 0 or 100% depending on status.
3. INCLUDES OTHER USEFUL FIXES/REFACTORINGS: This patch includes additional bug-fixes and code cleanups which would be nice to have merged in. They could be extracted into a separate patch (in hindsight that would have been the right approach), but since point 1) is (to me) a hard-requirement, the effort of separating out these fixes is probably not worth it.
Revision history for this message
| Trey Morris (tr3buchet) wrote : | # |
You have valid reasons for merging, and I definitely agree about hashing it out in a formal setting. I'd be on the side against in this setting.
Ease of code removal isn't the issue. My fear is once progress bars are supported we'll never be able to get rid of them. Much the same as how they were supported in the last cloud implementation and are still plaguing us in the next. It may take you <5 minutes to remove it, but months of meetings and bickering beforehand for us to convince people to "allow" it to be removed; to decide that would be okay. But if it were never merged in the first place and we decide against, there's the end of it.
Revision history for this message
| Trey Morris (tr3buchet) wrote : | # |
I hate the feeling that I'm opposing you in this Rick. We really should be arguing against this being in the spec..
Revision history for this message
| Rick Harris (rconradharris) wrote : | # |
Trey: I would say you're not really opposing me here, we're basically seeing things the same way.
I think the one difference is: I don't think we can pull an audible on API requirements, those need to be called in from the sidelines.
Once Essex gets here I'm sure we'll have a nice, vocal discussion as to whether we should break compatibility.
Revision history for this message
| Trey Morris (tr3buchet) wrote : | # |
Regrettably, I could do nothing to prevent the disaster which devastated Tristram.
review:
Approve
Revision history for this message
| Naveed Massjouni (ironcamel) wrote : | # |
> Regrettably, I could do nothing to prevent the disaster which devastated
> Tristram.
Did you fall hopelessly in love with Rick?
Revision history for this message
| OpenStack Infra (hudson-openstack) wrote : | # |
Attempt to merge into lp:nova failed due to conflicts:
text conflict in nova/utils.py
text conflict in nova/virt/fake.py
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
| 1 | === modified file 'nova/api/openstack/views/servers.py' |
| 2 | --- nova/api/openstack/views/servers.py 2011-09-09 13:48:38 +0000 |
| 3 | +++ nova/api/openstack/views/servers.py 2011-09-21 21:22:24 +0000 |
| 4 | @@ -137,11 +137,11 @@ |
| 5 | response = super(ViewBuilderV11, self)._build_detail(inst) |
| 6 | response['server']['created'] = utils.isotime(inst['created_at']) |
| 7 | response['server']['updated'] = utils.isotime(inst['updated_at']) |
| 8 | - if 'status' in response['server']: |
| 9 | - if response['server']['status'] == "ACTIVE": |
| 10 | - response['server']['progress'] = 100 |
| 11 | - elif response['server']['status'] == "BUILD": |
| 12 | - response['server']['progress'] = 0 |
| 13 | + |
| 14 | + status = response['server'].get('status') |
| 15 | + if status in ('ACTIVE', 'BUILD', 'REBUILD', 'RESIZE', |
| 16 | + 'VERIFY_RESIZE'): |
| 17 | + response['server']['progress'] = inst['progress'] or 0 |
| 18 | |
| 19 | response['server']['accessIPv4'] = inst.get('access_ip_v4') or "" |
| 20 | response['server']['accessIPv6'] = inst.get('access_ip_v6') or "" |
| 21 | |
| 22 | === modified file 'nova/compute/api.py' |
| 23 | --- nova/compute/api.py 2011-09-21 20:17:05 +0000 |
| 24 | +++ nova/compute/api.py 2011-09-21 21:22:24 +0000 |
| 25 | @@ -846,7 +846,8 @@ |
| 26 | |
| 27 | self.update(context, |
| 28 | instance_id, |
| 29 | - task_state=task_states.DELETING) |
| 30 | + task_state=task_states.DELETING, |
| 31 | + progress=0) |
| 32 | |
| 33 | host = instance['host'] |
| 34 | if host: |
| 35 | @@ -869,7 +870,8 @@ |
| 36 | instance_id, |
| 37 | vm_state=vm_states.ACTIVE, |
| 38 | task_state=task_states.STOPPING, |
| 39 | - terminated_at=utils.utcnow()) |
| 40 | + terminated_at=utils.utcnow(), |
| 41 | + progress=0) |
| 42 | |
| 43 | host = instance['host'] |
| 44 | if host: |
| 45 | @@ -1169,7 +1171,8 @@ |
| 46 | display_name=name, |
| 47 | image_ref=image_href, |
| 48 | vm_state=vm_states.ACTIVE, |
| 49 | - task_state=task_states.REBUILDING) |
| 50 | + task_state=task_states.REBUILDING, |
| 51 | + progress=0) |
| 52 | |
| 53 | rebuild_params = { |
| 54 | "new_pass": admin_password, |
| 55 | |
| 56 | === modified file 'nova/compute/manager.py' |
| 57 | --- nova/compute/manager.py 2011-09-21 15:54:30 +0000 |
| 58 | +++ nova/compute/manager.py 2011-09-21 21:22:24 +0000 |
| 59 | @@ -882,7 +882,9 @@ |
| 60 | migration_ref.instance_uuid) |
| 61 | |
| 62 | network_info = self._get_instance_nw_info(context, instance_ref) |
| 63 | - self.driver.destroy(instance_ref, network_info) |
| 64 | + self.driver.confirm_migration( |
| 65 | + migration_ref, instance_ref, network_info) |
| 66 | + |
| 67 | usage_info = utils.usage_from_instance(instance_ref) |
| 68 | notifier.notify('compute.%s' % self.host, |
| 69 | 'compute.instance.resize.confirm', |
| 70 | @@ -937,7 +939,7 @@ |
| 71 | local_gb=instance_type['local_gb'], |
| 72 | instance_type_id=instance_type['id']) |
| 73 | |
| 74 | - self.driver.revert_migration(instance_ref) |
| 75 | + self.driver.finish_revert_migration(instance_ref) |
| 76 | self.db.migration_update(context, migration_id, |
| 77 | {'status': 'reverted'}) |
| 78 | usage_info = utils.usage_from_instance(instance_ref) |
| 79 | @@ -961,7 +963,8 @@ |
| 80 | # of the instance down |
| 81 | instance_ref = self.db.instance_get_by_uuid(context, instance_id) |
| 82 | |
| 83 | - if instance_ref['host'] == FLAGS.host: |
| 84 | + same_host = instance_ref['host'] == FLAGS.host |
| 85 | + if same_host and not FLAGS.allow_resize_to_same_host: |
| 86 | self._instance_update(context, |
| 87 | instance_id, |
| 88 | vm_state=vm_states.ERROR) |
| 89 | @@ -1012,7 +1015,7 @@ |
| 90 | {'status': 'migrating'}) |
| 91 | |
| 92 | disk_info = self.driver.migrate_disk_and_power_off( |
| 93 | - instance_ref, migration_ref['dest_host']) |
| 94 | + context, instance_ref, migration_ref['dest_host']) |
| 95 | self.db.migration_update(context, |
| 96 | migration_id, |
| 97 | {'status': 'post-migrating'}) |
| 98 | @@ -1057,8 +1060,8 @@ |
| 99 | instance_ref.uuid) |
| 100 | |
| 101 | network_info = self._get_instance_nw_info(context, instance_ref) |
| 102 | - self.driver.finish_migration(context, instance_ref, disk_info, |
| 103 | - network_info, resize_instance) |
| 104 | + self.driver.finish_migration(context, migration_ref, instance_ref, |
| 105 | + disk_info, network_info, resize_instance) |
| 106 | |
| 107 | self._instance_update(context, |
| 108 | instance_id, |
| 109 | |
| 110 | === added file 'nova/db/sqlalchemy/migrate_repo/versions/049_add_instances_progress.py' |
| 111 | --- nova/db/sqlalchemy/migrate_repo/versions/049_add_instances_progress.py 1970-01-01 00:00:00 +0000 |
| 112 | +++ nova/db/sqlalchemy/migrate_repo/versions/049_add_instances_progress.py 2011-09-21 21:22:24 +0000 |
| 113 | @@ -0,0 +1,43 @@ |
| 114 | +# vim: tabstop=4 shiftwidth=4 softtabstop=4 |
| 115 | + |
| 116 | +# Copyright 2010 OpenStack LLC. |
| 117 | +# |
| 118 | +# Licensed under the Apache License, Version 2.0 (the "License"); you may |
| 119 | +# not use this file except in compliance with the License. You may obtain |
| 120 | +# a copy of the License at |
| 121 | +# |
| 122 | +# http://www.apache.org/licenses/LICENSE-2.0 |
| 123 | +# |
| 124 | +# Unless required by applicable law or agreed to in writing, software |
| 125 | +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT |
| 126 | +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the |
| 127 | +# License for the specific language governing permissions and limitations |
| 128 | +# under the License. |
| 129 | + |
| 130 | +from sqlalchemy import * |
| 131 | +from migrate import * |
| 132 | + |
| 133 | +from nova import log as logging |
| 134 | + |
| 135 | +meta = MetaData() |
| 136 | + |
| 137 | +instances = Table('instances', meta, |
| 138 | + Column("id", Integer(), primary_key=True, nullable=False)) |
| 139 | + |
| 140 | +# Add progress column to instances table |
| 141 | +progress = Column('progress', Integer()) |
| 142 | + |
| 143 | + |
| 144 | +def upgrade(migrate_engine): |
| 145 | + meta.bind = migrate_engine |
| 146 | + |
| 147 | + try: |
| 148 | + instances.create_column(progress) |
| 149 | + except Exception: |
| 150 | + logging.error(_("progress column not added to instances table")) |
| 151 | + raise |
| 152 | + |
| 153 | + |
| 154 | +def downgrade(migrate_engine): |
| 155 | + meta.bind = migrate_engine |
| 156 | + instances.drop_column(progress) |
| 157 | |
| 158 | === modified file 'nova/db/sqlalchemy/models.py' |
| 159 | --- nova/db/sqlalchemy/models.py 2011-09-21 12:04:37 +0000 |
| 160 | +++ nova/db/sqlalchemy/models.py 2011-09-21 21:22:24 +0000 |
| 161 | @@ -241,6 +241,8 @@ |
| 162 | access_ip_v4 = Column(String(255)) |
| 163 | access_ip_v6 = Column(String(255)) |
| 164 | |
| 165 | + progress = Column(Integer) |
| 166 | + |
| 167 | |
| 168 | class VirtualStorageArray(BASE, NovaBase): |
| 169 | """ |
| 170 | |
| 171 | === modified file 'nova/flags.py' |
| 172 | --- nova/flags.py 2011-09-20 17:41:23 +0000 |
| 173 | +++ nova/flags.py 2011-09-21 21:22:24 +0000 |
| 174 | @@ -434,3 +434,7 @@ |
| 175 | 'nova.compute.api:nova.notifier.api.notify_decorator'], |
| 176 | 'Module list representing monkey ' |
| 177 | 'patched module and decorator') |
| 178 | + |
| 179 | +DEFINE_bool('allow_resize_to_same_host', False, |
| 180 | + 'Allow destination machine to match source for resize. Useful' |
| 181 | + ' when testing in environments with only one host machine.') |
| 182 | |
| 183 | === modified file 'nova/tests/api/openstack/contrib/test_createserverext.py' |
| 184 | --- nova/tests/api/openstack/contrib/test_createserverext.py 2011-09-09 01:30:21 +0000 |
| 185 | +++ nova/tests/api/openstack/contrib/test_createserverext.py 2011-09-21 21:22:24 +0000 |
| 186 | @@ -1,4 +1,4 @@ |
| 187 | -# vim: tabstop=4 shiftwidth=4 softtabstop=4 |
| 188 | +# vim: tabstop=5 shiftwidth=4 softtabstop=4 |
| 189 | |
| 190 | # Copyright 2010-2011 OpenStack LLC. |
| 191 | # All Rights Reserved. |
| 192 | @@ -54,6 +54,7 @@ |
| 193 | "created_at": datetime.datetime(2010, 10, 10, 12, 0, 0), |
| 194 | "updated_at": datetime.datetime(2010, 11, 11, 11, 0, 0), |
| 195 | "security_groups": [{"id": 1, "name": "test"}], |
| 196 | + "progress": 0, |
| 197 | "image_ref": 'http://foo.com/123', |
| 198 | "instance_type": {"flavorid": '124'}, |
| 199 | } |
| 200 | @@ -119,7 +120,8 @@ |
| 201 | 'user_id': 'fake', |
| 202 | 'project_id': 'fake', |
| 203 | 'created_at': "", |
| 204 | - 'updated_at': ""}] |
| 205 | + 'updated_at': "", |
| 206 | + 'progress': 0}] |
| 207 | |
| 208 | def set_admin_password(self, *args, **kwargs): |
| 209 | pass |
| 210 | |
| 211 | === modified file 'nova/tests/api/openstack/contrib/test_volumes.py' |
| 212 | --- nova/tests/api/openstack/contrib/test_volumes.py 2011-09-16 18:35:10 +0000 |
| 213 | +++ nova/tests/api/openstack/contrib/test_volumes.py 2011-09-21 21:22:24 +0000 |
| 214 | @@ -1,4 +1,4 @@ |
| 215 | -# Copyright 2011 Josh Durgin |
| 216 | +# Copyright 2013 Josh Durgin |
| 217 | # All Rights Reserved. |
| 218 | # |
| 219 | # Licensed under the Apache License, Version 2.0 (the "License"); you may |
| 220 | @@ -43,6 +43,7 @@ |
| 221 | 'project_id': 'fake', |
| 222 | 'created_at': datetime.datetime(2010, 10, 10, 12, 0, 0), |
| 223 | 'updated_at': datetime.datetime(2010, 11, 11, 11, 0, 0), |
| 224 | + 'progress': 0 |
| 225 | }] |
| 226 | |
| 227 | |
| 228 | |
| 229 | === modified file 'nova/tests/api/openstack/test_server_actions.py' |
| 230 | --- nova/tests/api/openstack/test_server_actions.py 2011-09-06 11:31:39 +0000 |
| 231 | +++ nova/tests/api/openstack/test_server_actions.py 2011-09-21 21:22:24 +0000 |
| 232 | @@ -91,6 +91,7 @@ |
| 233 | "access_ip_v6": "", |
| 234 | "uuid": "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", |
| 235 | "virtual_interfaces": [], |
| 236 | + "progress": 0, |
| 237 | } |
| 238 | |
| 239 | instance["fixed_ips"] = { |
| 240 | |
| 241 | === modified file 'nova/tests/api/openstack/test_servers.py' |
| 242 | --- nova/tests/api/openstack/test_servers.py 2011-09-21 15:54:30 +0000 |
| 243 | +++ nova/tests/api/openstack/test_servers.py 2011-09-21 21:22:24 +0000 |
| 244 | @@ -162,7 +162,7 @@ |
| 245 | vm_state=None, task_state=None, |
| 246 | reservation_id="", uuid=FAKE_UUID, image_ref="10", |
| 247 | flavor_id="1", interfaces=None, name=None, key_name='', |
| 248 | - access_ipv4=None, access_ipv6=None): |
| 249 | + access_ipv4=None, access_ipv6=None, progress=0): |
| 250 | metadata = [] |
| 251 | metadata.append(InstanceMetadata(key='seq', value=id)) |
| 252 | |
| 253 | @@ -222,7 +222,8 @@ |
| 254 | "access_ip_v4": access_ipv4, |
| 255 | "access_ip_v6": access_ipv6, |
| 256 | "uuid": uuid, |
| 257 | - "virtual_interfaces": interfaces} |
| 258 | + "virtual_interfaces": interfaces, |
| 259 | + "progress": progress} |
| 260 | |
| 261 | instance["fixed_ips"] = { |
| 262 | "address": private_address, |
| 263 | @@ -548,7 +549,8 @@ |
| 264 | }, |
| 265 | ] |
| 266 | new_return_server = return_server_with_attributes( |
| 267 | - interfaces=interfaces, vm_state=vm_states.ACTIVE) |
| 268 | + interfaces=interfaces, vm_state=vm_states.ACTIVE, |
| 269 | + progress=100) |
| 270 | self.stubs.Set(nova.db.api, 'instance_get', new_return_server) |
| 271 | |
| 272 | req = webob.Request.blank('/v1.1/fake/servers/1') |
| 273 | @@ -645,7 +647,7 @@ |
| 274 | ] |
| 275 | new_return_server = return_server_with_attributes( |
| 276 | interfaces=interfaces, vm_state=vm_states.ACTIVE, |
| 277 | - image_ref=image_ref, flavor_id=flavor_id) |
| 278 | + image_ref=image_ref, flavor_id=flavor_id, progress=100) |
| 279 | self.stubs.Set(nova.db.api, 'instance_get', new_return_server) |
| 280 | |
| 281 | req = webob.Request.blank('/v1.1/fake/servers/1') |
| 282 | @@ -1527,6 +1529,7 @@ |
| 283 | "created_at": datetime.datetime(2010, 10, 10, 12, 0, 0), |
| 284 | "updated_at": datetime.datetime(2010, 11, 11, 11, 0, 0), |
| 285 | "config_drive": self.config_drive, |
| 286 | + "progress": 0 |
| 287 | } |
| 288 | |
| 289 | def server_update(context, id, params): |
| 290 | @@ -3819,7 +3822,8 @@ |
| 291 | "accessIPv6": "fead::1234", |
| 292 | #"address": , |
| 293 | #"floating_ips": [{"address":ip} for ip in public_addresses]} |
| 294 | - "uuid": "deadbeef-feed-edee-beef-d0ea7beefedd"} |
| 295 | + "uuid": "deadbeef-feed-edee-beef-d0ea7beefedd", |
| 296 | + "progress": 0} |
| 297 | |
| 298 | return instance |
| 299 | |
| 300 | @@ -3942,6 +3946,7 @@ |
| 301 | def test_build_server_detail_active_status(self): |
| 302 | #set the power state of the instance to running |
| 303 | self.instance['vm_state'] = vm_states.ACTIVE |
| 304 | + self.instance['progress'] = 100 |
| 305 | image_bookmark = "http://localhost/images/5" |
| 306 | flavor_bookmark = "http://localhost/flavors/1" |
| 307 | expected_server = { |
| 308 | |
| 309 | === modified file 'nova/tests/test_compute.py' |
| 310 | --- nova/tests/test_compute.py 2011-09-19 21:53:17 +0000 |
| 311 | +++ nova/tests/test_compute.py 2011-09-21 21:22:24 +0000 |
| 312 | @@ -584,7 +584,7 @@ |
| 313 | pass |
| 314 | |
| 315 | self.stubs.Set(self.compute.driver, 'finish_migration', fake) |
| 316 | - self.stubs.Set(self.compute.driver, 'revert_migration', fake) |
| 317 | + self.stubs.Set(self.compute.driver, 'finish_revert_migration', fake) |
| 318 | self.stubs.Set(self.compute.network_api, 'get_instance_nw_info', fake) |
| 319 | |
| 320 | self.compute.run_instance(self.context, instance_id) |
| 321 | |
| 322 | === modified file 'nova/tests/test_virt_drivers.py' |
| 323 | --- nova/tests/test_virt_drivers.py 2011-09-15 19:09:14 +0000 |
| 324 | +++ nova/tests/test_virt_drivers.py 2011-09-21 21:22:24 +0000 |
| 325 | @@ -185,7 +185,8 @@ |
| 326 | instance_ref = test_utils.get_test_instance() |
| 327 | network_info = test_utils.get_test_network_info() |
| 328 | self.connection.spawn(self.ctxt, instance_ref, network_info) |
| 329 | - self.connection.migrate_disk_and_power_off(instance_ref, 'dest_host') |
| 330 | + self.connection.migrate_disk_and_power_off( |
| 331 | + self.ctxt, instance_ref, 'dest_host') |
| 332 | |
| 333 | @catch_notimplementederror |
| 334 | def test_pause(self): |
| 335 | |
| 336 | === modified file 'nova/tests/test_xenapi.py' |
| 337 | --- nova/tests/test_xenapi.py 2011-09-13 20:33:34 +0000 |
| 338 | +++ nova/tests/test_xenapi.py 2011-09-21 21:22:24 +0000 |
| 339 | @@ -76,7 +76,7 @@ |
| 340 | db_fakes.stub_out_db_instance_api(self.stubs) |
| 341 | stubs.stub_out_get_target(self.stubs) |
| 342 | xenapi_fake.reset() |
| 343 | - self.values = {'id': 1, |
| 344 | + self.instance_values = {'id': 1, |
| 345 | 'project_id': self.user_id, |
| 346 | 'user_id': 'fake', |
| 347 | 'image_ref': 1, |
| 348 | @@ -132,7 +132,7 @@ |
| 349 | stubs.stubout_session(self.stubs, stubs.FakeSessionForVolumeTests) |
| 350 | conn = xenapi_conn.get_connection(False) |
| 351 | volume = self._create_volume() |
| 352 | - instance = db.instance_create(self.context, self.values) |
| 353 | + instance = db.instance_create(self.context, self.instance_values) |
| 354 | vm = xenapi_fake.create_vm(instance.name, 'Running') |
| 355 | result = conn.attach_volume(instance.name, volume['id'], '/dev/sdc') |
| 356 | |
| 357 | @@ -152,7 +152,7 @@ |
| 358 | stubs.FakeSessionForVolumeFailedTests) |
| 359 | conn = xenapi_conn.get_connection(False) |
| 360 | volume = self._create_volume() |
| 361 | - instance = db.instance_create(self.context, self.values) |
| 362 | + instance = db.instance_create(self.context, self.instance_values) |
| 363 | xenapi_fake.create_vm(instance.name, 'Running') |
| 364 | self.assertRaises(Exception, |
| 365 | conn.attach_volume, |
| 366 | @@ -369,7 +369,7 @@ |
| 367 | create_record=True, empty_dns=False): |
| 368 | stubs.stubout_loopingcall_start(self.stubs) |
| 369 | if create_record: |
| 370 | - values = {'id': instance_id, |
| 371 | + instance_values = {'id': instance_id, |
| 372 | 'project_id': self.project_id, |
| 373 | 'user_id': self.user_id, |
| 374 | 'image_ref': image_ref, |
| 375 | @@ -379,7 +379,7 @@ |
| 376 | 'os_type': os_type, |
| 377 | 'hostname': hostname, |
| 378 | 'architecture': architecture} |
| 379 | - instance = db.instance_create(self.context, values) |
| 380 | + instance = db.instance_create(self.context, instance_values) |
| 381 | else: |
| 382 | instance = db.instance_get(self.context, instance_id) |
| 383 | network_info = [({'bridge': 'fa0', 'id': 0, 'injected': True}, |
| 384 | @@ -623,28 +623,28 @@ |
| 385 | # Ensure that it will not unrescue a non-rescued instance. |
| 386 | self.assertRaises(Exception, conn.unrescue, instance, None) |
| 387 | |
| 388 | - def test_revert_migration(self): |
| 389 | + def test_finish_revert_migration(self): |
| 390 | instance = self._create_instance() |
| 391 | |
| 392 | class VMOpsMock(): |
| 393 | |
| 394 | def __init__(self): |
| 395 | - self.revert_migration_called = False |
| 396 | + self.finish_revert_migration_called = False |
| 397 | |
| 398 | - def revert_migration(self, instance): |
| 399 | - self.revert_migration_called = True |
| 400 | + def finish_revert_migration(self, instance): |
| 401 | + self.finish_revert_migration_called = True |
| 402 | |
| 403 | stubs.stubout_session(self.stubs, stubs.FakeSessionForMigrationTests) |
| 404 | |
| 405 | conn = xenapi_conn.get_connection(False) |
| 406 | conn._vmops = VMOpsMock() |
| 407 | - conn.revert_migration(instance) |
| 408 | - self.assertTrue(conn._vmops.revert_migration_called) |
| 409 | + conn.finish_revert_migration(instance) |
| 410 | + self.assertTrue(conn._vmops.finish_revert_migration_called) |
| 411 | |
| 412 | def _create_instance(self, instance_id=1, spawn=True): |
| 413 | """Creates and spawns a test instance.""" |
| 414 | stubs.stubout_loopingcall_start(self.stubs) |
| 415 | - values = { |
| 416 | + instance_values = { |
| 417 | 'id': instance_id, |
| 418 | 'project_id': self.project_id, |
| 419 | 'user_id': self.user_id, |
| 420 | @@ -654,7 +654,7 @@ |
| 421 | 'instance_type_id': '3', # m1.large |
| 422 | 'os_type': 'linux', |
| 423 | 'architecture': 'x86-64'} |
| 424 | - instance = db.instance_create(self.context, values) |
| 425 | + instance = db.instance_create(self.context, instance_values) |
| 426 | network_info = [({'bridge': 'fa0', 'id': 0, 'injected': False}, |
| 427 | {'broadcast': '192.168.0.255', |
| 428 | 'dns': ['192.168.0.1'], |
| 429 | @@ -732,7 +732,7 @@ |
| 430 | self.user_id = 'fake' |
| 431 | self.project_id = 'fake' |
| 432 | self.context = context.RequestContext(self.user_id, self.project_id) |
| 433 | - self.values = {'id': 1, |
| 434 | + self.instance_values = {'id': 1, |
| 435 | 'project_id': self.project_id, |
| 436 | 'user_id': self.user_id, |
| 437 | 'image_ref': 1, |
| 438 | @@ -743,22 +743,34 @@ |
| 439 | 'os_type': 'linux', |
| 440 | 'architecture': 'x86-64'} |
| 441 | |
| 442 | + migration_values = { |
| 443 | + 'source_compute': 'nova-compute', |
| 444 | + 'dest_compute': 'nova-compute', |
| 445 | + 'dest_host': '10.127.5.114', |
| 446 | + 'status': 'post-migrating', |
| 447 | + 'instance_uuid': '15f23e6a-cc6e-4d22-b651-d9bdaac316f7', |
| 448 | + 'old_instance_type_id': 5, |
| 449 | + 'new_instance_type_id': 1 |
| 450 | + } |
| 451 | + self.migration = db.migration_create( |
| 452 | + context.get_admin_context(), migration_values) |
| 453 | + |
| 454 | fake_utils.stub_out_utils_execute(self.stubs) |
| 455 | stubs.stub_out_migration_methods(self.stubs) |
| 456 | stubs.stubout_get_this_vm_uuid(self.stubs) |
| 457 | glance_stubs.stubout_glance_client(self.stubs) |
| 458 | |
| 459 | def test_migrate_disk_and_power_off(self): |
| 460 | - instance = db.instance_create(self.context, self.values) |
| 461 | + instance = db.instance_create(self.context, self.instance_values) |
| 462 | stubs.stubout_session(self.stubs, stubs.FakeSessionForMigrationTests) |
| 463 | conn = xenapi_conn.get_connection(False) |
| 464 | - conn.migrate_disk_and_power_off(instance, '127.0.0.1') |
| 465 | + conn.migrate_disk_and_power_off(self.context, instance, '127.0.0.1') |
| 466 | |
| 467 | def test_revert_migrate(self): |
| 468 | - instance = db.instance_create(self.context, self.values) |
| 469 | + instance = db.instance_create(self.context, self.instance_values) |
| 470 | self.called = False |
| 471 | self.fake_vm_start_called = False |
| 472 | - self.fake_revert_migration_called = False |
| 473 | + self.fake_finish_revert_migration_called = False |
| 474 | |
| 475 | def fake_vm_start(*args, **kwargs): |
| 476 | self.fake_vm_start_called = True |
| 477 | @@ -766,13 +778,14 @@ |
| 478 | def fake_vdi_resize(*args, **kwargs): |
| 479 | self.called = True |
| 480 | |
| 481 | - def fake_revert_migration(*args, **kwargs): |
| 482 | - self.fake_revert_migration_called = True |
| 483 | + def fake_finish_revert_migration(*args, **kwargs): |
| 484 | + self.fake_finish_revert_migration_called = True |
| 485 | |
| 486 | self.stubs.Set(stubs.FakeSessionForMigrationTests, |
| 487 | "VDI_resize_online", fake_vdi_resize) |
| 488 | self.stubs.Set(vmops.VMOps, '_start', fake_vm_start) |
| 489 | - self.stubs.Set(vmops.VMOps, 'revert_migration', fake_revert_migration) |
| 490 | + self.stubs.Set(vmops.VMOps, 'finish_revert_migration', |
| 491 | + fake_finish_revert_migration) |
| 492 | |
| 493 | stubs.stubout_session(self.stubs, stubs.FakeSessionForMigrationTests) |
| 494 | stubs.stubout_loopingcall_start(self.stubs) |
| 495 | @@ -791,17 +804,17 @@ |
| 496 | 'label': 'fake', |
| 497 | 'mac': 'DE:AD:BE:EF:00:00', |
| 498 | 'rxtx_cap': 3})] |
| 499 | - conn.finish_migration(self.context, instance, |
| 500 | + conn.finish_migration(self.context, self.migration, instance, |
| 501 | dict(base_copy='hurr', cow='durr'), |
| 502 | network_info, resize_instance=True) |
| 503 | self.assertEqual(self.called, True) |
| 504 | self.assertEqual(self.fake_vm_start_called, True) |
| 505 | |
| 506 | - conn.revert_migration(instance) |
| 507 | - self.assertEqual(self.fake_revert_migration_called, True) |
| 508 | + conn.finish_revert_migration(instance) |
| 509 | + self.assertEqual(self.fake_finish_revert_migration_called, True) |
| 510 | |
| 511 | def test_finish_migrate(self): |
| 512 | - instance = db.instance_create(self.context, self.values) |
| 513 | + instance = db.instance_create(self.context, self.instance_values) |
| 514 | self.called = False |
| 515 | self.fake_vm_start_called = False |
| 516 | |
| 517 | @@ -832,7 +845,7 @@ |
| 518 | 'label': 'fake', |
| 519 | 'mac': 'DE:AD:BE:EF:00:00', |
| 520 | 'rxtx_cap': 3})] |
| 521 | - conn.finish_migration(self.context, instance, |
| 522 | + conn.finish_migration(self.context, self.migration, instance, |
| 523 | dict(base_copy='hurr', cow='durr'), |
| 524 | network_info, resize_instance=True) |
| 525 | self.assertEqual(self.called, True) |
| 526 | @@ -841,8 +854,9 @@ |
| 527 | def test_finish_migrate_no_local_storage(self): |
| 528 | tiny_type_id = \ |
| 529 | instance_types.get_instance_type_by_name('m1.tiny')['id'] |
| 530 | - self.values.update({'instance_type_id': tiny_type_id, 'local_gb': 0}) |
| 531 | - instance = db.instance_create(self.context, self.values) |
| 532 | + self.instance_values.update({'instance_type_id': tiny_type_id, |
| 533 | + 'local_gb': 0}) |
| 534 | + instance = db.instance_create(self.context, self.instance_values) |
| 535 | |
| 536 | def fake_vdi_resize(*args, **kwargs): |
| 537 | raise Exception("This shouldn't be called") |
| 538 | @@ -866,12 +880,12 @@ |
| 539 | 'label': 'fake', |
| 540 | 'mac': 'DE:AD:BE:EF:00:00', |
| 541 | 'rxtx_cap': 3})] |
| 542 | - conn.finish_migration(self.context, instance, |
| 543 | + conn.finish_migration(self.context, self.migration, instance, |
| 544 | dict(base_copy='hurr', cow='durr'), |
| 545 | network_info, resize_instance=True) |
| 546 | |
| 547 | def test_finish_migrate_no_resize_vdi(self): |
| 548 | - instance = db.instance_create(self.context, self.values) |
| 549 | + instance = db.instance_create(self.context, self.instance_values) |
| 550 | |
| 551 | def fake_vdi_resize(*args, **kwargs): |
| 552 | raise Exception("This shouldn't be called") |
| 553 | @@ -897,7 +911,7 @@ |
| 554 | 'rxtx_cap': 3})] |
| 555 | |
| 556 | # Resize instance would be determined by the compute call |
| 557 | - conn.finish_migration(self.context, instance, |
| 558 | + conn.finish_migration(self.context, self.migration, instance, |
| 559 | dict(base_copy='hurr', cow='durr'), |
| 560 | network_info, resize_instance=False) |
| 561 | |
| 562 | |
| 563 | === modified file 'nova/tests/xenapi/stubs.py' |
| 564 | --- nova/tests/xenapi/stubs.py 2011-08-28 19:11:14 +0000 |
| 565 | +++ nova/tests/xenapi/stubs.py 2011-09-21 21:22:24 +0000 |
| 566 | @@ -295,9 +295,12 @@ |
| 567 | vm['is_control_domain'] = False |
| 568 | vm['domid'] = random.randrange(1, 1 << 16) |
| 569 | |
| 570 | + def VM_set_name_label(self, *args): |
| 571 | + pass |
| 572 | + |
| 573 | |
| 574 | def stub_out_migration_methods(stubs): |
| 575 | - def fake_get_snapshot(self, instance): |
| 576 | + def fake_create_snapshot(self, instance): |
| 577 | return 'vm_ref', dict(image='foo', snap='bar') |
| 578 | |
| 579 | @classmethod |
| 580 | @@ -327,7 +330,7 @@ |
| 581 | stubs.Set(vmops.VMOps, '_destroy', fake_destroy) |
| 582 | stubs.Set(vm_utils.VMHelper, 'scan_default_sr', fake_sr) |
| 583 | stubs.Set(vm_utils.VMHelper, 'scan_sr', fake_sr) |
| 584 | - stubs.Set(vmops.VMOps, '_get_snapshot', fake_get_snapshot) |
| 585 | + stubs.Set(vmops.VMOps, '_create_snapshot', fake_create_snapshot) |
| 586 | stubs.Set(vm_utils.VMHelper, 'get_vdi_for_vm_safely', fake_get_vdi) |
| 587 | stubs.Set(xenapi_conn.XenAPISession, 'wait_for_task', lambda x, y, z: None) |
| 588 | stubs.Set(vm_utils.VMHelper, 'get_sr_path', fake_get_sr_path) |
| 589 | |
| 590 | === modified file 'nova/utils.py' |
| 591 | --- nova/utils.py 2011-09-20 20:21:06 +0000 |
| 592 | +++ nova/utils.py 2011-09-21 21:22:24 +0000 |
| 593 | @@ -912,6 +912,20 @@ |
| 594 | return [{label: x} for x in lst] |
| 595 | |
| 596 | |
| 597 | +def timefunc(func): |
| 598 | + """Decorator that logs how long a particular function took to execute""" |
| 599 | + @functools.wraps(func) |
| 600 | + def inner(*args, **kwargs): |
| 601 | + start_time = time.time() |
| 602 | + try: |
| 603 | + return func(*args, **kwargs) |
| 604 | + finally: |
| 605 | + total_time = time.time() - start_time |
| 606 | + LOG.debug(_("timefunc: '%(name)s' took %(total_time).2f secs") % |
| 607 | + dict(name=func.__name__, total_time=total_time)) |
| 608 | + return inner |
| 609 | + |
| 610 | + |
| 611 | def generate_glance_url(): |
| 612 | """Generate the URL to glance.""" |
| 613 | # TODO(jk0): This will eventually need to take SSL into consideration |
| 614 | |
| 615 | === modified file 'nova/virt/driver.py' |
| 616 | --- nova/virt/driver.py 2011-09-21 15:54:30 +0000 |
| 617 | +++ nova/virt/driver.py 2011-09-21 21:22:24 +0000 |
| 618 | @@ -225,12 +225,11 @@ |
| 619 | """ |
| 620 | raise NotImplementedError() |
| 621 | |
| 622 | - def migrate_disk_and_power_off(self, instance, dest): |
| 623 | + def migrate_disk_and_power_off(self, context, instance, dest): |
| 624 | """ |
| 625 | Transfers the disk of a running instance in multiple phases, turning |
| 626 | off the instance before the end. |
| 627 | """ |
| 628 | - # TODO(Vek): Need to pass context in for access to auth_token |
| 629 | raise NotImplementedError() |
| 630 | |
| 631 | def snapshot(self, context, instance, image_id): |
| 632 | @@ -244,8 +243,8 @@ |
| 633 | """ |
| 634 | raise NotImplementedError() |
| 635 | |
| 636 | - def finish_migration(self, context, instance, disk_info, network_info, |
| 637 | - resize_instance): |
| 638 | + def finish_migration(self, context, migration, instance, disk_info, |
| 639 | + network_info, resize_instance): |
| 640 | """Completes a resize, turning on the migrated instance |
| 641 | |
| 642 | :param network_info: |
| 643 | @@ -253,8 +252,13 @@ |
| 644 | """ |
| 645 | raise NotImplementedError() |
| 646 | |
| 647 | - def revert_migration(self, instance): |
| 648 | - """Reverts a resize, powering back on the instance""" |
| 649 | + def confirm_migration(self, migration, instance, network_info): |
| 650 | + """Confirms a resize, destroying the source VM""" |
| 651 | + # TODO(Vek): Need to pass context in for access to auth_token |
| 652 | + raise NotImplementedError() |
| 653 | + |
| 654 | + def finish_revert_migration(self, instance): |
| 655 | + """Finish reverting a resize, powering back on the instance""" |
| 656 | # TODO(Vek): Need to pass context in for access to auth_token |
| 657 | raise NotImplementedError() |
| 658 | |
| 659 | |
| 660 | === modified file 'nova/virt/fake.py' |
| 661 | --- nova/virt/fake.py 2011-09-15 18:44:49 +0000 |
| 662 | +++ nova/virt/fake.py 2011-09-21 21:22:24 +0000 |
| 663 | @@ -130,12 +130,12 @@ |
| 664 | def poll_rescued_instances(self, timeout): |
| 665 | pass |
| 666 | |
| 667 | + def migrate_disk_and_power_off(self, context, instance, dest): |
| 668 | + pass |
| 669 | + |
| 670 | def poll_unconfirmed_resizes(self, resize_confirm_window): |
| 671 | pass |
| 672 | |
| 673 | - def migrate_disk_and_power_off(self, instance, dest): |
| 674 | - pass |
| 675 | - |
| 676 | def pause(self, instance, callback): |
| 677 | pass |
| 678 | |
| 679 | |
| 680 | === modified file 'nova/virt/xenapi/vm_utils.py' |
| 681 | --- nova/virt/xenapi/vm_utils.py 2011-09-10 17:56:54 +0000 |
| 682 | +++ nova/virt/xenapi/vm_utils.py 2011-09-21 21:22:24 +0000 |
| 683 | @@ -695,6 +695,10 @@ |
| 684 | return is_pv |
| 685 | |
| 686 | @classmethod |
| 687 | + def set_vm_name_label(cls, session, vm_ref, name_label): |
| 688 | + session.get_xenapi().VM.set_name_label(vm_ref, name_label) |
| 689 | + |
| 690 | + @classmethod |
| 691 | def lookup(cls, session, name_label): |
| 692 | """Look the instance i up, and returns it if available""" |
| 693 | vm_refs = session.get_xenapi().VM.get_by_name_label(name_label) |
| 694 | |
| 695 | === modified file 'nova/virt/xenapi/vmops.py' |
| 696 | --- nova/virt/xenapi/vmops.py 2011-09-21 15:54:30 +0000 |
| 697 | +++ nova/virt/xenapi/vmops.py 2011-09-21 21:22:24 +0000 |
| 698 | @@ -56,6 +56,9 @@ |
| 699 | 'nova.virt.xenapi.vif.XenAPIBridgeDriver', |
| 700 | 'The XenAPI VIF driver using XenServer Network APIs.') |
| 701 | |
| 702 | +RESIZE_TOTAL_STEPS = 5 |
| 703 | +BUILD_TOTAL_STEPS = 4 |
| 704 | + |
| 705 | |
| 706 | def cmp_version(a, b): |
| 707 | """Compare two version strings (eg 0.0.1.10 > 0.0.1.9)""" |
| 708 | @@ -111,20 +114,40 @@ |
| 709 | instance_infos.append(instance_info) |
| 710 | return instance_infos |
| 711 | |
| 712 | - def revert_migration(self, instance): |
| 713 | - vm_ref = VMHelper.lookup(self._session, instance.name) |
| 714 | + def confirm_migration(self, migration, instance, network_info): |
| 715 | + name_label = self._get_orig_vm_name_label(instance) |
| 716 | + vm_ref = VMHelper.lookup(self._session, name_label) |
| 717 | + return self._destroy(instance, vm_ref, network_info, shutdown=False) |
| 718 | + |
| 719 | + def finish_revert_migration(self, instance): |
| 720 | + # NOTE(sirp): the original vm was suffixed with '-orig'; find it using |
| 721 | + # the old suffix, remove the suffix, then power it back on. |
| 722 | + name_label = self._get_orig_vm_name_label(instance) |
| 723 | + vm_ref = VMHelper.lookup(self._session, name_label) |
| 724 | + |
| 725 | + # Remove the '-orig' suffix (which was added in case the resized VM |
| 726 | + # ends up on the source host, common during testing) |
| 727 | + name_label = instance.name |
| 728 | + VMHelper.set_vm_name_label(self._session, vm_ref, name_label) |
| 729 | + |
| 730 | self._start(instance, vm_ref) |
| 731 | |
| 732 | - def finish_migration(self, context, instance, disk_info, network_info, |
| 733 | - resize_instance): |
| 734 | + def finish_migration(self, context, migration, instance, disk_info, |
| 735 | + network_info, resize_instance): |
| 736 | vdi_uuid = self.link_disks(instance, disk_info['base_copy'], |
| 737 | disk_info['cow']) |
| 738 | + |
| 739 | vm_ref = self._create_vm(context, instance, |
| 740 | [dict(vdi_type='os', vdi_uuid=vdi_uuid)], |
| 741 | network_info) |
| 742 | if resize_instance: |
| 743 | self.resize_instance(instance, vdi_uuid) |
| 744 | + |
| 745 | + # 5. Start VM |
| 746 | self._start(instance, vm_ref=vm_ref) |
| 747 | + self._update_instance_progress(context, instance, |
| 748 | + step=5, |
| 749 | + total_steps=RESIZE_TOTAL_STEPS) |
| 750 | |
| 751 | def _start(self, instance, vm_ref=None): |
| 752 | """Power on a VM instance""" |
| 753 | @@ -147,9 +170,35 @@ |
| 754 | def spawn(self, context, instance, network_info): |
| 755 | vdis = None |
| 756 | try: |
| 757 | + # 1. Vanity Step |
| 758 | + # NOTE(sirp): _create_disk will potentially take a *very* long |
| 759 | + # time to complete since it has to fetch the image over the |
| 760 | + # network and images can be several gigs in size. To avoid |
| 761 | + # progress remaining at 0% for too long, which will appear to be |
| 762 | + # an error, we insert a "vanity" step to bump the progress up one |
| 763 | + # notch above 0. |
| 764 | + self._update_instance_progress(context, instance, |
| 765 | + step=1, |
| 766 | + total_steps=BUILD_TOTAL_STEPS) |
| 767 | + |
| 768 | + # 2. Fetch the Image over the Network |
| 769 | vdis = self._create_disks(context, instance) |
| 770 | + self._update_instance_progress(context, instance, |
| 771 | + step=2, |
| 772 | + total_steps=BUILD_TOTAL_STEPS) |
| 773 | + |
| 774 | + # 3. Create the VM records |
| 775 | vm_ref = self._create_vm(context, instance, vdis, network_info) |
| 776 | + self._update_instance_progress(context, instance, |
| 777 | + step=3, |
| 778 | + total_steps=BUILD_TOTAL_STEPS) |
| 779 | + |
| 780 | + # 4. Boot the Instance |
| 781 | self._spawn(instance, vm_ref) |
| 782 | + self._update_instance_progress(context, instance, |
| 783 | + step=4, |
| 784 | + total_steps=BUILD_TOTAL_STEPS) |
| 785 | + |
| 786 | except (self.XenAPI.Failure, OSError, IOError) as spawn_error: |
| 787 | LOG.exception(_("instance %s: Failed to spawn"), |
| 788 | instance.id, exc_info=sys.exc_info()) |
| 789 | @@ -169,7 +218,7 @@ |
| 790 | if vm_ref is not None: |
| 791 | raise exception.InstanceExists(name=instance_name) |
| 792 | |
| 793 | - #ensure enough free memory is available |
| 794 | + # Ensure enough free memory is available |
| 795 | if not VMHelper.ensure_free_mem(self._session, instance): |
| 796 | LOG.exception(_('instance %(instance_name)s: not enough free ' |
| 797 | 'memory') % locals()) |
| 798 | @@ -501,7 +550,8 @@ |
| 799 | """ |
| 800 | template_vm_ref = None |
| 801 | try: |
| 802 | - template_vm_ref, template_vdi_uuids = self._get_snapshot(instance) |
| 803 | + template_vm_ref, template_vdi_uuids =\ |
| 804 | + self._create_snapshot(instance) |
| 805 | # call plugin to ship snapshot off to glance |
| 806 | VMHelper.upload_image(context, |
| 807 | self._session, instance, template_vdi_uuids, image_id) |
| 808 | @@ -512,7 +562,7 @@ |
| 809 | |
| 810 | logging.debug(_("Finished snapshot and upload for VM %s"), instance) |
| 811 | |
| 812 | - def _get_snapshot(self, instance): |
| 813 | + def _create_snapshot(self, instance): |
| 814 | #TODO(sirp): Add quiesce and VSS locking support when Windows support |
| 815 | # is added |
| 816 | |
| 817 | @@ -529,7 +579,39 @@ |
| 818 | % locals()) |
| 819 | return |
| 820 | |
| 821 | - def migrate_disk_and_power_off(self, instance, dest): |
| 822 | + def _migrate_vhd(self, instance, vdi_uuid, dest, sr_path): |
| 823 | + instance_id = instance.id |
| 824 | + params = {'host': dest, |
| 825 | + 'vdi_uuid': vdi_uuid, |
| 826 | + 'instance_id': instance_id, |
| 827 | + 'sr_path': sr_path} |
| 828 | + |
| 829 | + task = self._session.async_call_plugin('migration', 'transfer_vhd', |
| 830 | + {'params': pickle.dumps(params)}) |
| 831 | + self._session.wait_for_task(task, instance_id) |
| 832 | + |
| 833 | + def _get_orig_vm_name_label(self, instance): |
| 834 | + return instance.name + '-orig' |
| 835 | + |
| 836 | + def _update_instance_progress(self, context, instance, step, total_steps): |
| 837 | + """Update instance progress percent to reflect current step number |
| 838 | + """ |
| 839 | + # FIXME(sirp): for now we're taking a KISS approach to instance |
| 840 | + # progress: |
| 841 | + # Divide the action's workflow into discrete steps and "bump" the |
| 842 | + # instance's progress field as each step is completed. |
| 843 | + # |
| 844 | + # For a first cut this should be fine, however, for large VM images, |
| 845 | + # the _create_disks step begins to dominate the equation. A |
| 846 | + # better approximation would use the percentage of the VM image that |
| 847 | + # has been streamed to the destination host. |
| 848 | + progress = round(float(step) / total_steps * 100) |
| 849 | + instance_id = instance['id'] |
| 850 | + LOG.debug(_("Updating instance '%(instance_id)s' progress to" |
| 851 | + " %(progress)d") % locals()) |
| 852 | + db.instance_update(context, instance_id, {'progress': progress}) |
| 853 | + |
| 854 | + def migrate_disk_and_power_off(self, context, instance, dest): |
| 855 | """Copies a VHD from one host machine to another. |
| 856 | |
| 857 | :param instance: the instance that owns the VHD in question. |
| 858 | @@ -537,6 +619,11 @@ |
| 859 | :param disk_type: values are 'primary' or 'cow'. |
| 860 | |
| 861 | """ |
| 862 | + # 0. Zero out the progress to begin |
| 863 | + self._update_instance_progress(context, instance, |
| 864 | + step=0, |
| 865 | + total_steps=RESIZE_TOTAL_STEPS) |
| 866 | + |
| 867 | vm_ref = VMHelper.lookup(self._session, instance.name) |
| 868 | |
| 869 | # The primary VDI becomes the COW after the snapshot, and we can |
| 870 | @@ -546,34 +633,43 @@ |
| 871 | base_copy_uuid = cow_uuid = None |
| 872 | template_vdi_uuids = template_vm_ref = None |
| 873 | try: |
| 874 | - # transfer the base copy |
| 875 | - template_vm_ref, template_vdi_uuids = self._get_snapshot(instance) |
| 876 | + # 1. Create Snapshot |
| 877 | + template_vm_ref, template_vdi_uuids =\ |
| 878 | + self._create_snapshot(instance) |
| 879 | + self._update_instance_progress(context, instance, |
| 880 | + step=1, |
| 881 | + total_steps=RESIZE_TOTAL_STEPS) |
| 882 | + |
| 883 | base_copy_uuid = template_vdi_uuids['image'] |
| 884 | vdi_ref, vm_vdi_rec = \ |
| 885 | VMHelper.get_vdi_for_vm_safely(self._session, vm_ref) |
| 886 | cow_uuid = vm_vdi_rec['uuid'] |
| 887 | |
| 888 | - params = {'host': dest, |
| 889 | - 'vdi_uuid': base_copy_uuid, |
| 890 | - 'instance_id': instance.id, |
| 891 | - 'sr_path': VMHelper.get_sr_path(self._session)} |
| 892 | - |
| 893 | - task = self._session.async_call_plugin('migration', 'transfer_vhd', |
| 894 | - {'params': pickle.dumps(params)}) |
| 895 | - self._session.wait_for_task(task, instance.id) |
| 896 | - |
| 897 | - # Now power down the instance and transfer the COW VHD |
| 898 | + sr_path = VMHelper.get_sr_path(self._session) |
| 899 | + |
| 900 | + # 2. Transfer the base copy |
| 901 | + self._migrate_vhd(instance, base_copy_uuid, dest, sr_path) |
| 902 | + self._update_instance_progress(context, instance, |
| 903 | + step=2, |
| 904 | + total_steps=RESIZE_TOTAL_STEPS) |
| 905 | + |
| 906 | + # 3. Now power down the instance |
| 907 | self._shutdown(instance, vm_ref, hard=False) |
| 908 | - |
| 909 | - params = {'host': dest, |
| 910 | - 'vdi_uuid': cow_uuid, |
| 911 | - 'instance_id': instance.id, |
| 912 | - 'sr_path': VMHelper.get_sr_path(self._session), } |
| 913 | - |
| 914 | - task = self._session.async_call_plugin('migration', 'transfer_vhd', |
| 915 | - {'params': pickle.dumps(params)}) |
| 916 | - self._session.wait_for_task(task, instance.id) |
| 917 | - |
| 918 | + self._update_instance_progress(context, instance, |
| 919 | + step=3, |
| 920 | + total_steps=RESIZE_TOTAL_STEPS) |
| 921 | + |
| 922 | + # 4. Transfer the COW VHD |
| 923 | + self._migrate_vhd(instance, cow_uuid, dest, sr_path) |
| 924 | + self._update_instance_progress(context, instance, |
| 925 | + step=4, |
| 926 | + total_steps=RESIZE_TOTAL_STEPS) |
| 927 | + |
| 928 | + # NOTE(sirp): in case we're resizing to the same host (for dev |
| 929 | + # purposes), apply a suffix to name-label so the two VM records |
| 930 | + # extant until a confirm_resize don't collide. |
| 931 | + name_label = self._get_orig_vm_name_label(instance) |
| 932 | + VMHelper.set_vm_name_label(self._session, vm_ref, name_label) |
| 933 | finally: |
| 934 | if template_vm_ref: |
| 935 | self._destroy(instance, template_vm_ref, |
| 936 | |
| 937 | === modified file 'nova/virt/xenapi_conn.py' |
| 938 | --- nova/virt/xenapi_conn.py 2011-09-21 15:54:30 +0000 |
| 939 | +++ nova/virt/xenapi_conn.py 2011-09-21 21:22:24 +0000 |
| 940 | @@ -189,14 +189,19 @@ |
| 941 | """Create VM instance""" |
| 942 | self._vmops.spawn(context, instance, network_info) |
| 943 | |
| 944 | - def revert_migration(self, instance): |
| 945 | - """Reverts a resize, powering back on the instance""" |
| 946 | - self._vmops.revert_migration(instance) |
| 947 | - |
| 948 | - def finish_migration(self, context, instance, disk_info, network_info, |
| 949 | - resize_instance=False): |
| 950 | + def confirm_migration(self, migration, instance, network_info): |
| 951 | + """Confirms a resize, destroying the source VM""" |
| 952 | + # TODO(Vek): Need to pass context in for access to auth_token |
| 953 | + self._vmops.confirm_migration(migration, instance, network_info) |
| 954 | + |
| 955 | + def finish_revert_migration(self, instance): |
| 956 | + """Finish reverting a resize, powering back on the instance""" |
| 957 | + self._vmops.finish_revert_migration(instance) |
| 958 | + |
| 959 | + def finish_migration(self, context, migration, instance, disk_info, |
| 960 | + network_info, resize_instance=False): |
| 961 | """Completes a resize, turning on the migrated instance""" |
| 962 | - self._vmops.finish_migration(context, instance, disk_info, |
| 963 | + self._vmops.finish_migration(context, migration, instance, disk_info, |
| 964 | network_info, resize_instance) |
| 965 | |
| 966 | def snapshot(self, context, instance, image_id): |
| 967 | @@ -229,10 +234,10 @@ |
| 968 | """Unpause paused VM instance""" |
| 969 | self._vmops.unpause(instance, callback) |
| 970 | |
| 971 | - def migrate_disk_and_power_off(self, instance, dest): |
| 972 | + def migrate_disk_and_power_off(self, context, instance, dest): |
| 973 | """Transfers the VHD of a running instance to another host, then shuts |
| 974 | off the instance copies over the COW disk""" |
| 975 | - return self._vmops.migrate_disk_and_power_off(instance, dest) |
| 976 | + return self._vmops.migrate_disk_and_power_off(context, instance, dest) |
| 977 | |
| 978 | def suspend(self, instance, callback): |
| 979 | """suspend the specified instance""" |
| 980 | |
| 981 | === modified file 'plugins/xenserver/xenapi/etc/xapi.d/plugins/glance' |
| 982 | --- plugins/xenserver/xenapi/etc/xapi.d/plugins/glance 2011-09-10 17:56:54 +0000 |
| 983 | +++ plugins/xenserver/xenapi/etc/xapi.d/plugins/glance 2011-09-21 21:22:24 +0000 |
| 984 | @@ -211,7 +211,11 @@ |
| 985 | snap_info = prepare_if_exists(staging_path, 'snap.vhd', |
| 986 | image_info[0]) |
| 987 | if snap_info: |
| 988 | - paths_to_move.append(snap_info[0]) |
| 989 | + # NOTE(sirp): this is an insert rather than an append since the |
| 990 | + # 'snapshot' vhd needs to be copied into the SR before the base copy. |
| 991 | + # If it doesn't, then there is a possibliity that snapwatchd will |
| 992 | + # delete the base_copy since it is an unreferenced parent. |
| 993 | + paths_to_move.insert(0, snap_info[0]) |
| 994 | # We return this snap as the VDI instead of image.vhd |
| 995 | vdi_return_list.append(dict(vdi_type="os", vdi_uuid=snap_info[1])) |
| 996 | else: |
| 997 | |
| 998 | === modified file 'plugins/xenserver/xenapi/etc/xapi.d/plugins/migration' |
| 999 | --- plugins/xenserver/xenapi/etc/xapi.d/plugins/migration 2011-07-20 15:16:36 +0000 |
| 1000 | +++ plugins/xenserver/xenapi/etc/xapi.d/plugins/migration 2011-09-21 21:22:24 +0000 |
| 1001 | @@ -48,7 +48,7 @@ |
| 1002 | |
| 1003 | # Discover the copied VHDs locally, and then set up paths to copy |
| 1004 | # them to under the SR |
| 1005 | - source_image_path = "%s/instance%d" % ('/images/', instance_id) |
| 1006 | + source_image_path = "/images/instance%d" % instance_id |
| 1007 | source_base_copy_path = "%s/%s.vhd" % (source_image_path, |
| 1008 | old_base_copy_uuid) |
| 1009 | source_cow_path = "%s/%s.vhd" % (source_image_path, old_cow_uuid) |
| 1010 | @@ -74,9 +74,12 @@ |
| 1011 | (new_cow_path, new_base_copy_path)) |
| 1012 | subprocess.call(shlex.split('/usr/sbin/vhd-util modify -n %s -p %s' % |
| 1013 | (new_cow_path, new_base_copy_path))) |
| 1014 | + |
| 1015 | logging.debug('Moving VHDs into SR %s' % sr_path) |
| 1016 | - shutil.move("%s/%s.vhd" % (temp_vhd_path, new_base_copy_uuid), sr_path) |
| 1017 | - shutil.move("%s/%s.vhd" % (temp_vhd_path, new_cow_uuid), sr_path) |
| 1018 | + # NOTE(sirp): COW should be copied before base_copy to avoid snapwatchd |
| 1019 | + # GC'ing an unreferenced base copy VDI |
| 1020 | + shutil.move(new_cow_path, sr_path) |
| 1021 | + shutil.move(new_base_copy_path, sr_path) |
| 1022 | |
| 1023 | logging.debug('Cleaning up temporary SR path %s' % temp_vhd_path) |
| 1024 | os.rmdir(temp_vhd_path) |
| 1025 | @@ -93,7 +96,7 @@ |
| 1026 | vhd_path = "%s.vhd" % vdi_uuid |
| 1027 | |
| 1028 | source_path = "%s/%s" % (sr_path, vhd_path) |
| 1029 | - dest_path = '%s:%sinstance%d/' % (host, '/images/', instance_id) |
| 1030 | + dest_path = '%s:/images/instance%d/' % (host, instance_id) |
| 1031 | |
| 1032 | logging.debug("Preparing to transmit %s to %s" % (source_path, |
| 1033 | dest_path)) |
So this looks really good from an implementation perspective. Beyond that, I really don't like that this kind of functionality is part of the API spec, which is of course not your fault. The following is a diatribe against the idea, not the patch.
Though the progress is done in a way that mostly keeps all the moving parts decoupled (since it's just a guess based on the number of expected steps rather than disk progress, as you mention,) I'm really not a fan of the false sense of security it will provide end users. Questions like "Why is my Linux instance at 25% for a quarter of the time a Windows instance is?" will pop up shortly. Moving to a model of disk progress has all sorts of other scary implications, like trying to track the copy progress through the XS plugin that is currently running (yuck) and I don't even know how you'd do it for the other hypervisors. It also opens up all kinds of synchronization issues when you start including zones and trying to bubble that data up to the API in a reasonably timely manner.
Long story short, I think including progress counters is a very strong statement regarding the type of cloud you expect it to be, and in this case, I don't think synchronous features like progress are indicative of the sort of scale we expect Nova to achieve.
If the community decides they want this, you can consider the above an "approve." I don't want to block it just because I might have a different idea of the Way Things Should Be™