| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-07-28 | |||
| 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 | artom | lyarwood, how does it break them? | |
| 19:08:30 | sean-k-mooney | although those proably would be the ones testing older microverions | |
| 19:08:30 | lyarwood | right so I've picked the max version in stable/queens | |
| 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 | |
| 19:14:09 | lyarwood | and stop chrun in the backports | |
| 19:14:13 | sean-k-mooney | then use 2.1 if you want | |
| 19:14:33 | lyarwood | then I'm writing tests against a version of the API we don't even support anymore | |
| 19:15:20 | sean-k-mooney | we do support it because of microversion but my point was more dont set a microverison and only popluate the minium filed in the create request you need | |
| 19:15:49 | lyarwood | right sorry I mean with microversion = None | |
| 19:15:51 | sean-k-mooney | we teachnicaly have to support very micoversion untill we go to nova v4 | |
| 19:16:02 | lyarwood | yeah true | |
| 19:16:20 | sean-k-mooney | microversion = None is the same as 2.1 | |
| 19:16:22 | sean-k-mooney | i think | |
| 19:16:23 | gmann | functional tests are with 'latest' by default i think doing with 'latest' make sense | |
| 19:16:48 | lyarwood | gmann: it's set to None in _IntegratedTestBase | |
| 19:16:54 | sean-k-mooney | gmann: that is how i have always written them unless i was testing a feature | |
| 19:17:12 | artom | gmann, I think they're none, if I have the right code in front of me: https://opendev.org/openstack/nova/src/branch/master/nova/tests/functional/api/client.py#L135 | |
| 19:17:17 | sean-k-mooney | in which case i might use the microverion it was added in | |
| 19:18:00 | sean-k-mooney | lyarwood: is _IntegratedTestBase the new base of the functional test or the old one that we should not use anymore | |
| 19:18:18 | sean-k-mooney | there are a few baseclasses we can use in the funcational tests | |
| 19:18:31 | lyarwood | sean-k-mooney: the former I think, stephenfin is talking about replacing ProviderUsageBaseTestCase with it | |
| 19:18:53 | sean-k-mooney | waith isnte ProviderUsageBaseTestCase the thing we were ment to be moving too | |
| 19:18:59 | lyarwood | https://github.com/openstack/nova/blob/master/nova/tests/functional/integrated_helpers.py#L1062-L1063 | |
| 19:22:12 | gmann | ah right, it is in ProviderUsageBaseTestCase not base and many tests side too | |
| 19:22:16 | sean-k-mooney | oh sorry _IntegratedTestBase is not the one with the weird implematnion of _wait_for_server_allocations | |
| 19:22:39 | artom | sean-k-mooney, that got fixed a while ago | |
| 19:22:46 | artom | stephenfin unified all the things | |
| 19:22:53 | sean-k-mooney | ya i rememebr | |
| 19:23:04 | sean-k-mooney | i just was not sure if we still had some remnetes | |
| 19:23:46 | openstackgerrit | Lee Yarwood proposed openstack/nova master: func: Add live migration rollback volume attachment tests https://review.opendev.org/743534 | |