| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-07-27 | |||
| 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. | |
| 15:22:49 | mriedem | i don't think you can do that | |
| 15:22:51 | mriedem | yeah | |
| 15:23:03 | fried_rice | on every periodic | |
| 15:23:11 | dansmith | okay, so anyone with MISC_SHARES now will failboat on upgrade to rocky | |
| 15:23:14 | fried_rice | update_from_provider_tree will never succeed. | |
| 15:23:23 | dansmith | and anyone that sets that on non-empty computes will stop reporting | |
| 15:23:55 | mriedem | oh right b/c upt removes the DISK_GB from the node provider if it sees it's in a sharing provider relationship | |
| 15:24:00 | dansmith | yeah | |
| 15:24:02 | fried_rice | Note that we didn't document that you could do this. | |
| 15:24:09 | mriedem | and if that DISK_GB is being used it will blow up on the remove | |
| 15:24:15 | mriedem | fried_rice: heh i know | |
| 15:24:17 | dansmith | fried_rice: and yet, it's in documentation and people have tried it, hence the bug yeah? | |
| 15:24:24 | sean-k-mooney | mriedem: in that instance. the flavor has root_gb=0 the imange is like 20MB in glance and we boot it on the dest without claim space in placement. the vm can use as much space as disk topology in the image specifies | |
| 15:24:50 | fried_rice | dansmith: The bug was opened because bhagyashri was working on it and I said it should have a bug report. | |
| 15:25:07 | mriedem | shared storage providers is definitely a feature/spec | |
| 15:25:17 | fried_rice | ...which we don't claim works yet. | |
| 15:25:19 | mriedem | given the upgrade/CI/etc | |
| 15:25:24 | mriedem | i know, but | |
| 15:25:31 | fried_rice | we should document that we *don't* support it. | |