| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-07-19 | |||
| 15:42:36 | artom | alex_xu, that's what I had at some point, Dan talked me out of it | |
| 15:42:53 | sean-k-mooney | we need the data that is pass back from can live migrate on dest | |
| 15:42:59 | artom | dansmith, efried ^^ (the CONF.my_ip to init_host() thing) | |
| 15:43:01 | sean-k-mooney | to be able to generate the updated xml | |
| 15:43:04 | efried | sean-k-mooney: Is https://review.opendev.org/#/c/638680/ still waiting on https://review.opendev.org/#/c/670189/ ? | |
| 15:43:45 | sean-k-mooney | kind of. im not changeing the public interface of the function it calls | |
| 15:44:03 | sean-k-mooney | but i suspect that if i chekcout that patch locally it will still expode | |
| 15:44:23 | sean-k-mooney | i can check that quickly. | |
| 15:46:37 | efried | artom: would you mind adding a teeny unit test that asserts _check_my_ip is called during init_host? | |
| 15:48:53 | artom | efried, ack, makes sense | |
| 15:49:25 | efried | artom: I recognize that a unit test for init_host as a whole is missing, and would be an appropriate add, but unrelated... | |
| 15:49:58 | artom | efried, hrmm, yeah, went looking, didn't find anything | |
| 15:50:04 | efried | yeah | |
| 15:50:08 | artom | So, maybe as a separate fup patch on top? | |
| 15:50:25 | artom | The full unit test, I mean | |
| 15:50:35 | efried | sure, that would be great, though I would also just not notice if it never happened | |
| 15:50:44 | efried | There are lots of gaps in unit test in libvirt/test_driver | |
| 15:50:49 | efried | lots | |
| 15:51:28 | artom | My thinking it, given how much I'll have to mock to just assert this one thing being called, might as well do the full thing | |
| 15:51:40 | artom | *is | |
| 15:51:41 | sean-k-mooney | efried: its not failing because my kernel does not support sev. so https://review.opendev.org/#/c/638680/27/nova/virt/libvirt/host.py@1066 prevent the call to get_domain_capablities | |
| 15:52:20 | efried | artom: There are other tests that call init_host but don't mock a zillion things. I didn't look too deeply into how they're doing that, but... | |
| 15:53:03 | efried | probably because of FakeLibvirtDriver | |
| 15:53:24 | artom | Ah, indeed | |
| 15:53:31 | efried | naw, even the ones that use real LibvirtDriver with FakeVirtAPI | |
| 15:53:39 | efried | so, it should be possible to do it without too much mocking. | |
| 15:54:38 | sean-k-mooney | if i comment out the if i get this trace back | |
| 15:54:40 | sean-k-mooney | http://paste.openstack.org/show/754594/ | |
| 15:55:11 | efried | though that leads me to wonder how you're getting away with not mocking netifaces.interfaces() and .ifaddresses() | |
| 15:55:33 | artom | get_machines_ips gets mocked out entirely | |
| 15:55:34 | efried | artom: Is your test actually running netifaces.interfaces() on whatever system you're invoking it on?? | |
| 15:55:47 | efried | yeah, I mean all the other tests that invoke init_host() :) | |
| 15:56:00 | artom | Oh, hah | |
| 15:56:05 | artom | I guess they do? | |
| 15:56:22 | efried | that's... not great | |
| 15:56:39 | efried | unpredictable results on linux, sure failure on other platforms, nah? | |
| 15:56:48 | artom | It's supposed to be cross-platform | |
| 15:56:50 | artom | But yeah | |
| 15:56:54 | efried | Not that I would really expect test_driver to work on another platform anyway, but... | |
| 15:57:04 | sean-k-mooney | artom: it should not be actully calling it however | |
| 15:57:27 | efried | so perhaps you need to fixture out get_machine_ips (or netifaces.interfaces) at a higher level | |
| 15:57:41 | sean-k-mooney | you shoudl be able to set up a mock in the setup function once | |
| 15:57:52 | artom | Yep | |
| 15:57:54 | artom | ;_: | |
| 15:58:05 | artom | And here I was thinking I'd get to do a pure removal patch :( | |
| 15:58:21 | efried | sorry artom. Your karma ran over your dogma. | |
| 15:58:51 | artom | Is that like my kar running over my dog, just with my ma involved? | |
| 16:00:27 | efried | just so | |
| 16:03:08 | efried | sean-k-mooney: I may need some help, or you should bring in a SME, reviewing that get_dowain_capabilities patch. kashyap? | |
| 16:05:17 | sean-k-mooney | sure. i could harden it more then i did but that at least fixes the orignial issue | |
| 16:05:56 | sean-k-mooney | im still not catching the libvirt exection if we ligitetly misconfigur the config | |
| 16:07:00 | sean-k-mooney | but we wont explode any more with the defaults | |
| 16:07:52 | sean-k-mooney | efried: do you know if aspires is still working on openstack? | |
| 16:08:08 | sean-k-mooney | i have not seen them involded in the sev work lately | |
| 16:08:09 | efried | I haven't seen him in quite a while, but I hope so. | |
| 16:08:14 | efried | yeah | |
| 16:08:24 | openstackgerrit | Artom Lifshitz proposed openstack/nova master: libvirt: move checking CONF.my_ip to init_host() https://review.opendev.org/671471 | |
| 16:08:52 | sean-k-mooney | maybe i should add Boris Bobrov to the review too since they seam to have taken over the sev stuff | |
| 16:11:33 | efried | artom: That rev wfm. Just one other thing: do functional tests have the possibility of hitting netifaces now? | |
| 16:12:03 | artom | OK, this isn't a rabbit whole anywhere, it's the pit of hell | |
| 16:12:18 | artom | *hole | |
| 16:12:26 | artom | efried, but yeah, they do, good thought | |
| 16:12:56 | artom | Maybe right up in nova/test.py? | |
| 16:13:09 | efried | artom: do any func tests use the real libvirt driver? | |
| 16:13:14 | efried | if they use fake, this is n/a | |
| 16:13:30 | efried | no, I don't want a libvirt-specific fixture in nova/test.py | |
| 16:13:33 | artom | They use the fake libvirt API, but real driver IIRC, no? | |
| 16:13:53 | efried | (don't tell me if there are already libvirt-isms dirtying up the base class) | |
| 16:14:09 | efried | artom: I don't know, I'm still pretty green on all this libvirt stuff. | |
| 16:14:11 | sean-k-mooney | artom: i dont think so | |
| 16:14:40 | sean-k-mooney | the unit test in generall dont use the drivers at all | |
| 16:14:48 | sean-k-mooney | the functional test defualt to the fake dirver | |
| 16:14:58 | sean-k-mooney | execpt the function test under the libvirt folder | |
| 16:15:10 | sean-k-mooney | which do as you said | |
| 16:15:22 | sean-k-mooney | e.g. fake libvirt api + real libvirt driver | |
| 16:16:00 | sean-k-mooney | you could add a test in here somewhare https://github.com/openstack/nova/tree/master/nova/tests/functional/libvirt | |
| 16:16:01 | artom | efried, well, wait a second, we had that before, no? | |
| 16:16:20 | efried | had what before? | |
| 16:16:26 | artom | Any migration test that didn't mock out get_host_ip_addr | |
| 16:16:33 | artom | It ended up calling down to netifaces eventually | |
| 16:16:43 | sean-k-mooney | we only have 2 migration tests i think | |
| 16:16:43 | efried | seems likely, yeah | |
| 16:16:44 | artom | Same for functional tests | |
| 16:17:27 | sean-k-mooney | https://github.com/openstack/nova/blob/master/nova/tests/functional/compute/test_live_migration.py | |
| 16:18:07 | artom | I don't think live migration hit that | |
| 16:18:14 | efried | and let the gate tell me which tests are affected. | |
| 16:18:14 | efried | I'm going to write a DNM with a poison fixture that will blow up any test that hits netifaces.interfaces() | |
| 16:18:14 | efried | let's settle this... | |
| 16:19:06 | efried | which is almost, but not quite, unrelated to your patch. | |
| 16:19:19 | efried | since you've expanded the number of code paths in which it could be getting called | |
| 16:19:31 | artom | I'd be OK waiting for that to report before proceeding | |
| 16:19:39 | artom | Even rebasing on top | |
| 16:19:48 | efried | Well | |
| 16:19:55 | efried | I'm not sure I want to *keep* the poison fixture | |
| 16:20:00 | efried | although I guess that wouldn't be an awful thing. | |
| 16:20:19 | artom | Well, we're either OK with listing the host interfaces, or we're not :) | |
| 16:20:33 | artom | If we're not, which seems the case, don't even bother DNM'ing it | |
| 16:20:37 | efried | yeah, that's a fair point. | |
| 16:20:42 | efried | okay, stand by | |
| 16:38:11 | openstackgerrit | Eric Fried proposed openstack/nova master: Poison netifaces.interfaces() in tests https://review.opendev.org/671773 | |