tests/storage/linstor: Added test_linstor_sr_expand_host and test_linstor_sr_expand_disk to test expand Linstor SR
#282
Conversation
rushikeshjadhav
force-pushed
the
feat-storage-linstor-612
branch
2 times, most recently
from
85472e1 to
740625c
Compare
rushikeshjadhav
force-pushed
the
feat-storage-linstor-612
branch
2 times, most recently
from
be3d230 to
191fadf
Compare
lib/host.py
Outdated
| def raw_disk_is_available(self, disk): | ||
| return self.ssh_with_result(['blkid', '/dev/' + disk]).returncode == 2 | ||
|
|
There was a problem hiding this comment.
This commit should come first of the PR, for the previous commits to work. Since this is the same as #301 this PR should rather be rebased on that one, with a note in the PR description.
| available_disks = h.available_disks() | ||
| # We need the disk to be "raw" (non LVM_member etc) to use | ||
| available_disks = [disk for disk in available_disks if h.raw_disk_is_available(disk)] | ||
| exapnsion_sr_disk = pytestconfig.getoption("expansion_sr_disk") |
There was a problem hiding this comment.
Corrected the typo.
| if exapnsion_sr_disk: | ||
| assert len(exapnsion_sr_disk) == 1, "This test requires only one --expansion-sr-disk parameter" | ||
| if "auto" == exapnsion_sr_disk[0]: | ||
| disks = available_disks | ||
| else: | ||
| assert exapnsion_sr_disk[0] in available_disks, "The expansion-sr-disk seems unavailable" | ||
| disks = exapnsion_sr_disk |
There was a problem hiding this comment.
There seems to be differnece between the 2 branches, where one limits to a single disk, and the other takes all available disks without checking. Do we really have a hard constraint for one disk, or not?
There was a problem hiding this comment.
Consider following cases:
--expansion-sr-disk is provided then its checked for the disk name e.g. sdc (uses exact disk) or auto (uses all disks).
--expansion-sr-disk is not provided but the test is called/selected due to markers then uses all disks. (This can be done with some sort of warning as logging enhancement. Or should we drop this way and abort?)
| logging.info("Found Disk {}".format(disk)) | ||
| device = '/dev/' + disk | ||
| try: | ||
| h.ssh(['pvcreate', '-ff', '-y', device]) |
There was a problem hiding this comment.
Is -ff really what we want (here and in other places)? That seems to open the possibility that a wrong flag could overwrite the system disks, no?
There was a problem hiding this comment.
Earlier it was suitable but now we have raw_disk_is_available() so we can reduce the force.
| assert int(new_sr_size) > int(sr_size) and resized is True, \ | ||
| f"Expected SR size to increase but got old size: {sr_size}, new size: {new_sr_size}" | ||
| logging.info("SR expansion completed") | ||
| vm.shutdown(verify=True) |
There was a problem hiding this comment.
This start/shutdown pair seems at first read to call for using a fixture, but then the test_start_and_shutdown_VM does introduce doubts. At the very least the test description should mention that the test includes a VM reboot.
There was a problem hiding this comment.
ok, will add it in the test docstring.
| logging.info("SR expansion completed") | ||
| vm.shutdown(verify=True) | ||
| # Ensure VM is able to start and shutdown on expanded SR | ||
| self.test_start_and_shutdown_VM(vm) |
There was a problem hiding this comment.
Do we really want to test the shutdown at this point? Those 2 lines could also be read as "test a VM powercycle", in which case a fixture could take care of the VM shutdown, wouldn't that make sense?
There was a problem hiding this comment.
Do you think we should introduce a fixture like this?
@pytest.fixture
def running_vm(vm_on_linstor_sr):
vm = vm_on_linstor_sr
vm.start()
yield vm
vm.shutdown(verify=True)
The intended flow is:
- Create SR
- Create & Run VM on SR
- Expand SR
- Shutdown VM - to confirm that the SR expansion hasn’t broken guest R/W operations
- Start-Shutdown VM - to verify that power-cycling still works normally after expansion
One could argue that expanding an SR might introduce subtle issues, and that full SR regression tests (e.g., read/write integrity, snapshots, performance benchmarks etc) should run against the expanded SR. In that case, we could drop the call to self.test_start_and_shutdown_VM(vm) here and delegate those checks to a broader SR test suite.
| """ | ||
| Identify hosts within the same pool, detect free disks, create LVM, and integrate it into LINSTOR SR. | ||
| """ |
There was a problem hiding this comment.
The commit message has a more detailed description, which would be better suited in this docstring.
| @@ -2,7 +2,7 @@ | |||
| import pytest | |||
| import time | |||
|
|
|||
| from .conftest import LINSTOR_PACKAGE | |||
| from .conftest import GROUP_NAME, LINSTOR_PACKAGE, LINSTOR_RELEASE_PACKAGE | |||
There was a problem hiding this comment.
Those parameter additions should also appear in data.py-dist
| def test_linstor_sr_expand_host(self, linstor_sr, host, hostB1, provisioning_type, | ||
| storage_pool_name, vm_on_linstor_sr): |
There was a problem hiding this comment.
This test does too much work that does not get torn down in case of failure. Joining the pool should be moved into a fixture, and thus setup of hostB2 should as well; teardown of the LVM stuff on this host only happens in exception handler, and should likely also happen in the success case, which also looks like a call for a fixture.
…hecks. Added docstring explaining what this new method does vs `disk_is_available`. Added type hints. Signed-off-by: Rushikesh Jadhav <[email protected]>
…specifying or auto-detecting Linstor SR expansion disks. Signed-off-by: Rushikesh Jadhav <[email protected]>
…lume group devices. Signed-off-by: Rushikesh Jadhav <[email protected]>
rushikeshjadhav
force-pushed
the
feat-storage-linstor-612
branch
from
191fadf to
b632add
Compare
…suitable disk(s) and expand LVM of Linstor SR across hosts. The test: - Ensures `--expansion-sr-disk` disk(s) are available on the hosts. - Configures LVM onto it and integrates into Linstor SR pool. - Expands SR capacity and validates the increase. Signed-off-by: Rushikesh Jadhav <[email protected]>
…e the expansion of Linstor SR by integrating a new host into the storage pool. The test: - Ensures the new host has available disks and installs required Linstor packages. - Configures LVM and integrates it into Linstor SR pool. - Expands SR capacity and validates the increase. - Handles cleanup in case of failure, including removing the node, ejecting the host, and uninstalling Linstor packages. Signed-off-by: Rushikesh Jadhav <[email protected]>
rushikeshjadhav
force-pushed
the
feat-storage-linstor-612
branch
from
b632add to
ffbdef2
Compare
This PR is rebased from #301.
For
test_linstor_sr_expand_diskto work correctly, it is important to find an available disk that is not only mounted but alsorawin nature so that it can be madeLVM_memberfor Linstor SR.