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

Run blockdev report commands after _make_loopbacked_devices() #218

Merged

Conversation

bgurney-rh
Copy link
Member

Related: #216

@bgurney-rh bgurney-rh self-assigned this Sep 28, 2023
@bgurney-rh bgurney-rh force-pushed the testharness-loopdevinfo branch from e9e019d to 86c9b5e Compare September 28, 2023 17:39
@bgurney-rh
Copy link
Member Author

Squashed into a single commit.

@bgurney-rh bgurney-rh marked this pull request as ready for review September 28, 2023 18:53
@bgurney-rh bgurney-rh force-pushed the testharness-loopdevinfo branch from 86c9b5e to d6fbfb0 Compare October 2, 2023 20:26
test_harness.py Outdated
@@ -121,6 +130,9 @@ def _gen_parser():
stratisd_cert_parser.add_argument(
"--verify-devices", help="Verify /dev/disk/by-id devices", action="store_true"
)
stratisd_cert_parser.add_argument(
Copy link
Member

Choose a reason for hiding this comment

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

Rather than using a number value, use the choices parameter to add_arguments and restrict the options to whatever levels the logging module implements.

Copy link
Member

Choose a reason for hiding this comment

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

Since we'll be logging for both stratisd_cert.py and stratis_cli_cert.py best to add this argument to the top-level parser, not just to stratisd's sub-parser.

Copy link
Member

Choose a reason for hiding this comment

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

In the main method, before namespace.func is invoked, configure the logger based on the value of the --log-level option. Instructions for how to do this are in docs at https://docs.python.org/3/howto/logging.html. Search for string "If you want to set the logging level from a command-line option such as:"

test_harness.py Outdated
@@ -94,7 +103,7 @@ def _run_stratis_cli_cert(namespace, unittest_args):
+ ["-v"]
+ unittest_args
)
_run_command(3, command)
_run_command(namespace, 3, command)
Copy link
Member

Choose a reason for hiding this comment

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

Do not pass namespace to _run_command. You don't need to since you've configured the logger in the main method and the logging module will do the rest.

Copy link
Member Author

Choose a reason for hiding this comment

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

On removing namespace, I realized that I used the wrong logging level for more verbose output. Now, to show the trace output, this command needs to be used:

# python3 test_harness.py --log-level debug stratisd_cert --verify-devices StratisdCertify.test_blockdev_list

Also, the "blockdev" commands are always being executed. Should I get the logging level, and conditionally run these?

Copy link
Member

@mulkieran mulkieran left a comment

Choose a reason for hiding this comment

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

Two minor things, then we should be good to merge.

test_harness.py Outdated
"--log-level",
help="Log level",
action="store",
choices=["info", "debug"],
Copy link
Member

Choose a reason for hiding this comment

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

The logging module defines several levels, one of which, notset, is a bit unusual.



logging.NOTSET | 0 | When set on a logger, indicates that ancestor loggers are to be consulted to determine the effective level. If that still resolves to NOTSET, then all events are logged. When set on a handler, all events are handled.
-- | -- | --
logging.DEBUG | 10 | Detailed information, typically only of interest to a developer trying to diagnose a problem.
logging.INFO | 20 | Confirmation that things are working as expected.
logging.WARNING | 30 | An indication that something unexpected happened, or that a problem might occur in the near future (e.g. ‘disk space low’). The software is still working as expected.
logging.ERROR | 40 | Due to a more serious problem, the software has not been able to perform some function.
logging.CRITICAL | 50 | A serious error, indicating that the program itsel

Might as well add them all to choices. Setting the default value to "info" seems right to me.

diagcmd = ["blockdev", option, device]
with subprocess.Popen(diagcmd, stdout=subprocess.PIPE) as proc:
blockdevoutput = proc.stdout.readline().strip().decode("utf-8")
logging.debug("output of %s: %s", diagcmd, blockdevoutput)
Copy link
Member

Choose a reason for hiding this comment

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

Try to use f-strings here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried before, but it failed linting with this:

test_harness.py:70:12: W1203: Use lazy % formatting in logging functions (logging-fstring-interpolation)

Copy link
Member

Choose a reason for hiding this comment

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

Ok!

Copy link
Member

@mulkieran mulkieran left a comment

Choose a reason for hiding this comment

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

Plz squash and I'll merge.

@bgurney-rh bgurney-rh force-pushed the testharness-loopdevinfo branch from 7e1105f to 41f97bc Compare October 4, 2023 17:25
@bgurney-rh
Copy link
Member Author

Squashed into a single commit.

@mulkieran mulkieran merged commit 55c61f8 into stratis-storage:master Oct 4, 2023
4 checks passed
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