Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-01-22
14:36:02 sean-k-mooney stephenfin: i need to read the patch again since its been a few weeks since i did so i may be mis remembering
14:43:27 gibi alex_xu: thanks for the update. I'm +2 on the bottom patch. I will review further in that chain soon
14:43:37 efried ameeda I believe Jay is trying to find a spot to work from at the moment.
14:44:33 gibi edmondsw: the SEA patch is on my list
14:44:53 edmondsw gibi great, tx
14:44:59 ameeda efried: hehe, so can you help me ?
14:45:15 efried ameeda Gosh, that depends. What's going on?
14:45:19 mriedem stephenfin: were you working on a nit fixes patch for the websocket proxy security series?
14:45:42 ameeda efried: can you please check this "https://review.openstack.org/#/c/526900/" and notice the scenario from the bug side ?
14:46:09 stephenfin mriedem: Yup, it merged. Lemme know if there's stuff I missed https://review.openstack.org/#/c/534368/
14:46:38 mriedem stephenfin: ah ok - was just wondering if you wanted to update that docs patch for my one comment or do it in a follow up?
14:47:37 efried ameeda Oh, this patch. Yeah, I looked it over a bit last week and accepted that it's not really in my wheelhouse, sorry.
14:47:44 stephenfin mriedem: If it's just that, I can edit on Gerrit
14:47:52 stephenfin mriedem: reply left, in any case
14:48:10 mriedem stephenfin: just edit inline and i'll +W
14:48:33 bauzas mriedem: welcome back
14:48:38 mriedem thanks
14:48:39 bauzas mriedem: for your pleasure, we have https://bugs.launchpad.net/nova/+bug/1744325
14:48:41 openstack Launchpad bug 1744325 in OpenStack Compute (nova) "If a rebuild is refused by the scheduler, the instance's imageref is not rolled back" [Critical,In progress] - Assigned to int32bit (int32bit)
14:48:57 openstackgerrit Stephen Finucane proposed openstack/nova master: doc: Document TLS security setup for noVNC proxy https://review.openstack.org/500544
14:49:19 stephenfin mriedem: Done (y)
14:49:44 mriedem bauzas: tagged for rc potential but not going to look at it for awhile
14:49:58 bauzas mriedem: I'm following the change
14:50:36 bauzas I'll see how it goes, and try to help next week if we haven't reached to a clear solution
14:51:28 ameeda efried: thank you !
14:51:34 mriedem it's not fun but i wouldn't say it's the worst bug i've ever seen,
14:51:52 mriedem considering until queens, if you tried rebuilding a volume-backed server, we'd always fail to do so and still change the imageref on the instance
14:54:16 bauzas mriedem: hmmm, you're right
15:02:25 cdent edleafe, efried: would be good to get this stack merged before we start merging all the big stuff so that it is useful in the inevitable debugging we will need to do: https://review.openstack.org/#/c/526557/
15:04:21 edleafe cdent: ack
15:05:24 Roamer` mriedem, welcome back! What are the chances of the StorPool volume attachment driver? I know it's close to the deadline, but you said if we got our ducks in a row... and the last duck, 533046, has been merged for a week
15:05:58 Roamer` and right now we're in a weird position where the main use case for our Cinder and os-brick drivers is not in OpenStack proper :)
15:06:01 mriedem bauzas: comments inline in https://review.openstack.org/#/c/536268/
15:06:24 bauzas mriedem: k, will look
15:06:29 mriedem bauzas: it's breaking evacuate potentially, and it's also missing rolling back a bunch of other properties, so it's probably not worth trying to rollback any properties since we'd be totally half assing it
15:06:42 mriedem might as well just put the instance into ERROR state and make the user rebuild with a valid image to fix things
15:06:50 Roamer` mriedem, sorry, of course I'm talking about https://review.openstack.org/#/c/140733/ (certainly this is not the only thing on your plate...)
15:07:15 mriedem Roamer`: i'll take a look
15:07:21 bauzas mriedem: oh, excellent catch
15:07:22 mriedem still trying to get my bearings
15:07:28 Roamer` mriedem, of course, thanks a lot!
15:07:53 mriedem bauzas: can you re-approve https://review.openstack.org/#/c/528128/ ?
15:08:07 bauzas mriedem: I'm half-reviewing, half-testing my own series, so apologies for not fully getting all the context in mind
15:08:07 mriedem so we can get the series of novaclient microversion changes flushed for a final release
15:08:14 mriedem bauzas: np
15:08:56 bauzas mriedem: oh, the novaclient change is just a rebase ? +wd
15:09:01 mriedem yeah
15:09:02 mriedem thanks
15:09:25 bauzas mriedem: I tried to look at the multiattach thing while you were on vacations
15:09:35 bauzas mriedem: that's an excellent job you made honestly
15:10:00 bauzas we're still missing the API bits, but I understand now the logic
15:10:23 bauzas I like the fact to short-cut the multiattach if we don't support that by the compute
15:12:45 mriedem bauzas: i saw thanks for reviewing those,
15:12:56 mriedem the short circuit was copied from artom's tagged attach stuff
15:14:39 efried cdent I'm still pretty confused what you're getting at here https://review.openstack.org/#/c/532290/
15:14:47 bauzas mriedem: btw. I have a question for you
15:15:04 bauzas mriedem: I'm hardly-testing my vGPU changes on a dedicated box
15:15:16 bauzas mriedem: and I'm testing all the instance operations
15:15:31 bauzas mriedem: for providing a good release note about what can be supported and what not
15:15:38 openstackgerrit Matthew Booth proposed openstack/nova master: Local disk serial numbers for the libvirt driver https://review.openstack.org/529380
15:15:46 efried cdent If ironic gets its traits and they're not different from what's in the cache, we won't make the API call. Which is as it should be. Am I missing something?
15:15:50 bauzas mriedem: so, the point is, when I tried to suspend an instance, libvirt popped up an exception
15:15:53 cdent efried: I'm asking for the authors of the patch to acknowledge that they are aware of the quoted lines of provider tree code. This is because elsewhere in the stack there's been discussion of "this doesn't work with provider tree yet"
15:16:03 bauzas mriedem: basically because we don't accept to hot-unplug mediated devices
15:16:11 cdent It could be all that is needed is "yeah, we know, it's fine"
15:16:54 bauzas mriedem: now, the question is, would you be OK if I would just transform that libvirt exception in a more-readable exception like "sorry, but you can't suspend your instance because $vGPUs"
15:17:03 bauzas and leave the instance into ERROR
15:17:20 bauzas of course, the user can reset the state or rebuild
15:17:48 mriedem bauzas: i don't think the instance should be in ERROR state in that case,
15:17:51 mriedem since we didn't change the guest
15:17:58 mriedem you'd get a fault recorded as to why the suspend failed
15:18:01 mriedem which is probably good enough
15:18:05 bauzas set it back to ACTIVE
15:18:08 bauzas then
15:18:14 bauzas mmm, good call
15:18:46 mriedem yeah i guess you'd revert to the original vm_state, which right now can only be active in the API
15:19:47 mriedem johnthetubaguy: replied to your question in the multiattach api change https://review.openstack.org/#/c/271047/ - i think we're covered for the attach flow, but in a different way
15:20:39 johnthetubaguy mriedem: ah... got it
15:22:46 cdent efried: so, based on your response are you aserting "don't worry, the provider tree will always be right because we've merged enough code for that to be true". If so, cool, but that wasn't clear from earlier discussion on the patches.
15:24:05 efried cdent I'll grant you we still have work to do on concurrency management, though we've asserted that that's only a theoretical problem at the moment due to The Big Semaphore and the single-source-of-control-ness for compute node RPs at the moment.
15:24:18 mgoddard_ cdent, efried: I think a simple answer here is that the resource tracker will always have called set_inventory_for_provider prior to calling set_traits_for_provider, and this ensures that the RPs are present
15:25:21 efried mgoddard_ cdent Ohhh, are we worried about having populated the cache with the relevant provider at this point? I didn't pick up on that at all.
15:25:47 cdent efried: yes, my query has been, all along: is the provider tree active for this code path?
15:26:51 efried cdent That's definitely a legitimate concern, because as documented on set_traits_for_provider (https://github.com/openstack/nova/blob/master/nova/scheduler/client/report.py#L1042) we don't attempt to create the provider. But furthermore, we don't do _ensure_provider either, so there had better have been something prior that populated the cache for that guy.
15:27:37 efried cdent I'm sure I'm just being obtuse, but I didn't understand that from your comments at all :(
15:27:40 mgoddard_ efried: yes, it's set_inventory_for_provider
15:28:58 efried mgoddard_ That's good; and I think it's worth adding a code comment to that effect to affirm that it's been considered. Good call cdent
15:29:27 cdent mgoddard_: how/where does set_inventory_for_provider get involved in the management of the ProviderTree?
15:29:53 efried cdent It calls _ensure_resource_provider first thing.
15:29:57 openstackgerrit Matt Riedemann proposed openstack/nova master: Add the nova-multiattach job https://review.openstack.org/532689
15:30:06 efried cdent Which creates the provider if it doesn't exist, but in any case populates/refreshes the cache
15:31:02 cdent okay, that's the missing piece of the pie, thank you.
15:31:16 mgoddard_ efried: good call, I'll add a comment
15:31:31 efried cdent The other code path, update_compute_node, does the same (_ensure_resource_provider)
15:31:43 cdent I just wanted to be sure, because of what people had said earlier about the potential for confusion
15:32:27 cdent those comments about potential for confusion had made it seem like there was a chance that the provider tree could either be wrong or even not exist
15:32:37 cdent but since it is established by _ensure_* is cool
15:32:51 efried cdent I doubt it's perfect
15:33:30 cdent and how does that make you feel?
15:34:36 efried cdent Dirty. So dirty.

Earlier   Later