Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2022-01-31
15:13:08 sean-k-mooney efried: iguess we were just tryign to avoid operators having upgrade pain
15:13:11 gmann * if you would change it to 400 always
15:13:19 sean-k-mooney nova will not repeat the parmaters today
15:13:30 sean-k-mooney so for nova this wont matter one way of another
15:14:32 efried yeah, as usual the reality is that it is vanishingly unlikely that anything in the field is going to be affected by this fix; it's a matter of doing The Right Thing software hygiene process-wise.
15:14:58 gibi I feel more hygenic if we not silently ignore query params
15:15:00 sean-k-mooney right so hygine wise accepting known invlaid input to me is wrong
15:15:33 efried um, that's not the point. I don't think anyone is disputing that there's a bug. It's a matter of the proper procedure to get the fix done.
15:15:42 opendevreview Stephen Finucane proposed openstack/nova stable/train: api: Reject duplicate port IDs in server create https://review.opendev.org/c/openstack/nova/+/827126
15:15:49 gibi I cannot fix the bug on stable branches if it is requires a microversion bump
15:16:09 gmann efried: yeah.
15:16:10 efried stable branches of what?
15:16:14 sean-k-mooney placment
15:16:16 gibi placenet
15:16:20 gibi placement
15:16:49 efried And those stable branches affect... what? Only OpenStack?
15:16:51 gmann it is definitely a bug but our code has made it working in some way even with not expected result
15:17:27 gibi efried: this argument can be used for any placement change. should we then drop all placement stable branches?
15:17:38 gmann or may be expected results as we cannot say users would not be using it for query B only but in mistaken way of leaving duplicate query
15:18:09 efried IMHO documenting the old broken behavior for stable branches would be adequate.
15:18:54 efried Stable consumers who are affected and care can fix their query if they wish.
15:18:55 gmann gibi: sean-k-mooney efried I feel best way for existing user or stable is to update the api-ref "required=A,B&required=B is unexpected behavior until microversion 1.39 and after it work like returning both "
15:19:32 gmann yeah documenting it is enough I think when we are anyways fixing it with microversion
15:20:32 gmann that is how we do for any other bugs also right? 'fix in new microversion and old microverison it is bug and we donot fix'
15:20:39 sean-k-mooney gmann: well the primary reason for supprot it in 1.39 is for the any traits feature
15:20:55 gibi I don't believe that we want to keep around an incorrect behavior just because somebody might depend on it. but at the same time we say stable consumers can fix their query. I would say master consumer can fix their query too after we forbid the wrong behavior
15:20:58 gmann sean-k-mooney: yeah, I mean if we are fixing this query format too
15:21:00 sean-k-mooney gmann: if one of the repeated section does not use in: you still shoudl not be using it
15:21:33 gibi sean-k-mooney: after 1.39 you can repeate required without in: prefix as well
15:21:56 sean-k-mooney yes but im not sure we shoudl actully allow that
15:21:59 sean-k-mooney it simpler too
15:22:05 sean-k-mooney but its not useing the api as intended
15:22:15 sean-k-mooney we can allow it but i dont think we shoudl encurage it
15:22:29 sean-k-mooney after all it we have a limited query string lenght
15:22:42 sean-k-mooney and this is much more wasteful the jsut combining them
15:23:01 sean-k-mooney so it may be valild but not idiomatic
15:23:39 gibi I will be back in an hour
15:23:44 gibi sorry
15:24:08 sean-k-mooney sorry didnt want to burn you out either
15:25:00 sean-k-mooney im not really asking for you to expiclty block the repated case without in specificaly but i woudl like us to docuemnt that it shoudl only be used for that usecase ideally
15:25:59 gmann I think blocking it with microversion and asking to do via "required=A,B" is better way and saying "required=A,B&required=B" in older microversion is unknown behaviour
15:26:38 gmann or even "required=A,B&required=B" is unknown behavior is any microversion , do via "required=A,B"
15:27:02 sean-k-mooney ok if that is what peopel prefer but i dont like having to opt into correct behavior which is what that forces
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] Introduce remote_managed tag for PCI devs https://review.opendev.org/c/openstack/nova/+/824834
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: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: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: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 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

Earlier   Later