| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-10-18 | |||
| 14:53:56 | johnthetubaguy | mriedem: yes | |
| 14:54:05 | efried | bauzas Of course; that one should be an easy +A hopefully. | |
| 14:54:17 | mriedem | johnthetubaguy: before https://github.com/openstack/nova/blob/e11a8687aef527eee9f7c733db6cccd5b902afbb/nova/virt/ironic/driver.py#L523 we'd have to check if there is an existing allocation for normalized_rc in placement for that instance | |
| 14:54:19 | mriedem | and if not, add it | |
| 14:54:27 | bauzas | cdent: I'm pretty against the idea to see the virt driver talking to placement | |
| 14:54:36 | mriedem | johnthetubaguy: that is at most redundant once per restart of nova-compute i agree | |
| 14:54:40 | johnthetubaguy | mriedem: +1 that's what I was trying to say above | |
| 14:54:44 | mriedem | because then self._migrated_instance_uuids.add(node.instance_uuid) | |
| 14:54:49 | johnthetubaguy | yep, yep | |
| 14:54:59 | johnthetubaguy | I mean its horrid, but seems the least horrid | |
| 14:55:29 | cdent | bauzas: yes, it is icky, but the non libvirt drivers (notably powervm and vmware) may very well need to do it despite the ickiness | |
| 14:55:54 | dansmith | cdent: why? | |
| 14:56:43 | mriedem | johnthetubaguy: ok we both said the same thing in https://bugs.launchpad.net/nova/+bug/1724589 :) | |
| 14:56:44 | openstack | Launchpad bug 1724589 in OpenStack Compute (nova) "Unable to transition to Ironic Node Resource Classes in Pike" [High,New] | |
| 14:56:47 | johnthetubaguy | mriedem: I can go and try code that up, to see how bad it looks | |
| 14:56:51 | cdent | dansmith: one example is the conversation that efried, rgerganov and efried were having earlier today about device management. /me locates link | |
| 14:57:03 | mriedem | johnthetubaguy: go nuts - you've got the recreate so you'll be able to tell if it fixes it | |
| 14:57:10 | efried | cdent I think having the allocations passed into the virt driver obviates any need to query placement further | |
| 14:57:31 | dansmith | cdent: I want compute asking the virt driver what it thinks it needs and then compute doing that work | |
| 14:57:45 | efried | or that ^ | |
| 14:57:47 | mriedem | so, what john and i are talking about is a special one off ase | |
| 14:57:48 | mriedem | *case | |
| 14:57:55 | dansmith | cdent: having two things managing allocations or RCs or anything like that gets us further into the territory we had before where things are stomping on each other | |
| 14:57:56 | mriedem | for the ironic flavor migration stuff | |
| 14:57:59 | johnthetubaguy | *arse | |
| 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 | |