[quagga-dev 16168] Re: [PATCH] Extend BGP_SEND_ASPATH_CHECK to cover confederations
On Mon, Oct 3, 2016 at 11:26 PM, Thorvald Natvig wrote: Hello Thorvald, Thanks for the detailed explanation. In fact, confederation feature, once assigned, is used in all transactions ( more info on rfc5065#section-4). For both reasons, I understand the check at announcement should be part of the BGP_SEND_ASPATH_CHECK, since no specific check is planned to be done at announcement. I would ack the fix, then. However, there is still a problem with the testing status ( https://ci1.netdef.org/browse/QUAGGA-QPWORK-357/ ). I would recommend to send the patch once again to quagga-dev, to trigger the running test, and see if the error still arises or not. If it still arises, Martin's help would be great, to identify the culprit ANVL test, or find out the root cause. I don't know if there can be some deprecated test ( rfc5065 is 9 years). Best Regards, Philippe > Hi, > > The #define is, by default, undefined. It's designed to cover > re-announcements of routes learned that contain your own AS. > Unfortunately, that #define only covers half the code; it's missing > for the code that deals with confederations. > > Let's use an example: > > My company has AS 123, and we have two locations. > Location "US" has subnet 1.2.3.0/24 using Router A. > Location "UK" has subnet 4.5.6.0/24 using Router B. > Connectivity between the two sites is over the the internet, passing > through one or more other AS on the way. > > Behind the router B sits another router; let's call this the Problem > Router or PR. Let's assume I've assigned my PR the AS 65000. > > If BGP_SEND_ASPATH_CHECK is defined, then Router B will detect that > the route to 1.2.3.0/24 contains its own AS (since both A and B are on > AS 123), and will not announce 1.2.3.0/24 to PR. Hence, PR can not > reach router A. > > This is clearly a problem, which is why this logic is only > conditionally enabled if BGP_SEND_ASPATH_CHECK is defined. By default > it remains undefined, so B will announce the route to PR. > > Let's reconfigure to a confederation, where B and PR form a > confederation with confederation-id 123. B will have have the internal > AS 65000, and PR has AS 65111. > > There are now two checks for the aspath. The check inside the #define > checks for the confederation-internal AS. B will check: Does the route > to 1.2.3.0/24 contain the AS 65000? It doesn't, so we pass the first > check. > The second check, which is not protected by the #define, checks "does > it contain the confed-id". Clearly, as-path to 1.2.3.0/24 does contain > the confed-id, so this route gets suppressed. > > All this patch does is make sure this second check, which is enabled > only for confederations, is covered by the same #define as the > non-confederation case. > > - Thorvald > > > On Mon, Oct 3, 2016 at 7:45 AM, Philippe Guibert > wrote: >> On Thu, Sep 29, 2016 at 7:25 PM, Thorvald Natvig >> wrote: >> >> Hello Thorvald, >> >>> Extend the check for BGP_SEND_ASPATH_CHECK to also cover confederations. >> .. >>> -#endif /* BGP_SEND_ASPATH_CHECK */ >>> >>>/* If we're a CONFED we need to loop check the CONFED ID too */ >>>if (CHECK_FLAG(bgp->config, BGP_CONFIG_CONFEDERATION)) >>> @@ -916,6 +915,7 @@ bgp_announce_check (struct bgp_info *ri, struct peer >>> *peer, struct prefix *p, >>> return 0; >>> } >>> } >>> +#endif /* BGP_SEND_ASPATH_CHECK */ >>> >> >> Unless unnoticed, the BGP_SEND_ASPATH_CHECK is not enabled in >> compilation process on quagga-dev continuous integration testing. >> So, assuming the error depicted by CI is related to BGPd ( >> interoperating with RIPng?), the problem could be due to the removal >> of code that has been undefined by the patch. >> >> Regarding the patch, confederation feature can be set by BGP VTY >> through "bgp confederation-identifier " command. >> I understand that with modification, no more check is done at >> announcement for condition to send BGP update to a conference-id >> group.No ? >> >> Is it your intention to not systematically test BGP conference id >> updates at announcement ? >> >> Best Regards, >> >> Philippe ___ Quagga-dev mailing list Quagga-dev@lists.quagga.net https://lists.quagga.net/mailman/listinfo/quagga-dev
[quagga-dev 16167] Re: Proposed 8 testing Update
Le 4 octobre 2016 3:58:59 AM Alexis Rosen a écrit : On Oct 3, 2016, at 11:21 AM, Vincent JARDIN wrote: Le 03/10/2016 à 15:08, Paul Jakma a écrit : My inclination is to release as is. Document regressions. Try fix them later. +1 ISTM the large majority of quagga users will not see any such documentation. They'll build/install using their distro's package manager. Then when things break, they'll come here looking for support, or else decide that quagga sucks and try something else. Agree. I do not think the intents were to announce a new stable tag/release but more to move forward so shared work and major OSPF fixing can happen on the head. Paul? Martin? ___ Quagga-dev mailing list Quagga-dev@lists.quagga.net https://lists.quagga.net/mailman/listinfo/quagga-dev
[quagga-dev 16165] Re: [PATCH] Extend BGP_SEND_ASPATH_CHECK to cover confederations
Hi, The #define is, by default, undefined. It's designed to cover re-announcements of routes learned that contain your own AS. Unfortunately, that #define only covers half the code; it's missing for the code that deals with confederations. Let's use an example: My company has AS 123, and we have two locations. Location "US" has subnet 1.2.3.0/24 using Router A. Location "UK" has subnet 4.5.6.0/24 using Router B. Connectivity between the two sites is over the the internet, passing through one or more other AS on the way. Behind the router B sits another router; let's call this the Problem Router or PR. Let's assume I've assigned my PR the AS 65000. If BGP_SEND_ASPATH_CHECK is defined, then Router B will detect that the route to 1.2.3.0/24 contains its own AS (since both A and B are on AS 123), and will not announce 1.2.3.0/24 to PR. Hence, PR can not reach router A. This is clearly a problem, which is why this logic is only conditionally enabled if BGP_SEND_ASPATH_CHECK is defined. By default it remains undefined, so B will announce the route to PR. Let's reconfigure to a confederation, where B and PR form a confederation with confederation-id 123. B will have have the internal AS 65000, and PR has AS 65111. There are now two checks for the aspath. The check inside the #define checks for the confederation-internal AS. B will check: Does the route to 1.2.3.0/24 contain the AS 65000? It doesn't, so we pass the first check. The second check, which is not protected by the #define, checks "does it contain the confed-id". Clearly, as-path to 1.2.3.0/24 does contain the confed-id, so this route gets suppressed. All this patch does is make sure this second check, which is enabled only for confederations, is covered by the same #define as the non-confederation case. - Thorvald On Mon, Oct 3, 2016 at 7:45 AM, Philippe Guibert wrote: > On Thu, Sep 29, 2016 at 7:25 PM, Thorvald Natvig > wrote: > > Hello Thorvald, > >> Extend the check for BGP_SEND_ASPATH_CHECK to also cover confederations. > .. >> -#endif /* BGP_SEND_ASPATH_CHECK */ >> >>/* If we're a CONFED we need to loop check the CONFED ID too */ >>if (CHECK_FLAG(bgp->config, BGP_CONFIG_CONFEDERATION)) >> @@ -916,6 +915,7 @@ bgp_announce_check (struct bgp_info *ri, struct peer >> *peer, struct prefix *p, >> return 0; >> } >> } >> +#endif /* BGP_SEND_ASPATH_CHECK */ >> > > Unless unnoticed, the BGP_SEND_ASPATH_CHECK is not enabled in > compilation process on quagga-dev continuous integration testing. > So, assuming the error depicted by CI is related to BGPd ( > interoperating with RIPng?), the problem could be due to the removal > of code that has been undefined by the patch. > > Regarding the patch, confederation feature can be set by BGP VTY > through "bgp confederation-identifier " command. > I understand that with modification, no more check is done at > announcement for condition to send BGP update to a conference-id > group.No ? > > Is it your intention to not systematically test BGP conference id > updates at announcement ? > > Best Regards, > > Philippe ___ Quagga-dev mailing list Quagga-dev@lists.quagga.net https://lists.quagga.net/mailman/listinfo/quagga-dev
[quagga-dev 16166] Re: Proposed 8 testing Update
On 3 Oct 2016, at 6:08, Paul Jakma wrote: Hi Martin, On Sun, 2 Oct 2016, Martin Winter wrote: I did a full test run of our RFC compliance testbed against volatile/patch-tracking/8/proposed/ff-2016093002 (Commit e4adc26) Results are at https://drive.google.com/drive/folders/0B8W_T0dxQfwxZFdJd1lPOXo3bHM?usp=sharing The biggest issues seem to be several new failures on OSPFv2 Thanks for this. My inclination is to release as is. Document regressions. Try fix them later. Ouch. OSPFv2 seems to be seriously broken. If you mean release as “an official release”, then I would vote against. If you think just merge to master, then maybe. Still don’t like it (at least without some attempt to fix it) - Martin ___ Quagga-dev mailing list Quagga-dev@lists.quagga.net https://lists.quagga.net/mailman/listinfo/quagga-dev
[quagga-dev 16164] Re: Proposed 8 testing Update
On Oct 3, 2016, at 11:21 AM, Vincent JARDIN wrote: > Le 03/10/2016 à 15:08, Paul Jakma a écrit : >> >> My inclination is to release as is. Document regressions. Try fix them >> later. > +1 ISTM the large majority of quagga users will not see any such documentation. They'll build/install using their distro's package manager. Then when things break, they'll come here looking for support, or else decide that quagga sucks and try something else. /a ___ Quagga-dev mailing list Quagga-dev@lists.quagga.net https://lists.quagga.net/mailman/listinfo/quagga-dev
[quagga-dev 16163] Re: Proposed 8 testing Update
Le 03/10/2016 à 15:08, Paul Jakma a écrit : My inclination is to release as is. Document regressions. Try fix them later. +1 ___ Quagga-dev mailing list Quagga-dev@lists.quagga.net https://lists.quagga.net/mailman/listinfo/quagga-dev
[quagga-dev 16162] Re: [PATCH] Extend BGP_SEND_ASPATH_CHECK to cover confederations
On Thu, Sep 29, 2016 at 7:25 PM, Thorvald Natvig wrote: Hello Thorvald, > Extend the check for BGP_SEND_ASPATH_CHECK to also cover confederations. .. > -#endif /* BGP_SEND_ASPATH_CHECK */ > >/* If we're a CONFED we need to loop check the CONFED ID too */ >if (CHECK_FLAG(bgp->config, BGP_CONFIG_CONFEDERATION)) > @@ -916,6 +915,7 @@ bgp_announce_check (struct bgp_info *ri, struct peer > *peer, struct prefix *p, > return 0; > } > } > +#endif /* BGP_SEND_ASPATH_CHECK */ > Unless unnoticed, the BGP_SEND_ASPATH_CHECK is not enabled in compilation process on quagga-dev continuous integration testing. So, assuming the error depicted by CI is related to BGPd ( interoperating with RIPng?), the problem could be due to the removal of code that has been undefined by the patch. Regarding the patch, confederation feature can be set by BGP VTY through "bgp confederation-identifier " command. I understand that with modification, no more check is done at announcement for condition to send BGP update to a conference-id group.No ? Is it your intention to not systematically test BGP conference id updates at announcement ? Best Regards, Philippe ___ Quagga-dev mailing list Quagga-dev@lists.quagga.net https://lists.quagga.net/mailman/listinfo/quagga-dev
[quagga-dev 16161] Re: Proposed 8 testing Update
Hi Martin, On Sun, 2 Oct 2016, Martin Winter wrote: I did a full test run of our RFC compliance testbed against volatile/patch-tracking/8/proposed/ff-2016093002 (Commit e4adc26) Results are at https://drive.google.com/drive/folders/0B8W_T0dxQfwxZFdJd1lPOXo3bHM?usp=sharing The biggest issues seem to be several new failures on OSPFv2 Thanks for this. My inclination is to release as is. Document regressions. Try fix them later. regards, -- Paul Jakma | p...@jakma.org | @pjakma | Key ID: 0xD86BF79464A2FF6A Fortune: Also, the Scots are said to have invented golf. Then they had to invent Scotch whiskey to take away the pain and frustration. ___ Quagga-dev mailing list Quagga-dev@lists.quagga.net https://lists.quagga.net/mailman/listinfo/quagga-dev
[quagga-dev 16160] Re: Doing something different wrt patch integration
Hi Lou, On Fri, 30 Sep 2016, Lou Berger wrote: Paul, Just restating things I've said here before : - I don't the issue is backlog It definitely was an issue. People were complaining about how long stuf had been waiting. I'm not imagining that, pretty sure. More generally, there are issue_s_. I don't think there is any /one/ issue, the solving of which there is a magic bullet for. - I do think the issue is that in order for organizations/companies/individuals to invest in an open source project that there needs to be process transparency, predicable releases, and a deterministic way to get submissions/patches into a release. process transparency, regular releases, deterministic process for submissions, yes, all good. Though "to get submissions/patches into a release" is assuming a bit too much. - I frankly don't see how the current model scales or satisfies this. Well, based on queue length it has objectively done better than what was there before. As the queue length has gone from increasing to decreasing, and from years of stuff waiting for action on the review to less than a years worth of stuff (assuming Martin's full protocol tests pass and we get a release out RSN). Is it the best model for a steady state? I don't know. Are there further improvements to make, no doubt. However, it seemed the best way to get stuff done on the backlog, in a fairly transparent and systematic way, given the lack of willing resources. - A bunch of the community worked together to come up with a proposed new process and wanted to try it out -- which, from my perspective, you vetoed. Cause it wanted to change _everything_: - The ethos and constitution of Quagga From one where the onus is on submitters to get submissions to a standard high enough in terms of (as applicable) design documentation and advance planning, code organisation and style, and testing, so that at least no one else strongly enough to object; To one where alliances of contributors, potentially some with next to _no prior involvement_ in Quagga, could band together to shove stuff through, and where reviewers with objections would have only days to try rally others to their view. To the extent I'm mistaken on that, it was to the extent this was under-specified (however, the intent of majority voting for stuff to go in was clearly there). I.e., code going in based on the 51% with the /lowest/ standards, and a more political and gameable process. I think the biggest gulf between the proposers of the mega-changes and myself was in this area. - The integration process was actually _gaining_ overhead and bureaucracy from a day to day process. Patches to require explicit 2nd ACKs - that's overhead. IME with Quagga, most stuff doesn't need ACKs. People will object to objectionable stuff, and they'll let it pass otherwise. A "Revisions needed" model for those minority of patches is more efficient than a "ack for every patch". - The proposed integration process was more a contributors wish list than a readily implementable integration process. Patches to go be applied within days, by who? How? I've written before about lessons learned in the past from Quagga, on patches can more easily fall through the cracks when it isn't clear who is responsible for applying. Also "days" - there are a number of problems with that, in various dimensions (see also above on ethos). The contributors view of the process is only half the story. It also has to work for and be efficient for the integrator(s). See also prior. If you increase the overheads for integrators, then things likely get worse. - Changing the communication tools from email to video conferencing. Real-time comms is nice for forming opinions, but it's ungood for definitive decision making comms. Quagga, as with many open-source projects, is distributed across the world, and it impossible to suit everyone (and people in global orgs may already have calendar competition for time-slots that suit multiple parts of the globe). AV also needs reliable bandwidth, not always a guarantee. (E.g., right now I don't have that). Also, I much prefer email for deliberative stuff. - Multiple branches and releases, with unclear relationships between them. Overall, the proposals were vague on many important things. On some of the more concrete points, they seemed to be objectively adding overheads (rather than decreasing), and changing the ethos of the project to one that I don't like (cause, I've done the "shove everything in" thing already - in the early days of Quagga; and it lead to quality issues that took a _lot_ of years to fix; so I don't want to go back there). So, let's change stuff for the better. There's a lot of scope for more automation, and moving more of the work to the contributor side: - some kind of auto-integrating head, to accumulate stuff from the list in a li
