[IMP] In some case, we might want to keep some lvm active #420
Conversation
cvinh
force-pushed
the
imp-keep_active_lvm
branch
2 times, most recently
from
c2c2109 to
8acac94
Compare
There was a problem hiding this comment.
Thanks! The patch looks generally fine (minus a few comments), but there are at least a couple missing points:
- Please add a some text/explanation in the body of the commit.
- Add a test (potentially at
lmv_test.py) covering the new config option and new the deactivate unused LVs logic.
lib/vdsm/storage/lvm.py
Outdated
| @@ -1162,8 +1165,10 @@ def deactivateUnusedLVs(vgname, skiplvs=()): | |||
| # List prepared images LVs if any | |||
There was a problem hiding this comment.
Wouldn't doing something like this achieve the same thing if we use lv names instead of full paths?
def deactivateUnusedLVs(vgname, skiplvs=KEEPACTIVE):I mean, technically, skipping LVs is already part of the API.
There was a problem hiding this comment.
Humm, we have the same lv name in different vg, that's why we prefer using complete path
ie : /dev/vg1/lv1, /dev/vg2/lv1
There was a problem hiding this comment.
Certainly skiplvs=KEEPACTIVE was not the best example here, as it mixes different vgs.
But my point was that there is already a logic for skiplvs in place, so I think it would be nice to consolidate the list before we get to the if lv.name in skiplvs: condition, filtering all lvs that pertain to vgname.
Something like (not tested):
skiplvs = skiplvs + tuple(lvm.split('/')[-1] for lvm in KEEPACTIVE if vgname == lvm.split('/')[-2])In this solution maybe worth using different than the complete path to ease the parsing.
Or even convert the KEEPACTIVE list into a dictionary, where the VG is the key and the value is a list of lvs that we can directly append to skiplvs based on vgname.
In some special configuration, we might need not to deactivate some lvm. This commit allows to define some lvm paths that will not be deactivated by vdsm. The list of the lvm paths can be configured in /etc/vdsm/vdsm.conf.d/keep_alive_lvm.conf: [lvm] keep_active = /dev/vg1/lv1,/dev/vg1/lv2,/dev/vg2/lv1 Signed-off-by: Cyril VINH-TUNG <[email protected]>
cvinh
force-pushed
the
imp-keep_active_lvm
branch
from
8acac94 to
9202b31
Compare
I'm sorry, I don't know how to add Tests, I would make some bad copy-paste ... 👎 |
|
Can you please point me out where I could add the information in the documentation ? |
It's ok, start with something simple, and I can help you refine it. I think something similar to test_bootstrap would do. Just make sure you You can see how to run the tests here: Check pytest documentation if you are unfamiliar: https://docs.pytest.org/en/8.2.x/getting-started.html |
No description provided.