| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-06-07 | |||
| 13:53:59 | stephenfin | mriedem: Yeah, if you've ideas I'd be happy to hear them. Those tests really aren't good but I'm out of ideas at this point | |
| 14:15:45 | mriedem | i'll tinker | |
| 14:21:39 | openstackgerrit | Matt Riedemann proposed openstack/nova master: WIP: Deprecate RetryFilter https://review.opendev.org/663953 | |
| 14:21:40 | mriedem | sean-k-mooney: let's see what blows up ^ | |
| 14:21:48 | mriedem | edleafe: ^ | |
| 14:24:13 | stephenfin | mriedem: Would you be okay with the named kwargs thing if it was a separate patch? I think it's a nice readability improvement but it probably shouldn't be munged with other stuff alright | |
| 14:24:24 | stephenfin | Referring to https://review.opendev.org/#/c/651311/ | |
| 14:26:04 | mriedem | i disagree that it's a readability improvement... | |
| 14:26:23 | mriedem | by that logic we'd use named kwargs on everything | |
| 14:27:01 | mriedem | stephenfin: i'm going to pull the series down and tinker with those notification tests for reschedule and if i come up with someting cleaner i can push that up and remove the kwargs in the next patch in the series while i'm at it | |
| 14:27:13 | sean-k-mooney | mriedem: i have disabled the retry filter in one of my nfv job iteration and it more orless seam to be fine so i suspect it will not blow up or not blow up too badly | |
| 14:27:41 | stephenfin | mriedem: Not everything. Just things that have a lot of arguments (it's subjective but I'd say > 5) | |
| 14:28:05 | sean-k-mooney | im on a triage call but ill file the bug and coment on your patch when im done | |
| 14:28:13 | mriedem | sean-k-mooney: bug? | |
| 14:28:31 | mriedem | it's not really a bug that we forgot to deprecate the RetryFilter in Rocky | |
| 14:28:42 | sean-k-mooney | ok | |
| 14:29:06 | sean-k-mooney | in that case ill just keep an eye on the test results :) | |
| 14:30:48 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Stop passing 'kwargs' to 'rebuild_instance' https://review.opendev.org/651312 | |
| 14:30:48 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Remove cells v1 parameter from 'ComputeTaskAPI.resize_instance' https://review.opendev.org/651311 | |
| 14:30:49 | openstackgerrit | Stephen Finucane proposed openstack/nova master: filters: Stop handling cells v1 https://review.opendev.org/651314 | |
| 14:30:49 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Stop passing 'delete_type' to 'terminate_instance' https://review.opendev.org/651313 | |
| 14:30:50 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Add reno for removed cells v1 policies https://review.opendev.org/662031 | |
| 14:30:50 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Remove nova.compute.*API() shims https://review.opendev.org/660527 | |
| 14:30:51 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Ensure controllers all call super https://review.opendev.org/660950 | |
| 14:30:51 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Remove 'InstanceUnknownCell' exception https://review.opendev.org/662411 | |
| 14:30:52 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Use named kwargs for resize_api https://review.opendev.org/663959 | |
| 14:36:32 | artom | bauzas, thanks for the note on https://review.opendev.org/#/c/644881/21, will address after this call. Also need to address mriedem's commit message feedback, which I completely forgot about the first time around | |
| 14:37:31 | mriedem | oh right you want to backport that.... | |
| 14:37:38 | mriedem | i was going to say we don't need a release note for a virt driver interface change, | |
| 14:37:42 | mriedem | but if we're backporting... | |
| 14:38:17 | mriedem | we did this recently for an ironic driver related fix and handled a TypeError in the backportable version and removed it in a follow up on master | |
| 14:39:04 | mriedem | https://github.com/openstack/nova/blob/stable/rocky/nova/compute/manager.py#L7648 | |
| 14:39:13 | mriedem | given that, we may want to follow the same pattern here | |
| 14:39:29 | mriedem | it is admittedly bending over backward for out of tree drivers which is being exceedingly nice | |
| 14:40:19 | artom | Wait, won't that leave vm_instance unset and blow up later? | |
| 14:40:52 | artom | Oh, actually no. | |
| 14:41:15 | artom | vm_power_state will be unset, but it's only used in an if | |
| 14:41:35 | artom | OK, I'll do something similar when I backport | |
| 14:41:39 | mriedem | for your change it would mean adding a try/except TypeError around the call to finish_revert_migration and if we get a TypeError, call again without the vifs_already_plugged kwarg | |
| 14:41:51 | mriedem | when you backport? if we do this, you'd do it on master, | |
| 14:41:57 | mriedem | and then follow up with a patch to remove that handling | |
| 14:42:06 | mriedem | otherwise your backport has new extra logic in it which sucks to review | |
| 14:42:25 | artom | Oh yeah *facepalm* | |
| 14:42:44 | artom | Why remove on master though? | |
| 14:43:11 | artom | That same method signature TypeError thing can also happen with oot drivers on master, no? | |
| 14:43:16 | artom | If they're really slow to update | |
| 14:44:54 | sean-k-mooney | artom: we have customers still running kilo | |
| 14:45:14 | sean-k-mooney | artom: i assume those customers are also slow to update there out of tree stuff | |
| 14:45:27 | artom | I doubt any of our customer have oot drivers :) | |
| 14:45:37 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Stop passing 'kwargs' to 'rebuild_instance' https://review.opendev.org/651312 | |
| 14:45:38 | openstackgerrit | Stephen Finucane proposed openstack/nova master: filters: Stop handling cells v1 https://review.opendev.org/651314 | |
| 14:45:38 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Stop passing 'delete_type' to 'terminate_instance' https://review.opendev.org/651313 | |
| 14:45:39 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Add reno for removed cells v1 policies https://review.opendev.org/662031 | |
| 14:45:39 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Remove nova.compute.*API() shims https://review.opendev.org/660527 | |
| 14:45:40 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Ensure controllers all call super https://review.opendev.org/660950 | |
| 14:45:40 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Remove 'InstanceUnknownCell' exception https://review.opendev.org/662411 | |
| 14:46:04 | sean-k-mooney | well not any that we support in anycase | |
| 14:47:33 | efried | I know for sure PowerVM oot driver is in production for real customers | |
| 14:48:10 | efried | if you change the virt driver interface on stable, they'll need to patch. | |
| 14:48:10 | bauzas | artom: cool with me | |
| 14:48:29 | artom | efried, right, I'm sure they exist, just most likely not among our customers | |
| 14:48:42 | mriedem | artom: with your upstream hat on, you shouldn't care about your downstream customers and OOT issues | |
| 14:49:02 | mriedem | iow, saying "our customers won't care" isn't a valid reason to not do it | |
| 14:49:11 | efried | artom: "our" meaning RH? | |
| 14:49:18 | mriedem | the upstream argument would be, "oot? f'em" | |
| 14:49:23 | mriedem | efried: yes | |
| 14:50:01 | efried | I don't know what qualifies as a RH "customer", but the products that are based on the oot PowerVM driver are packaged as some form of RH distribution. | |
| 14:50:15 | efried | so whoever's using them is a RH "customer" in some capacity. | |
| 14:50:15 | bauzas | mriedem: okay, I was thinking we could be good citizens and notify OOT maintainers, like asked in https://review.opendev.org/#/c/589085/ | |
| 14:50:40 | bauzas | anyway, I need to leave | |
| 14:51:30 | stephenfin | Who's a good person to review resize flow bugfixes? https://review.opendev.org/#/c/662522/ | |
| 14:52:10 | mriedem | stephenfin: i want to say alex_xu already had a bug / patch for that | |
| 14:52:15 | mriedem | but you'd have to ask him | |
| 14:52:36 | mriedem | stephenfin: gibi: i changed those notification sample resize reschedule tests to move the patching out of the side effect method, the diff is in here https://review.opendev.org/#/c/660774/4/nova/tests/functional/notification_sample_tests/test_instance.py | |
| 14:52:38 | artom | mriedem, efried, yeah, I meant our = RH, but... I'm saying I'll do the nice things for oot drivers in my patch, as just replying to sean-k-mooney that RH customers are most likely *not* among those affected | |
| 14:52:47 | mriedem | it's not a ton better, but it's different so see what you think | |
| 14:54:27 | mriedem | https://review.opendev.org/#/c/620874/ | |
| 14:55:27 | mriedem | stephenfin: -1 on your fix for the same reason i -1'ed alex_xu's change | |
| 14:56:00 | mriedem | sean-k-mooney: heh you even said, "this is a provisional +1 as i think this is correct assumeing we confirm the request_spec is not saved in its modifed form." | |
| 14:56:15 | mriedem | dangerous to +1 before you know that to be true | |
| 14:57:35 | sean-k-mooney | mriedem: i suspected it was true because we had whitebox test that were passing that implied the request spec was updated correctly on compute node but yes you are right i should have checked it end ot end first before +1 | |
| 14:57:52 | stephenfin | mriedem: Yeah, we were trying to figure out if it was saved anywhere but I haven't been able to trace a save on the request_spec anywhere | |
| 14:58:05 | stephenfin | Is there any reason we need to persist the request_spec on the object, tbh? | |
| 14:58:07 | mriedem | it's very clearly saved in the conductor | |
| 14:58:09 | stephenfin | *numa_topology | |
| 14:58:46 | mriedem | https://github.com/openstack/nova/blob/master/nova/conductor/manager.py#L367 | |
| 14:58:56 | mriedem | ^ is where i spend a good deal of my time fixing regressions | |
| 14:59:16 | mriedem | https://review.opendev.org/#/q/topic:bug/1669054+(status:open+OR+status:merged) | |
| 14:59:49 | mriedem | https://review.opendev.org/#/q/topic:bug/1830747+(status:open+OR+status:merged) | |
| 14:59:54 | stephenfin | I saw that, but that's only for cold migrations? | |
| 15:00:04 | mriedem | resize and cold migrate are the same flow stephen | |
| 15:00:20 | stephenfin | oh yeah | |
| 15:00:22 | sean-k-mooney | mriedem: so my original asstetion that we should use the temporay mutation context manager or somthing similar is proably correct? | |
| 15:01:06 | mriedem | sean-k-mooney: correct you're right on the money here https://review.opendev.org/#/c/662522/1/nova/compute/api.py@3562 | |
| 15:01:59 | mriedem | right on the MOONEY | |
| 15:02:00 | mriedem | get it?! | |
| 15:02:28 | sean-k-mooney | hehe i should frame that and put it on my wall | |
| 15:02:44 | mriedem | get a bad ass trucker hate made out of it | |
| 15:02:49 | sean-k-mooney | "mriedem: said i was right about somthing" :) | |
| 15:02:59 | mriedem | you're right about lots of things | |