| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-10-08 | |||
| 10:46:00 | gibi | I support that idea too. Downstream we use dpdk a lot, so would be nice to get coverage for it upstream. | |
| 10:46:15 | sean-k-mooney | fortunetly on the dpdk front its support by distros now so its now just a matter of turning it on and seting a few config values but still need to sit down and do that | |
| 10:46:38 | gibi | let me know if I can help | |
| 10:47:00 | sean-k-mooney | sure will do. when i get around to workign on the devstack patches ill let you know | |
| 11:09:46 | opendevreview | sean mooney proposed openstack/nova master: Ensure MAC addresses characters are in the same case https://review.opendev.org/c/openstack/nova/+/811947 | |
| 11:14:44 | sean-k-mooney | noonedeadpunk: hopefully you dont mind me updateing ^ | |
| 11:15:07 | sean-k-mooney | gibi: can you take a look at ^ when you have time | |
| 11:16:26 | gibi | ack I will try | |
| 11:19:48 | noonedeadpunk | sure I'm not! | |
| 12:20:59 | opendevreview | Lee Yarwood proposed openstack/nova-specs master: WIP libvirt: Allow Manila shares to be directly attached to instances https://review.opendev.org/c/openstack/nova-specs/+/813180 | |
| 14:37:22 | lyarwood | stupid Friday question but can anyone think of an API where we've changed the return code in a microversion? I want to fix the POST os-volume_attachments method to return 202 but I can't think of the best way to do this | |
| 14:39:17 | lyarwood | ah nvm I see just duplicate the actual method and decorate it | |
| 15:00:02 | bauzas | lyarwood: correct, I was about to point it to you | |
| 15:00:09 | bauzas | (sorry for the late ping, kids taxi) | |
| 15:00:20 | bauzas | lyarwood: do you want to discuss on your spec ? | |
| 15:00:20 | stephenfin | lyarwood: There are quite a few APIs that suffer from that issue. Are you just going to do the one or multiple? | |
| 15:04:11 | bauzas | lyarwood: fwiw, I'm leaving in 25 mins for the weekend | |
| 15:06:43 | lyarwood | bauzas: sorry was just getting a drink, happy to chat now if you had any questions etc | |
| 15:06:59 | bauzas | I briefly looked at your spec | |
| 15:07:21 | lyarwood | stephenfin: just os-volume_attachments for now but I'll likely start to address others once I've worked this out | |
| 15:07:48 | lyarwood | I'm not sure if people would prefer a single microversion for everything etc | |
| 15:08:10 | dansmith | so, I'm pretty sure that we originally said we wouldn't do microversions for things like this | |
| 15:08:27 | bauzas | if 500, yes | |
| 15:08:31 | lyarwood | that would make my life easier | |
| 15:08:31 | dansmith | because nobody is going to opt-in to a new return code, or opt out (back?) to the old one | |
| 15:08:38 | sean-k-mooney | dont we requrie a microversion if you are chaning the reponcoe code to one that is not currently used | |
| 15:08:42 | bauzas | I mean, 500 to something clearer, not a microversion | |
| 15:08:45 | dansmith | if you're changing other things about the api, then doing the return code as part of it makes sense | |
| 15:08:54 | dansmith | but not just a "2.98: change a bunch of 200s to 202s" | |
| 15:08:54 | bauzas | sec, giving you the API rules | |
| 15:09:13 | sean-k-mooney | well 200 to 202 is blocking to async | |
| 15:09:16 | lyarwood | I wonder why we have TODOs everywhere for this then | |
| 15:09:19 | dansmith | sean-k-mooney: yeah, I'm not saying do it without a microversion, I'm saying don't do it unless there's something else changing | |
| 15:09:44 | lyarwood | oh sorry I see | |
| 15:09:53 | sean-k-mooney | https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/202 | |
| 15:09:58 | lyarwood | well tbh it's never going to get done in that case | |
| 15:10:33 | sean-k-mooney | lyarwood: you want to move this to be an async api yes? | |
| 15:10:40 | dansmith | if we | |
| 15:10:44 | bauzas | https://specs.openstack.org/openstack/api-wg/guidelines/microversion_specification.html for the API general rules | |
| 15:10:46 | lyarwood | sean-k-mooney: it already is | |
| 15:10:55 | sean-k-mooney | oh then 200 is wrong ya | |
| 15:10:58 | bauzas | https://docs.openstack.org/nova/latest/contributor/microversions.html#when-do-i-need-a-new-microversion for nova specifics | |
| 15:10:58 | lyarwood | yes | |
| 15:11:04 | dansmith | are moving it from sync to async, then *of course* it makes sense to change, but just a microversion for changing the return code but not the behavior is kinda silly | |
| 15:11:22 | sean-k-mooney | right | |
| 15:11:28 | sean-k-mooney | im more ok with not haveing a microversion | |
| 15:11:32 | sean-k-mooney | if we are not changing behavior | |
| 15:11:35 | sean-k-mooney | just the return code | |
| 15:11:44 | dansmith | I'm not saying we should do that | |
| 15:11:46 | bauzas | technically, we agreed on asking for a microversion https://docs.openstack.org/nova/latest/_images/graphviz-a4c9682faf139450eed50fdcc7dd06df5677c197.png | |
| 15:12:04 | bauzas | unless it was changing from HTTP500 | |
| 15:12:05 | dansmith | I'm saying returning 200 for an async api that has always been async is unfortunate, but not that big of a deal | |
| 15:12:07 | sean-k-mooney | dansmith: you are say until we modify tese again dont change it | |
| 15:12:14 | dansmith | sean-k-mooney: yes | |
| 15:12:32 | dansmith | nobody is going to opt-in to "no new behavior but a more accurate return value" | |
| 15:12:45 | sean-k-mooney | i think that is also reasonable becasue of ^ | |
| 15:12:48 | bauzas | but we mention something : | |
| 15:12:50 | bauzas | " | |
| 15:12:50 | bauzas | The exception to not needing a microversion when returning a previously unspecified error code is the 400, 403, 404 and 415 cases. This is considered OK to return even if previously unspecified in the code since it’s implied given keystone authentication can fail with a 403 and API validation can fail with a 400 for invalid json request body. Request to url/resource that does not exist always fails with 404. Invalid content typ | |
| 15:12:50 | bauzas | es are handled before API methods are called which results in a 415. | |
| 15:12:50 | bauzas | " | |
| 15:12:57 | lyarwood | right but at least it's fixed later when they opt-in to some other change | |
| 15:13:06 | dansmith | bauzas: that has nothing to do with this case | |
| 15:13:15 | bauzas | nothing tells us to not ask for a microversion if changing from 200 | |
| 15:13:25 | bauzas | dansmith: sure, I was just looking at what we wrote :) | |
| 15:13:45 | bauzas | to see whether we said "cool if 200 -> 202 and async" | |
| 15:13:50 | sean-k-mooney | bauzas: under that policy we sould have to do a microversion but there is no untily in changing the repssonce doe and making it opt in | |
| 15:13:51 | dansmith | lyarwood: right but all of those kinds of death-by-1000-cuts things impact people, which is why we didn't do a massive cleanup early on | |
| 15:14:10 | dansmith | lyarwood: right now if you know that api returns 200 and you check for 200, then returning 202 could break client code even though absolutely nothing has changed | |
| 15:14:25 | dansmith | it'd be bad client code, granted, but.. it's pain for pretty much no gain | |
| 15:14:26 | bauzas | that's the point ^ | |
| 15:14:43 | lyarwood | dansmith: and how does that change anything by including it with other changes later on? | |
| 15:14:56 | lyarwood | dansmith: bad clients will still break | |
| 15:15:02 | sean-k-mooney | lyarwood: in the ohter api change later the clinet needed to be updated anyway | |
| 15:15:08 | bauzas | if clients expect a 200 on a call, we can't leave them wondering now why they get something else | |
| 15:15:09 | sean-k-mooney | e.g. there was already work for them to do | |
| 15:15:27 | dansmith | lyarwood: well, right, I'm saying it's not worth changing like ever, unless some other behavior changes that would require a behavioral change anyway | |
| 15:16:10 | dansmith | it's a developer purity thing, like many of our TODOs everywhere, but unless it impacts proper client behavior, it's more likely to break things than fix them, IMHO | |
| 15:16:16 | lyarwood | at least this way we could change all of them in one go and fix the same issue in the clients in one go | |
| 15:16:37 | lyarwood | instead you're suggesting different people fix them one by one at different times | |
| 15:16:37 | dansmith | when we started out do the v3 API, it was all this purity stuff, and we literally unrolled it all when we realized we were just making pain for people | |
| 15:16:56 | dansmith | lyarwood: again, I'm not | |
| 15:18:06 | lyarwood | okay it's Friday I'm likely missing something but given people don't want this I'm going to stop and move on | |
| 15:18:13 | sean-k-mooney | lyarwood: when we have done this clean up in the past we have paired it with other changes with the logic being a client tha want to use that new feature via a microversion need to be updated anyway so they can adapt to both change at the same time. | |
| 15:18:21 | dansmith | let me ask this way: what client behavior is going to be wrong if we return 200 that will be right if we return 202? | |
| 15:18:31 | dansmith | if there's something there, then it's worth changing | |
| 15:18:39 | dansmith | sean-k-mooney: right | |
| 15:19:37 | lyarwood | tbh with 200 to 202 it's not our clients that I'm worried about | |
| 15:19:56 | lyarwood | it's other callers outside of them and their understanding of the API | |
| 15:20:21 | dansmith | lyarwood: right, but ... what's the behavior or understanding that will be wrong? | |
| 15:20:22 | lyarwood | the CSI plugin in k8s for OpenStack for example has been misusing it for a while | |
| 15:20:29 | sean-k-mooney | your assumeing they are reading the reponce code but not the docs that say this is an async api | |
| 15:20:43 | lyarwood | dansmith: assuming it's sync and not polling | |
| 15:20:46 | dansmith | meaning, generally 202 means "if you query this thing right after the PUT/POST then you will not see it updated so don't expect it to be" .. is it that? or is it "this operation takes a while?" | |
| 15:21:17 | dansmith | lyarwood: but is the actual API operations sync and just not the attachment? | |
| 15:22:04 | lyarwood | we return after we cast to the compute to do the actual attachment | |
| 15:22:09 | sean-k-mooney | dansmith: i think k8s has been assuming that the attachment was completed when it got the 200 | |
| 15:22:32 | lyarwood | so it takes a while, but the CSI driver would start polling Cinder instead of Nova and that has caused various issues | |
| 15:22:48 | lyarwood | because they assumed the attachment in Nova was done at that point | |
| 15:23:05 | dansmith | I'm not sure that | |