Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-11-21
15:17:59 ayoung but that does not show up when the image boots, and I am guessing I need to pass that through to the instance somehow
15:19:13 sean-k-mooney if you pass a seperate kernel image in addtion to the root image i think we can pass the kernel arges to qemu
15:19:34 sean-k-mooney but in general im not sure how much that featuer is used or tested
15:19:57 sean-k-mooney i do not belive you can use it with just a root image
15:20:10 johnthetubaguy ayoung did you try os_command_line: https://github.com/openstack/nova/blob/1cd5563f2dd2b218db2422397c8aab394d484626/nova/objects/image_meta.py#L463
15:20:43 johnthetubaguy hmm, I am not so sure we do anything with that, ignore me
15:20:44 sean-k-mooney johnthetubaguy: isnt os_command_line jsut for lxc and other containers
15:20:57 sean-k-mooney like openvz
15:21:10 mriedem "The kernel command line to be used by the libvirt driver, instead of the default. For Linux Containers (LXC), the value is used as arguments for initialization. This key is valid only for Amazon kernel, ramdisk, or machine images (aki, ari, or ami)."
15:21:24 johnthetubaguy yeah, my bad
15:21:32 ayoung BTW, this is a pretty good argument for Nova supporting ignition the same way we do cloud-init...it happens earlier in the process
15:22:11 sean-k-mooney its also a good argument for ignition supporting the metadata service :P
15:22:12 ayoung I know that the openshift install, which works via terraform, does something to inject this value. I do not know what that is
15:22:54 ayoung So, I think the metadata service would work. I think what you are saying is that the ignition mech should default to the cloud-init URL, somehow?
15:23:35 ayoung Well, not the URL, but the host, and a separate URL specific to ignition.
15:23:48 sean-k-mooney i was suggesting that ignition could try to hit the metadta url and then load info form it
15:23:53 ayoung Yep
15:24:25 ayoung or a logical shift from it, like: http://meta-data-host/ignition
15:25:01 sean-k-mooney ayoung: anyway the way that was all ment to work was ehn you boot the vm you provide a sperate kernel image with the kernel command line parmater set and then nova woudl use the root iamge and kernel image when booting and pass the kernel args form teh kernel image to qemu
15:25:10 ayoung And Nova/metadata server then would be responsible for multiplexing between the different instances
15:26:00 ayoung I see. I don;t think that the installer (terraform) is doing any of that. But I can reproduce this afternoon and determine what it IS doing
15:35:58 mriedem efried: heh, oops https://review.opendev.org/#/c/694897/
15:36:01 mriedem not sure i/we missed that
15:36:15 mriedem i mean, it's zvm so totally forgettable but otherwise
15:37:58 openstackgerrit Dan Smith proposed openstack/nova master: ZVM: Implement update_provider_tree https://review.opendev.org/694897
15:44:14 aarents Hi dansmith, can you confirm this last upaste is ok for you since your were holding -1 on that https://review.opendev.org/#/c/670000 ? thanks !
15:45:08 dansmith aarents: yeah, I was waiting for CI before, but I'll circle back
15:45:24 dansmith aarents: we probably want to have a few people look at that and sanity check my thinking there
15:45:52 dansmith maybe gibi since he was previously okay with the other fix, at least
15:46:09 dansmith obviously mriedem is always good at everything
15:46:20 aarents yes good idea
15:47:32 mriedem umm, rescue + disk bus = i defer to lyarwood
15:47:56 dansmith eh?
15:48:08 dansmith it's not really rescue related,
15:48:11 mriedem "There is a case during rescue where this value can be mistakenly updated
15:48:11 mriedem to reflect disk bus property of rescue image (hw_disk_bus)."
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 :)

Earlier   Later