| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-07-28 | |||
| 16:57:04 | stephenfin | Add a TODO, resolve TODO in follow-up | |
| 16:57:14 | sean-k-mooney | ok well the first is intended to be backported and then a master only followup | |
| 16:57:24 | stephenfin | yup | |
| 16:57:35 | stephenfin | that's the overengineered idea, anyway | |
| 16:57:38 | stephenfin | :) | |
| 16:58:24 | sean-k-mooney | how did we end up with 2 elements | |
| 16:58:35 | stephenfin | read the bug report | |
| 16:58:39 | stephenfin | libvirt does it | |
| 16:59:12 | sean-k-mooney | really because it looks like we shoudl be updating the xml to only have one right | |
| 16:59:17 | sean-k-mooney | with the orginal code | |
| 16:59:35 | stephenfin | nope, we just update the first one we find | |
| 16:59:40 | stephenfin | not expecting there to be more | |
| 16:59:48 | sean-k-mooney | oh libvirt splits them | |
| 17:00:14 | sean-k-mooney | so <vcpusched vcpus="2-3" scheduler="fifo" priority="1"/> | |
| 17:00:23 | sean-k-mooney | becomes <vcpusched vcpus="2" scheduler="fifo" priority="1"/> | |
| 17:00:26 | sean-k-mooney | <vcpusched vcpus="3" scheduler="fifo" priority="1"/> | |
| 17:00:30 | sean-k-mooney | then we update the first one | |
| 17:00:59 | stephenfin | yup | |
| 17:01:43 | sean-k-mooney | then why can we not delete the vcpushed element and just not add the new one | |
| 17:01:57 | stephenfin | that's precisely what I'm doing | |
| 17:02:03 | sean-k-mooney | oh that is what your doing | |
| 17:02:10 | sean-k-mooney | in https://review.opendev.org/#/c/743568/1/nova/virt/libvirt/migration.py | |
| 17:02:17 | stephenfin | yuuup | |
| 17:02:46 | stephenfin | loads of context in the bug report and commit message itself, and it's pretty trivial to reproduce | |
| 17:02:52 | stephenfin | and with that, I'm out | |
| 17:02:54 | stephenfin | o/ | |
| 17:03:13 | sean-k-mooney | so i would just read the policy form the element before we remove it | |
| 17:03:31 | sean-k-mooney | here https://review.opendev.org/#/c/743568/1/nova/virt/libvirt/migration.py@130 | |
| 17:03:50 | sean-k-mooney | we can even assert that its the same for all element if we want | |
| 17:03:52 | stephenfin | <stephenfin> we should be trying to avoid introspecting that XML to try guess what was done previously | |
| 17:04:23 | sean-k-mooney | well yes which is why i said we shoulc caluated it form the flaovr/image | |
| 17:04:48 | sean-k-mooney | but if you dont want to do that and dont want to hardcode tehn introscpect is the only other option | |
| 17:05:19 | sean-k-mooney | wait | |
| 17:05:46 | sean-k-mooney | the check is "if 'sched_vcpus' and 'sched_priority' in info:" | |
| 17:06:03 | sean-k-mooney | what is info['sched_priority'] | |
| 17:06:24 | sean-k-mooney | its the dest numa toplogy object | |
| 17:06:39 | sean-k-mooney | which is calulated form the flavor and image | |
| 17:06:55 | sean-k-mooney | oh but that does not have the schulder right | |
| 17:07:02 | sean-k-mooney | just the priority | |
| 17:09:21 | sean-k-mooney | actully no info is this https://github.com/openstack/nova/blob/master/nova/objects/migrate_data.py#L113-L130 | |
| 17:20:29 | sean-k-mooney | stephenfin: so looking at the code you would have to pass the flavor/image down two function calls form | |
| 17:20:32 | sean-k-mooney | https://opendev.org/openstack/nova/src/branch/master/nova/virt/libvirt/driver.py#L8939 | |
| 17:20:38 | sean-k-mooney | or you could pass the instance object | |
| 17:20:49 | sean-k-mooney | i would proably pass the instance | |
| 17:21:41 | sean-k-mooney | stephenfin: im +1 on the first patch and -0.5 on the second | |
| 17:22:42 | sean-k-mooney | i think changing 2 internal function calls and caluating it form the flavor/image is cleaner then changing the objects | |
| 18:49:54 | lyarwood | stephenfin / artom ; https://review.opendev.org/#/q/topic:bug/1889108 updated btw if you have time this evening | |
| 18:55:59 | artom | lyarwood, left a comment on one of them. Not sure it's -1 worthy | |
| 18:57:57 | lyarwood | artom: well the issue itself isn't tied to any microversion but pinning on anything later than stable/queens and 2.60 just introduces pointless churn in the backports | |
| 18:58:38 | artom | lyarwood, so we could just remove the microversion= line altogether? | |
| 18:58:57 | lyarwood | artom: wouldn't it pin to latest then introducing churn in the backports? | |
| 18:59:09 | artom | How so? | |
| 18:59:48 | artom | I honestly don't know, but if we do, 'latest' for queens isn't 'latest' for master... | |
| 18:59:57 | lyarwood | microversion = None | |
| 19:00:00 | lyarwood | fun | |
| 19:01:50 | artom | lyarwood, so I went looking to remember how I did func tests for NUMA live migration | |
| 19:01:58 | artom | Which I've proposed as a U backport | |
| 19:02:13 | artom | https://opendev.org/openstack/nova/src/branch/master/nova/tests/functional/libvirt/test_numa_live_migration.py#L41 appears to run just fine in U | |
| 19:02:45 | artom | And then https://opendev.org/openstack/nova/src/branch/master/nova/tests/functional/libvirt/test_numa_live_migration.py#L387 is the "functional" reason I'm talking about it - ie, there's a need for something a specific microversion provides | |
| 19:03:22 | artom | So I would suspect 'latest' will be fine all the way down to queens | |
| 19:03:30 | lyarwood | artom: so microversion = None actually means the oldest? | |
| 19:03:38 | artom | I guess? | |
| 19:07:12 | lyarwood | artom: urgh this sucks | |
| 19:07:26 | artom | How so? | |
| 19:07:36 | lyarwood | artom: leaving it set to None breaks my use of loads of the helper methods from _IntegratedTestBase | |
| 19:07:45 | lyarwood | artom: _live_migrate etc | |
| 19:08:12 | sean-k-mooney | latest will break some tests | |
| 19:08:16 | lyarwood | feels weird writing functional tests against a version of the API we don't even support anymore tbh | |
| 19:08:30 | lyarwood | right so I've picked the max version in stable/queens | |
| 19:08:30 | sean-k-mooney | although those proably would be the ones testing older microverions | |
| 19:08:30 | artom | lyarwood, how does it break them? | |
| 19:08:47 | artom | lyarwood, my point is that microversio = 'latest' works just fine in queens | |
| 19:09:09 | sean-k-mooney | artom: what that technically means is use the latest version the nova client know about | |
| 19:09:21 | sean-k-mooney | not the latest version the api know about | |
| 19:09:23 | artom | sean-k-mooney, in func tests, so no novaclient, but yeah | |
| 19:09:25 | lyarwood | artom: latest in queens != latest in master | |
| 19:09:33 | artom | lyarwood, yep, so? | |
| 19:09:44 | artom | I fail to see the problem :) | |
| 19:10:02 | sean-k-mooney | lyarwood: artom means actully use "latest" not fine the latest microverion and use that number | |
| 19:10:16 | artom | Right, that | |
| 19:10:24 | sean-k-mooney | artom: does the "latest" string actully work in the api | |
| 19:10:30 | sean-k-mooney | i tought it was just in the client code | |
| 19:10:44 | artom | sean-k-mooney, https://opendev.org/openstack/nova/src/branch/master/nova/tests/functional/libvirt/test_numa_live_migration.py#L41 | |
| 19:10:53 | lyarwood | I'm writing this against master at the moment | |
| 19:11:01 | sean-k-mooney | cool so ya | |
| 19:11:21 | lyarwood | and I want to write it once and not have to update it with each backport as latest changes | |
| 19:11:30 | sean-k-mooney | microversion = 'latest' will be the latest version for any given release if you packport | |
| 19:11:59 | lyarwood | yeah but that's going to drop support for various things along the way meaning I'll need to constantly update the func test as I backport | |
| 19:12:16 | lyarwood | like the create args change loads between master and queens | |
| 19:12:19 | gmann | yes, 'latest' keyword is supported in API too | |
| 19:12:23 | lyarwood | networks for example | |
| 19:12:28 | sean-k-mooney | well you will have to do that anyway right | |
| 19:12:38 | gmann | it is api_version.max_api_version() | |
| 19:12:41 | lyarwood | not if I just picked the latest in queens when landing it in master | |
| 19:12:46 | artom | lyarwood, yeah, if you write your logic against APIs that only exist in U and up... | |
| 19:12:54 | artom | But I didn't see anything like that that jumped out | |
| 19:13:21 | sean-k-mooney | lyarwood: you could do that but you proably should also have a test for master then no? | |
| 19:13:47 | sean-k-mooney | what were you trying to test by the way | |
| 19:13:56 | lyarwood | nothing microversion specific | |
| 19:14:02 | lyarwood | this is just to keep the setup code sane | |