Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2022-01-31
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
17:18:48 gmann I mean we fix the code to make it return A and B is ok but making them error is issue
17:19:33 gmann gibi: we had similar (not exactly same ) issue in nova query param for silently ignoring few/unknown also and we could not change it without microversion
17:20:44 gibi OK, I'm dropping the bugfix from the patchseries of microversion 1.39. I still intend to land 1.39 this cycle. I don't have such mandate for the bugfix itself. so I prioritize
17:20:45 gmann gibi: sure, in that case we can just leave the current behavior as it is and not breaking anything and say "new microversion gives correct behavior"
17:21:27 gmann I am if we want to change for current microversion then we "make it more correct is fine but rejecting the request is not"
17:21:36 melwitt wait, I don't understand why the bug fix can't go in 1.39 if 1.39 is not released yet?
17:21:57 gmann I think gibi saying bug fix for older microversion also
17:22:08 gibi melwitt: the bug will disappera in 1.39
17:22:20 gibi melwitt: but I will not try to fix it in <1.39 now
17:22:28 gibi as it seem we cannot agree
17:22:59 melwitt ah ok
17:23:00 gibi 1.39 alway planned to allow repeating the required query param
17:23:17 gibi as in: needs to be repeated for (A or B) and (C or D) case
17:24:11 melwitt ok, so repeated was not officially supported < 1.39. sorry I had missed that
17:24:44 gmann yeah, it was always nor documented neither we knew how it work until gibi found it?
17:24:47 gibi melwitt: <1.39 there was not defined behavior for repeat
17:25:08 gibi the code happened to pares the last repetition
17:25:12 gibi parse
17:25:15 gibi and ignore the rest
17:25:49 melwitt gotcha.. thanks
17:28:48 opendevreview Balazs Gibizer proposed openstack/placement master: Extra tests around required traits https://review.opendev.org/c/openstack/placement/+/825846
17:28:49 opendevreview Balazs Gibizer proposed openstack/placement master: Extend the RP db query to support any-traits https://review.opendev.org/c/openstack/placement/+/825848
17:28:49 opendevreview Balazs Gibizer proposed openstack/placement master: Refactor trait normalization https://review.opendev.org/c/openstack/placement/+/825847
17:28:50 opendevreview Balazs Gibizer proposed openstack/placement master: Enhance doc of _get_trees_with_traits https://review.opendev.org/c/openstack/placement/+/825780
17:28:50 opendevreview Balazs Gibizer proposed openstack/placement master: DB layer should only depend on trait id not names https://review.opendev.org/c/openstack/placement/+/826490
17:28:51 opendevreview Balazs Gibizer proposed openstack/placement master: Add any-traits support for listing resource providers https://review.opendev.org/c/openstack/placement/+/826491
17:28:51 opendevreview Balazs Gibizer proposed openstack/placement master: Extend the RP tree DB query to support any-traits https://review.opendev.org/c/openstack/placement/+/825849
17:28:52 opendevreview Balazs Gibizer proposed openstack/placement master: Remove unused compatibility code https://review.opendev.org/c/openstack/placement/+/826493
17:28:52 opendevreview Balazs Gibizer proposed openstack/placement master: Add any-traits support for allocation candidates https://review.opendev.org/c/openstack/placement/+/826492
17:28:54 opendevreview Balazs Gibizer proposed openstack/placement master: Add microversion 1.39 to support any-trait queries https://review.opendev.org/c/openstack/placement/+/826719
17:29:23 melwitt dansmith, bauzas: just a fyi, oslo.limit got a new release so I cleaned up the unified limits set to use it and removed the haxx
17:30:12 gibi sean-k-mooney, gmann, melwitt, efried: I restored the patch series of microversion 1.39 not to change the behavior of the repeated required param handling in < 1.39 microversion https://review.opendev.org/q/topic:any-traits-support

Earlier   Later