Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2022-01-31
15:32:38 gmann It was the same issue for nova query param too, we were ignoring the param and users might not get the expected behavior what they think will get. But we did not fix that without microversion and have to fix in new microversion to avoid breaking any users
15:33:31 gmann why I say it break users is we allowed our API to be used that way and it return something and we do not know what users expect with API to be used that wrong way, expectation might be what our code return now.
15:34:28 sean-k-mooney gmann: yes i understand your argument, i just find it hard to accpet that argument when we no any qurty that does this today is invalid
15:35:00 sean-k-mooney /qurty/query
15:35:54 sean-k-mooney if the perference is to document this which it seams you and efried perfer and only fix going forward as part of the new feature we can do that
15:36:28 sean-k-mooney its what gibi initally did before i raiesed the question in the review
15:37:17 gmann yeah, that is better way IMO. documenting the known bug until this microversion and with new version we fixed it.
15:37:18 sean-k-mooney well technially gibi put in functional tests to assert the current behavior for old microverison to ensure he did not break it
15:37:54 sean-k-mooney gmann: to me this is like intentiolly leaving a cve open but ok
15:38:04 sean-k-mooney we know it wont affect nova
15:38:14 sean-k-mooney sicne we never generate this edge case
15:39:04 sean-k-mooney ironic dpeloyed in standalone mode without nova today i dont think uses placment
15:39:36 sean-k-mooney so im not aware of an consumer of placment that are likely to hit this other then endusers making direct curl requests
15:39:56 sean-k-mooney so the impact really shoudl be minor
15:40:08 gmann I know, and its a bug and we have to accept/live with it for older microversion as this is what our API returned even wrong or right does not matter but in some cases it might return right-as-per-usage
15:41:53 sean-k-mooney gmann: i can say very clearly that if we had a customer that relied on this in any way we would not supprot there continued use of it at the expence of the rest of our customers
15:42:58 opendevreview Imran Hussain proposed openstack/nova master: [nova/libvirt] Support for checking and enabling SMM when needed https://review.opendev.org/c/openstack/nova/+/825496
15:48:11 gmann yeah, that is what API is. If we allowed to use our API wrongly with some usable results then it is what we allowed and cannot change the API.
15:53:41 opendevreview Dmitrii Shcherbakov proposed openstack/nova master: [yoga] Add PCI VPD Capability Handling https://review.opendev.org/c/openstack/nova/+/808199
15:53:42 opendevreview Dmitrii Shcherbakov proposed openstack/nova master: [yoga] Include pf mac and vf num in port updates https://review.opendev.org/c/openstack/nova/+/824833
15:53:42 opendevreview Dmitrii Shcherbakov proposed openstack/nova master: [yoga] Introduce remote_managed tag for PCI devs https://review.opendev.org/c/openstack/nova/+/824834
15:53:43 opendevreview Dmitrii Shcherbakov proposed openstack/nova master: Bump os-traits to 2.7.0 https://review.opendev.org/c/openstack/nova/+/826675
15:53:43 opendevreview Dmitrii Shcherbakov proposed openstack/nova master: [yoga] Add support for VNIC_TYPE_SMARTNIC https://review.opendev.org/c/openstack/nova/+/824835
15:53:44 opendevreview Dmitrii Shcherbakov proposed openstack/nova master: Filter computes without remote-managed ports early https://review.opendev.org/c/openstack/nova/+/812111
15:56:44 dmitriis sean-k-mooney: submitted the changes in https://review.opendev.org/q/topic:2021-09-10-off-path-net-backends with a dependency on https://review.opendev.org/c/openstack/nova/+/819494/
16:00:23 opendevreview Imran Hussain proposed openstack/nova master: [nova/libvirt] Support for checking and enabling SMM when needed https://review.opendev.org/c/openstack/nova/+/825496
16:08:15 melwitt gibi: +1 to fix as bug without new microversion
16:19:24 stephenfin bauzas: Around? I think melwitt left this to you to confirm https://review.opendev.org/c/openstack/nova/+/819366
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 gibi or a valid one
16:47:04 melwitt sorry, trying to ask if required=VALID_A,INVAILD_B,VALID_C would raise 400?
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

Earlier   Later