| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-10-18 | |||
| 14:35:36 | efried | I basically just took that nonsensical chunk out, and fixed up those couple of words. | |
| 14:36:58 | openstackgerrit | Ghanshyam Mann proposed openstack/nova-specs master: Spec for API extensions policy removal https://review.openstack.org/508101 | |
| 14:38:06 | mriedem | johnthetubaguy: ok so normally, at least if you have 1 ocata compute, the update_available_resource periodic would run, that would get available nodes, which would refresh nodes from ironic, and that refresh does the resoure class / flavor migration thing, | |
| 14:38:27 | mriedem | and then as part of the update_available_resource periodic, the RT would update allocations for each instance on the node (if you have at least 1 ocata compute) | |
| 14:39:15 | mriedem | we don't really have a hook between the virt driver and the RT to say if the allocations should be updated, | |
| 14:39:35 | stephenfin | efried: Looks good to me. Thank you :) | |
| 14:39:36 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Moving more utils to ServerResourceAllocationTestBase https://review.openstack.org/499539 | |
| 14:39:37 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: factor out compute service start in ServerMovingTest https://review.openstack.org/503037 | |
| 14:39:37 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Test resource allocation during soft delete https://review.openstack.org/495159 | |
| 14:39:56 | stephenfin | dansmith: Could you stick https://review.openstack.org/361140 on your review backlog? | |
| 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 | openstackgerrit | Chris Dent proposed openstack/nova master: [placement] Clean up TODOs in allocations.yaml gabbit https://review.openstack.org/513057 | |
| 14:44:38 | kashyap | jaypipes: On vacation and on specs? Sheesh | |
| 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 | mriedem | because of the hash ring stuff | |
| 14:47:14 | johnthetubaguy | mriedem: will find you the link | |
| 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 ^ | |