| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-09-09 | |||
| 19:34:23 | artom | Oh, there's still https://review.opendev.org/#/c/634606/75/nova/compute/manager.py@7223 | |
| 19:34:30 | artom | dansmith, ^^ you might have ideas, if you have time | |
| 19:34:40 | artom | Sorry to ping and run, but that's what I have to do | |
| 20:12:27 | openstackgerrit | Merged openstack/nova master: Func test for migrate server with ports having resource request https://review.opendev.org/655113 | |
| 20:19:00 | openstackgerrit | Shilpa Devharakar proposed openstack/nova master: Nova object changes for forbidden aggregates request filter https://review.opendev.org/671072 | |
| 20:19:01 | openstackgerrit | Shilpa Devharakar proposed openstack/nova master: Add a new request filter to isolate aggregates https://review.opendev.org/671075 | |
| 20:19:01 | openstackgerrit | Shilpa Devharakar proposed openstack/nova master: DB API changes to get non-matching aggregates from metadata https://review.opendev.org/671074 | |
| 20:19:02 | openstackgerrit | Shilpa Devharakar proposed openstack/nova master: Docs for isolated aggregates request filter https://review.opendev.org/667952 | |
| 20:23:18 | dansmith | efried: around? | |
| 20:23:23 | efried | dansmith: yah | |
| 20:23:47 | dansmith | efried: gibi was +2 on this before I found one more thing that needed fixing in the object: https://review.opendev.org/#/c/671072/17..18/nova/objects/request_spec.py | |
| 20:23:56 | efried | yah | |
| 20:23:59 | dansmith | efried: permission to fast-approve since she made the change | |
| 20:24:02 | efried | please do | |
| 20:24:38 | efried | she had to fix some stuff up the pile for the new non-defaulting-to-set() as well. | |
| 20:25:31 | dansmith | I saw | |
| 20:25:55 | dansmith | efried: this one had a couple test tweaks from me which she also did, so I shan't fast-approve it, but maybe you can skim and get it underway: https://review.opendev.org/#/c/671075/20 | |
| 20:26:06 | efried | on it now | |
| 20:26:52 | dansmith | I don't think I've looked at the doc patch since it was megasplit so I'll leave that to you and gibi which have looked recently | |
| 20:29:47 | efried | ack. Thanks for your work on this dansmith | |
| 20:30:14 | shilpasd | dansmith: Gibi: Eric:Stephan: thank you for review | |
| 20:30:48 | dansmith | aye | |
| 20:52:05 | openstackgerrit | Shilpa Devharakar proposed openstack/nova master: Docs for isolated aggregates request filter https://review.opendev.org/667952 | |
| 20:52:06 | openstackgerrit | Eric Fried proposed openstack/nova master: DNM: uncover func tests with mismatched allocations https://review.opendev.org/681122 | |
| 21:12:13 | melwitt | mriedem: downstream this bug (fill_virtual_interface_list fails on second attempt) was hit and they proposed a patch with a different approach from the workaround you had worked on the past. I'm curious what you think about it https://review.opendev.org/678776 | |
| 21:20:03 | mriedem | ack, just getting caught up with scrollback | |
| 21:22:51 | mriedem | dansmith: so tl;dr on artom's series is we're not sure about all of the non-numa vpu pinning stuff that was recently added? | |
| 21:23:13 | mriedem | yeah this "now we have the infra in place to be able to fix this long-standing bug" | |
| 21:23:28 | artom | mriedem, sean-k-mooney tested it, there's an edge case that we're missing, but otherwise we're pretty confident | |
| 21:23:29 | mriedem | that's what i was thinking about that cpu pinning stuff being munged in | |
| 21:23:44 | artom | As I said to dansmith, at this point, it'd be more work to yank it out than to fix it | |
| 21:24:04 | artom | But I can go either way, if we want to be safe | |
| 21:24:30 | artom | It is indeed technically out of scope, for a long-standing latent thing | |
| 21:24:45 | mriedem | "also more churn doesn't make re-review easier" yup | |
| 21:24:53 | mriedem | i feel like this is mostly a re-review each time for me | |
| 21:25:20 | mriedem | it's not terrible though, but it definitely made me not able to feel comfortable with +2ing https://review.opendev.org/#/c/635229/ | |
| 21:26:04 | artom | mriedem, I get you. I keep finding more stuff to fix. Maybe the non-NUMA instance thing I jumped the gun on | |
| 21:26:17 | artom | Should have left it as is | |
| 21:40:00 | sean-k-mooney | mriedem: in the one case that is not covered by atroms code for the non numa instances it reverts to the master behavior | |
| 21:40:13 | sean-k-mooney | mriedem: in the other cases the behaivor appears to be correct | |
| 21:40:54 | sean-k-mooney | that said we could drop that change or add support for the last edgecase | |
| 21:41:20 | sean-k-mooney | both of which are tangental to the main content of the series | |
| 21:43:42 | mriedem | melwitt: i left comments, that's a bit hard to follow | |
| 21:43:48 | mriedem | obviously needs tests and such | |
| 21:44:05 | mriedem | it's been a long time since i remembered what triggered that bug | |
| 21:44:35 | mriedem | sean-k-mooney: mo cases mo problems | |
| 21:45:24 | mriedem | also, if that non-numa stuff is tracked in the LibvirtLiveMigrateNUMAInfo object, isn't that confusing given the name of the object? | |
| 21:46:06 | sean-k-mooney | mriedem: well as i said the behvior is either identical to the behavior on master or fixed so its stitcly better. yes that did occure to me but i didnt think of a better name yet | |
| 21:46:37 | melwitt | mriedem: ack, thanks | |
| 21:47:02 | mriedem | sean-k-mooney: how about just not putting non-numa things a numa object for migrate_data? | |
| 21:47:04 | sean-k-mooney | we could uses LibvirtLiveMigrateTopologyInfo | |
| 21:47:10 | sean-k-mooney | to mirror the toplogy api? | |
| 21:47:23 | sean-k-mooney | ya we could do that too | |
| 21:47:26 | mriedem | that or just put a different field on LibvirtLiveMigrateData | |
| 21:47:53 | mriedem | or otherwise yeah you need to make LibvirtLiveMigrateNUMAInfo more generic | |
| 21:47:56 | artom | mriedem, in any case, it seems we have enough misgivings about this first whack at it to not go ahead | |
| 21:48:04 | artom | And yeah, it's tangential to the NUMA case | |
| 21:48:19 | artom | I lumped it in because it's triggered by the same code | |
| 21:48:27 | artom | (driver._get_guest_numa_config) | |
| 21:49:11 | mriedem | sure, scope creep | |
| 21:49:24 | mriedem | killer of dreams and introducerer of regression bugs | |
| 21:49:33 | artom | Ack | |
| 21:49:36 | artom | Supper time, back after | |
| 21:50:17 | sean-k-mooney | this fell like a thing we would fix if we had an extra week but since we dont if ye prefer to remove it or move it to a patch at the end i guess that makes sense | |
| 21:51:04 | sean-k-mooney | i would perfer to keep it in in its half fixed state personllay but i also know how to work around it so ill leave that to ye to decided | |
| 21:51:38 | sean-k-mooney | this is a behavior i have been ware of for years so i kind of just assume this was going to be fixed as part of either artoms work or stephens | |
| 21:54:47 | mriedem | i felt pretty happy with the progress we had by thursday last week, we had a working CI run, you had done manual testing of upgrade paths, etc. dan and i did solid reviews, so i was thinking just fixing up those comments and going over again would do it, but the non-numa scope creep stuff threw a wrench into the mix adding what appears to be unnecessary risk | |
| 21:55:33 | mriedem | and this isn't the only thing i'm trying to review this week to get in so i'm not 100% focused on this, so frequently changing is costly | |
| 21:55:35 | sean-k-mooney | mriedem: well we can always jsut revert to that version that had been tested | |
| 21:55:45 | sean-k-mooney | ack | |
| 21:55:53 | mriedem | he'll still have to make changes, but yeah i know | |
| 21:56:12 | mriedem | it's going to need re-test either way i think since there were non-trivial changes outside of the non-numa pinning stuff since last week | |
| 21:56:44 | mriedem | melwitt: maybe we can get imacdonn to re-test, but he doesn't appear to be around, | |
| 21:56:55 | mriedem | but fwiw i was able to reproduce in devstack - i think i left notes in the bug report on how to do that | |
| 21:57:17 | mriedem | so i'm also not against just recreating in devstack w/o the fix, then applying the fix and assert the issue is gone | |
| 21:57:32 | mriedem | this security groups crap all gets burned out with nova-net at some point anyway | |
| 21:57:45 | mriedem | the fact we create a default security group when using neutron is pointless anyway since it's not used | |
| 22:01:15 | sean-k-mooney | artom: dansmith mriedem. if ye come to a decision on a path forward i will retest everything tommorow with whatever ye choose. | |
| 22:10:55 | mriedem | idk, i hate to make artom undo a bunch of work, but also don't like munging a bunch of other stuff in here just b/c it's convenient though out of scope | |
| 22:11:24 | mriedem | i'd prefer to separate the non-numa pinning stuff out | |
| 22:11:27 | mriedem | and deal with that later | |
| 22:11:46 | sean-k-mooney | i.e. make it a patch at the end | |
| 22:11:54 | sean-k-mooney | that we can merge when we get to it | |
| 22:12:02 | mriedem | yes | |
| 22:12:38 | mriedem | think of it like someone pushing a bug fix that also refactors a bunch of code unnecessarily, we're generally not cool with that | |
| 22:13:25 | sean-k-mooney | ya its an unrelated or tangentally related change in a non trivial feature | |
| 22:20:45 | openstackgerrit | Brin Zhang proposed openstack/python-novaclient master: Microversion 2.80: Add user_id/project_id to migration-list API https://review.opendev.org/675023 | |
| 22:25:47 | openstackgerrit | Merged openstack/nova master: Don't duplicate PlacementFixture in libvirt func tests https://review.opendev.org/681059 | |
| 22:40:27 | melwitt | mriedem: ack, gonna add more comments to the review as far as why this patch gets around the problem | |
| 22:40:36 | melwitt | (based on my read of the bug and code) | |
| 22:42:06 | mriedem | gibi: as for your series (when you're back) you've got a real failure in the cold migration revert with a mixed compute grenade scenario https://review.opendev.org/#/c/676140/18 | |
| 22:42:15 | mriedem | good thing you added that test coverage though :) | |
| 22:42:47 | efried | dansmith: would you please have another (hopefully last) look at the bottom two patches in the vpmem series https://review.opendev.org/#/c/678447/ | |
| 22:45:03 | efried | TIA. | |
| 22:45:03 | efried | (I would feel better if it was you doing that than me) | |
| 22:45:03 | efried | stephenfin: If you can hit the currently-bottom SEV patch, the rest is +A https://review.opendev.org/#/c/644565/ | |
| 23:47:48 | openstackgerrit | Takashi NATSUME proposed openstack/nova stable/stein: doc: Fix a broken reference link https://review.opendev.org/681141 | |
| 23:48:38 | openstackgerrit | Takashi NATSUME proposed openstack/nova stable/rocky: doc: Fix a broken reference link https://review.opendev.org/681142 | |
| 23:49:36 | openstackgerrit | Takashi NATSUME proposed openstack/nova stable/queens: doc: Fix a broken reference link https://review.opendev.org/681143 | |
| 23:49:40 | alex_xu | efried: yes, I don't found design problem and upgrade plan makes sense for me also. What I'm just doing is the manual test of the upgrade process, ensure thing works as expected now | |