Add zfs_refcount command. #270
Conversation
.github/scripts/install-drgn.sh
Outdated
| @@ -10,5 +10,6 @@ sudo apt install bison flex libelf-dev libdw-dev libomp5 libomp-dev | |||
| git clone https://github.com/osandov/drgn.git | |||
|
|
|||
| cd drgn | |||
| git checkout -b drgn_february 0a6aaaae5d31ed142448f8220e208d65478ec80d | |||
There was a problem hiding this comment.
Can we remove this line? If there is a problem with the github actions we fix the actions themselves. If there is a problem with drgn we fix drgn.
| # use the default dir unless alternate dir passed as arg2 to script | ||
| DATA_DIR="tests/integration/data" | ||
| if [ ! -z "$2" ]; then | ||
| DATA_DIR=$2 | ||
| fi |
There was a problem hiding this comment.
I think instead of giving the option for which dump to use, we should just go ahead and test both dumps, no option needed. This means a lot of things, so here is a preliminary checklist of the things we need to do:
[1] This script can accept and download more than one dumps, then placed them into their own respective directories under the DATA_DIR (the comment above will also need to be changed along with the code)
[2] The test infrastructure should be made aware that multiple dumps can exist and always run all commands that we have for both crash dumps
[3] The saved regression output should be moved into crash-dump-specific regression output directories
[4] The github actions will need to be adjusted to download both crash dumps
[5] The wiki steps to run the tests manually will need to be updated on the wiki to include both dumps (https://github.com/delphix/sdb/wiki/Integration-Tests)
We should do the above over multiple pull requests:
[1st PR] The first pull request should make the changes to this download script to accept multiple dumps, and the respective changes to the test infrastructure code and regression output
[2nd PR] The second pull request will be adding the second crash dump and its own regression output
[3rd PR] The last pull request will be adding the new zfs_refcount command
Another cool thing would be for the new crash dumps themselves to keep a README file on their compressed archive highlighting why we added it (e.g. old crash dump X did not have refcounts enabled and we want to test that).
tests/integration/infra.py
Outdated
| PRIMARY = "primary" | ||
| ALTERNATE = "alternate" |
There was a problem hiding this comment.
Instead of PRIMARY and ALTERNATE let's go ahead and have a list of crash dumps automatically detected by the target DATA_DIR. For example this would currently look like this with the two crash dumps that we have:
DATA_DIR/dump-<date 1>/{dump, mods, vmlinux, regression_output}
DATA_DIR/dump-<date 2>/{dump, mods, vmlinux, regression_output}
Keeping this structure as is makes it easy to add new crash dumps in the future.
tests/integration/infra.py
Outdated
|
|
||
|
|
||
| @staticmethod |
There was a problem hiding this comment.
static methods make sense in class context context. You can move this method in the Infra class.
sdb/commands/zfs/zfs_refcount.py
Outdated
| @staticmethod | ||
| def print_ref(obj: drgn.Object): | ||
| ptr = int(obj.ref_holder) | ||
| c = sdb.create_object("char *", ptr) | ||
| s = c.string_().decode("utf-8") | ||
| print(f"{hex(ptr)} {s} ") |
There was a problem hiding this comment.
This might be overkill, but it seems like this could be a PrettyPrinter for reference_t. Which could also be a Locator (finding reference_t's given a zfs_refcount_t).
sdb/commands/zfs/zfs_refcount.py
Outdated
| from sdb.commands.spl.spl_list import SPLList | ||
|
|
||
|
|
||
| class Zfs_Refcount(sdb.Locator, sdb.PrettyPrinter): |
There was a problem hiding this comment.
Can you remind me what the benefit is of declaring this as a Locator, despite not having any locator-specific methods (i.e. no_input() or @sdb.InputHandler-annotated methods).
There was a problem hiding this comment.
I changed it to just a pretty-printer.
class Zfs_Refcount(sdb.PrettyPrinter):
sdb/commands/zfs/zfs_refcount.py
Outdated
| c = sdb.create_object("char *", ptr) | ||
| s = c.string_().decode("utf-8") |
There was a problem hiding this comment.
From the example output, it looks like if the pointer is not a string, this results in an empty string? That seems fine, as long as it doesn't raise an exception.
There was a problem hiding this comment.
Hi @ahrens I played with this a bit and did finally get an exception generated from the decode method. I added a try/catch for it.
Codecov Report
@@ Coverage Diff @@
## master #270 +/- ##
==========================================
- Coverage 87.69% 87.48% -0.22%
==========================================
Files 63 64 +1
Lines 2577 2605 +28
==========================================
+ Hits 2260 2279 +19
- Misses 317 326 +9
Continue to review full report at Codecov.
|
|
I made a version with just the zfs_refcount command and standard test. |
Pull request checklist
Please check if your PR fulfills the following requirements:
Pull request type
Please check the type of change your PR introduces:
Issue Number: #192
What is the new behavior?
Does this introduce a breaking change?
Other information
Requires a crash dump with reference tracking enabled, so some of the testing infrastructure was refactored.