Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2021-10-08
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
15:33:15 lyarwood dansmith: it has been a rough 16 months, it wouldn't take much more
15:33:24 dansmith heh
22:23:34 opendevreview Julia Kreger proposed openstack/nova master: Ignore plug_vifs on the ironic driver https://review.opendev.org/c/openstack/nova/+/813263
#openstack-nova - 2021-10-09
03:35:26 opendevreview norman shen proposed openstack/nova master: Recreate mdev devices according to placement https://review.opendev.org/c/openstack/nova/+/810220
04:04:16 opendevreview norman shen proposed openstack/nova master: Recreate mdev devices according to placement https://review.opendev.org/c/openstack/nova/+/810220
13:08:56 opendevreview Julia Kreger proposed openstack/nova master: Ignore plug_vifs on the ironic driver https://review.opendev.org/c/openstack/nova/+/813263
#openstack-nova - 2021-10-10
14:19:47 opendevreview norman shen proposed openstack/nova master: Add a timeout for waiting volume snapshot ready https://review.opendev.org/c/openstack/nova/+/813297
#openstack-nova - 2021-10-11
07:45:43 bauzas good morning Nova
07:52:06 opendevreview Dmitriy Rabotyagov proposed openstack/nova master: Ensure MAC addresses characters are in the same case https://review.opendev.org/c/openstack/nova/+/811947
08:24:47 plibeau4 hello, I need your help about this proposal change: https://review.opendev.org/c/openstack/nova/+/803713.
08:50:25 gibi good morning
08:51:03 gibi this week feels to be as crazy as the last was for me. you can ping me but I cannot promise things :/
08:55:43 bauzas plibeau4: ack, will add your change for a review
08:56:09 bauzas thanks for reaching us
09:24:41 plibeau4 thx guys take your time it's just to have your feedback It's already in production on myside :)
09:25:17 plibeau4 and if you have time -> https://review.opendev.org/c/openstack/nova/+/803600
09:26:39 sean-k-mooney to me https://review.opendev.org/c/openstack/nova/+/803713 kind of feels like intoducing a race condetion
09:29:12 sean-k-mooney i guess its not a race as it does not quiesce the disk until after the dev rebase is complete deterministically
09:43:45 sean-k-mooney plibeau4: im not conviced moving the quiece is safe to have in production

Earlier   Later