| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-06-07 | |||
| 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: Stop passing 'delete_type' to 'terminate_instance' https://review.opendev.org/651313 | |
| 14:45:38 | openstackgerrit | Stephen Finucane proposed openstack/nova master: filters: Stop handling cells v1 https://review.opendev.org/651314 | |
| 14:45:39 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Remove nova.compute.*API() shims https://review.opendev.org/660527 | |
| 14:45:39 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Add reno for removed cells v1 policies https://review.opendev.org/662031 | |
| 14:45:40 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Remove 'InstanceUnknownCell' exception https://review.opendev.org/662411 | |
| 14:45:40 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Ensure controllers all call super https://review.opendev.org/660950 | |
| 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 | bauzas | artom: cool with me | |
| 14:48:10 | efried | if you change the virt driver interface on stable, they'll need to patch. | |
| 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 | 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:15 | efried | so whoever's using them is a RH "customer" in some capacity. | |
| 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 | |
| 15:04:01 | sean-k-mooney | :) ok so it should be a fairly simple fix so are you ok to proceed with stephens version or alex's | |
| 15:04:27 | mriedem | they are both broken | |
| 15:04:29 | mriedem | equally | |
| 15:04:43 | mriedem | i've duplicated stephen's bug against alex's since alex's came first, | |
| 15:04:48 | mriedem | but alex seems to have abandoned his change effectively | |
| 15:05:05 | mriedem | by "simple fix" you mean shove the new numa topology into the migration contexT? | |
| 15:05:48 | mriedem | i'm not sure we'd need to do that if we can, on revert, build the old numa topology from the old flavor that is in the migration context before we drop the migration context | |
| 15:08:43 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Remove 'ComputeManager._reschedule' https://review.opendev.org/660774 | |
| 15:08:49 | mriedem | gibi: stephenfin: ^ updated i'll +W | |
| 15:09:08 | sean-k-mooney | by simpel fix i ment we jsut need to reset the change state on the field so that it does not get saved when the condoctor calls save. | |
| 15:09:39 | gibi | mriedem: +2, thanks | |
| 15:10:24 | mriedem | sean-k-mooney: but we actually do need to persist the new numa topology on the request spec for any subsequent move operations | |
| 15:10:58 | sean-k-mooney | mriedem: yes but we update it on the compute host | |
| 15:10:59 | mriedem | otherwise i can (1) create my server with flavor1 and numa1, (2) resize to flavor2/numa2, confirm, (3) cold migrate and now i'm scheduling with flavor1's numa1 | |
| 15:11:06 | mriedem | sean-k-mooney: the request spec? | |
| 15:11:15 | mriedem | that would be an up-call... | |
| 15:11:25 | mriedem | that also doesn't solve your revert problem | |
| 15:11:58 | sean-k-mooney | ya we i think recalualate the numa toplogy in the driver and store it in the ... instance not he request spec never mind | |
| 15:12:24 | sean-k-mooney | so ya we need to actuly save it and then revert it | |
| 15:12:29 | stephenfin | sean-k-mooney: Definitely? I know we used to mess with it in the filters https://github.com/openstack/nova/blob/master/nova/scheduler/filters/numa_topology_filter.py#L61-L68 | |
| 15:15:06 | sean-k-mooney | well see the fact the numa_fit_instance_to_host fucntion modifes the request sepc is what made me suspect this would get updated by the compute node if we booted sucessfuly | |
| 15:15:24 | sean-k-mooney | but honestly we need 1 trace teh code and two test it locally to confirm | |
| 15:15:27 | stephenfin | We probably don't want to persist that change though, in any case | |
| 15:16:32 | sean-k-mooney | proably. the compute node really should not need to modify the request_spec | |
| 15:16:50 | sean-k-mooney | well in this instance | |
| 15:16:51 | mriedem | the compute service should never be saving changes to the RequestSpec, ever, since it's an up-call to the API DB | |
| 15:16:57 | mriedem | dansmith: agree with me here? ^ | |
| 15:17:24 | mriedem | stephenfin: as i explained above, the RequestSpec.numa_topology is used in the scheduler so yes it needs to be persisted for subsequent move operations, | |
| 15:17:36 | mriedem | however, on revert we have to make sure we revert the numa_topology in the request spec to the old flavor's values | |
| 15:17:37 | dansmith | mriedem: of course | |