| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-01-05 | |||
| 14:35:06 | sean-k-mooney | e.g. the device profile | |
| 14:35:19 | bauzas | people creating flavors with this key would previously get a 200 | |
| 14:35:32 | sean-k-mooney | ya | |
| 14:35:34 | bauzas | and now we're returning a 403 because of the cyborg deco | |
| 14:35:36 | bauzas | this is said | |
| 14:35:38 | bauzas | sad | |
| 14:35:43 | sean-k-mooney | sure | |
| 14:35:51 | bauzas | this would have required a microversion | |
| 14:35:57 | sean-k-mooney | no | |
| 14:36:05 | sean-k-mooney | extra specs never require a microverion | |
| 14:36:14 | sean-k-mooney | they are an unversioned part of the api | |
| 14:36:19 | bauzas | changing a 200 to 403 ? | |
| 14:36:32 | dansmith | anything could become a 403 at any point, right? | |
| 14:36:39 | dansmith | like you stop having permission to do something | |
| 14:36:52 | sean-k-mooney | well via a policy change yep | |
| 14:36:55 | bauzas | 403s are said to be accepted without new microversions | |
| 14:37:10 | bauzas | but there is a but | |
| 14:37:11 | bauzas | https://docs.openstack.org/nova/latest/contributor/microversions.html#f2 | |
| 14:37:35 | bauzas | getting a 403 because keystone check obviously doesn't require a new microversion | |
| 14:37:37 | sean-k-mooney | bauzas: to get the flavor extra spec validation you have to opt into it via a new microverion | |
| 14:38:01 | bauzas | either way, the ship has sailed | |
| 14:38:07 | bauzas | this deco is there for a while | |
| 14:38:20 | bauzas | and fwiw, happy to see you around dansmith :) | |
| 14:38:26 | sean-k-mooney | right but fundementally there is noting broken or wrong with this | |
| 14:38:38 | dansmith | bauzas: :) | |
| 14:38:56 | bauzas | sean-k-mooney: sure | |
| 14:39:09 | bauzas | anyway, let's continue to review the change | |
| 14:39:22 | sean-k-mooney | the only way to prevent this is to require that operators addign custom extra specs use either no namespace or custom: | |
| 14:39:47 | sean-k-mooney | that would be an improvment in my view but it would be an api change which would need a microverion for sure | |
| 14:39:57 | bauzas | sean-k-mooney: on a side note, https://review.opendev.org/c/openstack/nova/+/729563/26/nova/api/openstack/compute/shelve.py#59 means that now when you shelve, you're getting a 500 with cyborg, right? | |
| 14:40:19 | bauzas | since the exception wrapper is just added but the decorator was existing... | |
| 14:40:42 | sean-k-mooney | well we have patchs to enable cyborg shelve support right | |
| 14:41:02 | sean-k-mooney | oh it is the patch | |
| 14:41:39 | sean-k-mooney | exception.ForbiddenWithAccelerators should not be raised here anymore | |
| 14:42:34 | sean-k-mooney | bauzas: i havent review this in a while | |
| 14:43:03 | bauzas | sean-k-mooney: well, we decorated shelve() a while ago | |
| 14:43:05 | sean-k-mooney | why would self.compute_api.shelve(context, instance) raise that with this chagne? | |
| 14:43:16 | sean-k-mooney | bauzas: yes i know | |
| 14:43:24 | sean-k-mooney | but that decorator should be removed with this patch | |
| 14:43:28 | bauzas | so we were returning ForbiddenWithAccelerators up to the API | |
| 14:43:29 | sean-k-mooney | that was the whole point | |
| 14:43:48 | sean-k-mooney | we were not going to require a new micorverion for unshelve with cyborg | |
| 14:43:49 | bauzas | so in theory we should have managed such exception | |
| 14:44:06 | bauzas | because afaics, we were just passing it thru | |
| 14:44:11 | bauzas | leading to a 500 | |
| 14:44:20 | dansmith | I'm super confused | |
| 14:44:26 | bauzas | now, this patch addes the exception handle | |
| 14:44:41 | dansmith | the point of the decorator was to make the 500 into 403s, acting like "you don't have permission" instead of "this doesn't work" | |
| 14:44:44 | dansmith | so we could enable it later, | |
| 14:44:51 | dansmith | since this was not a new api, just a hole we created | |
| 14:44:56 | sean-k-mooney | so https://review.opendev.org/c/openstack/nova/+/729563/26/nova/compute/api.py#4166 | |
| 14:45:00 | bauzas | dansmith: I totally get it | |
| 14:45:04 | sean-k-mooney | we are doing @block_accelerators(until_service=54) | |
| 14:45:13 | sean-k-mooney | we could also just remove the decorator | |
| 14:45:14 | dansmith | catching the exception now is just to handle walking over the service version right? | |
| 14:45:18 | bauzas | dansmith: but afaics, we were not handling such exception on the API side | |
| 14:45:27 | dansmith | right, exactly, until version 54 | |
| 14:46:29 | sean-k-mooney | i tought we were elsewhwere | |
| 14:47:09 | dansmith | hmm, | |
| 14:47:16 | dansmith | so ForbiddenWithAccelerators has code=403, | |
| 14:47:20 | dansmith | but it doesn't inherit from Forbidden | |
| 14:47:30 | dansmith | so ... maybe we thought we were, but aren't? | |
| 14:47:38 | dansmith | does the API handle anything with code= set and turn that into an http code? | |
| 14:48:17 | sean-k-mooney | i think it does | |
| 14:48:30 | dansmith | if so, then we should be good | |
| 14:48:42 | bauzas | ah-ha ok | |
| 14:48:47 | sean-k-mooney | i think we have a generic handeler for anything that inherits form NovaException | |
| 14:49:02 | dansmith | which also means we probably don't need this special catch | |
| 14:49:06 | bauzas | my only worry is that this handler was returning a 500 | |
| 14:49:15 | bauzas | but given code=403, I guess we're safe | |
| 14:49:16 | dansmith | see, NovaException defaults to code=500, | |
| 14:49:32 | dansmith | so this overrides to 403 which I think is supposed to make us not 500 when this bubbles all the way to the api | |
| 14:49:46 | bauzas | yup, from what I remind | |
| 14:49:58 | dansmith | I'll reply to bauzas | |
| 14:50:02 | bauzas | cool | |
| 14:50:06 | dansmith | as I need a gerrit comment karma in 2021 :P | |
| 14:51:02 | sean-k-mooney | we probaly could extent that api samples to assert the correct behavior | |
| 14:51:24 | dansmith | do we have api samples tests for failures? I didn't think we did | |
| 14:51:48 | sean-k-mooney | not sure but im pretty sure we dont have any api sample tests for cyborg integration | |
| 14:53:14 | sean-k-mooney | i think we just have https://github.com/openstack/nova/blob/261416aeb0187cc7d420bb74d8b330aa66cc37b6/nova/tests/functional/api_sample_tests/test_shelve.py | |
| 14:53:18 | bauzas | oh no, we don't hzve them :) | |
| 14:53:40 | dansmith | man, it's going to take me a while to get used to new gerrit | |
| 14:53:53 | dansmith | it was only released for one day before I disappeared in 2020 and it looks so foreign to me | |
| 14:54:35 | sean-k-mooney | ya part of it remind me of really really old gerrit | |
| 14:54:56 | sean-k-mooney | but i keep going to click on things that have moved | |
| 14:55:55 | gibi | stephenfin: lets add uptime to both response | |
| 14:56:04 | bauzas | dansmith: good luck with the new gerrit | |
| 14:56:06 | gibi | dansmith: hey, welcome back! | |
| 14:56:13 | dansmith | gibi: o/ | |
| 14:56:14 | bauzas | but I'm finally used to it | |
| 14:56:44 | bauzas | haven't seen yet any fancy new feature that makes it worth tho | |
| 14:57:37 | dansmith | gotta move everything around in a UI every 18 months to stave off dementia I guess | |
| 14:57:42 | gibi | bauzas: one big plus for me that now gerrit shows diffs due to rebase with a different color that diff due the the patch I'm reviewing | |
| 14:57:54 | bauzas | ah, gtk then | |
| 14:57:54 | dansmith | orly | |
| 14:58:06 | bauzas | haven't seen it yet | |
| 14:58:07 | dansmith | I think they could probably have done that without moving everything | |
| 14:58:53 | sean-k-mooney | gibi: oh i havent noticed that yet | |
| 14:59:03 | gibi | for example there are orange lines here due to six removal https://review.opendev.org/c/openstack/nova/+/764292/6..9/nova/api/openstack/compute/servers.py | |
| 14:59:15 | bauzas | dansmith: fwiw, I'll require your expertise in a few weeks for the compute RPC API version bump, still stuck with damn errors | |