| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-04-20 | |||
| 17:02:44 | clarkb | mriedem: I've been checking e-r every morning and looking at the top gate bug :) just to make sure everything is happy | |
| 17:08:00 | johnthetubaguy | mriedem: so I just -1ed the thing I just +Wed https://review.openstack.org/#/c/438750 | |
| 17:08:15 | johnthetubaguy | mriedem: with a note about that begin_detaching stuff | |
| 17:08:53 | johnthetubaguy | mriedem: if what I said is total nonsense, I am happy for someone to over-ride that worry, the rest of it looks fine | |
| 17:19:59 | openstackgerrit | Lee Yarwood proposed openstack/nova master: encryptors: Switch to os-brick encryptor classes https://review.openstack.org/391597 | |
| 17:21:52 | ildikov | johnthetubaguy: isn't begin_detaching an API-level call? | |
| 17:22:15 | ildikov | johnthetubaguy: I mean it gets called before we even get deeper down to detach with the new flow | |
| 17:39:58 | mriedem | johnthetubaguy: i don't follow the concern in this patch | |
| 17:40:42 | mriedem | johnthetubaguy: as mentioned during the meeting, i thought begin_detaching could be used in both the old and new flows | |
| 17:45:08 | ildikov | mriedem: I think it's currently used before we get into the actual detach parts with both flows right now | |
| 17:45:18 | mriedem | yes it is | |
| 17:45:22 | mriedem | b/c it's used in the api | |
| 17:46:14 | ildikov | that's what I thought that it's in a different layer so it happens before we get to choose between the new/old flow | |
| 18:06:48 | openstackgerrit | Jay Pipes proposed openstack/nova master: placement: implement get_inventory() for libvirt https://review.openstack.org/457782 | |
| 18:18:48 | openstackgerrit | Ildiko Vancsa proposed openstack/nova master: Add Cinder v3 detach call to _terminate_volume_connections https://review.openstack.org/456896 | |
| 18:18:48 | openstackgerrit | Ildiko Vancsa proposed openstack/nova master: Add Cinder v3 detach to swap_volume https://review.openstack.org/456971 | |
| 18:18:49 | openstackgerrit | Ildiko Vancsa proposed openstack/nova master: Add Cinder v3 detach call to post_live_migration https://review.openstack.org/456988 | |
| 18:29:21 | ildikov | mriedem: lyarwood: johnthetubaguy: fixed up the chain ^^ | |
| 18:30:19 | lyarwood | ildikov: cool thanks | |
| 18:30:21 | ildikov | mriedem: lyarwood: johnthetubaguy: these two are not relying on _terminate_volume_connections: https://review.openstack.org/#/c/456851/ https://review.openstack.org/#/c/456877/ | |
| 18:30:50 | mriedem | ok | |
| 18:31:20 | ildikov | the last two need the almost approved base change as that's where attachment_delete is added | |
| 18:31:38 | ildikov | I can move those on top of that patch if we think it's going to be blocked for longer | |
| 18:31:45 | ildikov | I hope not though | |
| 18:31:53 | mriedem | you could just use the Depends-On tag | |
| 18:32:06 | ildikov | oh, ok good point | |
| 18:32:19 | ildikov | I always thought that's more for cross-repo dependencies... | |
| 18:33:23 | ildikov | lyarwood: all the patches will need some additional tests, so that can be picked up by anyone who's available | |
| 18:33:36 | ildikov | lyarwood: stvnoyes helped out with the base patch | |
| 18:34:06 | ildikov | lyarwood: stvnoyes: the two independent patches could be good candidates if either of you have bandwidth | |
| 18:34:13 | mriedem | ok i just commented on the lack of a test in the local delete in api one | |
| 18:34:19 | ildikov | lyarwood: stvnoyes: I can babysit the chain | |
| 18:34:24 | lyarwood | yeah I can dig into that tomorrow | |
| 18:34:50 | ildikov | mriedem: yep, tnx, that's a "known issue" at least | |
| 18:35:31 | ildikov | lyarwood: sounds good, tnx, please ping stvnoyes and me if you start with either of those or both so we don't work on it in parallel | |
| 18:35:48 | lyarwood | will do, thanks | |
| 18:37:31 | ge0rgi | Hello I am a CS working on my master thesis. I have a simple question: How do I raise an exception in nova compute manager so that the error message from the exception is displayed in horizon as well? Can I use notifications for this? I saw the notify_about_instance_usage method and that it accepts a fault parameter, however I don't see the message displayed. | |
| 18:38:11 | stvnoyes | lyarwood, ildikov- i am up for either one, how about I take https://review.openstack.org/#/c/456877/ ? | |
| 18:38:35 | ildikov | stvnoyes: sounds good to me, tnx! | |
| 19:16:27 | rajinir | Are live migration settings removed fom nova.conf? Looking at live_migration_progress_timeout | |
| 19:31:49 | efried | Is it the done thing to use save_and_reraise_exception instead of plain raise everywhere? | |
| 19:32:29 | efried | Like, are we really worried about something as simple as LOG.* changing the active exception context? | |
| 19:32:46 | efried | mriedem ^^ | |
| 19:38:12 | sdague | efried: you got an example? | |
| 19:38:49 | efried | sdague https://review.openstack.org/#/c/438598/13/nova/virt/powervm/vm.py@185 | |
| 19:38:59 | efried | etc. | |
| 19:41:02 | sdague | if it's legit just a log message, it's probably fine, we have a number of those in the code | |
| 19:41:20 | sdague | if you are doing anything more complicated, the save_and_reraise is probably better | |
| 19:42:23 | edmondsw | sdague, the comments at the top of save_and_reraise_exception indicate problems can be created switching greenthreads, so does it even matter how complicated what you're doing is? | |
| 19:43:01 | edmondsw | https://github.com/openstack/oslo.utils/blob/master/oslo_utils/excutils.py#L151 | |
| 19:44:06 | efried | edmondsw I don't see how it helps, considering the greenthread switch could happen in the middle of the save_and_reraise() setup anyway. | |
| 19:44:26 | efried | And that would have to be a base python bug if it really worked that way, nah? | |
| 19:44:49 | sdague | edmondsw: so maybe, but if that's the case we've got hundreds of issues to clean up in Nova | |
| 19:45:00 | cfriesen | sfinucan: you around? | |
| 19:45:01 | efried | sdague 56 ;-) | |
| 19:45:13 | sdague | efried: ok, 56 :) | |
| 19:45:19 | efried | We counted ;-) | |
| 19:45:41 | edmondsw | efried sdague I don't know, I'd just err on the side of caution and use the method... if it doesn't help, at least it doesn't hurt | |
| 19:45:49 | edmondsw | and hopefully it helps | |
| 19:45:51 | sdague | still, anyway, it seems like a standard we don't really need to hold the powervm folks to for new code | |
| 19:45:59 | edmondsw | sdague ok | |
| 19:47:57 | sdague | edmondsw: we would probably not be opposed to additional patches cleaning up that all over the place | |
| 19:48:20 | edmondsw | sdague sure :) | |
| 19:50:25 | efried | If it's the preferred SOP, I'll do it. Just extra code/logic for what seems like a fairly paranoid reason. | |
| 19:50:28 | openstackgerrit | Dan Smith proposed openstack/nova master: DNM: Canary test against multi-cell devstack https://review.openstack.org/458537 | |
| 19:50:29 | openstackgerrit | Dan Smith proposed openstack/nova master: Make target_cell() yield a new context https://review.openstack.org/458635 | |
| 19:50:29 | openstackgerrit | Dan Smith proposed openstack/nova master: WIP: Move to proper target_cell calling convention https://review.openstack.org/458634 | |
| 19:50:30 | openstackgerrit | Dan Smith proposed openstack/nova master: WIP: De-complicate some of the instance delete path https://review.openstack.org/458636 | |
| 19:50:40 | dansmith | mriedem: I think you will want to look at this: https://review.openstack.org/458636 | |
| 20:01:27 | mriedem | oh come on | |
| 20:01:43 | mriedem | dansmith: https://review.openstack.org/#/c/455007/ | |
| 20:02:15 | mriedem | but yeah, that was part of my master plan at some point here too, removing the context managers | |
| 20:02:25 | mriedem | since they are redundant since we stash the cell on the context | |
| 20:02:28 | mriedem | when looking up the instance | |
| 20:02:33 | dansmith | um, we can just remove that whole block? | |
| 20:02:45 | mriedem | and do normal local_delete? | |
| 20:03:07 | mriedem | or you mean this? https://review.openstack.org/#/c/455007/2/nova/compute/api.py | |
| 20:03:17 | dansmith | that yeah | |
| 20:03:27 | mriedem | it appears it's ok except for something with cells 1 | |
| 20:03:35 | mriedem | which i haven't debugged yet | |
| 20:03:54 | dansmith | okay, that's a different block than I'm working on right? | |
| 20:04:05 | mriedem | that whole block was written before https://review.openstack.org/#/c/367557/ | |
| 20:04:06 | mriedem | yes | |
| 20:04:17 | mriedem | ^ is what deletes the instance in conductor | |
| 20:04:23 | mriedem | if the build request is gone | |
| 20:04:41 | mriedem | and that code likely suffers from the same quota/local delete mess that _delete handles | |
| 20:04:48 | mriedem | i.e. we do the same gd local delete thing in 3 places | |
| 20:04:50 | dansmith | okay, I'm confused | |
| 20:06:11 | dansmith | I don't think we can remove the targeting dance that does different things with the instance and the quota stuff, until the patches I have below this that avoid mutating the context, right? | |
| 20:06:26 | mriedem | i haven't seen your patches yet | |
| 20:06:27 | mriedem | so idk | |
| 20:06:41 | mriedem | but that's why i didn't dig into removing the context dance yet | |
| 20:06:53 | mriedem | that's why i started with https://review.openstack.org/#/c/455007/ | |
| 20:06:55 | dansmith | right, so I guess Idon't understand the "oh come on" comment :) | |
| 20:07:05 | mriedem | since there was a big fat note in there about removing it after we deleted the instance in conducto | |
| 20:07:28 | mriedem | "oh come on" as in i thought we were doing the same thing, before i actually looked at your change | |
| 20:07:32 | mriedem | beyond the commit title | |
| 20:07:33 | mriedem | sucka | |
| 20:08:01 | mriedem | we are trying to converge on the same thing though - which is cleanup the 3-headed local delete monster we have | |
| 20:08:33 | dansmith | ...okay | |