| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-05-24 | |||
| 16:20:20 | melwitt | yeah... just wanted to understand whether we'll have a next step here or if this is it. sounds like realistically, there's not more we could do | |
| 16:20:23 | dansmith | yep.. IMHO, fix the short term and then let's properly design the bigger change after we see how that goes | |
| 16:20:37 | dansmith | and not armchair quarterback it the day before a holiday weekend (for some of us) :) | |
| 16:21:00 | melwitt | haha, no, I was only looking for a high-level ballpark idea of what it would look like | |
| 16:21:23 | melwitt | because when you said "separate wsgi part" initially, I didn't really know what that meant | |
| 16:50:26 | openstackgerrit | Merged openstack/nova master: Add --before to nova-manage db archive_deleted_rows https://review.opendev.org/556751 | |
| 16:58:51 | aspiers | gibi: you around? I'm trying to get a functional test to restart the compute service in a way which triggers init_host() | |
| 16:59:05 | aspiers | gibi: and I see you worked on restart_compute_service() | |
| 16:59:35 | aspiers | unfortunately that only restarts the Host, not the Service | |
| 17:02:52 | aspiers | I tried self.compute.stop(); self.start_compute() but the latter invokes nova.test.TestCase.start_service() which borks by trying to create a duplicate HostMapping | |
| 17:03:23 | aspiers | dansmith: I see your fingerprints on this code too :) | |
| 17:03:58 | aspiers | This is about the point where my newbie-ness makes it pretty hard to figure out a way forward | |
| 17:10:22 | openstackgerrit | Adam Spiers proposed openstack/nova master: Provide HW_CPU_AMD_SEV trait when SEV is supported https://review.opendev.org/638680 | |
| 17:11:40 | melwitt | aspiers: I don't know much about that but you reminded me of a patch of gibi's that might be of help https://review.opendev.org/512552 | |
| 17:12:55 | aspiers | melwitt: thanks! yeah that's the exact area of code I'm talking about | |
| 17:13:02 | aspiers | maybe that change will yield some clues | |
| 17:13:10 | melwitt | that's what I'm hoping :) | |
| 17:14:45 | dansmith | aspiers: "restart the host not the service" does not make sense to me, fwiw | |
| 17:15:29 | dansmith | aspiers: calling start_service() again is not the right thing to do, because it's really create_and_start_service() I think | |
| 17:15:45 | aspiers | yes, I think that's the problem | |
| 17:15:46 | dansmith | aspiers: if you want to re-run init_host() I would just run it | |
| 17:17:48 | dansmith | aspiers: the restart_compute_service() comment explains why doing a restart in a functional test isn't super realistic, and that's why it saves/restores the RT across the start/stop | |
| 17:18:01 | dansmith | so yeah, I think you just want to re-run init if that's what you're trying to do | |
| 17:18:05 | aspiers | OK, thanks | |
| 17:18:42 | aspiers | I wasn't sure if init_host() depended on a bunch of other stuff being run before it in the same context | |
| 17:19:09 | dansmith | well, you can see what it's doing | |
| 17:19:38 | aspiers | I guess self.basic_config_check() is the only real thing done before | |
| 17:19:48 | aspiers | and I don't need to re-run that | |
| 17:20:03 | dansmith | init_host doesn't really do much other than run the driver's init really.. what about it do you need to restart? | |
| 17:20:12 | dansmith | re-run the init_instance parts? | |
| 17:20:16 | aspiers | the SEV check I've added to it | |
| 17:20:23 | dansmith | oh okay | |
| 17:20:39 | aspiers | oh, actually I don't think this will be good enough by itself | |
| 17:20:47 | aspiers | because the SEV check only sets an instance variable | |
| 17:21:00 | aspiers | which is later consumed in u_p_t() to provide the trait | |
| 17:21:10 | aspiers | but maybe _run_periodics() will take care of that | |
| 17:23:26 | aspiers | ah, looks like my previous confusion was an incorrect assumption that in functional tests, self.compute is a Host object, but it's actually a Service or something | |
| 17:24:24 | dansmith | ...as in a real deployment | |
| 17:24:41 | aspiers | I think I just misread the code somewhere | |
| 17:24:49 | aspiers | or had a stack overflow in my brain | |
| 17:33:42 | aspiers | interesting LibvirtDriver.init_host() has an unused host parameter | |
| 17:48:40 | artom | aspiers, probably another virt driver needs it | |
| 17:48:53 | aspiers | yeah probably | |
| 17:50:26 | aspiers | artom: nope, none of them do | |
| 17:50:52 | artom | aspiers, heh, you actually checked :) | |
| 17:50:58 | aspiers | just | |
| 17:51:02 | artom | Historic then, maybe? Still needed by something out of tree? | |
| 17:51:52 | aspiers | https://opendev.org/openstack/nova/commit/f51526b596f3d89cda2ec4501e1 | |
| 17:51:58 | aspiers | historic | |
| 17:52:12 | aspiers | I think someone probably forgot to remove it during some redesign | |
| 17:52:58 | aspiers | it was used here https://opendev.org/openstack/nova/src/commit/f51526b596f3d89cda2ec4501e19baf085c534e0/nova/virt/libvirt_conn.py#L163 | |
| 17:53:16 | aspiers | would be a bit of a pain to trawl through the git history to find out what removed it | |
| 17:53:24 | aspiers | or maybe not | |
| 17:53:26 | aspiers | let's see | |
| 17:53:38 | ganso | mriedem: hmm this is weird, as it failed in the gate on PS-3 (parent was fix part 1 only) | |
| 17:55:15 | mriedem | ganso: i rebased the queens backport series so there is the (1) original fix, (2) the part 2 fix which makes the functional test pass and then (3) the functional test | |
| 17:55:25 | mriedem | the problem is, i commented out the fix from (2) and (3) still passed on queens | |
| 17:55:39 | mriedem | just got back to my desk though so haven't investigate | |
| 17:55:42 | mriedem | *investigated | |
| 17:56:32 | aspiers | artom: found it https://opendev.org/openstack/nova/commit/8d97118be776fcaad3053d1f93f61d339685a4ae | |
| 17:57:09 | ganso | mriedem: hmm this is weird, as it failed in the gate on PS-3 (parent was fix part 1 only) | |
| 17:57:39 | artom | aspiers, OK :) | |
| 17:57:57 | aspiers | artom: should I submit a patch to remove it? seems like technical debt | |
| 17:58:11 | aspiers | ah, would that break out-of-tree drivers? | |
| 17:58:18 | aspiers | in fact, are there any? | |
| 17:58:30 | mriedem | seems like ever since we enabled novnc in the multi-cell job there is a novnc test that continues to fail | |
| 17:58:32 | aspiers | lol, this is how little I know about nova | |
| 17:58:36 | artom | aspiers, https://github.com/openstack/nova-powervm | |
| 17:58:38 | mriedem | which is maybe bad for the consoleauth removal stuff... | |
| 17:59:02 | aspiers | artom: weird, so why is powervm also in-tree? | |
| 17:59:27 | mriedem | aspiers: there are several out of tree drivers | |
| 17:59:31 | artom | aspiers, ¯\_(ツ)_/¯ | |
| 17:59:37 | aspiers | :) | |
| 17:59:43 | aspiers | mriedem: thanks | |
| 17:59:47 | mriedem | the out of tree drivers are maintained by vendors that push more features into them than what's in the in-tree versions | |
| 17:59:51 | aspiers | gotcha | |
| 17:59:54 | mriedem | some aren't in tree at all, like lxd | |
| 17:59:58 | aspiers | thought it might be something like that | |
| 18:00:21 | aspiers | OK, so just randomly removing parameters from the ComputeDriver interface isn't cool | |
| 18:00:22 | mriedem | some we could probably deprecate from in-tree like xen, zvm and powervm | |
| 18:01:06 | mriedem | there are no guarantees on the virt driver interface since it's internal and not versoined | |
| 18:01:08 | mriedem | *versioned | |
| 18:01:24 | mriedem | we generally try to be nice and at least email to the list if we're changing an interface | |
| 18:01:28 | aspiers | right | |
| 18:02:03 | mriedem | also, yup, enabling n-novnc in the multi-cell job on 5/20 is when the test started failing https://github.com/openstack/nova/commit/c5b83c3fbca83726f4a956009e1788d26bcedde0#diff-7415f5ff7beee2cdf9ffe31e12e4c086 | |
| 18:02:07 | mriedem | melwitt: finucannot: ^ | |
| 18:02:12 | mriedem | i'm going to report a bug since i don't have time to dig in, | |
| 18:02:24 | mriedem | but that could put a damper on the remove-consoleauth shindig | |
| 18:02:38 | aspiers | well I suppose I could at least add a NOTE to the base class referencing https://opendev.org/openstack/nova/commit/8d97118be776fcaad3053d1f93f61d339685a4ae and https://opendev.org/openstack/nova/commit/f51526b596f3d89cda2ec4501e1 | |
| 18:03:07 | aspiers | to save anyone else from needing ancient archaelogy to understand why the parameter is there | |
| 18:03:12 | mriedem | i've walked backward into this conversation so i don't know what the context is | |
| 18:03:28 | aspiers | mriedem: I just noticed that the host parameter to ComputeDriver.init_host() is never used | |
| 18:03:33 | mriedem | i'd say we want the host arg in init_host | |
| 18:03:34 | aspiers | at least, not by anything in-tree | |
| 18:03:50 | mriedem | because we can use that for our fake virt driver to avoid global config | |
| 18:04:00 | mriedem | in fact i have a patch doing that i think | |
| 18:04:10 | aspiers | oh, I didn't think to check the fakevirt driver | |
| 18:04:25 | mriedem | CONF.host doesn't work in tests when you have multiple computes | |
| 18:04:36 | mriedem | https://review.opendev.org/#/c/656709/2/nova/virt/fake.py@149 | |
| 18:05:28 | artom | Wait, so we officially have a precedent for writing code *just* for CI to work? | |