| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-08-27 | |||
| 13:57:34 | artom | I need to double check, but it's probably already loaded | |
| 13:57:49 | dansmith | and/or has it on the instance which is sent via rpc anyway | |
| 13:58:31 | artom | And we can always add it to expected_attrs in the initial REST API method | |
| 13:58:52 | dansmith | you claim you already do that | |
| 13:58:55 | dansmith | also, | |
| 13:58:57 | sean-k-mooney | by the way im not really following what you are refering to are ye talkign about the LibvirtLiveMigrateNUMAInfo object | |
| 13:59:04 | dansmith | it should be lazy-loadable on the instance, and if not, make it | |
| 13:59:09 | dansmith | sean-k-mooney: no | |
| 13:59:14 | sean-k-mooney | oh ok | |
| 13:59:31 | artom | dansmith, am I? It's possible, it's been a while :) | |
| 13:59:44 | dansmith | artom: I left a question about it a few minutes ago | |
| 13:59:54 | artom | Right, but before that | |
| 13:59:57 | sean-k-mooney | oh the numa_toplogy field in https://review.opendev.org/#/c/634827/50/nova/objects/migrate_data.py@247 | |
| 14:00:28 | dansmith | artom: I think it's probably time for mriedem to take a run through these to see how he's feeling about some of the softer things I have complained about previously, | |
| 14:00:48 | dansmith | like the two nano patches in the middle, and some of the dead code stuff you're setting up | |
| 14:00:56 | dansmith | I also need to look at the functional test patch yet | |
| 14:01:20 | artom | dansmith, aha, it's loaded in the REST API layer since stephenfin's "disable LM for NUMA" patch | |
| 14:01:28 | dansmith | remind me why we can't test this with multinode in the gate? We can't have arbitrarily simple numa topo on a flavor that will always work, but exercise some of this? | |
| 14:01:55 | sean-k-mooney | do we need it for revert? initally i did not think we should need to have the source node numa topology in the LibvirtLiveMigrateData | |
| 14:02:19 | dansmith | artom: okay, well, I think that's a long walk for a short drink of water, and selective backports could break that assumption | |
| 14:03:14 | artom | dansmith, you're saying we could backport something that would stop loading instance.numa_topology in _migrate_live()? | |
| 14:03:33 | artom | That would break a whole lot of things | |
| 14:03:37 | dansmith | or backport this without that, or backport them both but at different times such that it's no longer a useful sentinel | |
| 14:03:56 | artom | NUMA LM isn't backportable... | |
| 14:03:58 | sean-k-mooney | we cant backport object change though | |
| 14:04:08 | mriedem | what are you talking about? why can't the API pre-load the instance.numa_topology field? | |
| 14:04:10 | dansmith | I'm just saying, tie the flag to the actual support, not to some other side effect you think is recent enough | |
| 14:04:42 | artom | dansmith, oh, sorry, we're talking about different things. I'll definitely replace numa_topology in migrate_data with a boolean sentinel | |
| 14:04:43 | mriedem | is there just some random, "if 'numa_topology' in instance: <make numa lm work>" logic? | |
| 14:04:56 | sean-k-mooney | mriedem: we are talking about is https://review.opendev.org/#/c/634827/50/nova/objects/migrate_data.py@247 requried or can we just make it a bool | |
| 14:04:57 | dansmith | mriedem: artom is saying the api is pre-loading it, and then he's checking somewhere to see if it's pre-loaded and determining whether or not the source is new enough to do the numa lm | |
| 14:04:57 | stephenfin | Wow, 'NUMAServersWithNetworksTest.test_cold_migrate_with_physnet' takes 12 seconds to run. That's a long-ass functional test | |
| 14:05:02 | artom | I was saying the destination will have the instance object with numa_topology loaded already | |
| 14:05:21 | sean-k-mooney | cant we just check the compute node verion in the conductor | |
| 14:05:50 | artom | sean-k-mooney, dansmith was saying there's caching that makes this fragile, IIRC | |
| 14:06:06 | dansmith | uh what? | |
| 14:06:09 | sean-k-mooney | we do it for sriov live migration | |
| 14:06:45 | artom | dansmith, there's no way I'll find the exact gerrit review now, but I definitely remember you being against checking individual compute hosts's service versions for this | |
| 14:07:13 | dansmith | artom: we do it for other things, but there was probably some other wrinkle | |
| 14:07:13 | dansmith | but | |
| 14:07:18 | dansmith | I'm not arguing for that here | |
| 14:07:28 | dansmith | I'm saying the thing you're using does not make any sense | |
| 14:07:34 | mriedem | piling on | |
| 14:08:06 | artom | dansmith, and I agree :) It's a good point, I'll change it to a boolean sentinel, and the dest can pull numa_topology from the instance object it already has | |
| 14:08:07 | mriedem | can't the dest check the source compute service version like we did for file-backed memory live migration? https://github.com/openstack/nova/blob/18.0.0/nova/virt/libvirt/driver.py#L6612 | |
| 14:08:17 | mriedem | do we need the boolean sentinel at all? | |
| 14:08:25 | sean-k-mooney | artom: we do this https://github.com/openstack/nova/blob/master/nova/conductor/tasks/live_migrate.py#L35-L58 | |
| 14:08:33 | dansmith | mriedem: we have a boolean for file-backed in that same object | |
| 14:08:39 | artom | mriedem, we need *something* for both computes to agree that they can do NUMA-LM | |
| 14:09:22 | sean-k-mooney | i would emulate https://github.com/openstack/nova/blob/master/nova/conductor/tasks/live_migrate.py#L203-L237 | |
| 14:09:28 | mriedem | so we do https://github.com/openstack/nova/blob/18.0.0/nova/objects/migrate_data.py#L237 | |
| 14:10:41 | mriedem | which is used when generating the xml on the source to pass to the dest https://github.com/openstack/nova/blob/46a3bcd80b41e99ec4923c7cf3d0f8dd8505e97c/nova/virt/libvirt/migration.py#L269 | |
| 14:12:36 | bauzas | just catching the discussion, but why can't we assume checking the RPC versions ? | |
| 14:12:45 | artom | Thinking about it, I believe the service version check wasn't accepted because we still want to allow operators to shoot themselves in the foot, so to speak | |
| 14:12:53 | bauzas | like we do for a couple of RPC intercommunication ? | |
| 14:13:05 | dansmith | bauzas: you can't see the client version from the server side in rpc | |
| 14:13:06 | sean-k-mooney | artom: we can still do it and but not make it fail | |
| 14:13:11 | artom | So we don't just block the LM entirely if one of the hosts is too old | |
| 14:13:17 | dansmith | there's still no reason to even be talking about this | |
| 14:13:23 | dansmith | all we need is to pass a boolean and we're done | |
| 14:13:30 | artom | Yeah, I'm with dansmith | |
| 14:13:36 | artom | Seems way more elegant | |
| 14:13:41 | bauzas | cool, we did that with scheduler, IIRC | |
| 14:13:50 | dansmith | conductor will strip it out if you're talking to an old compute automatically | |
| 14:13:53 | bauzas | we had a sentinel | |
| 14:14:07 | sean-k-mooney | well we can sett the bool in the conductor | |
| 14:14:22 | dansmith | no | |
| 14:14:31 | dansmith | it's migrate_data, set it in the compute | |
| 14:15:00 | dansmith | again, the only reason we're even talking about this, is artom was sending a whole topology object instead of saying "yes". | |
| 14:15:07 | sean-k-mooney | you could. we need to tell the dest if the souce supprot the feature | |
| 14:15:15 | sean-k-mooney | does that line up wiht the calls | |
| 14:15:29 | dansmith | he's already sending it from the compute at the right time | |
| 14:15:35 | dansmith | he's just saying too much. much like this discussion. | |
| 14:15:40 | sean-k-mooney | then ok | |
| 14:15:40 | artom | sean-k-mooney, I'll be blunt and pull rank (though not my own): in case two things work the same dansmith's opinion is worth more than yours :P And I say that with massive admiration for the amount of information you keep in your head | |
| 14:16:35 | sean-k-mooney | artom: fair its anorrying not to have these checks in the same place in the conductor with the other migration checks | |
| 14:16:47 | sean-k-mooney | but i agree we should just have a bool flag | |
| 14:16:51 | dansmith | no, this isn't a rank thing, it's just a simplicity thing... | |
| 14:16:54 | sean-k-mooney | so just update your current code | |
| 14:17:07 | artom | dansmith, a not very convincing thing, apparently :) | |
| 14:17:08 | dansmith | migratedata is created after we're done talking to conductor for the last time, AFAIK | |
| 14:17:20 | dansmith | so I'm not sure how conductor would ever set it for us | |
| 14:17:36 | artom | Yeah, it's created by the source compute, IIRC | |
| 14:17:42 | artom | The source driver, in fact | |
| 14:17:51 | dansmith | hence why this flag should be set by the computes | |
| 14:18:15 | dansmith | it also depends on the config of each compute, | |
| 14:18:21 | dansmith | which you can't inspect from conductor | |
| 14:18:35 | dansmith | if the two computes involved are pinned to 5.0, then this can't happen even if they both support it | |
| 14:18:38 | artom | https://github.com/openstack/nova/blob/18.0.0/nova/compute/manager.py#L6056-L6089 | |
| 14:19:21 | stephenfin | gibi: calling 'self._delete_server' at the end of the test did the trick. I don't know why. Missing mocks or something like that. Good enough for me though | |
| 14:19:36 | artom | I need to get one kid to daycare, after that it's hax time | |
| 14:20:51 | artom | mriedem, you OK with squashing, as per dansmith's -1 here: https://review.opendev.org/#/c/635229/48? | |
| 14:21:04 | sean-k-mooney | ok i was suggeting passing in the booll here https://github.com/openstack/nova/blob/master/nova/conductor/tasks/live_migrate.py#L318-L322 to check_can_live_migrate_destination but im fine with what dan suggest too | |
| 14:23:50 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Add support for translating CPU policy extra specs, image meta https://review.opendev.org/671801 | |
| 14:23:51 | openstackgerrit | Stephen Finucane proposed openstack/nova master: libvirt: Fold in argument to '_update_provider_tree_for_vgpu' https://review.opendev.org/676729 | |
| 14:23:51 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Add reshaper for PCPU https://review.opendev.org/674895 | |
| 14:23:52 | openstackgerrit | Stephen Finucane proposed openstack/nova master: libvirt: Simplify 'fakelibvirt.HostInfo' object https://review.opendev.org/678861 | |
| 14:23:56 | mriedem | artom: my head is buried in something else atm so you're going to have to wait | |
| 14:24:07 | artom | mriedem, understood | |
| 14:25:08 | stephenfin | dansmith: If you're bored, I went and wrote that online migration to allow us to drop the Liberty-era NUMA topology blobs https://review.opendev.org/#/c/537414 I don't want to brag, but I think it's perfect | |