| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-06-22 | |||
| 18:20:31 | artom | Well, both hosts know a bit, but don't have the whole picture | |
| 18:20:40 | dansmith | but, I dunno, generating xml in the ether without somewhat insta-validation like we get from passing it straight to libvirt is a recipe for weird issues I think | |
| 18:20:43 | artom | So, I think it makes more sense for the source to do it, but yeah, not idea | |
| 18:20:45 | artom | *ideal | |
| 18:21:08 | artom | Well, I wouldn't be using it directly | |
| 18:21:28 | dansmith | using what directly? libvirt? | |
| 18:21:35 | artom | The idea would be to update the bits of the source XML that have "newer versions" in the XML from the dest | |
| 18:21:42 | artom | Using the XML | |
| 18:21:56 | artom | As in, it won't just get .format_dom() into the XML the source sends | |
| 18:22:05 | artom | It'll be cherry-picks, so to speak | |
| 18:22:13 | dansmith | my point being you pass xml from libvirt version X with nova/libvirt/qemu config Y to another host, it mangles it according to libvirt version Z and config (oops out of letters) and then passes it back, which the source then passes to the destination again in the migration, where it could fail | |
| 18:22:53 | dansmith | generating a <cpu_pinning_stuff> fragment on the destination side and passing it back to the source makes more sense to me, if that's what you mean | |
| 18:23:09 | artom | It's exactly what I mean | |
| 18:23:16 | dansmith | okay, 52% on that | |
| 18:23:34 | dansmith | however, don't we have some FooBarNUMATopology objects that self-serialize we could use in migrate data for that? | |
| 18:23:47 | artom | Yeah, which is what the spec calls for | |
| 18:23:55 | dansmith | I like that a lot better, fwiw | |
| 18:23:59 | artom | But I'd end up replicating the code that does all the calculations | |
| 18:24:11 | dansmith | why? | |
| 18:24:30 | dansmith | don't we do the hardware.fit_to_underpants code on the objects? | |
| 18:24:37 | dansmith | (I dunno, it's friday) | |
| 18:25:01 | artom | dansmith, so, starting from https://github.com/openstack/nova/blob/18.0.0.0b1/nova/virt/libvirt/driver.py#L5171 | |
| 18:25:34 | artom | The instance_numa_topology is definitely used, and if the dest would pass a few more bits of into to the source it works out sort of OK | |
| 18:25:37 | dansmith | ah, it just returns the config, okay, | |
| 18:25:48 | dansmith | the destination just needs to return the other inputs to that function right? | |
| 18:25:49 | artom | But just look at the code for _get_guest_numa_config | |
| 18:26:04 | artom | Or _get_guest_memory_backing_config | |
| 18:26:14 | artom | That would all have to done again on the source | |
| 18:26:17 | dansmith | yeah, s'pretty gross | |
| 18:26:29 | dansmith | yeah, but, who cares? | |
| 18:26:37 | artom | About what? | |
| 18:26:48 | dansmith | about doing it again on the source before a migration | |
| 18:27:02 | artom | Unless we do some major refactoring, it'd have to be new code | |
| 18:27:13 | dansmith | wait, what? | |
| 18:27:26 | artom | Because https://github.com/openstack/nova/blob/18.0.0.0b1/nova/virt/libvirt/migration.py#L80 is what updates the XML on the source | |
| 18:27:29 | dansmith | oh, | |
| 18:27:32 | artom | Bare function not inside any class | |
| 18:27:35 | dansmith | get_host_numa_topology() | |
| 18:27:39 | artom | With no access to the driver | |
| 18:28:31 | dansmith | I think the key is that we have to look at host numa topo there, which means the dest has to send back the full topology for the source to use instead of its own, right? | |
| 18:28:55 | artom | Yeah | |
| 18:29:02 | artom | Or send the bits of the host numa topo that are important | |
| 18:29:08 | dansmith | yeah | |
| 18:29:14 | dansmith | well, I dunno | |
| 18:29:19 | artom | But... it seems to make more sense to have the dest calculate all that | |
| 18:29:29 | artom | Except we have no sensible way to send that over the wire | |
| 18:29:53 | dansmith | I see that sending the xml fragment would be more expedient | |
| 18:30:05 | dansmith | what does the guest config look like? | |
| 18:30:06 | artom | Or even a pickled blob | |
| 18:30:19 | dansmith | I mean, can we objectify the results of that method without too much fanfare? | |
| 18:30:35 | dansmith | the config fragment we need I mean | |
| 18:30:38 | artom | Lots of boilerplate code, but could be done | |
| 18:30:50 | artom | There's some nested "objects" in there too | |
| 18:31:03 | dansmith | can you link or pastebin me what it looks like? | |
| 18:31:04 | artom | So not trivial, but not super annoying either | |
| 18:31:37 | artom | Gimme a sec, there's a named enum in there and I need to find the actual objects | |
| 18:32:14 | artom | So there's https://github.com/openstack/nova/blob/18.0.0.0b1/nova/virt/libvirt/config.py#L1883 | |
| 18:32:58 | dansmith | no, I want to see an actual xml blob | |
| 18:33:01 | artom | Oh | |
| 18:33:18 | dansmith | surely we have something in the tests? (/me is scraping for it) | |
| 18:33:40 | artom | We must - or I can just spin up an instance and dump you that | |
| 18:34:53 | dansmith | is it <numatune> ? | |
| 18:35:48 | artom | There's a bunch of distinct ones, I actually don't know if there's a sinle "parent" element | |
| 18:35:56 | artom | *single | |
| 18:36:12 | dansmith | and some vcpu stuff I guess | |
| 18:36:44 | artom | <cputune> as wlel | |
| 18:36:46 | artom | *well | |
| 18:37:03 | dansmith | right, so at the end of the day, you're going to generate some new xml document which contains a subset of the things you need and on the source have to parse that expected structure and pick out the pieces you want | |
| 18:37:19 | artom | 'zactly | |
| 18:37:29 | dansmith | which you could do super hackily by just grabbing the root of any subtree and yank it straight into the xml you're sending to the other side in migrate, | |
| 18:37:47 | dansmith | but that would be pretty trusting of the other side, and wouldn't have a very high confidence that you're generating valid xml | |
| 18:37:58 | dansmith | and if you're going to inspect every piece of it to transplant it, | |
| 18:38:14 | dansmith | I kinda think that you might as well have used our own objects for that transport | |
| 18:38:32 | artom | Yeah, good point | |
| 18:38:41 | dansmith | instead of using another complex structure that kinda looks like libvirt's but isn't exactly, and doesn't have the rest of the ingrained patterns our devs are used to | |
| 18:39:10 | dansmith | so I dunno.. if it was one blob that was <all_numa_stuff> ... </> then it'd be a little more sane to just drop it in (maybe), but since it's not... | |
| 18:39:36 | artom | That being said, it's XML by the destination for the destination | |
| 18:39:53 | dansmith | it is, but it's yanked into a larger xml doc the destination hasn't seen yet | |
| 18:39:59 | artom | Does it really make sense for the source to do anything other than, as you said, grab the subtree and stick it in? | |
| 18:40:06 | artom | Also true | |
| 18:40:28 | dansmith | well, the source will have to merge or replace the vcpu elements, for example | |
| 18:40:53 | dansmith | if there was a non-numa-related attribute that the destination doesn't know how to calculate, for example, | |
| 18:41:18 | dansmith | then the source would have to reason about the mergeification of the numa-related fragments we got from the destination, in context of the other non-numa-related ones already in the xml | |
| 18:41:20 | artom | Replace, I would think - the instance numa topology hasn't changed, so for example '<vcpupin vcpu="0"' should not change | |
| 18:41:45 | dansmith | which may not happen now or later, but it supports the "just dropping the pieces into the existing xml doc may not make sense" argument | |
| 18:41:59 | dansmith | yeah, I'm trying to come up with a concrete example that exists today, which may not be possible, | |
| 18:42:06 | dansmith | but surely may in the future | |
| 18:42:42 | dansmith | oh so, | |
| 18:42:53 | dansmith | <cputune> has a ton of stuff in it, not all numa-related right? | |
| 18:43:13 | artom | So, NUMA is sort of an abuse of language | |
| 18:43:31 | artom | CPU pinning isn't strictly NUMA | |
| 18:43:46 | artom | But that info is stored in our NUMA objects | |
| 18:43:53 | dansmith | cputune specifies the cache layout.. will the destination generate that too? can it? what if it differs from what the guest has now for some reason? the source will have to decide I would think | |
| 18:43:57 | artom | And it's one of the things we have to update | |
| 18:44:05 | dansmith | cache is? | |
| 18:44:11 | dansmith | or you mean pinning | |
| 18:44:24 | artom | Yeah pinning | |
| 18:44:29 | dansmith | yeah, | |
| 18:44:40 | dansmith | so I mean basically you're asking the destination for a lot of fragments | |
| 18:44:59 | dansmith | and just yanking those into the source document just strikes me as unsafe, or fragile | |