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

Pimd daemon #92

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Pimd daemon #92

wants to merge 5 commits into from

Conversation

stoza
Copy link
Contributor

@stoza stoza commented Sep 18, 2020

the possibilities to specify a filename where the daemon should output stderr was added.
This can be done by giving a path to the variable log_file when adding the daemon.

This is useful because in the case of the pimd daemon, there is no dry_run since this daemon does not offer the possibility to check his config file. Like that user can easily find the stderr output in a file and check if all goes well or not.

Copy link
Member

@oliviertilmans oliviertilmans left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.

This one is a bit tedious, since it is introducing another PIM daemon beside the FRR one.
@jadinm will certainly more thoughts on this.

@@ -1,23 +0,0 @@
hostname ${node.name}
Copy link
Member

Choose a reason for hiding this comment

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

Please revert this.

This file configures the FRR pimd, and as such cannot be erased by this commit.


@property
def dry_run(self):
return 'echo 2BeOrNot2Be > /dev/null'
Copy link
Member

Choose a reason for hiding this comment

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

A more general solution would be to make this return an empty string/None, and then update the global dry_run callsite to check for these empty/None string (i;e., "if d.dry_run: ... else: log.warning('Cannot check the config validity of daemon %s', d.Name'))

This problem is bound to happen on more than one daemon...

@@ -0,0 +1,35 @@
from .base import RouterDaemon

class Pimd(RouterDaemon):
Copy link
Member

Choose a reason for hiding this comment

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

Please rename this something less ambiguous, given that 'PIMD' already exists...

""" This class configure the pim Daemon which can be found here:
https://github.com/troglobit/pimd
"""
NAME = 'pimd'
Copy link
Member

Choose a reason for hiding this comment

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

Same than above, define a more specific name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue here is that if I put a more specific name, for example 'pimd_troglobit' I got the following error:
RuntimeError: [pimd_troglobit] is not available in $PATH

@@ -126,7 +127,11 @@ def start(self):
self._old_sysctl[opt] = self._set_sysctl(opt, val)
# Fire up all daemons
for d in self.nconfig.daemons:
self._processes.popen(shlex.split(d.startup_line))
if (hasattr(d,'logfile')):
Copy link
Member

Choose a reason for hiding this comment

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

This feature should always be there, and not only for the daemons have a logfile attribute.

I'd suggest to always log stderr/stdout of all daemons, e.g.,

  • Create a temp logdir using daemon_logs tempfile.TemporaryDirectory(dir='tmp')
  • display it on the global logger
  • capture stdout/stderr for all daemon, e.g. stderr='%s/%s_%s_err.log' % (daemon_log, node, daemon_name)

@stoza
Copy link
Contributor Author

stoza commented Sep 25, 2020

yes it's a bit tedious but this is based on a suggestion on prof. Bonaventure.

@oliviertilmans
Copy link
Member

oliviertilmans commented Sep 30, 2020

Something that could work would be to detect at runtime which pimd is present on the system in the Main 'PIMD' daemon (e.g., through the 'pimd --version' output), and then override __new__ to use FRRPIMD (i.e, the current PIMD) or TroglobitPIMD (this PR).

Another possibility would be to have both daemon installed on the host (i.e., /opt/frr/... pimd, and /opt/pimd_troglobit/... pimd), and then prepend $PATH when starting them to select the right binary.

Similarly, you could update the install script to enable/disable FRR's PIMD (see https://github.com/FRRouting/frr/blob/master/configure.ac#L532) and optionally install this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants