| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-10-18 | |||
| 14:58:23 | cdent | dansmith: I agree we probably don’t want multiple places doing writes | |
| 14:58:35 | cdent | but reads, I’m not ure | |
| 14:58:45 | cdent | I don’t know for certain, at this stage I’m merely speculating | |
| 14:59:03 | dansmith | we should be passing anything in that it needs, otherwise we're likely duplicating queries | |
| 14:59:16 | dansmith | mriedem: can you explain what you think you need for ironic? | |
| 14:59:26 | johnthetubaguy | https://bugs.launchpad.net/nova/+bug/1724589 | |
| 14:59:27 | openstack | Launchpad bug 1724589 in OpenStack Compute (nova) "Unable to transition to Ironic Node Resource Classes in Pike" [High,In progress] - Assigned to John Garbutt (johngarbutt) | |
| 14:59:38 | mriedem | dansmith: it's in ^ | |
| 15:00:02 | mriedem | johnthetubaguy: one issue might be a chicken and egg with the custom RC inventory being available when we PUT /allocations/{consumer_uuid} | |
| 15:00:18 | mriedem | because the inventory gets updated via the RT periodic | |
| 15:01:00 | mriedem | so if i just set a custom rc on my ironic node, the periodic runs, it detects the new rc and will update inventory, but while in the driver we're migrating the instance because it's node has an rc now, if we try updating the allocation for that rc before the node RP inventory has that rc, it will fail with a 409 | |
| 15:01:06 | johnthetubaguy | mriedem: dang, yeah | |
| 15:01:06 | mriedem | "409 Conflict if there is no available inventory in any of the resource providers for any specified resource classes or inventories are updated by another thread while attempting the operation." | |
| 15:01:29 | johnthetubaguy | mriedem: I guess we skip adding it as migration complete, and go around again | |
| 15:01:41 | dansmith | mriedem: is comment #1 what I should read? | |
| 15:01:50 | mriedem | dansmith: or 2 | |
| 15:02:05 | mriedem | oh yeah 1 is my description of the problem | |
| 15:02:09 | mriedem | in 'merican | |
| 15:02:17 | johnthetubaguy | mriedem: I have a related thing here, I am not keen on what it could do across upgrade mind: https://review.openstack.org/#/c/513001 | |
| 15:02:28 | johnthetubaguy | heh | |
| 15:03:03 | dansmith | mriedem: this is because pike doesn't do healing of allocations right? | |
| 15:03:22 | johnthetubaguy | dansmith: +1 | |
| 15:03:28 | mriedem | yes | |
| 15:03:41 | mriedem | the flavor migration code has a comment asserting that the allocations are updated via the RT periodic | |
| 15:03:55 | mriedem | probably b/c it was written 1/2 a day before that changed :) | |
| 15:04:06 | dansmith | having the virt driver do that thing we decided was bad to have compute (at all) do, seems like a bad call to me, | |
| 15:04:13 | dansmith | although I understand something needs to do it | |
| 15:04:54 | dansmith | it'd be nice if we had some way to let the driver signal to compute that something fundamental has changed about the instance to force the heal, | |
| 15:04:58 | mriedem | could we get the scheduler to do it? probably not b/c the issue during scheduling isn't with the instance already consuming that node, it's a new build request and the scheduler thinking the node is free | |
| 15:05:11 | mriedem | dansmith: that's what i was thinking as an option above, | |
| 15:05:23 | mriedem | have a way for the RT to call into the driver to ask if the allocatoin should be forced | |
| 15:05:36 | dansmith | yeah | |
| 15:05:52 | mriedem | that might also fix the issue with trying to update the allocatoin before we've posted inventory for the newly set node.rc | |
| 15:05:54 | dansmith | which could apply to other virt drivers that have things happen underneath them (like vmware moving an instance within a cluster) | |
| 15:06:00 | johnthetubaguy | I am missing the trigger, a new build fails and triggers the refresh? | |
| 15:06:12 | jaypipes | stephenfin: k, done on the pci numa policies spec. | |
| 15:06:27 | mriedem | johnthetubaguy: where does the new build fail? scheduler or compute? | |
| 15:06:34 | mriedem | i thought it was compute and triggered a retry | |
| 15:06:50 | johnthetubaguy | yeah, its on the compute, ironic find the node is already in use by another instance | |
| 15:06:57 | mriedem | so spawn fails? | |
| 15:07:01 | johnthetubaguy | yeah | |
| 15:07:16 | johnthetubaguy | I should double check the logs on that | |
| 15:07:21 | stephenfin | jaypipes: Thank you sir. Much appreciated, as always | |
| 15:07:51 | jaypipes | stephenfin: no problem. or however one says no problem in Gaelic. | |
| 15:09:05 | mriedem | bauzas: you were +2 on https://review.openstack.org/#/c/361140/ before so are you still good | |
| 15:09:05 | mriedem | ? | |
| 15:09:23 | efried | An dtugann stephenfin Gaeilge? I was kind of guessing not... | |
| 15:10:15 | mriedem | johnthetubaguy: not sure i like the spawn fail / refresh thing, | |
| 15:10:28 | mriedem | trying to think of how we can set a flag for instances that have migrated but need their allocations updated by the RT | |
| 15:10:55 | mriedem | including instances that have had their flavors migrated already | |
| 15:11:06 | dansmith | the problem being we do the migration async right? | |
| 15:11:24 | dansmith | we come up and before we can do all of them we might get a boot request? | |
| 15:11:25 | mriedem | ah crap didn't think about that either | |
| 15:11:46 | dansmith | we could disable ourselves until the migration is complete | |
| 15:12:22 | mriedem | the async migration also means that if the RT asked the driver for any instances to forcefully update allocations, they might not be set yet, but the next pass or the periodic would hit them | |
| 15:12:32 | mriedem | i was thinking the driver is shoving stuff onto a work queue, | |
| 15:12:36 | mriedem | the RT pulls off that queue | |
| 15:12:50 | mriedem | once all instances are processed the queue remains empty | |
| 15:13:09 | johnthetubaguy | get_inventory, could we do the migration on that call, and return something if the allocation needs refreshing? | |
| 15:13:16 | dansmith | if we disable ourselves we can still do management operations of existing instances, and the scheduler won't send us new stuff until we're ready | |
| 15:13:32 | dansmith | johnthetubaguy: that's kinda icky | |
| 15:13:39 | dansmith | johnthetubaguy: I mean, really icky | |
| 15:13:50 | dansmith | johnthetubaguy: gross even | |
| 15:14:04 | johnthetubaguy | worse than virt driver calling to placement? | |
| 15:14:15 | dansmith | also recall that people can do this before anything starts back up during an upgrade | |
| 15:14:52 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Transform instance.exists notification https://review.openstack.org/403660 | |
| 15:14:53 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Add sample test for instance audit https://review.openstack.org/480955 | |
| 15:15:04 | dansmith | johnthetubaguy: it's just overloading get_inventory() as a general hook to do potentially a ton of db maintenance | |
| 15:15:11 | johnthetubaguy | so the alternative is dropping the resources:VCPU=0 in the new flavor | |
| 15:15:13 | dansmith | johnthetubaguy: it'd be better to just provide a proper hook I think | |
| 15:15:58 | johnthetubaguy | dansmith: I am agreeing its horrid, just thinking relative horrid here really | |
| 15:16:10 | dansmith | johnthetubaguy: it's relatively horrid yes :) | |
| 15:16:21 | dansmith | johnthetubaguy: you don't like the self disable? | |
| 15:16:37 | dansmith | if we do it in get_inventory we still have a race right? | |
| 15:16:41 | dansmith | with the scheduler | |
| 15:16:52 | johnthetubaguy | so its a race with when the flavor gets updated I guess | |
| 15:17:45 | mriedem | i'm not sure at which point you auto-disable | |
| 15:17:57 | dansmith | mriedem: on init_host(), if there are unmigrated instances | |
| 15:18:15 | dansmith | and then re-enable when you're done with the migration | |
| 15:18:44 | johnthetubaguy | but if the resource class isn't set on the node, we are never done | |
| 15:18:44 | mriedem | so originally when we did the flavor migration it was on init_host and then that moved because of the hash ring stuff | |
| 15:19:05 | dansmith | johnthetubaguy: that had to be set before queens, right? | |
| 15:19:09 | mriedem | yeah i'm trying to think if we re-enable too soon, or disable too long | |
| 15:19:29 | dansmith | so queens starting up can assume it's set no? | |
| 15:19:35 | johnthetubaguy | dansmith: yes, but this is really about making pike work | |
| 15:20:05 | johnthetubaguy | so right know I can't update the flavors in pike, for them to be set in time for queens, because of this bug | |
| 15:20:09 | dansmith | oh, was this migration in pike? I'm misremembering the timing | |
| 15:20:23 | johnthetubaguy | yeah, sadly | |
| 15:20:39 | mriedem | yeah we have to backport the fix | |
| 15:21:05 | dansmith | johnthetubaguy: but you can set the node class before pike, and you can run the migration before you start anything else up right/ | |
| 15:21:07 | mriedem | so i'm still thinking queue and hook callback from RT to driver | |
| 15:21:51 | dansmith | mriedem: explain your queue thing again? | |
| 15:22:18 | mriedem | so before https://github.com/openstack/nova/blob/e11a8687aef527eee9f7c733db6cccd5b902afbb/nova/virt/ironic/driver.py#L523 we're going to have to see if the instance already has an allocation for the normalized_rc, | |
| 15:22:22 | cdent | does anyone have a reference to previous discussion/decisions on why we don’t do regular allocation updates? (something I can read, rather than taking us off track here) | |
| 15:22:28 | mriedem | if it doesn't, we have to put the instance/rc into a queue, | |
| 15:22:52 | mriedem | when the RT periodic runs, it calls into the driver to say, give me any instances that need their allocations updated and we'd dequeue at that point | |
| 15:23:38 | mriedem | then ^ should auto-heal during the periodic as the admin is setting node rcs in ironic | |
| 15:23:40 | mriedem | while things are running | |
| 15:24:27 | mriedem | cdent: https://review.openstack.org/#/c/491012/ | |
| 15:24:43 | cdent | thanks | |