| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-08-26 | |||
| 15:20:03 | artom | Though I haven't traced how that's called, exactly | |
| 15:20:11 | dansmith | mriedem: okay I didn't think they did, but it's been a long time | |
| 15:20:25 | dansmith | mriedem: okay, yeah, host state is coming back to me a bit | |
| 15:20:36 | mriedem | right, HostState is a wrapper over ComputeNode | |
| 15:20:43 | dansmith | artom: that's a blank line | |
| 15:20:51 | mriedem | i recently found out one of the weighers looks at one of these values as well | |
| 15:20:56 | artom | dansmith, dammit, I was basing it on my local code | |
| 15:20:57 | artom | sec | |
| 15:21:15 | mriedem | ah yes https://github.com/openstack/nova/blob/stable/stein/nova/scheduler/weights/cpu.py#L43 | |
| 15:21:16 | dansmith | mriedem: okay I've dumped all of that out of my head as no longer necessary, thanks | |
| 15:21:34 | dansmith | okay, so _update_usage() is writing a new numa topo for the host | |
| 15:22:04 | artom | dansmith, https://github.com/openstack/nova/blob/master/nova/compute/resource_tracker.py#L1095 | |
| 15:22:24 | sean-k-mooney | yes the werigher do look at the numa info amoung other things | |
| 15:23:05 | dansmith | artom: okay _get_usage_dict() is what I'm looking for I think | |
| 15:23:39 | dansmith | it looks like we write that compute node record a bunch of times? | |
| 15:24:00 | dansmith | because we call it from update-from-migration, which generates usage and saves it, and would do that again for other migrations and other instances | |
| 15:24:09 | artom | Yeah, I can't imagine we were too efficient with that | |
| 15:24:26 | artom | That code is a whole bunch of _update_blah calling each other | |
| 15:24:44 | artom | It works though | |
| 15:24:59 | sean-k-mooney | so artom is afctully cretein the livemigration claim here https://review.opendev.org/#/c/634606/54/nova/compute/manager.py@6458 in heck_can_live_migrate_destination | |
| 15:25:14 | sean-k-mooney | *check_can_live_migrate_destination | |
| 15:25:31 | artom | At the end of train, Windriver found that the update resources periodic task would remove the new instance's usage because IIRC instance.numa_topology wasn't being updated | |
| 15:25:44 | mriedem | end of train? | |
| 15:25:48 | mriedem | rocky? | |
| 15:25:50 | artom | Err, stein | |
| 15:25:59 | artom | I always mix those up | |
| 15:26:03 | artom | Last cycle | |
| 15:26:38 | dansmith | artom: so to summarize, this new call is a short circuit to the dest to remove the migration usage from the compute node record's topology, which would also happen if the periodic ran and noticed the migration was no longer relevant to it | |
| 15:27:02 | artom | dansmith, yep | |
| 15:27:33 | dansmith | I think part of my problem is, I've mixed up the new-world placement arrangement, with the counting quotas stuff, and then the few remaining things that still use this system | |
| 15:27:44 | dansmith | heaven forbid we account for numbers in the same place | |
| 15:28:27 | openstackgerrit | Matt Riedemann proposed openstack/python-novaclient master: Clarify --migration-type migration value as cold migration https://review.opendev.org/678593 | |
| 15:28:37 | sean-k-mooney | well this is really still used to account for assignment of indiviula resouce rather then count the number of resouce avialble | |
| 15:29:00 | artom | dansmith, what sean-k-mooney said. We need to track specific resources like CPU cores, which placement will never do, so this will never fully go away | |
| 15:29:02 | dansmith | in the pci and numa case you mean | |
| 15:29:03 | mriedem | what is "this"? | |
| 15:29:04 | dansmith | sean-k-mooney: ^ | |
| 15:29:18 | artom | this == the resource tracker, and claims | |
| 15:29:22 | sean-k-mooney | no we need to do it for pinned cpus too | |
| 15:29:25 | mriedem | BBBZZZTTT | |
| 15:29:35 | sean-k-mooney | hugepages can be totally done in placmenet however | |
| 15:29:37 | artom | We need to say CPU 3 is used, not just "we have 1 CPU used out of 4" | |
| 15:29:46 | sean-k-mooney | we just need to do the work to move it | |
| 15:29:50 | mriedem | before removing the claims for them, the RT also "tracked" vcpu/ram/disk, just not at some individual device level | |
| 15:29:55 | mriedem | so the RT did both before train | |
| 15:29:59 | artom | mriedem, yep | |
| 15:30:04 | mriedem | hence me and dansmith asking "what is this?" | |
| 15:32:08 | sean-k-mooney | artom: there is a part that im not fully following in your code. i need to do a full review again but where are you calling into the hardware module to calulate the new numa toplogy/cpus/hugepages | |
| 15:32:25 | artom | sean-k-mooney, not directly, it's hidden in the claim | |
| 15:32:43 | sean-k-mooney | that code definetly exsited in the stien version but i have not traced it in the new version | |
| 15:33:03 | sean-k-mooney | ok can i redeploy this again today and test it by the way | |
| 15:33:09 | artom | sean-k-mooney, yeah, at fist I wanted to avoid claims entirely because their code is, umm, interesting | |
| 15:33:17 | dansmith | artom: you're calling that in the claim to validate page_size, | |
| 15:33:20 | artom | But if we don't want races we have to use them | |
| 15:33:21 | dansmith | but nothing else right? | |
| 15:33:50 | dansmith | from the claim I mean | |
| 15:33:58 | artom | dansmith, the page size is special because the "normal" virt.hardware code would allow a page size change | |
| 15:33:59 | dansmith | he probably wants _get_live_migrate_numa_info() in the libvirt patch | |
| 15:34:09 | artom | Since it only goes by the flavor extra spec | |
| 15:34:09 | dansmith | https://review.opendev.org/#/c/634828/48/nova/virt/libvirt/driver.py | |
| 15:34:15 | artom | So we need extra logic to block it | |
| 15:34:20 | dansmith | yeah, I know | |
| 15:34:27 | sean-k-mooney | dansmith: artom shoudl be calling the hardware moduel/RT to caluate a new numa toplogy, validate page size and claim pCPUs on the destation | |
| 15:34:27 | dansmith | he's asking where the new guest numa topo gets created I think | |
| 15:34:45 | dansmith | sean-k-mooney: yeah, I'm not sure about that last bit | |
| 15:34:54 | dansmith | validate page size yes, in the claim | |
| 15:35:05 | dansmith | calculate a new topology, in the above link | |
| 15:35:14 | dansmith | but not sure I see where the dest claims that, tbh | |
| 15:35:19 | sean-k-mooney | dansmith: it shoudl all be don in can_fit_numa_to_host or something like that let me see if i can find it | |
| 15:35:31 | artom | sean-k-mooney, https://github.com/openstack/nova/blob/master/nova/compute/claims.py#L138 | |
| 15:35:53 | sean-k-mooney | yes | |
| 15:35:55 | sean-k-mooney | hardware.numa_fit_instance_to_host | |
| 15:36:11 | sean-k-mooney | is the entry point to all the nuam/hugepage/pinning code | |
| 15:36:20 | dansmith | oh, that's already there because of regular migrations right? | |
| 15:36:41 | sean-k-mooney | it looks at all those requets and returns a numa toplogy object that fultiles all of the request or raise an excepiton | |
| 15:36:42 | artom | dansmith, that's actually the non-move Claim, so even booting instances use it | |
| 15:36:48 | dansmith | okay | |
| 15:36:52 | sean-k-mooney | dansmith: its tere for cold migration | |
| 15:36:59 | dansmith | yeah | |
| 15:37:01 | dansmith | whatever | |
| 15:38:48 | sean-k-mooney | if hardware.numa_fit_instance_to_host does not raise one of like50 exceptions then the host is valid and it returns the numa toplogy | |
| 15:39:40 | artom | exception.TooHardIGiveUpDoItYourself | |
| 15:39:52 | artom | exception.TheCloudWasAMistak | |
| 15:58:59 | sean-k-mooney | artom: :) that might be clearer then some of the messages we raise | |
| 16:01:52 | artom | sean-k-mooney, hah! exception.TurnBackWhileYouCan | |
| 16:30:39 | stephenfin | bauzas: If you haven't already gone home, want to send this through? https://review.opendev.org/#/c/672336/ | |
| 16:31:05 | bauzas | stephenfin: sure, that will help not going crazy with my visa application form | |
| 16:32:48 | bauzas | stephenfin: ouch, a bit hard to review given we need to make sure we don't regress on the test coverage | |
| 16:32:55 | bauzas | did efried checked it too ? | |
| 16:33:50 | efried | yeah, I did. | |
| 16:33:51 | stephenfin | Yeah, I haven't removed anything | |
| 16:33:55 | stephenfin | It's just moving things around | |
| 16:34:06 | stephenfin | I add a good few things to that later so I wanted it broken up | |
| 16:43:19 | bauzas | ok, let's make a trust bond | |
| 16:47:13 | efried | fungi: stephenfin is quoting you here https://review.opendev.org/#/c/677969/ as agreeing we don't need eggs. If that's so, would you mind throwing a +1 on there for me? | |
| 16:47:56 | fungi | efried: sure, just a sec | |
| 16:50:43 | efried | fungi, stephenfin: okay, just as a sanity check, I codesearched: http://codesearch.openstack.org/?q=egg%3Dnova&i=nope&files=&repos= | |
| 16:51:58 | fungi | as far as i know, that's merely how you tell pip what to name the package it's installing. in modern usage it's actually going to build a wheel from the code there | |
| 16:52:20 | fungi | because it's not starting from an actual package, pip doesn't know what the package name for that repository should be | |
| 16:52:49 | fungi | i believe the name of the option is a bit of unfortunate legacy naming for the sake of backwards compatibility | |