| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-06-12 | |||
| 20:32:40 | mriedem | in general, i assume any image we download from glance could be signed and verified | |
| 20:32:50 | dansmith | aren't we buried deep in the image code enough that we'd verify those like anything else/ | |
| 20:33:16 | mriedem | i think so https://review.openstack.org/#/c/561262/27/nova/virt/libvirt/driver.py@7432 | |
| 20:33:28 | mriedem | this is the trouble part that's missing passing through trusted_certs | |
| 20:34:20 | dansmith | mriedem: you know I didn't write any of that code right? | |
| 20:34:49 | mriedem | IT'S YOUR GD CODE NOW SMITH | |
| 20:34:59 | openstackgerrit | Artom Lifshitz proposed openstack/nova master: Add InstanceNUMATopology to LibvirtLiveMigrateData https://review.openstack.org/566398 | |
| 20:35:00 | openstackgerrit | Artom Lifshitz proposed openstack/nova master: libvirt live migration: fit NUMA topology on dest https://review.openstack.org/567242 | |
| 20:35:01 | openstackgerrit | Artom Lifshitz proposed openstack/nova master: Service version check for NUMA live migration https://review.openstack.org/566723 | |
| 20:35:02 | openstackgerrit | Artom Lifshitz proposed openstack/nova master: DNM: Depends on intel-nfv-ci-tests to test NUMA LM https://review.openstack.org/574872 | |
| 20:35:05 | mriedem | your prints are all over this | |
| 20:35:17 | mriedem | it's 1/8 my baby now too | |
| 20:35:22 | dansmith | I really have lost most of my context on it at this point | |
| 20:35:28 | dansmith | I just split this off from the main patch for them | |
| 20:35:32 | mriedem | i know | |
| 20:35:38 | mriedem | and this change could be split into 2 now too | |
| 20:35:43 | mriedem | virt changes and then compute manager changes | |
| 20:35:45 | mriedem | but i'm not going to do that | |
| 20:37:05 | dansmith | yeah, so I guess you're right, since they don't pass trusted_certs there, | |
| 20:37:18 | dansmith | but if they did what you suggest (which I'm pretty sure I had suggested already) then they'd auto-verify | |
| 20:39:30 | mriedem | yeah i moved the low-level calls to libvirt_utils.fetch_image to just use the in-scope instance variable and pass instance.trusted_certs rather than add a new parameter to the various _try_fetch_image* methods | |
| 20:39:37 | mriedem | that's how i found this bug | |
| 20:45:12 | mriedem | gdi, i can't really tell how to assert anything that is mocked out from ImageBackendFixture | |
| 20:50:52 | efried | mriedem: example? | |
| 20:51:31 | mriedem | don't have the energy | |
| 20:51:46 | efried | mriedem: You mean from something like | |
| 20:51:46 | efried | self.mock_create_ephemeral = \ | |
| 20:51:46 | efried | mock.create_autospec(driver.LibvirtDriver._create_ephemeral) | |
| 20:51:46 | efried | self.useFixture(fixtures.MonkeyPatch( | |
| 20:51:46 | efried | 'nova.virt.libvirt.driver.LibvirtDriver._create_ephemeral', | |
| 20:51:46 | efried | self.mock_create_ephemeral)) | |
| 20:51:46 | efried | you want to assert that the _create_ephemeral mock was called? | |
| 20:52:06 | mriedem | more or less yes, | |
| 20:52:07 | mriedem | was doing: | |
| 20:52:10 | efried | mriedem: You have to make the fixture save off the actual mock | |
| 20:52:13 | mriedem | img_backend_fixture = snaz | |
| 20:52:16 | mriedem | ... | |
| 20:52:29 | mriedem | img_backend_fixture.mocked_thing.assert_called_once_with(...) | |
| 20:52:42 | mriedem | and tried mocked_thing.return_value | |
| 20:53:02 | mriedem | i thought the autospeced mock was being saved in those variables | |
| 20:53:09 | efried | mriedem: So like: | |
| 20:53:10 | efried | self.mock_create_ephemeral = \ | |
| 20:53:10 | efried | mock.create_autospec(driver.LibvirtDriver._create_ephemeral) | |
| 20:53:10 | efried | fx = self.useFixture(fixtures.MonkeyPatch( | |
| 20:53:10 | efried | 'nova.virt.libvirt.driver.LibvirtDriver._create_ephemeral', | |
| 20:53:10 | efried | self.mock_create_ephemeral)) | |
| 20:53:10 | efried | self.mriedem_mock = fx.mock | |
| 20:53:36 | mriedem | ok, well... | |
| 20:53:43 | mriedem | i'm past caring really | |
| 20:55:04 | efried | mriedem: You're right, though, that create_autospec ought to be a mock you can assert on. | |
| 20:55:28 | mriedem | i just hate the libvirt driver unit tests | |
| 20:55:31 | mriedem | but that's not news | |
| 20:55:40 | efried | mriedem: I've got +2s on the bottom nine patches in the neutron port binding series. | |
| 20:55:52 | mriedem | hot diggity dog | |
| 20:56:00 | mriedem | how many patches are there? 25? | |
| 20:56:12 | efried | mriedem: I'm going to punt on the libvirt ones, at least unless someone begs. | |
| 20:56:25 | efried | looks like 15 | |
| 20:56:26 | mriedem | https://review.openstack.org/#/q/topic:bp/neutron-new-port-binding-api+(status:open+OR+status:merged) | |
| 20:56:52 | efried | yeah, those all show up in the series | |
| 20:57:08 | mriedem | thanks for hitting those, even before it was in a runway | |
| 20:57:15 | efried | yahyoubetcha. | |
| 20:57:25 | mriedem | despite me wanting to murder you for awhile on the ksa stuff... :) | |
| 21:00:50 | efried | mriedem: If I had a shiny nickel for every time someone wanted to murder me... | |
| 21:02:17 | efried | mriedem: Here's some great news on ksa adapter, btw. Once https://review.openstack.org/#/c/574784/ is in a release, we can stop sending raise_exc=False with every request! | |
| 21:05:39 | mriedem | small victories | |
| 21:07:13 | mriedem | you know, | |
| 21:07:18 | mriedem | this trusted certs thing, | |
| 21:07:28 | mriedem | doesn't seem like something that should be plumbed through the various virt drivers at all | |
| 21:07:32 | mriedem | should/needs to be | |
| 21:07:47 | mriedem | but that's how we eventually get from compute manager to image api download() | |
| 21:07:56 | mriedem | kind of sucks though | |
| 21:08:18 | mriedem | it doesn't have anything specific to do with the virt drivers at all | |
| 21:08:20 | mriedem | nor the guest | |
| 21:10:00 | mriedem | like, could we do something like monkey patch or partial the image download api method from the compute manager flows that call driver.spawn() and pass the instance.trusted_certs right there to download()? | |
| 21:10:06 | mriedem | so the virt drivers don't need to plumb all that in | |
| 21:10:23 | mriedem | dansmith: ^? | |
| 21:10:57 | dansmith | I mean | |
| 21:10:58 | dansmith | yes? | |
| 21:11:28 | dansmith | that seems kinda gross | |
| 21:11:39 | mriedem | i'm not saying it's awesome | |
| 21:11:42 | dansmith | the virt driver is just passing instance.trusted_certs to the image download stuff right? | |
| 21:11:53 | mriedem | but neither is doing this exact same plumbing in all of the virt drivers for someting that's virt agnostic | |
| 21:12:15 | dansmith | yeah, okay, but .. we already have all those paths, | |
| 21:12:32 | dansmith | we should clean it all up and unify what we can and not just partial our way to verification right? | |
| 21:12:38 | dansmith | because we need to raise something and have it handled properly, | |
| 21:12:50 | dansmith | and just doing that when the virt driver doesn't expect it is probably not "good for security" and stuff | |
| 21:13:55 | mriedem | yeah fine, | |
| 21:13:59 | mriedem | plus i'm not at all signing up for this | |
| 21:14:15 | mriedem | i'm just really sour on this by now | |
| 21:16:12 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Plumb trusted_certs through the compute service https://review.openstack.org/561262 | |
| 21:16:13 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Add trusted_image_certificates to REST API https://review.openstack.org/486204 | |
| 21:16:14 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Add notification support for trusted_certs https://review.openstack.org/563269 | |
| 21:16:15 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Add certificate validation docs https://review.openstack.org/560158 | |
| 21:16:16 | openstackgerrit | Matt Riedemann proposed openstack/nova master: WIP: Add trusted certs to feature support matrix docs https://review.openstack.org/574890 | |
| 21:20:20 | mriedem | melwitt: i think you should definitely get in on this hot trusted certs action since it's in a runway slot for the umpteenth time https://review.openstack.org/#/c/561262/ - is the current not-yet-approved bottom of the series | |
| 21:20:36 | mriedem | i will ruin the rest api change tomorrow | |
| 21:39:02 | melwitt | mriedem: I'll try. I reviewed some of it back before we had runways and it looked mostly fine to me... then people like yourself found all sorts of problems with it | |
| 22:27:05 | mnaser | does the placement api have type of way of listing all inventories in a single http request | |
| 22:27:22 | mnaser | like "detail" or something along those lines (context: looking for a quick way to see *real* amount of resources available) | |
| 22:29:57 | melwitt | you can get inventories for a resource provider in one request, but not for all resource providers https://developer.openstack.org/api-ref/placement/#resource-provider-inventories | |