| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-11-21 | |||
| 15:48:17 | dansmith | rescue is just one place where this side effect screws us | |
| 15:48:30 | dansmith | right, but.. the change is actually more generic | |
| 15:48:39 | dansmith | but yes, lyarwood would be a good person to look also | |
| 15:49:08 | mriedem | my gut feeling on something like this is it fixes one thing and breaks another | |
| 15:49:14 | mriedem | and i don't have the background on that | |
| 15:49:26 | dansmith | mriedem: check out my comments on the earlier set | |
| 15:49:43 | dansmith | mriedem: a patch from gary unceremoniously removed all instance.save()s he thought were unnecessary, | |
| 15:49:51 | dansmith | which removed the one right after this that was there originally, | |
| 15:49:57 | dansmith | so we just almost never save this change | |
| 15:49:59 | dansmith | and also, | |
| 15:50:10 | dansmith | making a change to instance within a "just generate me the xml" method is crazy wrong | |
| 15:50:50 | dansmith | but rescue just happens to tickle things right so we mangle the disk bus, but never save() it back to normal | |
| 15:51:06 | sean-k-mooney | we should not be saving the disk bus back but we shoudl be usign for the rescue xml | |
| 15:52:46 | sean-k-mooney | i dont think there has ever been an expection that the /dev/sd* names for the rescue boot would be the same as when the instnace is booted normally | |
| 15:53:06 | dansmith | sean-k-mooney: read the patches I linked in my analysis earlier | |
| 15:53:24 | mriedem | would have been useful to have that context in the commit message, if just summarized from the big comment in PS1 | |
| 15:53:39 | dansmith | sean-k-mooney: the original change from like 2012 was trying to use get_xml as a hook to update the info late after libvirt had chosen defaults | |
| 15:54:26 | mriedem | tl;dr https://review.opendev.org/#/c/119622/ removed the code that would persist this change and yet we can still incorrectly persist it in edge cases incorrectly (like rescue) and we shouldn't be modifying the instance in a _get* method anyway. | |
| 15:54:55 | sean-k-mooney | ok ya we should not | |
| 15:55:07 | dansmith | mriedem: exactly | |
| 15:55:15 | mriedem | so do that, dan can +2 and ill +W | |
| 15:55:20 | dansmith | aarents: ^ | |
| 15:59:41 | aarents | dansmith: So I rephrase commit message with more context ? | |
| 16:00:13 | dansmith | aarents: yeah just add all that context in there to make mriedem happy | |
| 16:00:23 | aarents | ok got it ! | |
| 16:00:58 | mgoddard | hi mriedem, got a minute to discuss https://review.opendev.org/#/c/684849 ? | |
| 16:15:17 | mriedem | mgoddard: sure | |
| 16:15:32 | mriedem | preface: i have lost a lot of context on that bug and fix | |
| 16:16:15 | mriedem | so i'll likely just abandon my changes and you can move forward | |
| 16:16:30 | mgoddard | do you happen to remember how the RP association becomes stale after your patch | |
| 16:16:36 | mgoddard | I can't work it out from the code | |
| 16:17:21 | mgoddard | https://review.opendev.org/#/c/684849/2/nova/tests/functional/regressions/test_bug_1841481.py@129 | |
| 16:21:35 | mriedem | i think the comment from L79 | |
| 16:21:47 | mriedem | because host1 deletes the provider between | |
| 16:21:47 | mriedem | # _check_for_nodes_rebalance and _refresh_associations. | |
| 16:24:08 | mriedem | i think because we either don't add the provider uuid to _association_refresh_time or we pop it out on failure if the provider doesn't exist | |
| 16:25:06 | mriedem | it's all linked to when the ResourceTracker calls SchedulerReportClient.get_provider_tree_and_ensure_root | |
| 16:25:11 | mriedem | and then you go down the rabbit hole | |
| 16:29:34 | mgoddard | ok, I think I see. The RP wasn't in _association_refresh_time because it hasn't been in the local tree yet. If the RP exists in placement, that means we only update _association_refresh_time after _refresh_associations is done | |
| 16:30:23 | mriedem | i'm going to say yes | |
| 16:34:17 | mgoddard | that part makes sense now. I still don't see how the node not being removed from the RT compute_nodes prevents placement from getting healed though - each time through the loop we call _update, which calls _update_placement | |
| 16:36:00 | mgoddard | I probably need to stop thinking about this, going a little mad. I ran your functional test with my patch chain and it seemed to fix the issue. | |
| 16:37:42 | efried | mriedem: yeah, I was *sure* that one was already implemented. Oh well :( | |
| 16:43:22 | mriedem | mgoddard: i don't want to think about it anymore either which is why i abandoned my changes | |
| 16:54:07 | mgoddard | mriedem: hopefully you (or someone) will face thinking about it to review my patches at some point | |
| 16:57:17 | mriedem | can i get a stable core here? https://review.opendev.org/#/q/topic:bug/1849409+branch:stable/rocky | |
| 16:57:34 | mriedem | the changes to queens/pike/ocata are dependent on that since i have to redo them | |
| 16:59:38 | mriedem | melwitt: were these something you wanted to get downstream? https://review.opendev.org/#/q/status:open+project:openstack/nova+branch:stable/stein+topic:heal_allocations_dry_run | |
| 16:59:47 | mriedem | i know eandersson was saying he used those in rocky | |
| 17:01:00 | mriedem | bauzas: could you take a look at these train backports? https://review.opendev.org/#/q/status:open+project:openstack/nova+branch:stable/train+topic:bug/1852610 | |
| 17:02:38 | melwitt | mriedem: I don't know that it's come up specifically (everything's on queens) but yeah definitely could use it I'm sure. really most likely is I'd want to get heal_allocations in queens in the first place (downstream) and then backport the --instance and --dry-run too | |
| 17:02:48 | donnyd | mriedem: :( | |
| 17:03:14 | melwitt | I dunno how doable that would be, I haven't tried it yet | |
| 17:03:27 | mriedem | donnyd: what? the lxc unicorn ci job that no one cares about? | |
| 17:03:35 | donnyd | I care | |
| 17:03:37 | donnyd | Lol | |
| 17:04:13 | mriedem | "care" and "care enough to work on" are different things, and i don't care enough to work on that anymore | |
| 17:04:54 | donnyd | Hence my :( | |
| 17:05:01 | mriedem | yeah i know | |
| 17:05:05 | mriedem | freedom ain't free and all that | |
| 17:06:04 | donnyd | Well I am very appreciative of the time that has been put in | |
| 17:07:12 | melwitt | https://www.youtube.com/watch?v=tzW2ybYFboQ | |
| 17:07:35 | donnyd | melwitt: zomg lol | |
| 17:07:59 | melwitt | :) | |
| 17:08:22 | mriedem | heh yo'uve never seen that? | |
| 17:08:34 | mriedem | it's the only reason i say it | |
| 17:09:21 | donnyd | Oh I have, and it makes me laugh every time | |
| 17:09:33 | mriedem | costs about a buck-o-five | |
| 17:10:05 | mriedem | ok on that note, my wife wants me out of the gd house for a few hours so i'm going to lunch, errands and then to be driven crazy working at a coffee shop so bbiab | |
| 17:17:14 | openstackgerrit | Lee Yarwood proposed openstack/nova master: block_device: Copy original volume_type when missing for snapshot based volumes https://review.opendev.org/694497 | |
| 17:59:05 | sean-k-mooney | gibi: our downstream qa just found an issue with how we report the bandwidth provires | |
| 18:00:26 | sean-k-mooney | nova creates the compute node rp with the hypervior_hostname as the RP name | |
| 18:00:52 | sean-k-mooney | which means if you change the compute node host with the host config value in the nova and neutron config | |
| 18:01:19 | sean-k-mooney | we cannot find the root RP | |
| 18:04:23 | sean-k-mooney | so for it to work compute node host and hypervior_hostname meed to match | |
| 18:04:38 | efried | that sounds like a problem with how we report providers in general. Are you really supposed to be able to change that config for an existing service? | |
| 18:04:52 | efried | do we actually rename the provider correctly in that case? | |
| 18:05:12 | sean-k-mooney | this is a clean deployment | |
| 18:05:15 | sean-k-mooney | so no rename | |
| 18:05:25 | sean-k-mooney | but that would also be a problem | |
| 18:07:02 | sean-k-mooney | efried: http://paste.openstack.org/show/786502/ | |
| 18:08:06 | sean-k-mooney | efried: nova is correctly using the host confiv vlaue "sriov01.localdomain" for the host as is neutron and that is also the vlaue set in the neutron port bindings | |
| 18:08:31 | sean-k-mooney | efried: the rp uses the hypervior_hostname which makes sense for ironic | |
| 18:08:54 | sean-k-mooney | but for libvirt this is an issue | |
| 18:09:25 | sean-k-mooney | sriov01.mobius.lab.eng.rdu2.redhat.com is actully the real hostname set in /etc/hosts | |
| 18:09:45 | sean-k-mooney | sorry /etc/hostname | |
| 18:09:54 | efried | can you get a UUID from `openstack hypervisor list`? | |
| 18:10:41 | sean-k-mooney | no it return the internal data base id intead which is a differnte issue | |
| 18:10:48 | sean-k-mooney | i gues a show might work | |
| 18:11:24 | sean-k-mooney | no no uuid | |
| 18:11:46 | efried | well, my point is that the compute node's *UUID* should be predictable (it's compute_node.uuid) | |
| 18:12:05 | efried | so, once again, trying to use RP name for *anything* is dangerous and brittle. | |
| 18:12:38 | sean-k-mooney | sure but only nova know the uuid | |
| 18:13:08 | openstackgerrit | John Garbutt proposed openstack/nova master: WIP: review comments around unit test idea https://review.opendev.org/695547 | |
| 18:13:09 | efried | that can't be true | |
| 18:13:21 | openstackgerrit | Lee Yarwood proposed openstack/nova master: block_device: Copy original volume_type when missing for snapshot based volumes https://review.opendev.org/694497 | |
| 18:13:25 | sean-k-mooney | why cant it. | |
| 18:13:27 | efried | you telling me there's no way to discover the compute node UUID from the host? | |
| 18:13:44 | efried | seems like I asked mriedem about this the other day... | |
| 18:13:58 | sean-k-mooney | not via the hyperviors api | |