Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2021-03-24
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 stephenfin dansmith: like that? (need tests obv) ^
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: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
16:14:20 stephenfin I don't think so. Not yet
16:14:26 stephenfin Keep it small
16:14:27 dansmith why?
16:14:42 stephenfin That's v. easy fix in a follow-up
16:15:02 stephenfin I mean I can but it's more test damage
16:15:07 bauzas right
16:15:10 dansmith well, that's the point here
16:15:15 bauzas some methods pass kwargs
16:15:27 bauzas then, you're dommed
16:15:29 bauzas doomed*
16:15:32 dansmith either we're going for consistency or we aren't right?
16:18:12 stephenfin *eventual consistency
16:18:22 stephenfin I can change the other things whenever. I can only change this right now
16:18:42 stephenfin and I have the follow-up ready. I just don't think it's wise to merge it right now since it's huge
16:21:07 dansmith well, my opinion is that this is all just naming and not useful for paying down any real debt (like deprecated parameters) so I'd vote to just punt on it anyway
16:24:21 bauzas the other problem I see is with the gate
16:24:37 bauzas getting at least a grenade-multinode result
16:24:57 dansmith for sure we need to see and scrutinize a grenade-multinode result at least
16:25:13 bauzas the good thing is that both prep_resize and resize_instance() are tested IIRC
16:31:45 bauzas I have to stop now as I need to taxi my daughter to some doctor's appointment
16:31:53 bauzas but I can be back later on in the evening
16:36:08 openstackgerrit Stephen Finucane proposed openstack/nova master: DNM: Rename 'instance_type' parameter https://review.opendev.org/c/openstack/nova/+/782804
16:48:17 openstackgerrit Stephen Finucane proposed openstack/nova master: rpc: Rename 'instance_type' client argument https://review.opendev.org/c/openstack/nova/+/782825
16:48:49 stephenfin dansmith: A'ight, there's the client change ^ Pretty simple after all. I think I'd still keep it separate but we could squash it in if we wanted to
16:49:40 stephenfin Also, for the previous patch, there doesn't appear to have been a test for backleveling of 'prep_resize' so we might want to adding that as a follow-up even if we don't do this (it seems to be working fine, thankfully, based on the test)

Earlier   Later