[vdo] Capture more information about vdo volumes #3717
Conversation
|
In this case I moved the 'lvs' command to the VDO plugin because that will guarantee that the command is run when the conditions for the other VDO command captures are met. If I add it to the LVM plugin, I'll have to add extra logic to make sure VDO is in place, and I thought it was a more natural place to have it in the VDO than the LVM one. But I can change it if the logic is not sound. |
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
1 similar comment
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
jcastill
force-pushed
the
jcastillo-capture-more-vdo-data
branch
2 times, most recently
from
c99b9b9
to
b4b5448
Compare
|
Added the missing comma and another extra command. With this change now we should capture both non-LVM and LVM VDO devices |
jcastill
force-pushed
the
jcastillo-capture-more-vdo-data
branch
from
b4b5448
to
23c7536
Compare
sos/report/plugins/vdo.py
Outdated
| vdo_cols = 'vdo_deduplication,vdo_index_state' | ||
| lvs_opts = '-a -o +' |
There was a problem hiding this comment.
Why the two extra variables, if you use either of them just once?
There was a problem hiding this comment.
Two reasons:
- The LVS command was too long before I removed the headers option, so using these two variables I was within the limit, and
- The request has a very big list of columns and I'll need to split the output in at least two variables to make sure is readable.
I'm still playing with the columns I need to display, so the name vdo_cols will probably change.
There was a problem hiding this comment.
OK, could be.. Bit redundant for the code, more readable for users.
There was a problem hiding this comment.
This is still a draft PR so I'll see how can I improve it
sos/report/plugins/vdo.py
Outdated
| vdo_cols = 'vdo_deduplication,vdo_index_state' | ||
| lvs_opts = '-a -o +' | ||
| lvm_vdos = self.collect_cmd_output(f"lvs {lvs_opts}{vdo_cols}") | ||
| for vdo in lvm_vdos['output'].splitlines(): |
There was a problem hiding this comment.
What if the lvs command would raise an error - is it safe to parse its output like this? (we often guard 'output' under if lvm_vdos['status'] == 0: - but here I see this plugin lacks this check (cf. lines 28-29).
There was a problem hiding this comment.
I'll do this, sorry I missed the check.
jcastill
force-pushed
the
jcastillo-capture-more-vdo-data
branch
from
23c7536
to
abe4b00
Compare
Capture more information about vdo volumes Related: VDO-5797 Signed-off-by: Jose Castillo <[email protected]>
jcastill
force-pushed
the
jcastillo-capture-more-vdo-data
branch
from
abe4b00
to
7a682a1
Compare
Capture more information about vdo volumes
Related: VDO-5797
Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines