Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2021-10-08
15:08:54 dansmith but not just a "2.98: change a bunch of 200s to 202s"
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 lyarwood yes
15:10:58 bauzas https://docs.openstack.org/nova/latest/contributor/microversions.html#when-do-i-need-a-new-microversion for nova specifics
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 es are handled before API methods are called which results in a 415.
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 "
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 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:37 lyarwood instead you're suggesting different people fix them one by one at different times
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
15:23:17 dansmith is reasonable behavior to assume from either 200 or 202 though
15:23:47 dansmith you're giving it attachment information, which it is storing, and that triggers some behavior right?
15:24:21 dansmith like, IIRC, 200 is for "I've stored your object, here's the ID you can use to get it again" and 202 is for "I will store that later, but I can't necessarily give you its identifier yet" right?
15:24:27 sean-k-mooney for a 202 they cannot assuem its complete and should expect to poll nova for the compleation
15:25:05 lyarwood dansmith: yeah but my understanding with 200 was that processing was complete with the return
15:25:08 sean-k-mooney for 200 generally you would assume it blocks until its completed but its valid to retrun before that
15:25:17 lyarwood dansmith: and as sean-k-mooney said, 202 requires additional processing
15:25:34 dansmith I think that's true of the REST object and not necessarily the actions implied from the thing you're storing
15:26:18 sean-k-mooney lyarwood: has the k8s csi plugin be fixed to pool nova by the way
15:26:21 dansmith but again, as sean-k-mooney said, this is really a result of them not reading the docs that say it's async and implying which part of the operation is synchronous from the 200
15:26:26 sean-k-mooney regardeless of if we cahnge the api
15:26:36 lyarwood sean-k-mooney: https://github.com/kubernetes/cloud-provider-openstack/issues/1645 not yet
15:27:28 sean-k-mooney ok but they are aware of it and it will get eventually fixed
15:27:33 lyarwood yup sure, just outlining my motivation to fix this
15:27:51 lyarwood ;)
15:27:58 sean-k-mooney :)
15:28:17 sean-k-mooney i assume we have a customer hitting this so that normally seed up fixing things
15:28:21 dansmith to be clear, making it 202 won't fix this, we're just assuming the human that wrote the code would have assumed they should do something different if the code was 202 right?
15:28:38 lyarwood yup that's the assumption, it wouldn't fix anything in CSI
15:29:01 lyarwood I got triggered by the TODO today and wanted to take a swing a killing it
15:29:06 sean-k-mooney if we wnated to fix CSI issue in nova we would have to block
15:29:11 dansmith so, I mean.. we're like changing something that could easily break other clients that did read the docs, based on the assumption that someone who didn't would have done the right thing if it was different
15:29:14 sean-k-mooney wich i dont think we want to do
15:29:18 dansmith pretty meh :)
15:30:26 bauzas ok, sorry guys, I need to bail out from now
15:31:48 lyarwood bauzas: \o if you want to chat on Monday ping me an email, I should have time to chat once I'm on London
15:31:55 lyarwood in*
15:32:15 bauzas lyarwood: don't worry, I'll use the power of community brainstorm to move on with your spec ;)
15:32:32 dansmith I think we've all put on a few pandemic pounds, but being "on london" would require a substantial increase in waist size, I think

Earlier   Later