| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-10-18 | |||
| 14:40:08 | stephenfin | sean-k-mooney, cdent: Ye might like to look at that again too ^ | |
| 14:40:11 | mriedem | johnthetubaguy: random thoughts: we could have the ironic driver set a flag when a flavor was migrated and the RT calls into the driver to see if the flag was set and allocations should be forcefully updated (kind of gross and maybe racy), | |
| 14:40:30 | mriedem | johnthetubaguy: we could have the driver update allocations on it's own...also kind of gross but at least very specific case | |
| 14:40:33 | dansmith | stephenfin: I can, but the queue is long and I'm not sure I'm the best person to review that | |
| 14:40:43 | dansmith | I guess I don't really know who is though | |
| 14:41:01 | johnthetubaguy | mriedem: yeah, being part of the instance flavor migration doesn't seem totally crazy | |
| 14:41:17 | stephenfin | dansmith: That's the problem :( jaypipes would be the best person, but he has his hands full with nested-resource-providers | |
| 14:41:30 | dansmith | stephenfin: he has his hands full with being a slacker | |
| 14:41:35 | stephenfin | dansmith: You could just approve it and assume it's perfect? :P | |
| 14:41:42 | johnthetubaguy | mriedem: I attempted to write it all up here: https://bugs.launchpad.net/nova/+bug/1724589 | |
| 14:41:43 | openstack | Launchpad bug 1724589 in OpenStack Compute (nova) "Unable to transition to Ironic Node Resource Classes in Pike" [High,New] | |
| 14:41:45 | stephenfin | Well, that goes without saying | |
| 14:41:55 | mriedem | another idea is the RT could check to see if there are any new resource classes in the flavor, but that would be complicated - and the only way to tell new from old is by checking existing allocations in placement - not something we want to do while holding a lock in the RT | |
| 14:43:10 | johnthetubaguy | mriedem: I don't mind a nova-manage cmd you have to run to update the allocations for the node you are on? | |
| 14:43:15 | jaypipes | stephenfin: I'm currently reviewing the PCI NUMA policy spec. | |
| 14:43:23 | jaypipes | stephenfin: also, still on vacation.. | |
| 14:43:36 | edmondsw | efried here are the nova stable cores: https://review.openstack.org/#/admin/groups/540,members | |
| 14:43:37 | johnthetubaguy | mriedem: although that breaks our upgrade ABI... | |
| 14:43:46 | jaypipes | stephenfin: but I just got some coffee and fuck it, might as well do some work. | |
| 14:44:38 | kashyap | jaypipes: On vacation and on specs? Sheesh | |
| 14:44:38 | openstackgerrit | Chris Dent proposed openstack/nova master: [placement] Clean up TODOs in allocations.yaml gabbit https://review.openstack.org/513057 | |
| 14:44:48 | kashyap | jaypipes: Setting a baaaaad example, I tell ya | |
| 14:45:15 | mriedem | johnthetubaguy: i'd rather we do it automatically | |
| 14:45:27 | stephenfin | jaypipes: You're a better man than I. I didn't know you were on vacation | |
| 14:45:32 | johnthetubaguy | mriedem: yeah, I will have a dig | |
| 14:45:48 | mriedem | johnthetubaguy: one issue though is i think once we migrate the flavor, we no longer check this stuff in the ironic driver, | |
| 14:46:05 | mriedem | so anyone that has already upgraded and migrated the flavors, not sure if we can correct the issue for them | |
| 14:46:17 | mriedem | without detecting if the instance allocations include the resource class or not, and if not, add it | |
| 14:46:37 | mriedem | which sucks since that would run every time | |
| 14:46:38 | jaypipes | stephenfin: it's cool duder. I'll be back into my normal swing of things later today. | |
| 14:46:48 | jaypipes | stephenfin: just got back from Ohio at midnight last night. | |
| 14:46:50 | johnthetubaguy | mriedem: it would only be once per process restart to do that allocation check | |
| 14:47:04 | mriedem | johnthetubaguy: the flavor migration thing happens on every node refresh | |
| 14:47:14 | johnthetubaguy | mriedem: will find you the link | |
| 14:47:14 | mriedem | because of the hash ring stuff | |
| 14:48:12 | johnthetubaguy | mriedem: before this line is only one per process restart, I think: https://github.com/openstack/nova/blob/e11a8687aef527eee9f7c733db6cccd5b902afbb/nova/virt/ironic/driver.py#L523 | |
| 14:48:53 | johnthetubaguy | mriedem: well, per instance you create too, there is the _migrated_instance_uuids guard to stop the worst of it | |
| 14:49:03 | stephenfin | jaypipes: Gotcha. Looking through efried's spec rn, but if there's anything you want reviews on let me know | |
| 14:49:19 | jaypipes | stephenfin: will do. cheers. | |
| 14:50:27 | mriedem | https://github.com/openstack/nova/blob/e11a8687aef527eee9f7c733db6cccd5b902afbb/nova/virt/ironic/driver.py#L536-L539 | |
| 14:50:33 | mriedem | "By adding just the custom RC to the existing flavor extra_specs, the periodic call to update_available_resources() will add an allocation against the custom resource class, and prevent placement from thinking that that node is available." | |
| 14:50:34 | mriedem | gah | |
| 14:50:44 | mriedem | except it won't... | |
| 14:51:02 | cdent | I kinda wish we could figure out some way to have that back. | |
| 14:52:12 | cdent | if we can’t, then I think we should be okay with the virt driver doing it | |
| 14:52:25 | mriedem | johnthetubaguy: this is where we start the migration checking https://github.com/openstack/nova/blob/e11a8687aef527eee9f7c733db6cccd5b902afbb/nova/virt/ironic/driver.py#L716 | |
| 14:52:27 | cdent | we’re seeing increasing situations where the virt driver being able to talk to placement is sueful | |
| 14:52:32 | mriedem | any time a node is refreshed | |
| 14:53:00 | dansmith | cdent: you mean compute | |
| 14:53:08 | cdent | no, I mean virt driver | |
| 14:53:12 | mriedem | dansmith: not in this case | |
| 14:53:14 | efried | bauzas Could you take a look at https://review.openstack.org/#/c/510957/ please? | |
| 14:53:18 | johnthetubaguy | mriedem: but most of it doesn't happen, as we check https://github.com/openstack/nova/blob/e11a8687aef527eee9f7c733db6cccd5b902afbb/nova/virt/ironic/driver.py#L555 | |
| 14:53:31 | mriedem | johnthetubaguy: so consider i've upgraded to pike, added resource classes to my existing nodes, and those migrated the flavor extra specs | |
| 14:53:32 | mriedem | we get to https://github.com/openstack/nova/blob/e11a8687aef527eee9f7c733db6cccd5b902afbb/nova/virt/ironic/driver.py#L523 | |
| 14:53:34 | dansmith | mriedem: I really want to avoid the virt driver talking to placement | |
| 14:53:46 | bauzas | efried: sure, top prio just being specs reviews for today | |
| 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 | 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:06 | johnthetubaguy | mriedem: dang, yeah | |
| 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 | |