| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-07-28 | |||
| 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 | |
| 19:23:48 | openstackgerrit | Lee Yarwood proposed openstack/nova master: Add regression tests for bug #1889108 https://review.opendev.org/743289 | |
| 19:23:48 | openstack | bug 1889108 in OpenStack Compute (nova) "failures during driver.pre_live_migration remove source attachments during rollback" [High,In progress] https://launchpad.net/bugs/1889108 - Assigned to Lee Yarwood (lyarwood) | |
| 19:23:48 | openstackgerrit | Lee Yarwood proposed openstack/nova master: compute: Don't delete the original attachment during pre LM rollback https://review.opendev.org/743319 | |
| 19:23:48 | openstackgerrit | Lee Yarwood proposed openstack/nova master: func: Add CinderFixture to _IntegratedTestBase https://review.opendev.org/743535 | |
| 19:23:50 | sean-k-mooney | what is confution me is that _IntegratedTestBase has a preceding underscore | |
| 19:24:02 | sean-k-mooney | and i tought we were not ment to be using it directly anymore | |
| 19:24:13 | sean-k-mooney | ane moving to the public base class | |
| 19:25:09 | sean-k-mooney | oh that todo was added 2 weeks ago https://github.com/openstack/nova/commit/c60f90cb2f8e2a30ffee5abeebd78f9eee3c6b25 | |
| 19:26:57 | sean-k-mooney | so ya one of the main deltas right now is microversion = None in _IntegratedTestBase and microversion = 'latest' in ProviderUsageBaseTestCase | |
| 19:27:29 | sean-k-mooney | https://github.com/openstack/nova/blob/master/nova/tests/functional/integrated_helpers.py#L978 vs https://github.com/openstack/nova/blob/master/nova/tests/functional/integrated_helpers.py#L1075 | |
| 19:29:55 | gmann | both are used in same amount so it is like half of functional tests run with 'None' and half with 'latest' | |
| 19:30:33 | sean-k-mooney | many other set it expcitly | |
| 19:31:14 | sean-k-mooney | or dont use either | |
| 19:31:40 | sean-k-mooney | the last regression i wrote did not use either of the base classes since the kept getting rewritten | |
| 19:31:43 | sean-k-mooney | https://github.com/openstack/nova/blob/master/nova/tests/functional/regressions/test_bug_1835822.py#L22-L23 | |
| 19:31:59 | gmann | running with 'latest' can give better coverage of latest code set and capture if something effecting functionality in latest (as new version on top of old one)code path | |