| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-09-09 | |||
| 18:45:05 | sean-k-mooney | if its not set then we shoudl ensure the cpuset attibute is not in the final xml othersize set it to its value | |
| 18:45:06 | dansmith | so use your judgment I think | |
| 18:45:27 | dansmith | but I think it's okay to exclude fixing that longstanding bug in your series and follow up with the actual fix, however that looks | |
| 18:46:53 | sean-k-mooney | ok so if the souce does not have vcpu_pin_set and the dest has we go form <vcpu placement='static'>1</vcpu> to <vcpu placement='static' cpuset='4-7,12-15'>1</vcpu> | |
| 18:46:54 | artom | dansmith, I know sunk costs is a fallacy, but I think at this point it'd be as much work (for me, anyways) to take it out as it would be to fix it | |
| 18:46:58 | sean-k-mooney | which is what we expect | |
| 18:47:04 | sean-k-mooney | now to check the other way around | |
| 18:47:12 | dansmith | artom: copy that | |
| 18:47:15 | artom | sean-k-mooney, I expect that to be broken | |
| 18:47:24 | sean-k-mooney | well it works correctly | |
| 18:47:45 | sean-k-mooney | source no vcpu_pin_set dest vcpu_pin_set works | |
| 18:47:54 | sean-k-mooney | i need to test the other way now | |
| 18:48:02 | artom | Yes *that's* the broken bit :) | |
| 18:48:54 | sean-k-mooney | kind of | |
| 18:49:04 | sean-k-mooney | it migrated but did not update | |
| 18:49:10 | sean-k-mooney | which is the existing master behavior | |
| 18:49:28 | artom | Yeah, it's what I mean by broken | |
| 18:49:35 | artom | Technically I could it as is :D | |
| 18:49:37 | sean-k-mooney | and a hard reboot fixes it | |
| 18:49:39 | artom | Talk about half-assed | |
| 18:49:52 | artom | "I started fixing it, but then I saw something shiny" | |
| 18:50:18 | sean-k-mooney | well you fix 3 or the 4 combindations | |
| 18:50:32 | sean-k-mooney | it works when the dest has it set or both have it set | |
| 18:50:40 | sean-k-mooney | just not when the souce only has it set | |
| 18:51:12 | sean-k-mooney | so that is better then without your code by a large margin and in the final case it matains the old behavior | |
| 18:51:58 | artom | Eh, I'd not be comfortable leaving it as is | |
| 18:51:59 | sean-k-mooney | so its better or equal to the old behavior so i woudl leave it in and maybe add a todo which we can fix and backport later when we have time | |
| 18:52:02 | artom | Take out completely, sure | |
| 18:52:06 | artom | Fix it, yeah | |
| 18:52:17 | artom | But not this half-assedness | |
| 18:52:57 | sean-k-mooney | ok well its up to you. either fix it or revert it out. im going to get stuff for dinner then contiue reviewing | |
| 18:53:15 | sean-k-mooney | ill be afk for about an hour then ill be back | |
| 18:53:48 | artom | sean-k-mooney, cheers! | |
| 19:09:08 | openstackgerrit | Merged openstack/nova master: Add min service level check for migrate with bandwidth https://review.opendev.org/680394 | |
| 19:10:09 | openstackgerrit | Merged openstack/nova master: migrate: Add bw min service level check of source compute https://review.opendev.org/680395 | |
| 19:21:00 | openstackgerrit | Merged openstack/nova master: resize: Add bw min service level check of source compute https://review.opendev.org/680396 | |
| 19:21:09 | openstackgerrit | Merged openstack/nova master: prepare func test env for moving servers with bandwidth https://review.opendev.org/655109 | |
| 19:33:48 | artom | mriedem, finished addressing your "code" feedback, the tests feedback will be after school run and supper | |
| 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 | |