On 5/16/20, Dave Barach (dbarach) <dbar...@cisco.com> wrote: > API messages in network byte order made sense 10 years ago when I worked > with a mixed x86_64 / ppc32 system. As Damjan points out, API > interoperability between big-endian and little-endian systems is a boutique > use-case these days. > > Timing is key. We won’t be able to cherry-pick API message handler fixes > across an endian-order flag-day. If we decide to switch to native byte > order, we’d better switch right before we pull our next LTS release.
Do I understand it right that the idea here would be to flip the endianness right before pulling stable/2009 branch ? --a > > FWIW... Dave > > From: vpp-dev@lists.fd.io <vpp-dev@lists.fd.io> On Behalf Of Damjan Marion > via lists.fd.io > Sent: Saturday, May 16, 2020 7:23 AM > To: Andrew Yourtchenko <ayour...@gmail.com> > Cc: Christian Hopps <cho...@chopps.org>; Ole Troan <otr...@employees.org>; > vpp-dev <vpp-dev@lists.fd.io>; Jerome Tollet (jtollet) <jtol...@cisco.com> > Subject: Re: (Q about fixing endianness bugs in handlers) Re: [vpp-dev] > Proposal for VPP binary API stability > > > Knowing that even hard-core big-endian systems like PowerPC decided to > switch to Little endian and that situation where binary api will be > exchanged between BE and LE systems is extremely small, maybe we should > consider removing endian conversion from APIs and simply represent data > in the native form. Looks like this is fertile ground for new bugs… > > Thoughts? > > — > Damjan > > >> On 15 May 2020, at 16:20, Andrew Yourtchenko >> <ayour...@gmail.com<mailto:ayour...@gmail.com>> wrote: >> >> There's a very interesting couple of gerrit changes that just came in >> today that is worth discussing, >> and they are a perfect case study to further clarify the process - so >> I tweaked the subject accordingly.. >> The API message itself is relatively minor, but regardless of what is >> agreed that makes a good case study. >> >> Backstory: >> >> Once upon a time on Aug 20 2019, commit 053204ab changed >> sw_interface_set_rx_mode.mode from u8 to an enum, but an htonl >> conversion >> function didn't make it there (enums are u32 by default as far as I can >> see). >> >> This was after the 19.08 branch pull, and it wasn't ever tackled, so >> this (buggy) behavior ended being in 20.01, 20.01.1, and in the >> current 20.05-rc1. >> >> Fast forward a bit, today I was pinged about the two changes - one for >> master, one for stable/2001: >> >> https://gerrit.fd.io/r/c/vpp/+/26879 - in master - forces the enum to be a >> u8 >> >> https://gerrit.fd.io/r/c/vpp/+/26915 - in stable/2001 - adds the >> htonl() call and changes the existing ("buggy") >> behavior in the 20.01.2 - thus would silently break any API consumers >> that coded against the previous "buggy" behavior. >> >> And then we have a question open about stable/2005, which "by the >> letter" can potentially accept only the second approach, since it is >> past the API freeze. >> >> Additional bit: this API is not touched in make test, so this bug had >> slipped through. >> >> So there are the following options: >> >> 1) Merge both patches, and treat the 20.05 similar to 20.01, thus >> making a "silent change" in both, but making the master look closer to >> a 19.08 format. >> >> 2) Leave the 20.05 and 20.01 alone with the "buggy" behavior, and >> merge the master patch that shrinks the enum down to 1 byte >> >> 3) Merge the 20.01 and cherry-pick it to master and 2005 - fixing the >> endianness of the u32 enum everywhere, but making an effective "silent >> change" in 20.01&20.05 >> >> 4) merge the patch in master that shrinks the enum down to one byte, >> and cherry-pick it to 20.01 and 20.05 - thus breaking the contract of >> "no api changes" but at least this gets to be explicitly visible early >> on. >> >> 5) under the current proposal, if the API message is defined as >> "production" then there would need to be a new "in-progress" message >> in master with either of the two fixes, the buggy message would need >> to be marked as "deprecated". And under the current proposal by the >> 20.09 the "in-progress" message would become "production", the current >> message would be shown as "deprecated", to be deleted in 21.01. >> >> So, the feedback that I would appreciate on the above: >> >> 1) the least worst course of action "right now", for this particular >> issue. I discussed this with Ole and we have different opinions, so I >> would like the actual API users to chime in. Please tell me which is >> the least worst option from your point of view :-) >> >> 2) What is the best course of action in the future. Note, this is also >> the simpler case in that there is a way to trigger a CRC-affecting >> change by forcing the enum to be a u8. What would have been the best >> course of action if it was simply a missing ntohl() for a field that >> noone complained about for 1.5 releases. Can we assume that no-one >> used that API and "fix the representation" ? >> >> 3) would there be an interest in having a sort of registry of "who >> wants to know about things related to each API" - so that if a bug >> like this arises that requires a behavior change to fix, I could poll >> the affected folks and maybe be able to get away with (1) or (3) from >> above ? >> >> And a tweak to the process (and potentially tooling) with regards to >> going to "production API status": >> >> An API that is not touched during a "make test" can not be moved >> beyond the "in-progress" status. Adding the unit test implies the >> commitment from the API contributor to follow the option (5) above >> even for "trivial endianness bugs". >> >> Thoughts ? >> >> --a >> >> >> >> >> On 5/14/20, Christian Hopps <cho...@chopps.org<mailto:cho...@chopps.org>> >> wrote: >>> API stability is borderline critical for a project like VPP I think. Yes, >>> it >>> can be used stand-alone, but real value is added by building products on >>> top >>> of it. >>> >>> Also important is having an API framework that allows for >>> backward-compatible changes to the API for making improvements. >>> >>> I'm not sure if VPP has the latter, but if there's too much pain with >>> your >>> proposed rules I think the latter should addressed rather than >>> sacrificing >>> the former. So I support your rules. >>> >>> Thanks, >>> Chris. >>> >>>> On May 14, 2020, at 8:28 AM, Ole Troan >>>> <otr...@employees.org<mailto:otr...@employees.org>> wrote: >>>> >>>> Andrew and I have discussed a process around API changes with the goal >>>> to >>>> limit the impact on VPP consumers. >>>> One big painpoint in tracking VPP releases for users of the VPP binary >>>> has >>>> been API changes. >>>> >>>> We want to ensure: >>>> - A production API never changes. >>>> >>>> - A production API can be deprecated with one release notice. >>>> E.g. show_version_2 is introduced in 20.01 and show_version_1 is marked >>>> as >>>> deprecated. >>>> show_version_1 is marked with "option delete_by = "v20.05"; >>>> >>>> - APIs marked as experimental / not released can change with no notice. >>>> Added support for option status = "in_progress"; >>>> >>>> This patch: >>>> https://gerrit.fd.io/r/c/vpp/+/26881 >>>> >>>> has a tool "crcchecker" that we intend to run as part of the verify >>>> job. >>>> It will block modifications to existing production APIs. >>>> make checkstyle-api >>>> It also supports dumping API manifests and make API comparisions across >>>> releases/revisions/commits. >>>> >>>> A production API is defined by having .api semantic major version > 0. >>>> New messages can override that by setting status="in_progress". >>>> >>>> The consequence of this is that developers must maintain two (or more) >>>> versions of an API for some time. >>>> This is obviously painful, but at least it aligns cost and benefit >>>> better >>>> than when we forced the API users to do it. >>>> >>>> Looking back from 17.01 onwards, it looks like we on average have done >>>> about 50 backwards incompatible changes per release. >>>> That's ignoring the work on API typing, which have resulted in >>>> significantly more changes. >>>> >>>> The hope is that we can start v20.09 development with this process in >>>> place. >>>> >>>> What do people think? >>>> >>>> Best regards, >>>> Ole >>> >>> >> > >
-=-=-=-=-=-=-=-=-=-=-=- Links: You receive all messages sent to this group. View/Reply Online (#16428): https://lists.fd.io/g/vpp-dev/message/16428 Mute This Topic: https://lists.fd.io/mt/74228149/21656 Group Owner: vpp-dev+ow...@lists.fd.io Unsubscribe: https://lists.fd.io/g/vpp-dev/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
