Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2021-03-24
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)
16:50:55 stephenfin I realize it doesn't ultimately matter, but when it's this easy I really do think we should be including this. It'd mean we're just left with some DB/o.vo references to instance types. I could live with those
17:57:55 stephenfin oh, lovely http://paste.openstack.org/show/803882/
17:58:11 stephenfin creating a resource provider on placement with a duplicate name results in a HTTP 5xx
17:59:27 melwitt amazed that hasn't been hit until now
17:59:52 stephenfin You and me both. That's super dumb
18:00:21 melwitt hehehe
18:00:58 stephenfin I won't be able to work on it today, but if anyone else has time the simple reproducer is to use microversion 1.20 and create a resource provider specifying only the name
18:01:24 stephenfin nope, don't even need the microversion
18:01:38 stephenfin $ openstack resource provider create foo; openstack resource provider create foo
18:01:46 stephenfin delightful
18:01:52 melwitt :)
18:02:31 melwitt that'd be easy to add to the gabbits I think
18:05:11 stephenfin for sure
18:05:16 stephenfin I'll take a look tomorrow, no doubt
18:05:24 melwitt kewl
18:55:27 sean-k-mooney we defintly have hit duplicte RPs before
18:55:41 sean-k-mooney but i tough that gave you a 4XX error
18:56:09 sean-k-mooney you get this if you rename a compute node and change its hostname or you delete the compute service and its created with a different uuid
18:56:34 sean-k-mooney sorry only the later case has the name conflcit
18:56:55 sean-k-mooney where you keep the same compute node hostname but delete the compute service and let it get recteated
18:57:10 sean-k-mooney the compute service uuid change but the RP name will be the same
18:58:54 sean-k-mooney stephenfin: melwitt we have test for that in placment already i think https://github.com/openstack/placement/blob/master/placement/tests/functional/gabbits/resource-provider-duplication.yaml

Earlier   Later