-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Add Python 3 rewrite of arc_summary.py (#6873) #6892
Conversation
79155b5
to
e30a5e6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I'm OK with providing both the old and new versions of this.
cmd/arc_summary/test_arc_summary3.py
Outdated
@@ -0,0 +1,116 @@ | |||
#!/usr/bin/env python3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can add the test cases to the ZFS Test Suite by placing them in tests/zfs-tests/tests/functional
and adding them to the tests/runfiles/linux.run
run file. There are few trivial ones for the existing arc* scripts under tests/zfs-tests/tests/functional/cli_user/misc/
. It would be nice to have test coverage for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a monkey-see, monkey-do test that is basically a straight copy of the one for arc_summary.py with the new parameters, and included it in Makefile and the linux.run
. Don't see a way to automatically trigger the Python 3 test script test_arc_summary3.py included in cmd/arc_summary
? That would be nice to do it additionally, because it can test individual functions. Would it make sense to move that script somewhere in the test folder as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to move that script somewhere in the test folder as well?
Yes, your going to need to and then add a simply wrapper which calls it with log_must
.
Uh, I just noticed that I didn't include a line for arc_summary3.py in |
Yes, you can simply be able to add it to the |
So I moved the Python test file over to the other ksh test routines and added a ksh call. Python is really, really unhappy about putting the test that far away from the actual program, so I had to do some trickery with the imports in the test routine, which then annoys flake8 and pylint (so I expect those tests to fail). Unless I'm missing something, we can have one or the other -- the test routine with the other tests or happy linters. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, we're going to need to keep the test routine with the other tests. Is there a way to suppress the linter warnings?
|
||
import os | ||
import sys | ||
FILE_LOCATION = "../../../../../../cmd" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the ZTS can be run from either the installed zfs-tests
package or in-tree it's not safe to assume the location of arc_summary3. All the ZTS knows for certain is that by adding arc_summary3.py
to tests/zfs-tests/include/commands.cfg
(which you should do) the utility will be in the default PATH
. Perhaps the calling script can pass in the full path?
Including arc_summary3.py in AFAIK, the linters will fail as long as the imports don't come first, so any manipulation of the paths is going to make them unhappy. One unorthodox, brute-force way of dealing with the whole problem would be to create a symbolic link to the original arc_summary3.py in the same folder as the test ( |
We're using http://flake8.pycqa.org/en/latest/user/violations.html#ignoring-violations-with-flake8 |
@behlendorf Thanks for pointing out inline flake8 stuff, testing that now. Will figure out PATH next. |
Importing PATH into test_arc_summary3.py now works on my machine in-tree when simulated with a manipulated PATH. Added comments to explain the trickery involved and why we turned off flake8 for the test program. Fingers crossed. |
tests/runfiles/linux.run
Outdated
@@ -408,7 +408,7 @@ tests = ['zdb_001_neg', 'zfs_001_neg', 'zfs_allow_001_neg', | |||
'zpool_offline_001_neg', 'zpool_online_001_neg', 'zpool_remove_001_neg', | |||
'zpool_replace_001_neg', 'zpool_scrub_001_neg', 'zpool_set_001_neg', | |||
'zpool_status_001_neg', 'zpool_upgrade_001_neg', 'arcstat_001_pos', | |||
'arc_summary_001_pos', 'dbufstat_001_pos'] | |||
'arc_summary_001_pos', 'arc_summary3_001.pos', 'dbufstat_001_pos'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you intentionally omit arc_summary3_002.pos
?
0280938
to
1c78460
Compare
f9e6edc
to
648e947
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The word for software that should no longer be used is DEPRECATED, not DEPRECIATED.
A few of the doc strings for functions in arc_summary3.py are full sentences but don't end in periods. Other than the above, the code looks good on a read-through. Running the plain command (arc_summary vs. arc_summary3), I get similar results. The numbers are rounded to one place rather than two, but that seems fine to me and I assume is intentional. I ran it on systems with and without L2ARC. The L2ARC writes aren't being abbreviated. I'm not sure if that's intentional.
arc_summary3:
|
@rlaager, thanks for checking, especially the L2ARC stuff; I'll try to get to the fixes this week. -- @behlendorf at this point, I would suggest we just drop the Python test program, because it's adding too much complexity for too little value; even if we get it to work, nobody is going to be able to figure it out later. I'd include a link to the test program so it's not lost, and then we just go with the "native" ZFS test suite? -- Thanks again! |
@scotws sounds good. I agree, the test program isn't worth the bother in this case. Thanks. |
# HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT | ||
# LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY | ||
# OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF | ||
# SUCH DAMAGE. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@scotws What is the license of this file? Maybe use CDDL too, as you did in arc_summary3_002_pos.ksh?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gmelikov Oops, thought I had, will take a look (but it'll be the weekend before I can get to any of this). Note that we've decided to remove that whole file from the tree because it's causing more trouble than it is worth; I'll keep it my github page with a link in the patch notes.
0b2601f
to
be9a07b
Compare
Sorry, I'm having problems understanding what is going wrong with the tests, for instance
with buildbot/Ubuntu 17.10 x86_64 (STYLE). Did I delete some file by mistake or leave a line in I shouldn't have? |
@scotws the issues with the bots should be resolved, I've resubmitted the failed builds and I'll see about giving this another review. Thanks! |
ad8718e
to
3bedf7d
Compare
@behlendorf I'm not sure what is failing here, the Linux build? I didn't change anything 😃 ! |
@sckobras actually everything is passing which is great, the "Kernel.org Built-in" builder isn't your fault and needs to be addressed in a different PR. I'd like to do some manual testing with this before merging, but it looks very close to ready. @gmelikov @rlaager time permitting it would be great if you could kick the tires a bit too. |
cmd/arc_summary/arc_summary3.py
Outdated
by the different "short scale" and "long scale" representations in | ||
English, which use the same words for different values. See | ||
https://en.wikipedia.org/wiki/Names_of_large_numbers and | ||
https://physics.nist.gov/cuu/Units/prefixes.html . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd probably leave off the period on optionally add a colon after "and".
cmd/arc_summary/arc_summary3.py
Outdated
|
||
|
||
def section_archits(kstats_dict): | ||
"""Print information on how the caches are accessed ("arc hits") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd consider this a full sentence and use a period.
cmd/arc_summary/arc_summary3.py
Outdated
|
||
|
||
def section_dmu(kstats_dict): | ||
""" Collection information on the DMU""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Collect" instead of "Collection"
cmd/arc_summary/arc_summary3.py
Outdated
|
||
|
||
def section_l2arc(kstats_dict): | ||
""" Collect information on L2ARC device if present. If not, tell user |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stray leading space
cmd/arc_summary/arc_summary3.py
Outdated
|
||
|
||
def section_zil(kstats_dict): | ||
""" Collect information on the ZFS Intent Log. Some of the information |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stray leading space
I threw in some comments about minor typographical things. While those ideally should be cleaned up, they're minor, and I changed my review to Approve so I'm not blocking this. |
c66fa11
to
6872572
Compare
cmd/arc_summary/arc_summary3.py
Outdated
|
||
command = ["/sbin/modinfo", request, "-0"] | ||
info = subprocess.run(command, stdout=subprocess.PIPE, | ||
universal_newlines=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On Centos 7.4, running python 3.4.5, I'm hitting the following error when running arc_summary3.py
without arguments. My guess is this is caused by running zfs in-tree without installing kmods on the system.
Can you update this function to read the module version from /sys/module/{spl,zfs}/version
instead. This will always work and is authoritative since it's the version of the module currently loaded in to the kernel. That may differ from what modinfo
reports if new kmods have been built but not yet loaded.
Traceback (most recent call last):
File "./cmd/arc_summary/arc_summary3.py", line 833, in <module>
main()
File "./cmd/arc_summary/arc_summary3.py", line 790, in main
print_header()
File "./cmd/arc_summary/arc_summary3.py", line 354, in print_header
zfs = get_version('zfs')
File "./cmd/arc_summary/arc_summary3.py", line 327, in get_version
modinfo = subprocess.run(command, stdout=subprocess.PIPE,
AttributeError: 'module' object has no attribute 'run'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@behlendorf If this is my friend CentOS with Python 3.4, then the problem is that they are using an old version of subprocess (seriously, what is it with CentOS?). See https://docs.python.org/3/library/subprocess.html , where it says "New in version 3.5". We could add a check at the beginning to make sure that it's 3.5 or later and fail gracefully; the good news is that this problem should go away by itself in a few months.
I assume we still want to check /sys/module
and not modinfo
? Will try to get to that over the weekend, but could be early next week. Thanks for the help!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume we still want to check /sys/module and not modinfo?
Yes, and it looks like get_description
and get_version
are the only callers subprocess
so perhaps we can avoid this compatibility issue entirely.
the good news is that this problem should go away by itself in a few months.
Why is that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll see what I can do about subprocess
next week ... I remember there was a work-around. -- I'm assuming that CentOS at some point will be upgrading Python 3 -- we're almost at Python 3.7, so 3.4 is pretty ancient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@scotws does look like CentOS provides both python 3.4 and 3.6 from the EPEL repository. But if at all possible let's try and do something compatible with both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Full disclosure: subprocess.run()
says it uses the io.TextIOWrapper
default encoding, which says it uses locale.getpreferredencoding(False)
. If you wanted to match more closely, you could use that instead of sys.stdin.encoding
in my example. On my Ubuntu system, though, sys.stdin.encoding
is UTF-8
(from my terminal) and locale.getpreferredencoding(False)
is ANSI_X3.4-1968
. I have always used sys.stdin.encoding
(or stdout
) and it's always been fine for me. Using sys.stdin.encoding
seems like the superior option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rlaager Thanks for the suggestions! There are actually two issues at the moment:
The first one is the subprocess.run()
problem, which affects a very small number of machines -- if I'm seeing this correctly, it is only some CentOS versions. This will go away by itself as the last distributions catch up to Python 3.5, so I was actually planning to keep the current, high-level code and throw an IF/THEN
version check to the effect of < 3.5
around it with the older subprocess.check_output()
for the stragglers as you suggest. When everybody is at 3.5 in a few months, we simply remove the IF/THEN
. Yes, this is crude, but keeps the recommended, cleaner code for most machines, and is temporary anyway.
The second issue is that @behlendorf has pointed out we should be reading stuff from /sys/module/{spl,zfs}/version
instead of using modinfo
(like the original arc_summary.py
) so we report what's actually in the kernel instead of what has been built but maybe not loaded (which I had honestly not thought of). That's also trivial, except that there seems no way to get the parameters descriptions from /sys/module
.
My current plan there is, if the user wants descriptions, we load the parameters from /sys/module
and use them as keys to a dictionary of parameters/descriptions we get from modinfo
. Yes, that runs the risk that some descriptions might change between loaded/built-but-not-loaded, but that should be small. -- Unless I've missed something and there is some other way to retrieve the descriptions from /sys/modules
, which would of course be best.
Thanks again!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When everybody is at 3.5 in a few months.
I wouldn't get your hopes up for CentOS 7. Since it's an enterprise distribution they're particuarly adverse to change and it wouldn't shock me if python 2.7 remains the default python version until it's end-of-life in 2024.
The good news is that python 3.6 is also provided by the distribution, worst case we can do something distribution specific here and require #!/bin/python36
. But if we can fallback gracefully to something compatible that would be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
personally, I don't see check_output going away anytime soon, but...
there no need to check versions, check to see if you can import run from subprocess thusly:
info = ''
if 'run' in dir(subprocess):
info = subprocess.run(...)
elif 'check_output' in dir(subprocess):
info = subprocess.check_output(...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@richardelling Thanks, hadn't thought of that, that's pretty much what I did now.
@behlendorf Sorry, took longer than I thought to get around to this. Could you please run it on your CentOS system again, I tried to test it for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just one last requested fix!
cmd/arc_summary/arc_summary3.py
Outdated
descs = {} | ||
target_prefix = 'parm:' | ||
|
||
# We would prefer to do this with /sys/modules -- see the discusion at |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/discusion/discussion
cmd/arc_summary/arc_summary3.py
Outdated
raw_output = info.stdout.split('\0') | ||
else: | ||
info = subprocess.check_output(command, universal_newlines=True) | ||
raw_output = info.split('\0') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there! Everything now works as intended when the kmods exist. However we still throw an ugly exception when they're missing. Can you just handle this a bit more gracefully and not print an error. This case can legitimately come up for at least two reasons.
- ZFS was built direclty in to the kernel and not as an independant kmod, or
- a developer is using a custom kmod which was built in the zfs srouce in-tree and not loaded with modprobe
modinfo: ERROR: Module spl not found.
Traceback (most recent call last):
File "./cmd/arc_summary/arc_summary3.py", line 849, in <module>
main()
File "./cmd/arc_summary/arc_summary3.py", line 843, in main
section_calls[section](kstats)
File "./cmd/arc_summary/arc_summary3.py", line 693, in section_spl
descriptions = get_descriptions('spl')
File "./cmd/arc_summary/arc_summary3.py", line 305, in get_descriptions
info = subprocess.check_output(command, universal_newlines=True)
File "/usr/lib64/python3.4/subprocess.py", line 617, in check_output
raise CalledProcessError(retcode, process.args, output=output)
subprocess.CalledProcessError: Command '['/sbin/modinfo', 'spl', '-0']' returned non-zero exit status 1
cmd/arc_summary/arc_summary3.py
Outdated
print(format_raw_line(name, value)) | ||
|
||
# Tunables and SPL must be handled separately because they come from a | ||
# different source and have desciptions the user might request |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/desciptions/descriptions
@behlendorf Good point. I've thrown a |
@scotws i think we should be using diff --git a/arc_summary3.py b/arc_summary3.py
index 21b57e9..adcb781 100644
--- a/arc_summary3.py
+++ b/arc_summary3.py
@@ -308,7 +308,7 @@ def get_descriptions(request):
info = subprocess.check_output(command, universal_newlines=True)
raw_output = info.split('\0')
- except CalledProcessError:
+ except subprocess.CalledProcessError:
print("Error: Descriptions not available (can't access kernel module)")
sys.exit(1) Follow-up question: is it ok to |
Add new script arc_summary3.py as a complete rewrite of the arc_summary.py tool (see issue openzfs#6873) Add new options: -g/--graph - Display crude graphic representation of ARC status and quit -r/--raw - Print all available information as minimally formatted list (for grep) -s/--section - Print a single section. This replaces -p/--page, which is kept for backwards use but marked as depreciated Add new sections with information on ZIL and SPL. Notify user if sections L2ARC and VDEV are skipped instead of failing silently. Add warning that -p/--page option is depreciated. Developed for Python 3.5. Signed-off-by: Scot W. Stevenson <scot.stevenson@gmail.com>
@loli10K Well, when you put it like that, it seems painfully obvious 😃. Thanks, flake8 is happy now, too. -- As for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me now! This should be ready to go once the builders finish with it.
Codecov Report
@@ Coverage Diff @@
## master #6892 +/- ##
==========================================
+ Coverage 76.42% 76.43% +<.01%
==========================================
Files 327 327
Lines 103866 103866
==========================================
+ Hits 79384 79389 +5
+ Misses 24482 24477 -5
Continue to review full report at Codecov.
|
@behlendorf Yay! And thanks again for all the help. |
Description
Add new Python 3 script
arc_summary3.py
as a complete rewrite ofarc_summary.py
to display basic information on the ARC status and various other parameters. This is provided in addition - not as a replacement - to the existingarc_summary.py
tool. See #6873 for a discussion of the reasoning behind adding a new version of this tool while keeping a legacy version as well.New options:
Adds new sections with information on the ZIL and SPL.
We now notify the user if sections L2ARC and VDEV are skipped instead of failing silently; note VDEV caching is currently disabled and slated for possible removal (see source code). Adds information on the ZFS and SPL versions to the header.
The -s/--section option is intended to replace the page number system, which required the user to remember which page number was of interest. The -p/--page options are still supported, but marked as DEPRECIATED. Current legal sections are
arc archits dmu l2arc spl tunables vdev zil
. It should be easier now to add and modify sections.The -r/--raw option is intended to work with other tools such as
grep
. It respects the -a/-d options (alternate output format / descriptions included) where possible.The output of the -g/--graph option is intended to give a quick, rough overview as a visual orientation. An example (Ubuntu 16.04 LTS x86_64 with 24 GB RAM, 8 GB ARC max, ZFS stock version 0.6.5.9-2 with
/home
as ZFS mirror pool immediately after starting Civilization VI on Steam on otherwise quiet machine):F
is for MFU,R
for MRU, andO
is used for "other" if necessary (not present in this example).arc_summary3.py
was developed for Python 3.5. This follows the version of Python currently installed in Ubuntu 16.04 LTS. Few systems will have Python 3.6 installed yet.Known issues
The new script is based on the same internal logic as the original, so any error or issue present there will probably show up here as well. For instance, the number of anonymous hits can be negative the way it is calculated in both scripts; they both simply hide any negative value.
This script will probably make a bunch of test suites unhappy where Python 3 is not included. There is no experience with this script under extreme conditions (for example ARC throttling).
How Has This Been Tested?
There is a unittest script
test_arc_summary3.py
at https://gist.github.com/scotws/aaf5d9c9317081e249b664a371ec4907Most testing was done in-tree, comparing the output to that of the current
arc_summary.py
version.The L2ARC section has not seen any real-world use because I do not have access to a L2ARC device on my machine.
Other
Switching to Python 3 results in a noticeably smaller file size despite the addition of several new features. Output of
wc
for both scripts:Types of changes
Checklist:
Signed-off-by
.