| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-07-27 | |||
| 15:03:45 | mriedem | because we have FIXME notes all over the migration code | |
| 15:03:52 | sean-k-mooney | i guess unless we are migrating with a block migraion to a different storage provider | |
| 15:03:58 | dansmith | fried_rice: I have fixmes about it being broken and known | |
| 15:04:02 | fried_rice | yup | |
| 15:04:22 | dansmith | fried_rice: so, yeah, I'm not sure why we landed the patch to do that for inventory in that case, but.. alas | |
| 15:04:58 | fried_rice | dansmith: So that we wouldn't be double-reporting inventory allocations. | |
| 15:05:20 | fried_rice | dansmith: Can't you only migrate an instance that's on volume storage anyway? | |
| 15:05:22 | dansmith | fried_rice: right, but that has been broken since forever, and this change means we *lose* data | |
| 15:05:25 | dansmith | no | |
| 15:05:34 | fried_rice | what happens to the disk? | |
| 15:05:37 | mriedem | ssh to the dest | |
| 15:05:43 | dansmith | hah | |
| 15:05:43 | fried_rice | eek, really? | |
| 15:05:44 | dansmith | it gets migrated | |
| 15:05:48 | dansmith | either block migration or shared (non-volume) storage in teh backend | |
| 15:05:57 | fried_rice | Okay, so what are we expecting to happen here? | |
| 15:06:06 | dansmith | for live, and yeah, scp to dest for the cold migration case | |
| 15:06:10 | fried_rice | I would have thought we would ssh the data to whatever disk got allocated on the dest. | |
| 15:06:29 | dansmith | I think we need to remove that bit of the inventory logic that doesn't expose DISK_GB | |
| 15:06:39 | dansmith | so that we don't get split allocations that we trash during a migration | |
| 15:06:48 | dansmith | because we'll end up with instances with no DISK_GB allocation at all | |
| 15:06:50 | fried_rice | which may or may not be the same provider as we started on, but to a different spot on that disk - which would be something to fix later | |
| 15:06:52 | dansmith | and then start overcommitting | |
| 15:07:20 | fried_rice | I don't understand that thinking. And IMO it is premature to land a patch to yank that out until we've demonstrated that anything bad happens. | |
| 15:07:34 | dansmith | that's why I'm trying to write a test | |
| 15:07:40 | fried_rice | sounds good. | |
| 15:07:43 | fried_rice | need help? | |
| 15:08:14 | dansmith | I asked for help and now am working on using that functional test to do my bidding | |
| 15:09:22 | mriedem | i believe this is the problem https://github.com/openstack/nova/blob/master/nova/conductor/tasks/migrate.py#L48 | |
| 15:09:32 | mriedem | b/c we're assuming only allocations on the source compute node provider | |
| 15:09:34 | fried_rice | I think the worst that happens is we fail to remove the original allocation for the DISK_GB on the sharing provider. What happens after that depends on whether we migrated to a compute node with or without sharing disk. But the doubled allocation leaves us in no worse shape than we were before this fix, I would have thought. | |
| 15:09:35 | mriedem | and copy those to the migration consumer | |
| 15:09:41 | mriedem | which won't include the DISK_GB allocation on the shared provider | |
| 15:09:54 | sean-k-mooney | fried_rice: dansmith do we handel flavors with root_gb=0 in placement by the way. preplacement we jsut did not track there disk usage properly. im assuming that is stil broken | |
| 15:10:09 | mriedem | sean-k-mooney: fixed like 1 week ago | |
| 15:10:26 | mriedem | sean-k-mooney: https://review.openstack.org/#/q/topic:bug/1469179+(status:open+OR+status:merged) | |
| 15:10:28 | sean-k-mooney | mriedem: fixed by reading disk size form image? | |
| 15:10:32 | dansmith | fried_rice: and I think we lose the disk allocation silently | |
| 15:10:42 | fried_rice | dansmith: mriedem: oic, yeah, that makes sense. | |
| 15:10:45 | mriedem | sean-k-mooney: no, we don't request DISK_GB allocations for bfv | |
| 15:11:06 | sean-k-mooney | mriedem: i was thinking about the non boot form volume case | |
| 15:11:16 | fried_rice | I didn't realize we don't go through GET /a_c to request the resources on the dest. | |
| 15:11:36 | mriedem | yes this is what removes the instances allocations https://github.com/openstack/nova/blob/master/nova/conductor/tasks/migrate.py#L60 | |
| 15:11:40 | mriedem | from all providers | |
| 15:12:06 | mriedem | fried_rice: we do to pick the dest host during scheduling | |
| 15:12:20 | sean-k-mooney | mriedem: for example the nano flavor with with the cirros image in devstack with no volume for the guest. | |
| 15:12:26 | fried_rice | mriedem: GET /a_c or just GET /rps?resources=... ? | |
| 15:12:36 | mriedem | GET /a_c, | |
| 15:12:49 | mriedem | we have to do that in the scheduler to figure out which providers to filter for a dest host | |
| 15:12:51 | fried_rice | mriedem: and then ignore that result and just copy the resources from the src to the dest? | |
| 15:13:01 | mriedem | i'm looking to confirm that | |
| 15:13:09 | fried_rice | mriedem: well, you could have used GET /rps?resources=... as well | |
| 15:13:19 | mriedem | sure but we don't in the scheduler | |
| 15:13:36 | fried_rice | The right thing would be to use GET /a_c to pick the host *and* create the allocations. Then we wouldn't be having this problem. | |
| 15:13:54 | mriedem | oh you know what, | |
| 15:14:03 | mriedem | yes that's what we o | |
| 15:14:04 | mriedem | *do | |
| 15:14:28 | mriedem | we move the existing allocs from the instance on the source node to the migration record, | |
| 15:14:34 | mriedem | and then call the scheduler and claim on the dest host | |
| 15:14:42 | mriedem | so the migration has allocs on source host and instance has allocs on dest host | |
| 15:14:52 | mriedem | then on successful migration we delete the migration allocs on the source host | |
| 15:15:08 | mriedem | on failure, we delete allocs for instance on dest and move allocs from migratoin on source host to instane | |
| 15:15:09 | mriedem | *instance | |
| 15:15:17 | fried_rice | oh, so what's actually happening is we're erroneously losing the DISK_GB allocation for a minute during the migration, but picking it up again on the dest. | |
| 15:15:26 | mriedem | so we don't hit _move_operation_alloc_request in the scheduler report client | |
| 15:15:41 | cdent | dansmith: the root problem in your test is that two compute nodes are not in the aggregate, when you do the put for that it is coming up 404, so the resource providers don't exist yet, not sure why that is | |
| 15:15:51 | dansmith | cdent: ah, okay | |
| 15:16:13 | dansmith | mriedem: hmm, so we end up double-claiming on the shared provider? | |
| 15:16:41 | dansmith | mriedem: I thought even with the new accounting we had to grab the allocation for the provider in question and regenerate it, which would mean the instance's allocation on the dest wouldn't include the shared one | |
| 15:16:42 | mriedem | i don't think so...as eric said, we'll remove the allocs for the instance on the shared provider, | |
| 15:16:47 | mriedem | then claim on the dest during scheduling | |
| 15:17:26 | dansmith | because we do a full regular schedule? | |
| 15:17:28 | mriedem | i think on a revert or failed migration we'd eff that up though | |
| 15:17:30 | mriedem | yes | |
| 15:17:36 | mriedem | *EXCEPT* in the case of forced live migrate | |
| 15:17:39 | dansmith | oh you're saying we drop the disk allocation but only because we don't copy it for the migration | |
| 15:17:42 | mriedem | we don't go through the scheduler there | |
| 15:17:48 | mriedem | dansmith: yeah | |
| 15:18:10 | dansmith | so, | |
| 15:18:22 | mriedem | on a revert or failed resize, we'll delete the allocs for the instance on the dest host (created by the scheduler) and move those back from the migration to the instance, but the migration allocs won't be on the sharing provider | |
| 15:18:24 | dansmith | what happens if placement picks a different sharing provider than we had before? our disk doesn't actually move | |
| 15:18:27 | mriedem | so we'd lost the DISK_GB alocs in that case | |
| 15:18:38 | dansmith | ah, yeah, anything where we use the migration's allocations would be wrong | |
| 15:19:17 | mriedem | dansmith: yup that's this https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L4138 | |
| 15:19:33 | mriedem | well, we wouldn't hit that yet | |
| 15:19:44 | mriedem | the migration consumer will only have VCPU and MEMORY_MB allocations against the source node | |
| 15:19:52 | mriedem | so this https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L4155 | |
| 15:20:25 | mriedem | so that's definitely busted - we could easily test that with a resize revert test and verify the DISK_GB allocation for the instance is gone | |
| 15:20:41 | dansmith | and forced live | |
| 15:20:52 | mriedem | i haven't stepped through forced live yet (or evac for that matter) | |
| 15:21:06 | sean-k-mooney | dansmith: we should only be able to pick a different provider in a block migrate case correct? if we do not set that flag we should not allow the shareing provider to change | |
| 15:21:07 | dansmith | also, | |
| 15:21:26 | dansmith | migrate to an older node that doesn't have this will drop the shared disk allocation | |
| 15:21:44 | dansmith | because placement will allocate from its own disk inventory, even though it's the same pool, | |
| 15:21:49 | mriedem | sean-k-mooney: re: "for example the nano flavor with with the cirros image in devstack with no volume for the guest." i don't know what you're asking me | |
| 15:21:52 | dansmith | and then when we upgrade that node, we won't convert the allocations | |
| 15:21:54 | dansmith | in fact, | |
| 15:22:09 | dansmith | any upgrade where we boot up on rocky code and change our inventory will break all our allocations right? | |
| 15:22:32 | dansmith | fried_rice: cdent what happens if I have allocations against my disk_gb inventory and then I update my inventory with no disk_gb ? | |
| 15:22:49 | fried_rice | The inv update will bounce 409 InventoryInUse. | |