Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gRPC northbound plugin #4082

Merged
merged 3 commits into from
May 7, 2019
Merged

Conversation

rwestphal
Copy link
Member

This PR adds a new gRPC-based northbound plugin to FRR.

This plugin provides an easy-to-use interface to configure and monitor FRR programmatically, using a variety of different programming languages (e.g. python, ruby, go, etc). It should be an alternative for users who want to automate FRR routers without going through the complexities of the NETCONF/RESTCONF protocols [1], while still benefiting from the advantages of using YANG-modeled data.

To use this plugin, each daemon must be started using the -M grpc:PORT parameter. The daemons are managed separately [2] and must listen on different TCP ports. As of now the gRPC plugin listens on the loopback address only, support for remote clients with SSL/TLS authentication will be added later.

Once the FRR daemons are started with the gRPC plugin, management clients can execute all RPCs defined in the FRR Northbound service definition file: https://github.com/opensourcerouting/frr/blob/grpc-nb-plugin/grpc/frr-northbound.proto

Here's an example client script written in Ruby: https://gist.github.com/rwestphal/6528ebeb8a612711ca3771b843a4eb14

Documentation for this and the other northbound plugins should be added later on a separate PR.

Needless to say, only daemons converted to the new northbound model will benefit from this new plugin.

Performance

The gRPC plugin delivers much better performance compared to what we get when using the ConfD/Sysrepo plugins, especially when fetching bulk operational data.

The table below shows the time required to fetch 1kk RIP routes using a variety of different methods:

Client Protocol/Command Encoding Time
ConfD NETCONF XML 1m28.6s
Sysrepo NETCONF XML 50.7s
gRPC - XML 12.8s
gRPC - JSON 12.2s
CLI show yang operational-data /frr-ripd:ripd format xml XML 11s
CLI show yang operational-data /frr-ripd:ripd format json JSON 10s
CLI show ip rip json [3] JSON 4s
CLI show ip rip Unstructured text 1.1s

As it can be seen, while the gRPC plugin performs better compared to the other northbound clients, it's still slower compared to using the CLI "show ... json" commands to obtain JSON-encoded data. There's a lot of room for optimization though. The callback given to nb_oper_data_iterate() can be refactored to use lyd_new() and lyd_new_leaf() instead of the expensive lyd_new_path() function (which does full XPath parsing). And the operational-data northbound callbacks can be refactored as well to reduce the amount of memory allocations. Once these optimizations are in place, we should be able get much better performance using the gRPC plugin to fetch bulk operational data.

Caveats

  • YANG notifications and confirmed commits are not supported at this point.
  • A separate pthread is created for gRPC since it needs to run its own even loop. This has some implications however. While this PR introduces a lock to protect the running_config global variable, calling the northbound configuration callbacks from a pthread other than the main one is unsafe as lots of other variables aren't protected by any lock. What needs to be done to address this issue is to create some kind of thread communication mechanism to process the Commit() RPC in the main pthread and return the results to the gRPC pthread. This plugin should be considered experimental until that work is done.

Implementation

This plugin was written in C++ since gRPC doesn't have official support for C.


[1] NETCONF is supported by the ConfD and Sysrepo plugins, and RESTCONF is supported by the ConfD plugin only.
[2] The long term goal is to move all northbound plugins to watchfrr in order to centralize and simplify the way we configure FRR, but this should take a while to happen
[3] This command doesn't exist in vanilla FRR and was added for benchmarking purposes only.

@NetDEF-CI
Copy link
Collaborator

Continuous 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-7134/

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

Warnings Generated during build:

Checkout code: Successful with additional warnings
Report for lib_errors.c | 4 issues
===============================================
< WARNING: line over 80 characters
< #339: FILE: /tmp/f1-10035/lib_errors.c:339:
< WARNING: line over 80 characters
< #343: FILE: /tmp/f1-10035/lib_errors.c:343:
<TITLE>clang_check</TITLE>

clang_check

@LabN-CI
Copy link
Collaborator

LabN-CI commented Apr 4, 2019

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

Results table
_ _
Result SUCCESS git merge/4082 5763720
Date 04/04/2019
Start 13:36:29
Finish 14:00:19
Run-Time 23:50
Total 1816
Pass 1816
Fail 0
Valgrind-Errors 0
Valgrind-Loss 0
Details vncregress-2019-04-04-13:36:29.txt
Log autoscript-2019-04-04-13:37:14.log.bz2
Memory 496 506 429

For details, please contact louberger

@pguibert6WIND pguibert6WIND self-assigned this Apr 5, 2019
@donaldsharp donaldsharp requested review from riw777 and srimohans April 9, 2019 15:36
Copy link
Member

@pguibert6WIND pguibert6WIND left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just first commit done.
more wil be done later.

@@ -413,7 +413,8 @@ enum nb_error {

/* Northbound clients. */
enum nb_client {
NB_CLIENT_CLI = 0,
NB_CLIENT_NONE = 0,
NB_CLIENT_CLI,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why having a NONE_CLIENT ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not strictly necessary, but it's useful when you want to indicate the absence of a value without using zero directly (which would actually translate to NB_CLIENT_CLI). Currently NB_CLIENT_NONE is being used by the nb_running_unlock() function to indicate that no client owns the configuration lock.

Copy link
Member

@riw777 riw777 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These look good to me. The locks could be a little tricky--they look correct to me, but it might be good if someone else takes a look to make certain I didn't miss something. Otherwise, I'll push this in a couple of days.

AC_LANG([C])
AC_PROG_CC
AC_PROG_CPP
AC_PROG_CXX
AM_PROG_CC_C_O
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will it be possible to compile frr with C-only ?
I mean, I expect no implicit dependency with C compiler

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. The autoconf documentation says that AC_PROG_CXX will set CXX to g++ if it fails to find a C++ compiler, without returning an error. This means a C++ compiler won't be required to build FRR (except if you want to build the gRPC module).

@mjstapp
Copy link
Contributor

mjstapp commented Apr 15, 2019

would the async mode of using grpc be a better fit for us - or isn't that supported by the c++ version?

@rwestphal
Copy link
Member Author

would the async mode of using grpc be a better fit for us - or isn't that supported by the c++ version?

I don't see which benefit we'd gain from using the async model. gRPC runs its own event loop on a separate pthread, so there's no chance it can block the main pthread in any way. Also, the sync model can handle multiple parallel requests normally (upon initialization, gRPC spawns a pthread pool for itself). I read somewhere that the advantage of the async model is higher level of control over threading, at the cost of much more complex code. This probably only makes sense for applications that need to handle thousands of RPCs per second, which shouldn't be our case.

I suspect however that we might need the async model in the future to implement subscriptions to YANG notifications. The subscriptions will be long-lived stream RPCs that will block indefinitely, and I'm not sure if we can do this using the sync model. Something to keep in mind for later.

@pguibert6WIND
Copy link
Member

I got an issue when trying to compile.
is there something to do, so that ./configure will trigger the error, whereas below, ./configure did not tell me anything.

CXX 
c++0x_warning.h:32:2 : error: #error This file requires compiler and library support for the ISO C++ 2011 standard. This support must be enabled with the -std=c++1 or -std=gnu++11 compiler options.
#error This file requires compiler and library support

@rwestphal
Copy link
Member Author

Works on my machine 😂

Seriously speaking, the gRPC module shouldn't require C++11 as you can see below:

# rm lib/lib_grpc_la-northbound_grpc.lo
# make V=1
true
make  all-am
make[1]: Entering directory '/mnt/renato/git/frr'
/bin/bash ./libtool  --tag=CXX   --mode=compile g++ -DHAVE_CONFIG_H -DSYSCONFDIR=\"/etc/frr/\" -DCONFDATE=20190416 -I.  -I. -I./include -I./lib -I. -I./include -I./lib   -pthread -I/usr/local/include  -MT lib/lib_grpc_la-northbound_grpc.lo -MD -MP -MF lib/.deps/lib_grpc_la-northbound_grpc.Tpo -c -o lib/lib_grpc_la-northbound_grpc.lo `test -f 'lib/northbound_grpc.cpp' || echo './'`lib/northbound_grpc.cpp
libtool: compile:  g++ -DHAVE_CONFIG_H -DSYSCONFDIR=\"/etc/frr/\" -DCONFDATE=20190416 -I. -I. -I./include -I./lib -I. -I./include -I./lib -pthread -I/usr/local/include -MT lib/lib_grpc_la-northbound_grpc.lo -MD -MP -MF lib/.deps/lib_grpc_la-northbound_grpc.Tpo -c lib/northbound_grpc.cpp  -fPIC -DPIC -o lib/.libs/lib_grpc_la-northbound_grpc.o
libtool: compile:  g++ -DHAVE_CONFIG_H -DSYSCONFDIR=\"/etc/frr/\" -DCONFDATE=20190416 -I. -I. -I./include -I./lib -I. -I./include -I./lib -pthread -I/usr/local/include -MT lib/lib_grpc_la-northbound_grpc.lo -MD -MP -MF lib/.deps/lib_grpc_la-northbound_grpc.Tpo -c lib/northbound_grpc.cpp -o lib/lib_grpc_la-northbound_grpc.o >/dev/null 2>&1
mv -f lib/.deps/lib_grpc_la-northbound_grpc.Tpo lib/.deps/lib_grpc_la-northbound_grpc.Plo
/bin/bash ./libtool  --tag=CXX   --mode=link g++  -pthread -I/usr/local/include  -avoid-version -module -shared -export-dynamic  -o lib/grpc.la -rpath /usr/local/lib/frr/modules lib/lib_grpc_la-northbound_grpc.lo lib/libfrr.la grpc/libfrrgrpc_pb.la -L/usr/local/lib -lgrpc -lgrpc++ -lprotobuf -pthread -ldl -lm -lcrypt   -ljson-c -lrt
libtool: link: rm -fr  lib/.libs/grpc.la lib/.libs/grpc.lai lib/.libs/grpc.so
libtool: link: g++  -fPIC -DPIC -shared -nostdlib /usr/lib/gcc/x86_64-linux-gnu/6/../../../x86_64-linux-gnu/crti.o /usr/lib/gcc/x86_64-linux-gnu/6/crtbeginS.o  lib/.libs/lib_grpc_la-northbound_grpc.o   -Wl,-rpath -Wl,/mnt/renato/git/frr/lib/.libs -Wl,-rpath -Wl,/mnt/renato/git/frr/grpc/.libs lib/.libs/libfrr.so grpc/.libs/libfrrgrpc_pb.so -L/usr/local/lib -lgrpc -lgrpc++ /usr/local/lib/libprotobuf.a -ldl -lcrypt -ljson-c -lrt -L/usr/lib/gcc/x86_64-linux-gnu/6 -L/usr/lib/gcc/x86_64-linux-gnu/6/../../../x86_64-linux-gnu -L/usr/lib/gcc/x86_64-linux-gnu/6/../../../../lib -L/lib/x86_64-linux-gnu -L/lib/../lib -L/usr/lib/x86_64-linux-gnu -L/usr/lib/../lib -L/usr/lib/gcc/x86_64-linux-gnu/6/../../.. -lstdc++ -lm -lc -lgcc_s /usr/lib/gcc/x86_64-linux-gnu/6/crtendS.o /usr/lib/gcc/x86_64-linux-gnu/6/../../../x86_64-linux-gnu/crtn.o  -pthread -pthread   -pthread -Wl,-soname -Wl,grpc.so -o lib/.libs/grpc.so
libtool: link: ( cd "lib/.libs" && rm -f "grpc.la" && ln -s "../grpc.la" "grpc.la" )
make[1]: Leaving directory '/mnt/renato/git/frr'

Notice that neither -std=c++1 nor -std=gnu++11 are being used and the code is compiling just fine.

Please provide more details about this problem like a full build log so that I can try to see what's going wrong.

@pguibert6WIND
Copy link
Member

ok; I managed to compile, thanks to github.com/grpc/issues/16739.
also the C++11 was due to the environment I Was using.
I finally could run rip daemon with the grpc port opened : ripd -M grpc

I could not go further, since I lack some information on ruby ( for experimenting the testing case).
adding to that , the performance issue case, I would have liked to use it with staticd , but for this I will wait that yang be applied to staticd too.

@rwestphal
Copy link
Member Author

rwestphal commented Apr 19, 2019

I could not go further, since I lack some information on ruby ( for experimenting the testing case).

First you need to install gRPC for ruby:

# gem install grpc
# gem install grpc-tools

Then you need to generate ruby libraries from the FRR service definitions file:

$ grpc_tools_ruby_protoc --ruby_out=./ruby --grpc_out=./ruby frr-northbound.proto

Now the example ruby script should be able to include the files below and you should be good to go:

require "grpc"
require "frr-northbound_services_pb"

A similar process should be required if you want to configure/monitor FRR using other languages, like Python and Go.

EDIT:
For installing the gRPC core, I've written down these commands a while ago:

$ git clone -b $(curl -L https://grpc.io/release) https://github.com/grpc/grpc
$ cd grpc
$ git submodule update --init
$ make
$ sudo make install
$ cd third_party/protobuf
$ sudo make install

It's also possible to install gRPC from your package manager, but this isn't mentioned by the gRPC documentation so it's probably not a good idea.

@Jafaral
Copy link
Member

Jafaral commented Apr 22, 2019

@rwestphal Looks like this PR got out of date!

@qlyoung qlyoung self-requested a review April 25, 2019 21:08
…onfiguration

The ability to lock the running configuration to prevent other users
from changing it is a very important one. We already supported
the "configure exclusive" command but the lock was applied to
the CLI users only (other clients like ConfD could still commit
configuration transactions, ignoring the CLI lock). This commit
introduces a global lock for the running configuration that is
shared by all northbound clients, and provides a public API to
manipulate it. This way other northbound clients will also be able
to lock/unlock the running configuration if required (the upcoming
gRPC northbound plugin will have RPCs for that).

NOTE: this is a management-level lock for the running configuration,
not to be confused with low-level locks used to avoid data races.

Signed-off-by: Renato Westphal <renato@opensourcerouting.org>
The upcoming gRPC-based northbound plugin will run on a separate
pthread, and it will need to have access to the running configuration
global variable.  Introduce a rw-lock to control concurrent access
to the running configuration. Add the lock inside the "nb_config"
structure so that it can be used to protect candidate configurations
as well (this might be necessary depending on the threading scheme
of future northbound plugins).

Signed-off-by: Renato Westphal <renato@opensourcerouting.org>
This is an experimental plugin for now. Full documentation will
come later.

Signed-off-by: Renato Westphal <renato@opensourcerouting.org>
@LabN-CI
Copy link
Collaborator

LabN-CI commented Apr 26, 2019

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

Results table
_ _
Result SUCCESS git merge/4082 ec2ac5f
Date 04/26/2019
Start 17:25:37
Finish 17:49:16
Run-Time 23:39
Total 1813
Pass 1813
Fail 0
Valgrind-Errors 0
Valgrind-Loss 0
Details vncregress-2019-04-26-17:25:37.txt
Log autoscript-2019-04-26-17:26:22.log.bz2
Memory 446 431 374

For details, please contact louberger

@NetDEF-CI
Copy link
Collaborator

Continuous 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-7384/

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

Warnings Generated during build:

Checkout code: Successful with additional warnings
Report for lib_errors.c | 4 issues
===============================================
< WARNING: line over 80 characters
< #339: FILE: /tmp/f1-1864/lib_errors.c:339:
< WARNING: line over 80 characters
< #343: FILE: /tmp/f1-1864/lib_errors.c:343:

CLANG Static Analyzer Summary

  • Github Pull Request 4082, comparing to Git base SHA 86336f6

No Changes in Static Analysis warnings compared to base

12 Static Analyzer issues remaining.

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

@qlyoung qlyoung merged commit 6915af7 into FRRouting:master May 7, 2019
@rzalamena rzalamena deleted the grpc-nb-plugin branch August 7, 2020 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants