| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-10-18 | |||
| 14:27:54 | efried | mriedem Right, and I'm led to understand the set of cores on stable is not the same as master, but I don't really understand how it works. Guess I'm asking what needs to be done to get it in? | |
| 14:27:58 | johnthetubaguy | mriedem: yes | |
| 14:28:14 | mriedem | johnthetubaguy: ok yeah i seem to remember this coming up too....maybe dansmith remembers | |
| 14:28:53 | mriedem | efried: yes different core group on stable https://review.openstack.org/#/admin/groups/540,members | |
| 14:28:56 | mriedem | b/c different rules | |
| 14:29:04 | dansmith | um what | |
| 14:29:10 | mriedem | efried: https://docs.openstack.org/project-team-guide/stable-branches.html | |
| 14:29:13 | dansmith | johnthetubaguy: mriedem I'm not sure what you're talking about | |
| 14:29:22 | dansmith | I'm on a call right now so I'm a bit distracted | |
| 14:29:38 | mriedem | efried: we could use more stable cores, so if that's something you're interested in helping with, please dig in, get to know the rules, and do reviews | |
| 14:29:47 | efried | mriedem ack | |
| 14:30:18 | mriedem | basically means don't backport features, or backward incompatible changes, all things start on master and go backward, and there are support phases for what's appropriate to backport based on severity | |
| 14:30:56 | mriedem | johnthetubaguy: ok, got it - want to start by reporting a bug? | |
| 14:31:19 | johnthetubaguy | mriedem: yeah, will do, I was hoping I just miss-read the docs | |
| 14:33:11 | mriedem | johnthetubaguy: https://review.openstack.org/#/c/492964/ is the change i was thinking of, | |
| 14:33:14 | mriedem | slightly different though | |
| 14:33:21 | mriedem | that's about reporting inventory, not allocations | |
| 14:33:29 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Extract instance allocation removal code https://review.openstack.org/513041 | |
| 14:33:38 | gibi | mriedem: fixed your nit in ^^ | |
| 14:35:15 | openstackgerrit | Eric Fried proposed openstack/nova-specs master: PCI NUMA Policies https://review.openstack.org/361140 | |
| 14:35:23 | efried | stephenfin ^ | |
| 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? | |