Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2022-01-31
16:24:36 opendevreview Merged openstack/nova master: api: Reject duplicate port IDs in server create https://review.opendev.org/c/openstack/nova/+/827070
16:30:19 opendevreview Jonathan Race proposed openstack/nova master: Adds Pick guest CPU architecture based on host arch in libvirt driver support https://review.opendev.org/c/openstack/nova/+/822053
16:34:44 gibi so there is no clear direction, half of us are up to fix the bug without microversion bump and half of use say fix is with the anyhow incoming 1.39 microversion and document the bug on stable
16:37:44 gibi should I create two separate patch series as see which one land first? or will that simply mean both series will have -1 from the other group?
16:38:12 bauzas stephenfin: I'm surprised I can't see my vote already on the powervm deprecation
16:39:13 bauzas stephenfin: damn shit, I should take a picture
16:39:36 bauzas I literrally found the tab open with my +2 vote and a comment without being submitted
16:40:45 bauzas melwitt: gibi: stephenfin: deprecated powervm, that's it.
16:40:56 gibi bauzas: +1
16:41:36 bauzas if anyone knows any FF plugin that puts the tab in red when you're on a gerrit vote without submitting since 1 day, this would be appreciated :D
16:41:53 bauzas but I assume this is a bit a corner case
16:43:03 melwitt I guess there could also be a hybrid option that doesn't raise a 400 for the current microversion but honors all of the valid ones and then in the next microversion start the 400? I dunno
16:44:26 gibi melwitt: do you mean start translating required=A&required=B to required=A,B withoout a microverison bump?
16:44:54 opendevreview Jonathan Race proposed openstack/nova master: Adds Pick guest CPU architecture based on host arch in libvirt driver support https://review.opendev.org/c/openstack/nova/+/822053
16:45:21 gmann +1 on that, as long as we do not change the return code for old microversion. translating is also ok
16:46:01 gibi but that also breaks the users who realied on the wrong behavior
16:46:06 gibi so I don't get the rules then
16:46:07 melwitt gibi: not sure (sorry) I don't know the detail of how it works. would doing that raise a 400 with invalid trait? I was thinking that if raising a 400 for invalid trait in current microversion is a problem, could just ignore the invalid and only use the valid
16:46:37 gibi melwitt: currently placement ignores repeated required param and only parse the _last_ one out from the query string
16:46:57 gibi this way anything in an earlier requried param is lost
16:47:02 gibi it can be an invalid trait
16:47:04 melwitt sorry, trying to ask if required=VALID_A,INVAILD_B,VALID_C would raise 400?
16:47:04 gibi or a valid one
16:47:15 gibi that is raising 400 today
16:47:21 melwitt ack
16:47:34 gibi required=INVALID_B,&required=VALID_A is accepted and INVALID_B is ignored
16:47:45 melwitt yeah so I wasn't thinking translation then, if the goal is to avoid making old queries fail that used to pass
16:49:09 gibi making old invalid queries pass is up for debate as you see above
16:49:29 melwitt just to collect all the valid ones and honor them and ignore the invalid ones (current microversion) and then in a new microversion could start rejecting invalid with 400. maybe it's a dumb idea but I'm just trying to help
16:49:41 melwitt ah.. k
16:50:17 gibi melwitt: in a new microversion I proposed to translate required=A&required=B to mean required=A,B and if A is invalid then the query is invalid
16:51:00 gibi the question is should we add 400 before the new microversion
16:51:19 gibi one side says it is an interop issue as API behavior is changing
16:51:28 gibi other side says that we are fixing a bug
16:51:36 gibi and one should not opt into a bugfix
16:51:54 gmann and along with interop, any user using it for valid response of getting 'B' will also be broken even they are using it wrongly
16:52:15 gibi and I think those users should fix their query
16:52:44 gibi as their query is highly misleding
16:52:45 gmann but we allowed our API to be succeed for them
16:52:55 gibi yes, we made a bug
16:53:19 gibi I think it was never intentional to parse required=A&required=B as only required=B and ignore the A filter
16:53:38 gibi but we clearly disagree on this
16:55:57 gibi where we draw the line between API behavior and fixable bug?
16:56:09 melwitt yeah.. so I think the worst part about the bug is ignoring the valid repeated params. and I was thinking to avoid 400'ing users who have an accidental invalid trait in a old repeated passing query, we could fix honoring of the valid traits and leave the invalid to be ignored by the query parsing. then begin the 400 in the next microversion
16:57:37 gibi melwitt: but I think gmann argues that if we start handling required=A&required=B as required=A,B today to accept the valid repeats, then that is also a change in API behavior so requires a microversion
17:00:01 gmann gibi: sorry, may be I am not clear. My concern is to change the 200 to 400 for existing users querying required=A&required=B and ok with having 'B' response.
17:00:30 gmann with new microversion we can fix it in any ways, allow or translate or 400
17:00:46 melwitt yeah, that would cause a 400 if A or B are invalid. I'm saying do a temporary-for-1.39 thing where you scan all required= and only use the valid traits and silently ignore invalid traits without a 400. with the goal being to apply all valid required traits without introducing a 400 in that case
17:03:05 gibi gmann: so would you be supportive to keep 200 response but filter for both A and B _without_ a microversion bump?
17:03:42 gibi melwitt: in 1.39 (in a bump) I'm intended to do a proper translation including rejecting invalids
17:03:43 melwitt I'm trying to think of other past cases where the behavior was clearly wrong, have we never introduced 400 for that before?
17:03:47 stephenfin bauzas: :D nw, thank you!
17:04:05 gmann gibi: and no change in 1.39 ?
17:04:31 gibi gmann: in 1.39 there a new `in:` prefix is introduced
17:04:50 gibi required=in:A,B means A or B
17:05:01 gibi but we keep required=A,B as A and B
17:05:54 gmann gibi: ok, so that is different new things which is ok. what about required=A&required=B ? return A and B or 400 in >1.39 ?
17:06:07 melwitt gmann: 1.38 is the current microversion
17:06:28 gibi I propose that in 1.39 required=A&required=B is translated to required=A,B
17:06:29 gmann melwitt: I mean gibi new microversion 1.39
17:06:37 melwitt earlier I wrongly thought 1.39 was current
17:06:51 gibi gmann: but sean suggest to make that 400 instead there too
17:06:52 gmann I too initially until I checked in doc :)
17:07:14 gibi sorry for not being clear about that
17:07:18 gmann gibi: +1 on making 400 there which will be a clear usage instead of supporting multiple way
17:07:27 melwitt I think translation is fine in 1.39
17:07:29 gmann I mean with mivroversion
17:07:45 melwitt I was thinking what to do for <= 1.38
17:08:10 gmann melwitt: but then we end up supporting 1. required=A&required=B 2. required=in:A,B 3. required=A,B
17:08:28 gibi to note that is the easiest to support from code perspective :)
17:08:40 gibi to reject 3. I have to add extra logic
17:08:41 gmann IMO, in new microversion we can make first two to avoid confusion
17:09:08 melwitt what's wrong with supporting repeated and non repeated in 1.39?
17:09:15 gibi also note that repeating required=in: is needed to be able to express (A or B) and (C or D)
17:09:43 gmann anyways with new microversion, we can see what all to support. but my only concern is for older microversion we do not change anything what it is today
17:10:05 gmann and document in api-ref that required=A&required=B is unknown behavior until v1.39
17:10:19 gibi gmann: do we have a description that defines the line between API behavior (that is unfixable in a bugfix) and non API behavior that fixabelk in a bugfix
17:10:22 gibi ?
17:10:24 melwitt even honoring valid traits? I think at the very least it should be changed to honor the valid traits
17:12:54 gmann melwitt: yeah, for invalid I am ok. my concern is on required=VALID_A&required=VALID_B return VALID_B response today
17:13:54 gmann gibi: that is always debatable :). But IMO anything working successfully today even with wrong usage of API is what we should not change. in this case required=A&required=B, user gets 'B' response is successfully case.
17:14:24 gibi gmann: does successfull means http 2xx response?
17:14:32 gmann and we do not know user expectation is only to get B and by mistake they added A too in early query param
17:14:39 gmann gibi: ^^
17:14:54 gmann this case ^^ not just 200
17:15:30 melwitt hm, ok. I guess we disagree there, I think that it should be fixed to return VALID_A and VALID_B even for the current microversion
17:15:57 gmann melwitt: I am fine with that but not with returning 400 in this case for current microversion
17:15:58 gibi melwitt: I agree that we disagree :) and I would simply reject repeated params in the current microverison
17:16:25 gibi gmann: I don't get your last point to melwitt
17:16:50 gibi gmann: if we start returning A and B response for required=A&required=B but the user wants to get B only then it is a behavior change
17:16:59 melwitt fwiw I'm not as concerned about the 400, my main concern is honoring the valid traits in the current microversion
17:17:04 gibi as she get B so far but not any more
17:17:22 gibi melwitt: yeah, it seems 3 of us has 3 different area of concern :)
17:17:27 gibi fun :0
17:17:29 gibi ;)
17:17:32 melwitt yeah 😂
17:17:44 gmann gibi: well, we at least does break them in term of return code. and say required=A&required=B we meant for return A and B and we fixed it now.
17:18:01 gmann but returning 400 for them break them as they get something and then they get error
17:18:44 gibi gmann: I'm pretty sure if the relied on gettin RPs with B traits only and now getting empty result as we returning onyl RPs with both A and B will break tem
17:18:47 gibi them

Earlier   Later