| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2023-01-25 | |||
| 17:12:51 | sean-k-mooney | if you request something at the api that requries a new rpc version we shoudl not back levle | |
| 17:12:56 | sean-k-mooney | that should be an api error | |
| 17:13:23 | sean-k-mooney | we normally use a compute service bump to allow use to detect this in the api | |
| 17:13:28 | sean-k-mooney | before getting to the rpc code | |
| 17:13:32 | bauzas | and that's what sahid does | |
| 17:13:55 | bauzas | but I don't really like us returning exceptions we don't really manage upside | |
| 17:14:03 | dansmith | making a call, getting an exception and making it again with different stuff is wasteful *and* wrong, | |
| 17:14:17 | sean-k-mooney | right and we are not doing that | |
| 17:14:25 | dansmith | okay | |
| 17:14:33 | sean-k-mooney | where it can be backleveled we do prepare/version check | |
| 17:14:50 | bauzas | dansmith: tl;dr sahid is adding a service check that verifies all computes are upgraded before adding a parameter | |
| 17:14:58 | sean-k-mooney | but if you use the new parmater then its not valide to remove it | |
| 17:15:00 | dansmith | ack | |
| 17:15:44 | bauzas | by default the param is set to None on the API method | |
| 17:15:45 | sean-k-mooney | bauzas: yep i would break our api microversioning to backlevel in this case | |
| 17:15:55 | bauzas | then it calls the conductor and then the compute | |
| 17:16:01 | sean-k-mooney | bauzas: only for the new microversion no? | |
| 17:16:14 | sean-k-mooney | we backlevel if its actully None | |
| 17:16:22 | sean-k-mooney | i need to pull up the patch again | |
| 17:16:32 | tobias-urdin | is the uuid of a mdev just a random uuidutils.uuid() or is it tracked somewhere in placement? | |
| 17:16:45 | sean-k-mooney | tobias-urdin: totally random | |
| 17:17:01 | tobias-urdin | sean-k-mooney: ack ty | |
| 17:17:33 | bauzas | sean-k-mooney: technically with sahid's proposal, we backlevel by the if condition | |
| 17:18:31 | bauzas | but ok, I see my mistake, I'll clarify my second comment | |
| 17:18:46 | bauzas | either way, my first comment remains valid, we lack a negative test | |
| 17:18:49 | sean-k-mooney | we do it by if not client.can_send_version(version): | |
| 17:19:49 | sean-k-mooney | so if we cant send the version and target_state is not None we raise | |
| 17:19:54 | sean-k-mooney | https://review.opendev.org/c/openstack/nova/+/858383/25/nova/compute/rpcapi.py#1106 | |
| 17:20:01 | sean-k-mooney | this is what you are askign about yes | |
| 17:20:48 | bauzas | yep, you convinced me on the RPC contracty | |
| 17:21:00 | bauzas | my second comment is not valid | |
| 17:21:29 | bauzas | that being said, I'm not super happy with those generic exceptions being raised without being properly captured at the API side | |
| 17:21:37 | bauzas | but that's not worth a -1 | |
| 17:21:50 | bauzas | my -1 is just about missing unittests on the RPC checks | |
| 17:21:51 | sean-k-mooney | im ok with useing pop to do the removal but we woudl need to check the result to determin if we raise or reducet the verison | |
| 17:22:15 | bauzas | sean-k-mooney: we're on agreement | |
| 17:22:29 | bauzas | that's what I mean by the RPC contract | |
| 17:22:48 | bauzas | if this parameter is set to something, we can't backlevel | |
| 17:22:54 | sean-k-mooney | oh its missing a negitive test | |
| 17:22:57 | bauzas | you convinced me | |
| 17:23:04 | bauzas | sean-k-mooney: yup, that's why I -1 | |
| 17:23:07 | sean-k-mooney | ya you are right i tough this was unit tested but only the happy path | |
| 17:23:32 | bauzas | and I was +0 on the fact we raise a NovaException without properly capturing it on the API level, but that wasn't a patch blocker | |
| 17:25:48 | sean-k-mooney | i tought that would be confreted we are raisng nova excption below here too https://review.opendev.org/c/openstack/nova/+/858383/25/nova/compute/rpcapi.py#1116 | |
| 17:26:03 | sean-k-mooney | *converted | |
| 17:26:27 | sean-k-mooney | with that said we want this to be a 400 not a 500 | |
| 17:26:44 | sean-k-mooney | oh but this is not hooked up to the api yet | |
| 17:26:50 | sean-k-mooney | so we have to check the second patch | |
| 17:27:50 | bauzas | sean-k-mooney: I checked and this isn't the case | |
| 17:28:33 | bauzas | that being said, again, not -1ing on this, rather +0ing, because this situation shouldn't arrive thanks to the version check | |
| 17:29:09 | bauzas | this is just us not being enough prescriptive on the exception handling | |
| 17:29:29 | sean-k-mooney | right but it would be good to have negitive unit tests in the first patch and a negitive funcitonl test in the secod | |
| 17:29:33 | bauzas | (usually I hate standard exceptions that are meaningless and hard to identify) | |
| 17:29:37 | sean-k-mooney | bauzas: because even if fully upgraded | |
| 17:29:44 | sean-k-mooney | you could have an rpc pin set in the config right | |
| 17:29:51 | sean-k-mooney | that woudl pass the compute service check | |
| 17:29:51 | bauzas | true | |
| 17:29:58 | sean-k-mooney | so we dont want this to be a 500 | |
| 17:30:19 | sean-k-mooney | it should be a 400 or maybe 409 | |
| 17:30:21 | bauzas | then I turn my api patch review vote to -1 | |
| 17:30:29 | bauzas | because of the pinset | |
| 17:30:46 | bauzas | sean-k-mooney: are you sure that if we pin the rpc versions we hit this ? | |
| 17:31:01 | bauzas | I thought the service check would say "'meh nah"' | |
| 17:31:10 | bauzas | oh | |
| 17:31:14 | bauzas | no, you're right | |
| 17:31:16 | sean-k-mooney | if we pin to 6.0 the can send version check will fail for 6.2 | |
| 17:31:26 | bauzas | true | |
| 17:31:39 | bauzas | the RPC version check will fail to accept 6.2 | |
| 17:31:41 | sean-k-mooney | so we should assert that returns somethign other then a 500 at the api | |
| 17:31:41 | bauzas | butn, | |
| 17:31:59 | bauzas | the api version check on check_min_versions() will say 'surely, you can call' | |
| 17:32:08 | sean-k-mooney | yep | |
| 17:32:33 | sean-k-mooney | but i dont think this is a probelm in the code nessisarly | |
| 17:32:44 | sean-k-mooney | just in the test coverage and perhapse the excpetion raised | |
| 17:33:01 | sean-k-mooney | we use 409 to comunicate this in other places | |
| 17:33:52 | sean-k-mooney | bauzas: do you want to capture that in the review | |
| 17:34:05 | sean-k-mooney | or shall i an link to this irc conversation | |
| 17:35:08 | sean-k-mooney | i think we just need a few more edgecases here https://review.opendev.org/c/openstack/nova/+/858384/34/nova/tests/functional/api_sample_tests/test_evacuate.py | |
| 17:35:10 | bauzas | sean-k-mooney: just left comments | |
| 17:35:33 | bauzas | but I can explain this to sahid later | |
| 17:35:46 | sean-k-mooney | ack | |
| 17:36:13 | sean-k-mooney | thanks for bringing this up | |
| 17:36:40 | opendevreview | Merged openstack/nova stable/xena: Reproduce bug 1981813 in func env https://review.opendev.org/c/openstack/nova/+/859314 | |
| 17:36:45 | bauzas | and yeah HTTP409 Conflict makes perfect sense | |
| 17:37:37 | bauzas | I'll just double check the cve patches on fly | |
| 18:03:53 | gibi | Uggla: I left some comment in the API patch of the Manial series https://review.opendev.org/c/openstack/nova/+/836830 | |
| 18:04:15 | opendevreview | Merged openstack/nova stable/xena: [stable-only][cve] Check VMDK create-type against an allowed list https://review.opendev.org/c/openstack/nova/+/871622 | |
| 18:05:30 | opendevreview | Merged openstack/nova stable/xena: Gracefully ERROR in _init_instance if vnic_type changed https://review.opendev.org/c/openstack/nova/+/859315 | |
| 20:08:34 | opendevreview | Merged openstack/nova stable/yoga: [stable-only][cve] Check VMDK create-type against an allowed list https://review.opendev.org/c/openstack/nova/+/871624 | |
| 21:56:14 | opendevreview | Ghanshyam proposed openstack/nova stable/wallaby: DNM: testing tempest pin for stable/wallaby https://review.opendev.org/c/openstack/nova/+/871798 | |
| 22:00:01 | opendevreview | Ghanshyam proposed openstack/nova stable/xena: DNM: testing tempest pin for stable/wallaby https://review.opendev.org/c/openstack/nova/+/871800 | |
| 22:27:42 | tobias-urdin | proposed new releases for xena, yoga and zed to get the CVE-2022-47951 out the door, those backports are merged and no open patches on branches https://review.opendev.org/c/openstack/releases/+/871802 | |
| #openstack-nova - 2023-01-26 | |||
| 00:33:30 | sean-k-mooney | gibi: bauzas if you could take a look at this in ye're morning or melwitt if your around https://review.opendev.org/c/openstack/nova/+/867324 | |
| 00:43:56 | melwitt | sean-k-mooney: these options don't follow how [workarounds] is supposed to be a False/True do this or don't thing :/ but I see why it's being done | |
| 00:44:31 | sean-k-mooney | i dont wnat to put them in the libvirt section as the workaround they are extending was ment to be deleted eventually | |
| 00:44:42 | sean-k-mooney | and when you use it it "taints" the domain | |
| 00:45:22 | sean-k-mooney | so at least downstream we had to get approval form our virt team to have this not "viod the warrenty" when it comes to support | |
| 00:46:00 | sean-k-mooney | i would be fine with moving them to the libvirt section if the libvirt api we asked for was actully added to libvirt | |
| 00:46:24 | sean-k-mooney | we ask for a top level api to do the exact same thing that did not mark the domain as tainted | |
| 00:46:46 | sean-k-mooney | so if that ever becomes a thing i would be happy to move the optiosn to the libvirt section | |