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.

Sign up for GitHub

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

Add support for smart command #15

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

Add support for smart command #15

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0edb4f1
Chronial/snapraid-runner Issue #3: Add support for `smart` command
k3vmcd Sep 24, 2017
9f565f1
Fix running 'snapraid smart' from cron
k3vmcd Oct 2, 2017
8f44ea2
Cross-Platform SMART
k3vmcd Jan 3, 2018
618adfc
Simplify smart support
Chronial May 15, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions snapraid-runner.conf.example
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,7 @@ password =
enabled = false
percentage = 12
older-than = 10

[smart]
; set to true to run smart after sync and scrub
enabled = false
27 changes: 23 additions & 4 deletions snapraid-runner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,14 @@ def snapraid_command(command, args={}, ignore_errors=False):
stdout=subprocess.PIPE,
stderr=subprocess.PIPE)
out = []
threads = [
tee_log(p.stdout, out, logging.OUTPUT),
tee_log(p.stderr, [], logging.OUTERR)]
if command == 'smart':
threads = [
tee_log(p.stdout, out, logging.SMART),
tee_log(p.stderr, [], logging.OUTERR)]
else:
threads = [
tee_log(p.stdout, out, logging.OUTPUT),
tee_log(p.stderr, [], logging.OUTERR)]
Copy link
Owner

Choose a reason for hiding this comment

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

Which output belongs to which command is always clearly identifiable from the headers. Why does smart need this logging trickery?

Copy link
Author

Choose a reason for hiding this comment

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

I created a new SMART logging level that is output in both "short" and "long" logging modes since it is highly likely that it is always desired to view the SMART output if you are sending emails on success. It does not output if set to only error notification emails since the log level is set one level higher than error logging. This section ensures the SMART "OUTPUT" logging is redirected into "SMART" logging while leaving all other logging as-is.

Copy link
Owner

Choose a reason for hiding this comment

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

Ah, ok – that makes sense. Please add a comment about that to where you define that loglevel.
Also add an output_log_level parameter to this function and use that instead of this if.

Copy link
Author

Choose a reason for hiding this comment

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

I added a comment on line 175 to highlight the SMART logging level creation, but I'm not sure what you're looking for in the output_log_level instead of this if

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for the changes. I meant that the signature should be snapraid_command(command, args={}, ignore_errors=False, output_log_level=logging.OUTPUT) and then use the parameter instead of guessing based on the name of the command.

for t in threads:
t.join()
ret = p.wait()
Expand Down Expand Up @@ -132,7 +137,7 @@ def load_config(args):
global config
parser = ConfigParser.RawConfigParser()
parser.read(args.conf)
sections = ["snapraid", "logging", "email", "smtp", "scrub"]
sections = ["snapraid", "logging", "email", "smtp", "scrub", "smart"]
config = dict((x, defaultdict(lambda: "")) for x in sections)
for section in parser.sections():
for (k, v) in parser.items(section):
Expand All @@ -150,6 +155,7 @@ def load_config(args):

config["smtp"]["ssl"] = (config["smtp"]["ssl"].lower() == "true")
config["scrub"]["enabled"] = (config["scrub"]["enabled"].lower() == "true")
config["smart"]["enabled"] = (config["smart"]["enabled"].lower() == "true")
Copy link
Owner

@Chronial Chronial Oct 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Is this save, or will this raise an exception if the smart section is missing from the config? I don't remember ^^.

Copy link
Author

Choose a reason for hiding this comment

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

I'll test & confirm what happens if the config omits this section and design a method to handle if it raises an exception.

Copy link
Author

Choose a reason for hiding this comment

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

Confirmed it does not throw an error with latest code on Windows & Linux.

Copy link
Owner

Choose a reason for hiding this comment

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

thx 👍

config["email"]["short"] = (config["email"]["short"].lower() == "true")
config["snapraid"]["touch"] = (config["snapraid"]["touch"].lower() == "true")

Expand All @@ -165,6 +171,8 @@ def setup_logger():
logging.addLevelName(logging.OUTPUT, "OUTPUT")
logging.OUTERR = 25
logging.addLevelName(logging.OUTERR, "OUTERR")
logging.SMART = 26
logging.addLevelName(logging.SMART, "SMART")
root_logger.setLevel(logging.OUTPUT)
console_logger = logging.StreamHandler(sys.stdout)
console_logger.setFormatter(log_format)
Expand Down Expand Up @@ -288,6 +296,17 @@ def run():
finish(False)
logging.info("*" * 60)

if config["smart"]["enabled"]:
logging.info("Running SMART...")
# Expand PATH to include /usr/sbin for running smartctl from cron
os.environ["PATH"] += os.pathsep + "/usr/sbin"
Copy link
Owner

Choose a reason for hiding this comment

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

This is clearly not cross-platform :). Even on unix it makes strong assumptions about the user's setup. I assume you had problems with the binary not being found? Why don't you make the binary path a setting?

Copy link
Author

Choose a reason for hiding this comment

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

Good point, I definitely overlooked the fact that snapraid is cross-platform and wrote this for Linux.

Copy link
Owner

Choose a reason for hiding this comment

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

You should probably remove this and just make sure the command is run in the correct environment.

Copy link
Author

Choose a reason for hiding this comment

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

I handled this with known paths in my latest commit. What do you think of the changes to make it cross-platform?

Copy link
Owner

Choose a reason for hiding this comment

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

It look very comprehensive, but I don't think this script should make any attempt to find the executable. That is just very complicated and the PATH system of the OSs is perfectly suitable to handle this job. I will have a look and do some changes to this PR.

try:
snapraid_command("smart")
except subprocess.CalledProcessError as e:
logging.error(e)
finish(False)
logging.info("*" * 60)

logging.info("All done")
finish(True)

Expand Down