Re: [vpp-dev] Fixing nat44ed vrf api handlers

2023-03-27 Thread 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?

Daniel

-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#22770): https://lists.fd.io/g/vpp-dev/message/22770
Mute This Topic: https://lists.fd.io/mt/97774402/21656
Group Owner: vpp-dev+ow...@lists.fd.io
Unsubscribe: https://lists.fd.io/g/vpp-dev/leave/1480452/21656/631435203/xyzzy 
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [vpp-dev] Fixing nat44ed vrf api handlers

2023-03-22 Thread filvarga 
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š  napísal(a):

> 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
>
> 
>
>

-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#22751): https://lists.fd.io/g/vpp-dev/message/22751
Mute This Topic: https://lists.fd.io/mt/97774402/21656
Group Owner: vpp-dev+ow...@lists.fd.io
Unsubscribe: https://lists.fd.io/g/vpp-dev/leave/1480452/21656/631435203/xyzzy 
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [vpp-dev] Fixing nat44ed vrf api handlers

2023-03-22 Thread 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

-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#22750): https://lists.fd.io/g/vpp-dev/message/22750
Mute This Topic: https://lists.fd.io/mt/97774402/21656
Group Owner: vpp-dev+ow...@lists.fd.io
Unsubscribe: https://lists.fd.io/g/vpp-dev/leave/1480452/21656/631435203/xyzzy 
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [vpp-dev] Fixing nat44ed vrf api handlers

2023-03-22 Thread 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  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  napísal(a):
>
>> Hey Daniel,
>>
>> 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š 
>> wrote:
>>
>> 
>>
>> 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
>>
>>
>> Thanks,
>> Daniel
>>
>>
>>
>>
>>
>>
>>
> 
>
>

-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#22749): https://lists.fd.io/g/vpp-dev/message/22749
Mute This Topic: https://lists.fd.io/mt/97774402/21656
Group Owner: vpp-dev+ow...@lists.fd.io
Unsubscribe: https://lists.fd.io/g/vpp-dev/leave/1480452/21656/631435203/xyzzy 
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [vpp-dev] Fixing nat44ed vrf api handlers

2023-03-22 Thread filvarga 
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  napísal(a):

> Hey Daniel,
>
> 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š  wrote:
>
> 
>
> 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
>
>
> Thanks,
> Daniel
>
>
>
>
> 
>
>

-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#22748): https://lists.fd.io/g/vpp-dev/message/22748
Mute This Topic: https://lists.fd.io/mt/97774402/21656
Group Owner: vpp-dev+ow...@lists.fd.io
Unsubscribe: https://lists.fd.io/g/vpp-dev/leave/1480452/21656/631435203/xyzzy 
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [vpp-dev] Fixing nat44ed vrf api handlers

2023-03-22 Thread Andrew Yourtchenko 
Hey Daniel,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 :-)--aOn 22 Mar 2023, at 10:36, Daniel Béreš  wrote: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
Thanks,Daniel


-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#22747): https://lists.fd.io/g/vpp-dev/message/22747
Mute This Topic: https://lists.fd.io/mt/97774402/21656
Group Owner: vpp-dev+ow...@lists.fd.io
Unsubscribe: https://lists.fd.io/g/vpp-dev/leave/1480452/21656/631435203/xyzzy 
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-