Exa-Networks / exabgp

The BGP swiss army knife of networking
Other
2.07k stars 443 forks source link

Trying to run exabgp on non-mainstream platform is ... frustrating #1121

Open he32 opened 1 year ago

he32 commented 1 year ago

I'm running the old version of exabgp (3.*) on NetBSD/amd64 for a distributed DNS resolver service. This has been stable and has been working flawlessly.

Trying to upgrade to exabgp 4 has so far turned out to be a frustrating experience -- and more so than it ought to be IMHO. I will do my best to come up with constructive input on each of the issues, so this is just a summary of the observed issues so far:

I know the above descriptions are mostly not "actionable", I will try to dig into each of them separately with an own more detailed description and a separate issue for each of them. This is therefore just to summarize the observations so far, as a start.

he32 commented 1 year ago

I have apparently managed to not reproduce the "does nothing" failure mode I observed earlier -- I have exabgp4 up and running on a separate test machine, including "exabgp-cli". And I think I have a handle on the "missing syslog" issue, will open a separate issue about that.

thomas-mangin commented 1 year ago

Thank you for this return @he32 - I will do what I can to help. Would this patch be what you would like to see changed and work for you?

diff --git a/src/exabgp/logger/handler.py b/src/exabgp/logger/handler.py
index 1013874e..e45dd38c 100644
--- a/src/exabgp/logger/handler.py
+++ b/src/exabgp/logger/handler.py
@@ -1,5 +1,6 @@
 # A wrapper class around logging to make it easier to use

+import os
 import sys
 import logging
 import logging.handlers as handlers
@@ -43,9 +44,14 @@ def getLogger(name=None, **kwargs):

 def _syslog(**kwargs):
+    # python for loop variable scope leaks
+    for syslog_file in ['/var/run/log', 'dev/log']:
+        if os.path.exists(syslog_file):
+            break
+
     formating = kwargs.get('format', SHORT)
     handler = handlers.SysLogHandler(
-        address=kwargs.get('address', '/dev/log'),
+        address=kwargs.get('address', syslog_file),
         facility=kwargs.get('facility', 'syslog'),
     )
     handler.setFormatter(logging.Formatter(formating))
he32 commented 1 year ago

I suspect that would be a problem for Darwin, at least my MacOS (up-to-date arm64) only has /var/run/syslog and none of the other two. NetBSD appears to cope fine with /dev/log, I don't have a FreeBSD system to check. However, I think I would change 'syslog' in the next line to 'daemon'.

he32 commented 1 year ago

Or, better yet, provide a way for the administrator to override the default facility used for syslog. But as stated above, I would also like to argue that the default should be daemon and neither user nor syslog, ref. this excerpt from my <syslog.h>:

#define LOG_USER        (1<<3)  /* random user-level messages */
#define LOG_DAEMON      (3<<3)  /* system daemons */
#define LOG_SYSLOG      (5<<3)  /* messages generated internally by syslogd */
thomas-mangin commented 1 year ago

The way the exabgp env variables are set up, it would be a large configuration change to do this but it may be worth it. I will look into it as it would allow moving syslog out of file logging section and have both which may be desirable in some cases.

thomas-mangin commented 1 year ago

before I get to that point, is this uncontrovesial?

diff --git a/src/exabgp/logger/handler.py b/src/exabgp/logger/handler.py
index 1013874e..0d9a2147 100644
--- a/src/exabgp/logger/handler.py
+++ b/src/exabgp/logger/handler.py
@@ -43,10 +43,16 @@ def getLogger(name=None, **kwargs):

 def _syslog(**kwargs):
+    syslog_file = '/dev/log'
+    if sys.platform == 'netbsd':
+        syslog_file = '/var/run/log'
+    if sys.platform == 'darwin':
+        syslog_file = '/var/run/syslog'
+
     formating = kwargs.get('format', SHORT)
     handler = handlers.SysLogHandler(
-        address=kwargs.get('address', '/dev/log'),
-        facility=kwargs.get('facility', 'syslog'),
+        address=kwargs.get('address', syslog_file),
+        facility=kwargs.get('facility', 'daemon'),
     )
     handler.setFormatter(logging.Formatter(formating))
     return handler
he32 commented 1 year ago

That looks OK to my eyes, at least.

he32 commented 1 year ago

Although if you want to reduce the noise, you could drop the test for netbsd, since it's fine with using /dev/log.

he32 commented 1 year ago

The way the exabgp env variables are set up, it would be a large configuration change to do this but it may be worth it.

As long as the default is made "sane" and is documented, I would perhaps not put too much effort into the ability to override the facility, at least if this was the only thing which should be fixed in this area.

I'm working on an update to the exabgp.1 man page, at least bringing it up to 4.2.21 level, as that's what I'm testing with at the moment. A few new "env" variables has been introduced since that man page was last updated, and exabgp.api.highres seems to be gone, at least according to "exabgp --fi".

I'll take a stab at exabgp.conf.5 a little later, and will submit these as one or two pull requests when I'm sort-of satisfied.

I will look into it as it would allow moving syslog out of file logging section and have both which may be desirable in some cases.

OK.

he32 commented 1 year ago

exabgp.1 man page update waiting in https://github.com/Exa-Networks/exabgp/pull/1123

he32 commented 1 year ago

exabgp.conf.5 man page update waiting in https://github.com/Exa-Networks/exabgp/pull/1124

thomas-mangin commented 1 year ago

Thank you, I have merged a change to help syslog. I will review when I have some time.