Locked Fixing nat44ed vrf api handlers
|
Daniel Béreš
Hello Guys, I wrote a short patch to fix a couple api handlers for nat44ed [1] .It was probably forgotten, so I'm reminding myself. I still have a question about whether it is possible to cherry-pick the patch to vpp v23.02 and 22.10. [1] https://gerrit.fd.io/r/c/vpp/+/38459
|
|
|
|
Andrew Yourtchenko
Hey Daniel,
toggle quoted message
Show quoted text
I had a quick look and it looks like the APIs in question are also not marked as experimental ? If I am right, then to me seems like a silent behavior change to a production API - something we have heard repeated complaints from the downstream consumers for…. I had -2’d it for now - we should definitely discuss it here first; as is i would consider it a no-go even for a master, but I am happy to hear others opinions and be convinced otherwise :-) --a On 22 Mar 2023, at 10:36, Daniel Béreš <daniel.beres@...> wrote:
|
|
|
|
Hi, Yes I fully agree with Andrew on changing APIs. Though the api was broken in the first place. If used incorrect message is returned. Because of that I am not even sure if it is functional at all. It is more likely that nobody before addressed this issue because this feature is not used. So I would suggest either to set it as deprecated and add v2 which will be flagged as experimental or change it on master because It might not be even functional. Best regards, Filip Varga st 22. 3. 2023 o 14:11 Andrew Yourtchenko <ayourtch@...> napísal(a):
|
|
|
|
Andrew Yourtchenko
Some more context: after discussion with Ole: I think I might have overstepped the authority in terms of "-2" - it is component maintainers' decision if they want to put even completely breaking change into their component that sets the world on fire - if the CI lets that through - it is their decision. My apologies for this - I changed my vote to "-1". As for some substance: Looks like there are two issues in one commit (always useful to split even if they are small :) 1) incorrect message IDs used in replies. This, we could argue, would have been hard (if possible at all) to make work - I will let the govpp maintainers to prove me wrong, but at least my experience with Rust version of APIs - those are not usable. With my release manager hat on - I could be convinced to have this cherry-picked even into the stable branches. 2) the endianness of the fields in the nat44_ed_vrf_tables_send_details: here, the message ID looks correct, so one can make a conservative conjecture that an API user might have used the API, but "fixed" it on the client side. So the "fix" on the VPP side would trigger unexpected changes required on their side. Given the message is not marked experimental, this is a silent violation of what we promise about API compatibility. With my release manager hat on - I definitely would not like to see it cherry-picked to stable branches, even if maintainers decide it's "good enough" to have it merged to master (which I am also not a fan of, hence my now -1 :) The reason I am so "pig-headed" about the API changes is - every alternative-free modification creates "urgent" work downstream. I am sure the consumers have worked out the processes that it does not turn into "drop your friday night party with friends and fix it"-urgent, but it is still unplanned work. And we promise with "production" APIs that there will be no unplanned work in their maintenance. Hopefully it gives a bit more context and the background of my reasoning :-) --a On Wed, Mar 22, 2023 at 2:26 PM filvarga <filipvarga89@...> wrote:
|
|
|
|
Daniel Béreš
Thanks for the reasonable explanations. So the option could be splitting it into two patches. The first could fix replies and the second could contain a new experimental API nat44_ed_vrf_tables_send_details_v2. I think it might be okay after that. Daniel |
|
|
|
If you decide to split the patch and do a second version. I would like to see the first version set as deprecated. So when removed we can put it again in without having to have _v2 _v3 etc. Best regards, Filip Varga st 22. 3. 2023 o 15:33 Daniel Béreš <daniel.beres@...> napísal(a):
|
|
|
|
Daniel Béreš
I split it up and it's ready for review: https://gerrit.fd.io/r/c/vpp/+/38459 https://gerrit.fd.io/r/c/vpp/+/38551 I have last one question: In case API doesn't return all data (e.g nat44_ed_vrf_tables_send_details missing table_vrf_id assign of value) and if I decide to add it. Is it considered as silent behaviour change? |
|
|