Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2021-03-24
15:08:50 melwitt gibi: thanks for finding and fixing the wsgi app unit test bug!
15:23:06 stephenfin bauzas: dansmith: Yeah, that's fine if it's way too much work. With that said, I'm surprised that it _is_ a lot of work. I expected it would just be a shim, and iirc there are only a handful of places where we explicitly set the 'instance_type' kwarg directly in tests
15:23:35 stephenfin I also have a patch ready to replace instance_type everywhere else, if that was a blocker, though making it a dependency might be too much churn
15:23:49 dansmith stephenfin: I'm not sure it is a lot of work, as noted above, but I think bauzas was saying that he tried and a bunch of tests fail
15:24:33 stephenfin It's really not a big deal, I agree, but I think it's so trivial and we won't have a chance to fix it until 7.0, which it's reasonable to suggest won't ever happen :)
15:24:48 stephenfin Ah, that would be fair
15:25:46 dansmith stephenfin: it's just not a big enough deal to justify the uncertainty at this point to me
15:26:01 gibi melwitt: I saw it failing today and remembered that there was a recent change in the wsgi_app
15:26:08 dansmith it's just internal developer-only cleanliness and just isn't that important, I think is the assertion the three of us are making above
15:26:21 melwitt gibi: u rock :)
15:26:26 gibi :)
15:26:32 dansmith but it's bauzas' baby, so whatever he wants to do, IMHO
15:29:43 bauzas stephenfin: well, again, I'm not saying it's impossible to do
15:30:15 bauzas stephenfin: but in order to change the param name, we need to also modify the RPC client to provide the right name between the versions
15:30:25 bauzas and if so, we need to write tests
15:30:42 bauzas (and change some tests which would pass the original name directly)
15:30:58 stephenfin If it helps, I count 7 existing tests we'd need to change
15:31:12 bauzas stephenfin: my only concern is the time
15:31:15 dansmith bauzas: yeah, also the new tests, for sure
15:31:17 bauzas RC1 is tomorrow
15:31:37 bauzas and we can't merge later if we want this for Wallaby
15:33:02 stephenfin ack, okay that is super close
15:33:15 tosky melwitt: thanks, will do; I suspect master is not affected (we switched to focal since then, and probably ceph version), but let's see
15:33:46 melwitt ack, sounds cool
15:36:37 bauzas stephenfin: but again, your concern is valid
15:36:53 bauzas what we can do for Xena is at least to change the var name within the methods
15:37:06 bauzas and just keep this ugly param name as it is
15:37:15 bauzas until we cut a new RPC version
15:37:45 bauzas but writing a good fat comment above the method explaining that 7.0 should change this param would also help to not miss the target again
15:46:30 gibi lyarwood: I have a question/suggestion in https://review.opendev.org/c/openstack/nova/+/781138/4/nova/compute/api.py#5495
15:54:47 openstackgerrit Ghanshyam proposed openstack/nova master: DNM: testing tempest https://review.opendev.org/c/openstack/nova/+/782798
15:58:41 openstackgerrit Stephen Finucane proposed openstack/nova master: DNM: Rename 'instance_type' parameter https://review.opendev.org/c/openstack/nova/+/782804
15:59:08 stephenfin bauzas: ^ I can change my vote if you _really_ don't want to do this, but fwict that's all we need ^
16:00:02 stephenfin I couldn't find an example of a good test to make sure the calls are translated. I could do a simple mock of the v6 method and use 'assert_called_with', but we don't use named kwargs in the v5 proxy so there isn't really anything to check
16:00:31 bauzas stephenfin: first thing, you'd need to rebase your change on top of my RPC bump so we would also need to modify the 5.0 proxy to pass the right arg name
16:00:40 stephenfin did I not do that?
16:01:01 stephenfin yeah, it's on top of your change already
16:01:06 bauzas oh
16:01:12 bauzas sorry, missed this
16:01:13 bauzas but,
16:01:30 bauzas we need to pass the right arg within the 5.0 proxy
16:01:49 stephenfin see above - you're doing that already
16:02:10 dansmith the client needs to pass a different arg for 5.x
16:02:21 bauzas like https://review.opendev.org/c/openstack/nova/+/782804/1/nova/compute/manager.py#10706
16:02:22 dansmith and we need a test for both modes
16:02:33 stephenfin aha, gotcha. Sec
16:02:42 bauzas stephenfin: we call the 6.0 method within the 5.0 proxy one
16:02:50 lyarwood gibi: yup good point, it's always provided by c-vol but yeah it isn't required in the api so .get() is safer
16:02:55 bauzas so, instance_type should be transformed into flavor
16:03:00 bauzas within the proxty
16:03:07 gibi lyarwood: if it is always provided then we should document it in our API ref
16:03:19 bauzas stephenfin: and then we would need to test the API versioning
16:03:21 dansmith bauzas: and the client needs the same, based on the version pin
16:03:24 bauzas yup
16:03:51 bauzas https://review.opendev.org/c/openstack/nova/+/782804/1/nova/compute/rpcapi.py#963
16:04:15 bauzas we should say s/t like "if not can_send(6.0): instance_type = flavor
16:04:25 dansmith no, you can't do that,
16:04:28 bauzas and then pass instance_type instead of flavor
16:04:31 dansmith because it'll break if we introduce a 6.1
16:04:38 dansmith it needs to check the first digit I think
16:04:50 bauzas oh yea
16:04:52 bauzas sorry
16:04:54 bauzas indeed
16:05:04 bauzas we would only pin the change to 6.0
16:05:25 bauzas but yeah
16:05:51 dansmith for now, but then we have to fix that immediately before the next RPC minor or we'll start sending the 5.x version for 6.1, so I think we need to be more careful
16:05:58 lyarwood gibi: I can do that in a fup but I think that would be a microversion bump right?
16:06:09 openstackgerrit Stephen Finucane proposed openstack/nova master: WIP: Convert client also https://review.opendev.org/c/openstack/nova/+/782810
16:06:19 gibi lyarwood: if it is already provided and we are not rejecting it then it is a bug in the API ref
16:06:19 stephenfin dansmith: like that? (need tests obv) ^
16:06:32 bauzas dansmith: right, and 7.0 bump could also just support 6.y where y>1
16:06:35 gibi I guess we are not really enforcing schema on that api
16:06:52 gibi lyarwood: it is totally OK to fix the API ref separately
16:06:54 gibi later
16:06:58 lyarwood kk
16:07:02 openstackgerrit Lee Yarwood proposed openstack/nova master: compute: Reject requests to commit intermediary snapshot of an inactive instance https://review.opendev.org/c/openstack/nova/+/781138
16:07:16 bauzas stephenfin: no, for the reason dansmith mentioned
16:07:25 bauzas we can't only accept 6.0
16:07:50 dansmith bauzas: well, I might be wrong about that in this case, I was kinda thinking of it differently
16:08:24 dansmith I guess this works because can_send_version(6.0) will be true even once we move to 6.1
16:08:43 stephenfin well then weren't we doing that already?
16:08:50 stephenfin currently we're passing version=5.1
16:08:53 bauzas dansmith: not if we bumped 7.0 and we only support 6.5 and later (per say)
16:09:13 dansmith bauzas: yeah, but this will all be removed before we'd bump to 7,
16:09:25 dansmith I was worried about the version calculus for 6.x where x>0 not 7
16:09:30 stephenfin so we'd have to change that if we added a 6.x that affected that method
16:10:01 bauzas stephenfin: no, we pass either 6.0, 5.1 or 5.0 depending on the server
16:10:15 dansmith *depending on the pin
16:10:19 bauzas right
16:10:37 stephenfin cctxt = client.prepare(server=host, version=version)
16:10:48 bauzas that's the client version
16:10:58 stephenfin oh, I '_ver' is doing the magic
16:11:04 bauzas not the service version we discover based on the pin
16:11:09 stephenfin *the '_ver' method
16:11:40 bauzas the _ver method is temporary
16:12:00 bauzas just to avoid reproducing dumb checks
16:12:38 bauzas that's the pattern we used in the past bumps and I shamelessly reproduced it
16:13:08 dansmith patterns are good :)
16:13:16 bauzas provided people understand them.
16:14:06 dansmith stephenfin: you also want to change the client signature to use flavor too, I'm sure

Earlier   Later