Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-10-18
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
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?

Earlier   Later