| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-03-06 | |||
| 20:27:46 | mriedem | so i was going to start working on a fup patch for some of my comments in the earlier reshaper patches and then stack yours on top of that, but then ran out of steam to start doing that yesterday, | |
| 20:27:57 | mriedem | and really kind of need to be working more on the cross-cell stuff | |
| 20:28:04 | mriedem | so if you're looking for fun things to fup on, go nuts | |
| 20:29:46 | efried | fup my nuts | |
| 20:33:46 | openstackgerrit | Chris Friesen proposed openstack/nova master: Add missing libvirt exception during device detach https://review.openstack.org/641480 | |
| 20:34:42 | cfriesen | melwitt: this is for the bug you took a look at a few weeks ago. ^ | |
| 20:34:43 | mriedem | whoa | |
| 20:35:07 | melwitt | cfriesen: ack | |
| 20:35:17 | melwitt | on my list | |
| 20:42:33 | openstackgerrit | Eric Fried proposed openstack/nova master: Test proper allocation of devices during reshape https://review.openstack.org/639854 | |
| 20:42:45 | efried | mriedem: There's that one with the implicit comma thingy removed. | |
| 20:42:58 | efried | now fupping helper method for mdev name | |
| 20:44:46 | mriedem | efried: did you see my comment about that earlier in the series? | |
| 20:45:02 | mriedem | https://review.openstack.org/#/c/599208/18/nova/virt/libvirt/driver.py@6835 | |
| 20:45:11 | efried | yes | |
| 20:45:13 | mriedem | i think we should just make _get_mediated_device_information handle the dash to underscore conversion | |
| 20:45:21 | mriedem | so the caller (like your test) doesn't need to care | |
| 20:45:24 | efried | ack | |
| 20:46:54 | mriedem | do you want to just also fix those temporarily/PR typos in the test code in the same change as your test wrinkle or a new generic follow up? | |
| 20:46:58 | efried | mriedem: one caller only has the name. Want the method to be smart and figure out which one it got, or take one of two kwargs, or... | |
| 20:47:35 | mriedem | i don't follow | |
| 20:48:24 | efried | _get_mediated_device_information is called in two places. In one of them, the one you pointed out, I have the UUID and convert to the name, which is the part that sucks. In the other, I only have the name and would have to do the reverse if I wanted the arg to be uuid. | |
| 20:48:25 | mriedem | i guess you could add a helper method if that's more clear | |
| 20:49:06 | mriedem | i figured at the start of _get_mediated_device_information it would just dev_name = dev_name.replace('-', '_') | |
| 20:49:10 | mriedem | and then it doesn't matter | |
| 20:50:14 | efried | the method uses both the name and the uuid. I think a pair of helpers might be clearest. | |
| 20:50:46 | mriedem | oh you're talking about this right? https://review.openstack.org/#/c/599208/18/nova/virt/libvirt/driver.py@6161 | |
| 20:51:16 | mriedem | whatever, a simple _get_mdev_name(uuid) is also easy | |
| 21:01:35 | openstackgerrit | Eric Fried proposed openstack/nova master: FUP for test_reshape https://review.openstack.org/641482 | |
| 21:01:40 | efried | mriedem: gathered them together ^ | |
| 21:02:48 | efried | 'cept I seem to have f'ed that up somehow. | |
| 21:03:16 | mriedem | dos2unix nice | |
| 21:04:06 | efried | whaaa | |
| 21:04:33 | efried | oh | |
| 21:04:47 | efried | thought you were saying my line feeds were wrong | |
| 21:06:17 | mriedem | so is this ok or are tests failing? | |
| 21:07:03 | efried | failing. We must be monkey patching libvirt/utils somewhere | |
| 21:07:33 | efried | f it, I'll move the utils into the driver. | |
| 21:07:35 | mriedem | fakelibvirt... | |
| 21:07:57 | mriedem | https://github.com/openstack/nova/blob/master/nova/tests/unit/virt/libvirt/fake_libvirt_utils.py | |
| 21:09:39 | efried | that's... crazy | |
| 21:10:16 | mikal | There's heaps of that stuff from back before we understood how unit testing worked. | |
| 21:10:32 | mikal | Most of its removable with conversion to mock patches now. | |
| 21:10:43 | mikal | (i.e. that's now been done for most of nova-net because I like doomed projects) | |
| 21:11:48 | efried | clearly | |
| 21:12:14 | mikal | *shrug* That code is 8 years old and no one has ever been offended enough to clean it up, so we're all complicit in it being like that. | |
| 21:12:17 | openstackgerrit | Eric Fried proposed openstack/nova master: FUP for test_reshape https://review.openstack.org/641482 | |
| 21:12:43 | efried | oh ffs | |
| 21:12:44 | mikal | It made sense at the time with the tools available then. | |
| 21:12:58 | openstackgerrit | Eric Fried proposed openstack/nova master: FUP for test_reshape https://review.openstack.org/641482 | |
| 21:13:07 | efried | okay mriedem, hopefully that one is okay. | |
| 21:13:45 | efried | I'm pretty offended mikal. But yeah, I don't know if I'm offended enough to clean it up. | |
| 21:15:13 | mriedem | we're talking about fake_libvirt_utils being offensive right? | |
| 21:15:18 | efried | yes | |
| 21:15:22 | mriedem | not just mikal showing up and randomly offending people | |
| 21:15:26 | efried | hahaha | |
| 21:15:41 | dansmith | also, | |
| 21:15:47 | dansmith | mikal is in his PJs currently, which may offend you | |
| 21:16:08 | efried | for some values of "PJs". | |
| 21:16:10 | dansmith | and for all we know, australian PJs might be like american birthday suits | |
| 21:16:18 | efried | swhat I'm sayin | |
| 21:16:30 | efried | ...with a pouch | |
| 21:17:35 | mikal | Look. I've only told one of you to fuck off so far today. I'm really not being _unusually_ offensive. | |
| 21:18:00 | mriedem | i was going to say "as is his wont" but figured that might offend him, but i see it wouldn't | |
| 21:19:28 | efried | We can hug it out, mikal, but let's get one thing perfectly clear: | |
| 21:19:31 | efried | No pants, no hug. | |
| 21:19:52 | mriedem | i'll hug a nude man | |
| 21:19:55 | mriedem | i don't care | |
| 21:20:08 | mriedem | i'm told it will be cold on the receiving end though | |
| 21:20:15 | dansmith | last week mriedem told me he prefers sweaty guys | |
| 21:20:27 | dansmith | I don't really like where this is going | |
| 21:20:59 | mriedem | i'll go back to the fire i just started in the tc channel | |
| 21:21:09 | dansmith | to warm your icy embrace? | |
| 21:28:44 | melwitt | dansmith: going through the change to add a test case for an instance mapping that has no instance (manual cleanup or purge) and was thinking, what should we do in that case? if we don't migrate the user_id, the record would keep getting found. should we migrate it to some sentinel value? or bother trying to get it from request spec? or other? | |
| 21:31:50 | cfriesen | are there docs on how to add new privsep commands in nova? | |
| 21:32:06 | efried | cfriesen: You are right on time, mikal just put on pants | |
| 21:32:18 | efried | if anyone has that answer, it's him. | |
| 21:32:36 | mikal | Well, I actually _left_ to put on pants, but you get the idea. | |
| 21:32:55 | mikal | cfriesen: do you want a pre-pants possibly grumpy answer, or a post pants probably still grumpy answer? | |
| 21:33:14 | cfriesen | :) | |
| 21:34:11 | mikal | cfriesen: https://www.madebymikal.com/adding-oslo-privsep-to-a-new-project-a-worked-example/ is an overly complete answer with way more detail than you care about. | |
| 21:34:11 | cfriesen | I'm easy | |
| 21:34:38 | mikal | cfriesen: but the short answer is its pretty trivial. Decide where in nova/privsep to put the method. Add the method with the decorator. Call the method from other places. Profit. | |
| 21:35:13 | mikal | cfriesen: the only real warts are around style (don't pass in command lines, pass in args that are used to generate a command line); and unit testing hoops if efried notices that test coverage has dropped. | |
| 21:35:30 | cfriesen | sweet, thanks. | |
| 21:35:45 | mikal | cfriesen: I'd be happy to do a worked example in a blog post, but you're the first person to ever ask because no one loves me and my life is meaningless. | |
| 21:36:20 | cfriesen | (I'm trying to resolve a problem with emulated TPM and cold migration...the software TPM daemon runs as root but nova doesn't, and I need to copy the root-owned file to the dest node.) | |
| 21:36:27 | mikal | cfriesen: although when I think of it, https://review.openstack.org/#/c/624593/ is probably close enough to documentation for now. Copy that. | |
| 21:36:54 | mikal | Oh, so you just want to read a file with escalated permissions? | |
| 21:36:58 | mikal | That's already implemented IIRC. | |
| 21:37:36 | mikal | cfriesen: http://git.openstack.org/cgit/openstack/nova/tree/nova/privsep/path.py#n27 is what you want to read a file as root. | |
| 21:37:48 | cfriesen | I suspect I need to modify SshDriver.copy_file() and RsyncDriver.copy_file() to optionally run with root privileges. | |
| 21:38:00 | mikal | cfriesen: with maybe a http://git.openstack.org/cgit/openstack/nova/tree/nova/privsep/path.py#n73 thrown in for the destination file. | |
| 21:39:03 | mikal | Ummmm. Why not just change the permissions on the file, sync, then change the permissions at the destination? | |
| 21:39:13 | mikal | Instead of running the sync as root? | |
| 21:40:17 | cfriesen | might work. It's tricky because root might not even be set up for passwordless ssh, and the resize code path doesn't do RPCs to set things up the way live migration does. | |
| 21:40:35 | cfriesen | still studying the flow to figure out how to make it work | |
| 21:41:47 | mikal | Ok, well I'm going to put on pants and stuff, but ping me if you need more random pointers. | |
| 21:50:21 | openstackgerrit | Eric Fried proposed openstack/nova master: WIP: Make fake_libvirt_utils DIAF https://review.openstack.org/641489 | |
| 21:50:31 | efried | mikal, mriedem: let's see how that goes ^ | |