Fix a number of VPN SAFI related issues, add vrf flavor of vnc commands #75

louberger · 2017-01-16T02:29:25Z

Issues:
bgpd: allow VPN next hop to be different AFI than NLRI next hop (Issue #71)
bgpd: fix RD stomping by update group code (Issue #71)
bgpd: partial revert of vpn/encap safi show changes (Issue #14)
To remove commands duplicated in bgp_mplsvpn/_encap
Also until vpn/encap specific show pieces are merged into
generic afi/safi show.
Fixes/cleanup
bgpd rfapi: fix issue where advertised prefixes were not being disambiguated
by RD
bgpd rfapi: use VN as nexthop for MPLS tunnels too
Also minor show format cleanup
bgpd rfapi: add NVE/VRF name to show vnc registrations
new commands (partail set from RD/RT discussion)
bgpd: add vrf-policy config using existing vnc code
add add/clear vrf prefix

To remove commands duplicated in bgp_mplsvpn/_encap Also until vpn/encap specific show pieces are merged into generic afi/safi show. Signed-off-by: Lou Berger <lberger@labn.net>

Signed-off-by: Lou Berger <lberger@labn.net>

…FRRouting#71) Signed-off-by: Lou Berger <lberger@labn.net>

Also minor show format cleanup Signed-off-by: Lou Berger <lberger@labn.net>

add add/clear vrf prefix Signed-off-by: Lou Berger <lberger@labn.net>

…iguated by RD Signed-off-by: Lou Berger <lberger@labn.net>

NetDEF-CI · 2017-01-16T02:45:40Z

Continous Integration Result: FAILED

See below for issues.
CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-53/

This is a comment from an EXPERIMENTAL automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

Get source and apply patch from patchwork: Successful

Building Stage: Failed

Fedora24 amd64 build: Successful
Ubuntu1204 amd64 build: Successful
Ubuntu1604 amd64 build: Successful
Ubuntu1404 amd64 build: Successful
FreeBSD11 amd64 build: Successful
Debian8 amd64 build: Successful
NetBSD6 amd64 build: Successful
CentOS7 amd64 build: Successful
CentOS6 amd64 build: Successful
OpenBSD60 amd64 build: Successful
NetBSD7 amd64 build: Successful
FreeBSD10 amd64 build: Successful
FreeBSD9 amd64 build: Successful

OmniOS amd64 build: Failed

Make failed for OmniOS amd64 build:
(see full make log at /browse/FRR-FRRPULLREQ-53/artifact/CI010BUILD/ErrorLog/log_make.txt)

  CC       rfapi/rfapi_vty.o
rfapi/rfapi_vty.c: In function rfapiDeleteLocalPrefixesByRFD:
rfapi/rfapi_vty.c:3304:12: error: expected identifier or ( before = token
   int noop = 1;
            ^
rfapi/rfapi_vty.c:3320:16: error: expected expression before ) token
   while (noop--)                /* to preserve old code structure */
                ^
rfapi/rfapi_vty.c:3483:17: error: h undeclared (first use in this function)

OmniOS amd64 build: Unknown Log <config.status>
URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-53/artifact/CI010BUILD/config.status/config.status

Signed-off-by: Lou Berger <lberger@labn.net>

NetDEF-CI · 2017-01-16T13:48:34Z

Continous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-55/

This is a comment from an EXPERIMENTAL automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

donaldsharp · 2017-01-16T14:57:33Z

bgpd/rfapi/rfapi_vty.c

@@ -3317,7 +3317,7 @@ rfapiDeleteLocalPrefixesByRFD (struct rfapi_local_reg_delete_arg *cda,
      rfapiQprefix2Rprefix (pPrefix, &rprefix);
    }

-  while (noop--)                /* to preserve old code structure */
+  while (loop--)                /* to preserve old code structure */


Get rid of loop/noop and use a do { ....} while (0); structure? So we know it's supposed to be run one time?

donaldsharp · 2017-01-16T15:07:38Z

bgpd/bgp_updgrp_packet.c


          if (bgp_debug_update(peer, NULL, NULL, 0))
-            zlog_debug ("u%" PRIu64 ":s%" PRIu64 " %s send UPDATE w/ nexthop %s",
+            zlog_debug ("u%" PRIu64 ":s%" PRIu64 " %s send UPDATE w/ nexthop %s%s",
                    PAF_SUBGRP(paf)->update_group->id, PAF_SUBGRP(paf)->id,


We should fix this indentation too if we are modifying the indentation to be correct for the line below

humm, I thought there was a bias against this.

We have two choices here, either make the + line line up with the PAF_SUBGRP.. line or move the PAF_SUBGRP... line line up with the correct spot for zlog_debug. Adding a code change where we now have two separate indentations breaks the rule of 'do what the code does around me' that we try to have. Of the two I would prefer fixing the indentation right. The point of not changing lines is that it makes it harder to merge. But since we are already changing the line above and line below the PAF_SUBGRP line it will kill any merges there. As such choose one indentation for the zlog_debug and use it.

Well this is code written by your colleges -- and I didn't change any alignment -- I just added to the output. I really don't think changes should include gratuitous whit space changes. This said, I'll do whatever is needed to end this discussion;-) How do you want it formatted?

Lou -

You clearly changed the alignment for the function call to be different on different lines and arguing that you didn't is disingenuous. In what world is it acceptable coding style to intentionally put in mixed style in the middle of a function call?

huh, I'm responding to the lines quoted above. If you're referring to different lines -- my apologies. I'll go and look at the full patch set and try to understand what you might be referring to. (Alternatively, tell me the line numbers.) Either way, I'll make whatever white space changes...

bgp_updgrp_packet.c: 488-491

Thats helpful -- I'll check it out.

donaldsharp · 2017-01-16T15:15:02Z

Aren't these changes to the api directly what we are discussing in the RD/RT thread?

louberger · 2017-01-16T20:12:04Z

yes. Basically accessing existing (VNC) function using new syntax.

Signed-off-by: Lou Berger <lberger@labn.net>

NetDEF-CI · 2017-01-17T02:41:55Z

Continous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-57/

This is a comment from an EXPERIMENTAL automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

pguibert6WIND

hello,
I approve the changes on the vrf-policy discussion.
Two questions:

I did not see the code for configuring rt import | export command.
Maybe I misread. Is it present somewhere ?
About the code change, for continuation. I understand this is for stable/2.0. Does it mean I need to rebase all on stable/2.0, if I want to continue rd/rt development ?

pguibert6WIND · 2017-01-17T10:48:52Z

bgpd/rfapi/bgp_rfapi_cfg.c

+  if (rfg->rfp_cfg)
+    XFREE (MTYPE_RFAPI_RFP_GROUP_CFG, rfg->rfp_cfg);
+  listnode_delete (bgp->rfapi_cfg->l2_groups, rfg);
+


not better to remove from the list, before unchaining it ?

the member (rfg->rfp_cfg) is being freed before the node (rfg) is freed.

pguibert6WIND · 2017-01-17T10:50:57Z

bgpd/rfapi/bgp_rfapi_cfg.c

+                  {
+
+                    vty_out (vty, "!Error: Can't convert rd%s", VTY_NEWLINE);
+                  }


if it is for show running, this if condition should never occur

fair enough -- that said, I like handling unexpected error paths. Do you really think this needs to be removed.

pguibert6WIND · 2017-01-17T10:52:03Z

bgpd/rfapi/bgp_rfapi_cfg.c

+                     VTY_NEWLINE);
+          }
+        vty_out (vty, "  exit-vrf-policy%s", VTY_NEWLINE);
+        vty_out (vty, "!%s", VTY_NEWLINE);


vty_out (vty, " exit-vrf-policy%s", VTY_NEWLINE);

one single space character.

I think the alignment is right, here's sample output:
!
vrf-policy green
rd auto:nh:11
rt both 2002:99
exit-vrf-policy
!

maybe i am wrong, i am used to this. up to you if all bgp subnode commands are like that.
vrf-policy green
rd auto:ng:11
rt both 2002:99
exit-vrf-policy

pguibert6WIND · 2017-01-17T11:09:06Z

bgpd/bgp_attr.c

@@ -2643,6 +2643,8 @@ bgp_packet_mpattr_start (struct stream *s, afi_t afi, safi_t safi, afi_t nh_afi,
  stream_putw (s, afi);
  stream_putc (s, (safi == SAFI_MPLS_VPN) ? SAFI_MPLS_LABELED_VPN : safi);

+  if (nh_afi == AFI_MAX)
+    nh_afi = (attr->extra->mp_nexthop_len == 16 ? AFI_IP6 : AFI_IP);
  /* Nexthop */


mp_nexthop_len can be other than 16 for MPLS_VPN IPV6.

you are correct!!!

also see a related issue in bgp_route.c

pguibert6WIND · 2017-01-17T11:12:21Z

bgpd/bgp_route.c

     (p->family == AF_INET6 || peer_cap_enhe(peer))) || \
-    (safi == SAFI_ENCAP && attr->extra->mp_nexthop_len == 16))
+    ((safi == SAFI_ENCAP || safi != SAFI_MPLS_VPN) &&\
+     attr->extra->mp_nexthop_len == 16))



maybe a macro could detect if it is nexthop ipv6 or ipv4 by reusing defines used in code from bgp_info_nexthop_cmp() ?
BGP_ATTR_NHLEN_IPV6_GLOBAL_AND_LL,
BGP_ATTR_NHLEN_IPV6_GLOBAL
BGP_ATTR_NHLEN_VPNV6_GLOBAL

...

I like the idea of a macro. How about just testing if against IPV6_MAX_BYTELEN (< = IPv4)

!= 0 and < IPV6_MAX_BYTELEN

in bgp_route.h
#define BGP_NEXTHOP_AFI_FROM_NHLEN(nhlen)
((nhlen) < IPV4_MAX_BYTELEN ? 0 :
((nhlen) < IPV6_MAX_BYTELEN ? AFI_IP : AFI_IP6))

pguibert6WIND

one other question, it seems 1634b1b should be adapted too.

eqvinox · 2017-01-17T15:47:19Z

I'm tempted to NAK 57693ce as falling under "new feature". We can't keep piling stuff on stable/2.0...

louberger · 2017-01-17T16:02:12Z

Much of this is bug fixes. The config/vty addition is of course not so is a valid comment, but it is new cli support for existing vnc function....

louberger · 2017-01-17T20:02:08Z

Non VRF changes are now part of #83
-- will close this one VRF pull request is submitted

louberger · 2017-01-22T14:59:03Z

VRF part covered in #97

Some changes to try to resolve/understand CI failures on master

louberger added 7 commits January 15, 2017 13:05

bgpd: partial revert of vpn/encap safi show changes (Issue FRRouting#14)

5d11a6c

To remove commands duplicated in bgp_mplsvpn/_encap Also until vpn/encap specific show pieces are merged into generic afi/safi show. Signed-off-by: Lou Berger <lberger@labn.net>

bgpd: fix RD stomping by update group code (Issue FRRouting#71)

be6eff5

Signed-off-by: Lou Berger <lberger@labn.net>

bgpd: allow VPN next hop to be different AFI than NLRI next hop (Issue …

74031c1

…FRRouting#71) Signed-off-by: Lou Berger <lberger@labn.net>

bgpd rfapi: use VN as nexthop for MPLS tunnels too

b64a046

Also minor show format cleanup Signed-off-by: Lou Berger <lberger@labn.net>

bgpd: add vrf-policy config using existing vnc code

57693ce

add add/clear vrf prefix Signed-off-by: Lou Berger <lberger@labn.net>

bgpd rfapi: add NVE/VRF name to show vnc registrations

596669b

bgpd rfapi: fix issue where advertised prefixes were not being disamb…

1a5ba79

…iguated by RD Signed-off-by: Lou Berger <lberger@labn.net>

bgpd rfapi: change noop to loop - speculative fix to regression failure

ff9b38b

Signed-off-by: Lou Berger <lberger@labn.net>

louberger mentioned this pull request Jan 16, 2017

BGP VRF Route Distinguisher configuration + VRF Import Processing + VRF Multipath #44

Closed

donaldsharp reviewed Jan 16, 2017

View reviewed changes

louberger added 2 commits January 16, 2017 20:45

bgpd rfapi: improve do once loop

dce28cb

Signed-off-by: Lou Berger <lberger@labn.net>

bgpd: whitespace change for consistency

9b37abe

Signed-off-by: Lou Berger <lberger@labn.net>

pguibert6WIND approved these changes Jan 17, 2017

View reviewed changes

pguibert6WIND reviewed Jan 17, 2017

View reviewed changes

louberger mentioned this pull request Jan 17, 2017

Fix a number of VPN/Encap SAFI related issues (no VRF CLI changes) #83

Merged

louberger closed this Jan 22, 2017

louberger deleted the working/2.0/patch-set/1 branch February 2, 2017 03:04

cfra referenced this pull request in opensourcerouting/frr Nov 29, 2018

Merge pull request #75 from LabNConsulting/working/netns_failures

f6641c2

Some changes to try to resolve/understand CI failures on master

Fix a number of VPN SAFI related issues, add vrf flavor of vnc commands #75

Fix a number of VPN SAFI related issues, add vrf flavor of vnc commands #75

Conversation

louberger commented Jan 16, 2017

NetDEF-CI commented Jan 16, 2017

Continous Integration Result: FAILED

Get source and apply patch from patchwork: Successful

Building Stage: Failed

OmniOS amd64 build: Failed

NetDEF-CI commented Jan 16, 2017

Continous Integration Result: SUCCESSFUL

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donaldsharp commented Jan 16, 2017

louberger commented Jan 16, 2017

NetDEF-CI commented Jan 17, 2017

Continous Integration Result: SUCCESSFUL

pguibert6WIND left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pguibert6WIND left a comment

Choose a reason for hiding this comment

eqvinox commented Jan 17, 2017

louberger commented Jan 17, 2017

louberger commented Jan 17, 2017

louberger commented Jan 22, 2017