| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2023-01-25 | |||
| 16:07:08 | opendevreview | Merged openstack/nova stable/zed: [stable-only][cve] Check VMDK create-type against an allowed list https://review.opendev.org/c/openstack/nova/+/871616 | |
| 16:25:29 | bauzas | sean-k-mooney: +2d sahid's implementation of stopping evacuated instances | |
| 16:35:00 | opendevreview | Balazs Gibizer proposed openstack/nova stable/wallaby: Reproduce bug 1981813 in func env https://review.opendev.org/c/openstack/nova/+/859320 | |
| 16:35:01 | opendevreview | Balazs Gibizer proposed openstack/nova stable/wallaby: Gracefully ERROR in _init_instance if vnic_type changed https://review.opendev.org/c/openstack/nova/+/859321 | |
| 16:38:54 | opendevreview | Balazs Gibizer proposed openstack/nova stable/victoria: Reproduce bug 1981813 in func env https://review.opendev.org/c/openstack/nova/+/869583 | |
| 16:38:55 | opendevreview | Balazs Gibizer proposed openstack/nova stable/victoria: Gracefully ERROR in _init_instance if vnic_type changed https://review.opendev.org/c/openstack/nova/+/869584 | |
| 16:42:33 | opendevreview | Balazs Gibizer proposed openstack/nova stable/ussuri: Reproduce bug 1981813 in func env https://review.opendev.org/c/openstack/nova/+/869585 | |
| 16:42:34 | opendevreview | Balazs Gibizer proposed openstack/nova stable/ussuri: Gracefully ERROR in _init_instance if vnic_type changed https://review.opendev.org/c/openstack/nova/+/869586 | |
| 16:52:01 | sahid | thank you bauzas ++ | |
| 16:59:22 | opendevreview | Balazs Gibizer proposed openstack/nova stable/train: Reproduce bug 1981813 in func env https://review.opendev.org/c/openstack/nova/+/869673 | |
| 16:59:23 | opendevreview | Balazs Gibizer proposed openstack/nova stable/train: Gracefully ERROR in _init_instance if vnic_type changed https://review.opendev.org/c/openstack/nova/+/869674 | |
| 17:00:08 | bauzas | sahid: I'm really sorry, but I forgot to look at your dependent patch and I found something :( | |
| 17:00:15 | bauzas | sahid: https://review.opendev.org/c/openstack/nova/+/858383/25 | |
| 17:01:07 | bauzas | sahid: tl,dr: you return an exception if a caller asks for a target_state parameter that the compute doesn't know | |
| 17:02:02 | bauzas | sahid: thinking out loud, I think this would be better to just *not* provide the target_state parameter if the compute is old | |
| 17:02:28 | gibi | the vmdk cv victoria patch https://review.opendev.org/c/openstack/nova/+/871699/ will be get kicked out of the gate as the commit message has a hash but that hash is not laneded yet https://zuul.opendev.org/t/openstack/build/0e2475a0312d4cdaa0774a0fc20c42ce/log/job-output.txt#1552 it seems the [stable-only] tag only disables the hash check if there is no hash in the commit message | |
| 17:02:32 | bauzas | this shouldn't be arriving, since you verify that all computes are upgraded, but I'd prefer us to make it clear | |
| 17:02:43 | bauzas | gibi: ack | |
| 17:04:18 | gibi | I will go and remove the hash from the commit message to keep landing the fixes in parallel | |
| 17:06:45 | opendevreview | Balazs Gibizer proposed openstack/nova stable/wallaby: [stable-only][cve] Check VMDK create-type against an allowed list https://review.opendev.org/c/openstack/nova/+/871557 | |
| 17:07:01 | opendevreview | Balazs Gibizer proposed openstack/nova stable/victoria: [stable-only][cve] Check VMDK create-type against an allowed list https://review.opendev.org/c/openstack/nova/+/871699 | |
| 17:07:19 | opendevreview | Balazs Gibizer proposed openstack/nova stable/ussuri: [stable-only][cve] Check VMDK create-type against an allowed list https://review.opendev.org/c/openstack/nova/+/871702 | |
| 17:08:45 | bauzas | gibi: sean-k-mooney: I'm actually surprised to see some RPC pattern returning an exception if a compute is too old, instead of just remove the parameter from the call we do | |
| 17:09:20 | bauzas | if we really want to have RPC backwards compat, the RPC client needs to adapt to what the manager supports | |
| 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 | |