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

New Monitor: remote queries via ssh #1398

Open
wants to merge 16 commits into
base: develop
Choose a base branch
from

Conversation

wsw70
Copy link
Contributor

@wsw70 wsw70 commented May 7, 2024

This is a preliminary PR for a new Monitor that uses SSH to connect to a remote machine, run a command, parse it results to extract a value which is then monitored.

Why preliminary?

  • it is fully working but since the paradigm is different than other monitors I am looking for comments and ideas for changes. A RFC of sorts.
  • it uses match which is a Python 3.10+ feature. I will make it retro-compatible with 3.7 as a new branch when this PR is tested/reviewed, awaiting for Simplemonitor to be, someday, bumped to 3.27 (EBV - Expected Bump Version)
  • I have it working for some time already with various systems and, well, it works. But YMMV. And I want to know typical mileages to account for them in the code.

Important note for @jamesoff : I had to change in envconfig.py ConfigParser to RawConfigParser otherwise the regex I read in was escaped (see the commit). I do not think it changes anything because as far as I could tell you do not rely on escaping when reading the configuration.

So try it out (git checkout monitor_remote_ssh) and let me know!

simplemonitor/Monitors/remote_ssh.py Fixed Show resolved Hide resolved
simplemonitor/util/envconfig.py Fixed Show resolved Hide resolved
@jamesoff
Copy link
Owner

jamesoff commented May 7, 2024

Thanks! I'm away until next week so won't be able to take a proper look at this until then - I'm not ignoring it :)

Copy link
Owner

@jamesoff jamesoff left a comment

Choose a reason for hiding this comment

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

Had a quick look (I'm travelling again!) and this looks pretty good. Added a couple of thoughts :)

I'm fine with the change to ConfigParser, don't think that will cause any issues.

simplemonitor/Monitors/remote_ssh.py Outdated Show resolved Hide resolved
simplemonitor/Monitors/remote_ssh.py Outdated Show resolved Hide resolved
simplemonitor/Monitors/remote_ssh.py Outdated Show resolved Hide resolved
# extract and cast the actual value
command_result = stdout.read().decode("utf-8") # let's hope for the best
client.close() # it's only now that we are done with the client
actual_value = re.match(self.regex, command_result).groups()[0]
Copy link
Owner

Choose a reason for hiding this comment

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

I was wondering if it would be better to require a named group in the regexp, so that can be picked out specifically rather than requiring the user to craft a regexp which makes the first capturing group the one they need?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can do that but what would be the value of several capturing groups? There is just one that will be used anyway.

if test_succeeded:
return self.record_success(self.success_message.format(actual_value))
else:
return self.record_fail(f"actual value: {actual_value} | operator: {self.operator} | target value: {self.target_value}")
Copy link
Owner

Choose a reason for hiding this comment

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

Any value in letting the user also customise the failure message in a similar fashion to the success one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will require some adjustments - I will have to test how doable it is to pass the format to the actual logging message (right now there is just one "placeholder" ({}) for actual_value in the configuration string)

@jamesoff
Copy link
Owner

I just bumped the version of parakimo on the develop branch btw (finally merged dependabot's PR)

@wsw70
Copy link
Contributor Author

wsw70 commented May 21, 2024

I'm fine with the change to ConfigParser, don't think that will cause any issues.

done in f5bb0ef

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