Add Pseudowire management in Zebra #601

bingen · 2017-05-22T11:48:50Z

When Zebra receives a PW from ldpd, it tries to install it in kernel and
in case of failure notifies back to ldpd daemon and enqueues the PW for
retry.

The installation to kernel is not yet implemented as Linux kernel
doesn't have support for it yet. It will be done through a module hook,
so it can be outsourced to a remote dataplane.

Signed-off-by: ßingen bingen@voltanet.io

When Zebra receives a PW from ldpd, it tries to install it in kernel and in case of failure notifies back to ldpd daemon and enqueues the PW for retry. The installation to kernel is not yet implemented as Linux kernel doesn't have support for it yet. It will be done through a module hook, so it can be outsourced to a remote dataplane. Signed-off-by: ßingen <bingen@voltanet.io>

NetDEF-CI · 2017-05-22T12:31:59Z

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

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.

CLANG Static Analyzer Summary

Github Pull Request 601, comparing to Git base SHA 74572f0

No Changes in Static Analysis warnings compared to base

132 Static Analyzer issues remaining.

See details at
https://ci1.netdef.org/browse/FRR-FRRPULLREQ-786/artifact/shared/static_analysis/index.html

donaldsharp · 2017-05-23T15:18:50Z

This looks isolated and after a review from @rwestphal it can be integrated

NetDEF-CI · 2017-05-30T09:12:25Z

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

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.

CLANG Static Analyzer Summary

Github Pull Request 601, comparing to Git base SHA 74572f0

No Changes in Static Analysis warnings compared to base

132 Static Analyzer issues remaining.

See details at
https://ci1.netdef.org/browse/FRR-FRRPULLREQ-821/artifact/shared/static_analysis/index.html

rwestphal · 2017-05-30T12:48:33Z

@bingen please push this against master and not stable/3.0, as it is a new feature. More feedback soon.

rwestphal

Good work, this is the missing piece of VPLS support in FRR.

Please address my nits and re-open this against master. In the meantime I'll cook a patch on top of this to install the LDP signaled pseudowires in the kernel when running on OpenBSD, mostly as a proof of concept.

rwestphal · 2017-05-30T13:47:54Z

ldpd/lde.h

@@ -188,6 +190,7 @@ void		 fec_snap(struct lde_nbr *);
 void		 fec_tree_clear(void);
 struct fec_nh	*fec_nh_find(struct fec_node *, int, union ldpd_addr *,
 		    ifindex_t, uint8_t);
+void 		 update_local_label (struct fec_node *fn, int connected);


unused prototype

rwestphal · 2017-05-30T13:49:36Z

ldpd/ldp_zebra.c

 int
 kmpw_set(struct kpw *kpw)
 {
+	zebra_send_kpw (ZEBRA_KPW_ADD, zclient, kpw);


Please remove the TODO comment below.

Better yet, rewrite this function like this:
return (zebra_send_kpw (ZEBRA_PW_ADD, zclient, kpw));

rwestphal · 2017-05-30T13:49:48Z

ldpd/ldp_zebra.c

 	return (0);
 }

 int
 kmpw_unset(struct kpw *kpw)
 {
+	zebra_send_kpw (ZEBRA_KPW_DELETE, zclient, kpw);


rwestphal · 2017-05-30T13:50:37Z

ldpd/lde.h

@@ -25,6 +25,8 @@
 #include "openbsd-tree.h"
 #include "if.h"

+#define RETRY_SYNC_PW_INTERVAL 5 /* in seconds */


This is not used anywhere

rwestphal · 2017-05-30T13:54:42Z

lib/zclient.h

@@ -94,6 +94,9 @@ typedef enum {
  ZEBRA_LABEL_MANAGER_CONNECT,
  ZEBRA_GET_LABEL_CHUNK,
  ZEBRA_RELEASE_LABEL_CHUNK,
+  ZEBRA_KPW_ADD,


Forgive the bikeshedding, but I'd prefer to remove the "K" from these constants.

ZEBRA_PW_ADD and ZEBRA_PW_DELETE look better.

rwestphal · 2017-05-30T14:20:26Z

ldpd/ldpd.h

 	int			 af;
 	union ldpd_addr		 nexthop;
 	uint32_t		 local_label;
 	uint32_t		 remote_label;
 	uint8_t			 flags;
+	uint32_t                 pwid;
+	char			 vpn_name[L2VPN_NAME_LEN];
+	unsigned short           ac_port_ifindex;
 };


What is the ac_port_ifindex for? It's not being used anywhere.

Also, the other members you're adding here are not necessary in my opinion.

We don't want to send protocol-specific pseudowire attributes to zebra, like pwid, lsr_id and vpn_name. Currently only ldpd supports L2VPNs, but in the future we should implement support for BGP-signaled L2VPNs as well.

I think that pseudowires can be uniquely identified by their ifname and ifindex, zebra doesn't need to track more information than that.

rwestphal · 2017-05-30T14:22:03Z

ldpd/ldp_zebra.c

+{
+	struct stream		*s;
+
+	debug_zebra_out("ILM %s PW %u (%s) ifindex %hu, type %d. %s -> nexthop %s label %s",


rwestphal · 2017-05-30T14:26:06Z

ldpd/ldp_zebra.c

+		zebra_size_t length, vrf_id_t vrf_id)
+{
+	struct stream		*s;
+	uint8_t status;


padding not consistent with the rest of the code

bingen · 2017-05-31T09:12:57Z

@rwestphal thanks for your feedback, I will address it soon but a first comment about where to submit the PR to. This is supposed to be related to commit 595b4be which is in stable/3.0, so I think it should be there. As we talked by e-mail some time ago, with the code in stable/3.0 as it is now, it will show the PW status as UP by default, even if it's not true.

rwestphal · 2017-05-31T20:09:03Z

@bingen In my opinion this PR is way too big for stable/3.0.

If you really need the pseudowire manager on stable/3.0 you can always create an internal fork to accommodate your custom requirements.

The commit you pointed out didn't break anything because VPLS is not supposed to work without a pseudowire manager in zebra. On stable/3.0, all pseudowires signaled by ldpd are going to /dev/null. We could revert 595b4be, but would that make any difference?

If the label is implicit null it's not installed in Zebra, so this check doesn't make sense. Signed-off-by: ßingen <bingen@voltanet.io>

NetDEF-CI · 2017-06-01T09:26:56Z

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

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.

CLANG Static Analyzer Summary

Github Pull Request 601, comparing to Git base SHA 74572f0

No Changes in Static Analysis warnings compared to base

132 Static Analyzer issues remaining.

See details at
https://ci1.netdef.org/browse/FRR-FRRPULLREQ-839/artifact/shared/static_analysis/index.html

rwestphal · 2017-06-01T14:01:27Z

zebra/zebra_pw.c

-        zlog_warn ("No label found in rib for PW %u at VPN %s", pw->pwid,
-                   pw->vpn_name);
-        return 1;
-      }


If the label is implicit null it's not installed in Zebra, so this check doesn't make sense.

Not really, you can't remove this.

The P from VPLS stands for Private. Removing these lines introduces a security issue as you break the isolation between the VPLS customers.

Receiving an implicit-null label for the pseudowire's remote endpoint is different than receiving no label at all.

If you receive an implicit-null label, it means that the remote endpoint is directly connected to you. In this case, due to the default PHP behavior, you can send the pseudowire frames with the VPN label only. You don't need to push an additional label to tunnel the packets to the remote endpoint.

Now let's say that you didn't receive any label for the pseudowire's remote endpoint. In this case, you don't know if the remote endpoint is directly connected to you or not. If you send L2VPN frames pushing just one label (the VPN label) and the nexthop is not the remote endpoint, then it will not understand what to do with the VPN label. Even worse, it might have allocated the very same label for another VPLS client and forward the packet to it. Now you have a security issue. This is possible because labels are locally allocated in standard MPLS networks (when segment routing is not in use).

With that said, the real problem here is that we're not adding LDP's implicit-null labels to routes. Removing the out_label != MPLS_IMP_NULL_LABEL checks below (zserv.c) should solve the problem.

if (command == ZEBRA_MPLS_LABELS_ADD) { mpls_lsp_install (zvrf, type, in_label, out_label, gtype, &gate, ifindex); if (out_label != MPLS_IMP_NULL_LABEL) mpls_ftn_update (1, zvrf, type, &prefix, gtype, &gate, ifindex, distance, out_label); } else if (command == ZEBRA_MPLS_LABELS_DELETE) { mpls_lsp_uninstall (zvrf, type, in_label, gtype, &gate, ifindex); if (out_label != MPLS_IMP_NULL_LABEL) mpls_ftn_update (0, zvrf, type, &prefix, gtype, &gate, ifindex, distance, out_label); }

Ya, you are right, good catch. I saw the other code (where it discards Implicit Null labels) but I didn't touch because I thought that it would be there for a reason and I was afraid of breaking something else.
I'll try removing it and restoring the check. If it works I'll add it to my final PR when I address all of your comments. Are you sure it's safe to install those implicit null labels? Why was that code put there then?
Thanks.

It should be safe.

Back when I wrote this code I thought we could avoid making unnecessary changes in the RIB because implicit-null labels are never sent on the wire. But it turns out that we need to keep track of implicit-null labels in the RIB because of this issue you brought up. rt_netlink.c and zebra_mpls_openbsd.c have additional checks that will ensure that the implicit-null labels are never installed in the kernel.

bingen · 2017-06-01T17:48:32Z

@rwestphal About the branching I completely disagree. This PR is addressing something that was removed in a previous commit, as you said here "we assume that zebra will do that for us". So without this PR stable/3.0 is with no doubt broken. You say nobody uses it. Well, you already know that we do, but if that's true, why would it matter then to accept this PR here? I see very inconsistent to accept in stable branch the removal of a check but not its substitute. It makes no sense to me.
About creating an internal fork, ya, of course we can do that, but then we'll have to be manually merging all the bugfixes that come up to stable 3.0, so not very helpful.
Finally, looking at here, I wouldn't say that this is new to master precisely because of what I said (I'm just restoring something previously removed in stable/3.0), so I don't understand how you are applying these criteria here.
But anyway, these are my final thoughts. As you have to accept the PR, if you still disagree with me I'll submit my new PR against master.

rwestphal · 2017-06-01T18:01:18Z

My point is that with or without that commit VPLS doesn't work on stable/3.0. That commit just prepares the ground for the pseudowire manager, reverting it for example wouldn't change anything.

For me it's questionable if your PR is adding a new feature or fixing a bug, it's kinda doing both. Generally speaking I'd prefer to avoid big changes on a stable branch. But since the code is well contained I'll speak with the other maintainers and see if we can add an exception here.

donaldsharp · 2017-06-06T15:11:43Z

some more issues that need to be worked on a bit more.

rwestphal · 2017-06-07T18:10:21Z

As we talked in Slack before, WQ_RETRY_LATER doesn't work the way one would expect. After a failure, a new attempt to install the pseudowire is made right away, without waiting for the workqueue's holdtime (zebra->pwq->spec.hold). Since zebra->pwq->spec.max_retries is set to 3 by default, if something goes wrong then 4 attempts to install the pseudowire are made in a row and then we just give up.

In my VPLS test setup things are not going quite right because of this problem.

One other problem is that nexthop resolution is being performed only when the pseudowire is installed. If the route and/or label used to reach the pseudowire's remote endpoint changes, we need to reinstall the pseudowire in the kernel/hardware.

With that said, we have two possibilities here:
1 - Use a poll-based approach: every N seconds loop through all available pseudowires and check the ones that need to be installed/updated/removed;
2 - Use an event-based approach: do the same thing every time the RIB is updated (e.g. in the end of meta_queue_process_complete()).

Approach #2 is more responsive but potentially more expensive. One other problem with #2 is that the installation of a pseudowire can fail for other reasons other than failed nexthop resolution (e.g. hardware running out of resources). In this case, we can wait too long to re-attempt to install a pseudowire if the RIB is not being changed. Maybe implementing a hybrid of #1 and #2 would be the way to go, so we're good in all cases. Does anyone have an opinion on this?

rwestphal · 2017-06-08T13:36:51Z

Update: I talked to @eqvinox yesterday and he suggested that using our NHT code might be appropriate to solve the pseudowire nexthop resolution problem. I'm investigating this idea right now.

rwestphal · 2017-06-08T23:11:31Z

Implemented here nexthop tracking in ldpd.

Now ldpd registers the nexthop of the pseudowires with zebra and keeps track if they are resolvable or not. When the nexthop resolution for a pseudowire changes, ldpd will send a message to zebra asking to reinstall or uninstall the pseudowire.

The only issue here is that ldpd needs labeled routes for its pseudowires remote endpoints (bgpd should have the same requirement for MPLS VPN routes). In other words, a resolved nexthop is only valid if it's labeled, otherwise the pseudowire can't be installed.

To address this issue I did some small changes in the ZEBRA_NEXTHOP_UPDATE message to include the label of the nexthops as well. So far so good, but there's a problem. When we add a label to a route, zebra doesn't notify its NHT clients that this route was updated.

Apparently the problem comes down to this line: https://github.com/FRRouting/frr/blob/ce7fce36b/zebra/zebra_rnh.c#L870

We do set ROUTE_ENTRY_NEXTHOPS_CHANGED when we add a label to a route, but this flag is always unset before we reach this compare_state() function.

More precisely, the NHT call trace is usually like this:

meta_queue_process_complete()
  zebra_evaluate_rnh()
    zebra_rnh_evaluate_entry()
      zebra_rnh_eval_nexthop_entry()
        compare_state()
          CHECK_FLAG(r1->status, ROUTE_ENTRY_NEXTHOPS_CHANGED)
        zebra_rnh_notify_protocol_clients()
          send_client()

meta_queue_process_complete() is the completion_func callback of the RIB workqueue. When it's called the ROUTE_ENTRY_NEXTHOPS_CHANGED flag has been cleared from all routes already (https://github.com/FRRouting/frr/blob/ce7fce36b/zebra/zebra_rib.c#L1561).

With that said, I propose to remove the aforementioned line (zebra_rib.c#L1561) in order to solve this problem. Removing this line will allow the ROUTE_ENTRY_NEXTHOPS_CHANGED flag to propagate until zebra_evaluate_rnh(), allowing it to detect changed nexthops. This flag is then unset in zebra_rnh_clear_nhc_flag() for all routes (https://github.com/FRRouting/frr/blob/ce7fce36b/zebra/zebra_rnh.c#L719).

Any thoughts? I'm not super sure if doing this is safe or not so I'd appreciate feedback from others.

donaldsharp · 2017-06-09T11:38:05Z

To close the loop from a private Conversation @rwestphal and I had last night. I do not believe modifying zebra_rnh.c is the right thing to do. Currently when labels change for a route entry we are not considering that a change in the nexthop so we are not calling the rnh code correctly. We should be modifying rib_process( and really nexthop_active_update/nexthop_active..) to be able to notice when labels are changed in the nexthop

bingen · 2017-06-13T16:38:59Z

Replaced by #710

bingen mentioned this pull request May 22, 2017

zebra: Distributed Dataplane module #522

Closed

eqvinox requested a review from rwestphal May 22, 2017 15:47

Sync PW status in ldpd after check_lsp

44d3e2a

rwestphal reviewed May 30, 2017

View reviewed changes

zebra: Remove NH label check for PW

1a9c8f1

If the label is implicit null it's not installed in Zebra, so this check doesn't make sense. Signed-off-by: ßingen <bingen@voltanet.io>

rwestphal reviewed Jun 1, 2017

View reviewed changes

donaldsharp added the iterating label Jun 13, 2017

bingen closed this Jun 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Pseudowire management in Zebra #601

Add Pseudowire management in Zebra #601

bingen commented May 22, 2017

NetDEF-CI commented May 22, 2017

donaldsharp commented May 23, 2017

NetDEF-CI commented May 30, 2017

rwestphal commented May 30, 2017

rwestphal left a comment

rwestphal May 30, 2017

rwestphal May 30, 2017

rwestphal May 30, 2017

rwestphal May 30, 2017

rwestphal May 30, 2017

rwestphal May 30, 2017

rwestphal May 30, 2017

rwestphal May 30, 2017

bingen commented May 31, 2017

rwestphal commented May 31, 2017

NetDEF-CI commented Jun 1, 2017

rwestphal Jun 1, 2017

bingen Jun 1, 2017

rwestphal Jun 1, 2017

bingen commented Jun 1, 2017

rwestphal commented Jun 1, 2017

donaldsharp commented Jun 6, 2017

rwestphal commented Jun 7, 2017

rwestphal commented Jun 8, 2017

rwestphal commented Jun 8, 2017

donaldsharp commented Jun 9, 2017

bingen commented Jun 13, 2017

Add Pseudowire management in Zebra #601

Add Pseudowire management in Zebra #601

Conversation

bingen commented May 22, 2017

NetDEF-CI commented May 22, 2017

Continous Integration Result: SUCCESSFUL

CLANG Static Analyzer Summary

No Changes in Static Analysis warnings compared to base

132 Static Analyzer issues remaining.

donaldsharp commented May 23, 2017

NetDEF-CI commented May 30, 2017

Continous Integration Result: SUCCESSFUL

CLANG Static Analyzer Summary

No Changes in Static Analysis warnings compared to base

132 Static Analyzer issues remaining.

rwestphal commented May 30, 2017

rwestphal 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

bingen commented May 31, 2017

rwestphal commented May 31, 2017

NetDEF-CI commented Jun 1, 2017

Continous Integration Result: SUCCESSFUL

CLANG Static Analyzer Summary

No Changes in Static Analysis warnings compared to base

132 Static Analyzer issues remaining.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bingen commented Jun 1, 2017

rwestphal commented Jun 1, 2017

donaldsharp commented Jun 6, 2017

rwestphal commented Jun 7, 2017

rwestphal commented Jun 8, 2017

rwestphal commented Jun 8, 2017

donaldsharp commented Jun 9, 2017

bingen commented Jun 13, 2017