| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-08-23 | |||
| 13:02:46 | AJaeger | agreed | |
| 13:57:18 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Do not log token in the novncproxy logs https://review.opendev.org/678234 | |
| 14:05:37 | openstackgerrit | Eric Fried proposed openstack/nova master: Update SDK fixture for openstacksdk 0.35.0 https://review.opendev.org/678237 | |
| 14:11:49 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Process [compute] in $NOVA_CPU_CONF in nova-next https://review.opendev.org/672800 | |
| 14:11:53 | alex_xu | stephenfin: nice | |
| 14:13:25 | alex_xu | mriedem: I replied your email, let me know what can make you have more confidence on this | |
| 14:15:46 | dansmith | artom: you've got a bunch of legit unit test fails | |
| 14:16:15 | artom | dansmith, yeah, those have been fixed locally | |
| 14:16:16 | dansmith | artom: I'm also concerned about the validity of some of the other tests that aren't failing even though you're using the wrong dst_numa_config in some of the code and/or tests | |
| 14:16:37 | dansmith | artom: okay cool, I just lightly dusted the bottom several patches with more comments | |
| 14:16:41 | artom | dansmith, I think that's because I was asserting obj.dst_numa_config and setting it as well | |
| 14:16:46 | dansmith | up to the one you said was the top of the things you've updated | |
| 14:17:08 | artom | So the code sets dst_numa_config, and the test checks it as well | |
| 14:17:12 | dansmith | artom: okay, I guess just setting a non-field property on the object, okay | |
| 14:17:20 | artom | dansmith, yeah | |
| 14:17:34 | dansmith | well, let's make sure to get all those right :D | |
| 14:17:45 | artom | dansmith, so you're saying I need to squash the RPC changes into the top patch? | |
| 14:17:49 | dansmith | I guess no real testing was done with the updated set since the top weren't updated anyway | |
| 14:17:58 | artom | I split them to keep patch size as small as possible | |
| 14:18:04 | artom | But looks like it'll be unavoidable here :( | |
| 14:18:11 | artom | dansmith, yeah, grep took care of that | |
| 14:18:45 | dansmith | artom: squash the rpc one with the top one because of the version/capability advertisement? | |
| 14:18:52 | artom | dansmith, yeah | |
| 14:19:18 | dansmith | that's probably too obtuse of a solution.. I mean it'll work I guess, but hopefully we can split them a little more than that | |
| 14:19:37 | dansmith | flipping their order is probably better | |
| 14:20:03 | artom | Ohhh | |
| 14:20:11 | dansmith | get the code in place, just don't allow it to be called from RPC until the second one, where you actually bump the versions and add the params | |
| 14:20:12 | dansmith | and remove the extra service version bump of course | |
| 14:20:13 | dansmith | amirite? | |
| 14:20:44 | artom | Makes sense, though I need to think through how that works for params | |
| 14:21:04 | dansmith | it'll require some tweaking of things I'm sure, but that general ordering should work for you I think | |
| 14:21:34 | dansmith | artom: I also think you probably need to maintain a -W on the bottom patch, if you're going to have it in various states of not-really-working (or I can -2 it) | |
| 14:21:57 | artom | It's that sort of expected as the series goes through review? | |
| 14:22:02 | dansmith | artom: since it's going to mostly require a manual verification (which right now would fail hard), we probably want to wait to land much of it until it's actually proven to be working | |
| 14:22:03 | artom | But I can do that, sure | |
| 14:22:12 | artom | Ah, in that sense | |
| 14:22:18 | artom | Yeah, makes sense | |
| 14:22:24 | dansmith | just for someone not really paying attention, | |
| 14:22:36 | dansmith | because your bottom few patches look devilishly small | |
| 14:23:04 | artom | I'm devious like that :D | |
| 14:23:41 | artom | Actually the func test on top is only failing because I explicitly added an assertFalse because I wanted to see the logging | |
| 14:23:41 | dansmith | the claim one wouldn't be terrible to land in isolation, but the next one is an object schema commitment, which is still not ready to land, so... | |
| 14:24:00 | artom | I can remove that so we can get at least some confidence in the gate | |
| 14:24:02 | dansmith | okay I'm not sure how that can be the case with you using the wrong field names in the object, | |
| 14:24:08 | dansmith | unless it's really that fake | |
| 14:24:23 | artom | dansmith, well ok, it *was* failing because of the assertFalse | |
| 14:24:28 | dansmith | oh okay I see | |
| 14:24:32 | artom | *now* it might be broken for real :D | |
| 14:24:37 | dansmith | I hope it is :) | |
| 14:24:44 | dansmith | otherwise it doesn't do much for confidence :) | |
| 14:25:13 | artom | dansmith, yeah: "AttributeError: 'module' object has no attribute 'NUMAHostInfo'" | |
| 14:27:09 | efried | artom: qq: Est-ce que c'est vrai que, si je dis, "on ne permet pas l'un ou l'autre," même si tu connais ce que je veux dire, ce n'est pas strictement correcte? Il faudrait dire, "on ne permet pas ni l'un ni l'autre"? | |
| 14:27:38 | artom | efried, on ne permet ni l'un ni l'autre | |
| 14:27:49 | efried | sorry, yah | |
| 14:28:01 | artom | Ou même "aucun des deux n'est permis" | |
| 14:28:37 | dansmith | artom: uh, that's not the one I was worried about, so maybe there's another problem? | |
| 14:28:48 | artom | dansmith, it probably just hit that one first | |
| 14:28:59 | dansmith | point is, I didn't catch that one | |
| 14:29:45 | artom | dansmith, oh, wait, that's from an unrelated stephenfin commit | |
| 14:29:57 | artom | dansmith, tell you want, before addressing any other feedback, I'll un-WIP the func test | |
| 14:30:04 | artom | *what | |
| 14:30:14 | dansmith | un-WIP and fix? | |
| 14:30:20 | artom | Yeah, fix was implied :) | |
| 14:30:28 | dansmith | juuuuust checking :) | |
| 14:30:43 | artom | Haha, there are limits to my lazyness ;) | |
| 14:30:57 | dansmith | well, you can see my level of confidence in your laziness I guess | |
| 14:31:50 | artom | ;_; | |
| 14:32:10 | dansmith | efried: are you a candidate reviewer for this numa live migration stuff? seems like it's probably somewhat in your wheelhouse and sphere of interest | |
| 14:32:36 | dougsz | In cells v2, do you run online_data_migrations only when *all* cells are upgraded? | |
| 14:32:55 | dansmith | dougsz: you can run them against individual cells as they're upgraded or afterwards against all of them | |
| 14:33:40 | AJaeger | efried, so are fine with https://review.opendev.org/677178, https://review.opendev.org/677176 and https://review.opendev.org/677177, please ? | |
| 14:34:25 | efried | dansmith: sphere of interest, as dictated by my employer, yes. Wheelhouse, not in the slightest. I've been hoping people who actually know wtf they're looking at will do the dirty work here. | |
| 14:35:02 | efried | AJaeger: I'm not stable. dansmith might be able to push those for you, though. | |
| 14:35:03 | dansmith | efried: um, it's numa and little bit of rpc | |
| 14:35:28 | dansmith | efried: but I'm trying to make sure it's clean and then will have to look for another reviewer to +W and figured that might be you | |
| 14:35:46 | dansmith | efried: it's really not very complicated.. much less so than I originally thought when discussing the meta details | |
| 14:35:48 | dougsz | dansmith: thanks - so to upgrade one cell, i run online_data_migrations in the cell, and then again at the top level? | |
| 14:36:29 | dansmith | dougsz: "run again at the top level" meaning "run again against all cells once they're all updated" ? | |
| 14:36:46 | efried | dansmith: I will dig in if I need to, but I'm still way low on the learning curve of both numa and rpc. | |
| 14:36:51 | mriedem | alex_xu: ack, and replied | |
| 14:36:55 | dansmith | efried: opportunity? :) | |
| 14:37:00 | mriedem | alex_xu: tl;dr do whatever you want | |
| 14:37:00 | efried | yeah | |
| 14:37:36 | dansmith | heh | |
| 14:38:42 | alex_xu | I want to sleep now :) | |
| 14:39:12 | artom | efried, FWIW, the crux of it is converting parts of https://libvirt.org/formatdomain.html#elementsNUMATuning and https://libvirt.org/formatdomain.html#elementsCPUTuning to objects and back | |
| 14:39:46 | artom | So it's not touching any of the nasty code in nova/virt/hardware.py | |
| 14:40:37 | artom | Or just get alex_xu to do it, if he's more up to speed on the libvirt'y things ;) | |
| 14:40:46 | mriedem | dansmith: i'm likely your other core for numa live migration | |
| 14:40:58 | AJaeger | efried: ah, thanks | |
| 14:41:04 | dansmith | mriedem: I didn't think you'd want to | |
| 14:41:14 | dansmith | mriedem: and/or I didn't want to know what pound of flesh you'd want in return :D | |
| 14:41:15 | mriedem | i've gone through it before...in rocky probably at tihs point | |
| 14:41:27 | dansmith | yeah, I know | |
| 14:41:46 | mriedem | and i'm more familiar with the live migration and rt claims flows that many | |
| 14:41:51 | mriedem | *than | |
| 14:41:59 | dansmith | I know, but .. see above :) | |
| 14:42:07 | mriedem | however, i also have confidence issues with it | |
| 14:42:46 | mriedem | so i've been waiting for another core to dig in first, root out the obvious stuff, and someone (intel/windriver) to come along and say they've actually tested it in a lab | |
| 14:42:57 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Mask the token used to allow access to consoles https://review.opendev.org/220622 | |