-
Notifications
You must be signed in to change notification settings - Fork 6
Add tests for corruption recovery on LINSTOR SR #360
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,11 +3,14 @@ | |
| import pytest | ||
|
|
||
| import functools | ||
| import json | ||
| import logging | ||
| import os | ||
| from dataclasses import dataclass | ||
|
|
||
| import lib.commands as commands | ||
| from lib.common import safe_split | ||
| from lib.sr import SR | ||
|
|
||
| # explicit import for package-scope fixtures | ||
| from pkgfixtures import pool_with_saved_yum_state | ||
|
|
@@ -19,6 +22,7 @@ | |
| from lib.pool import Pool | ||
| from lib.sr import SR | ||
| from lib.vdi import VDI | ||
| from lib.vm import VM | ||
|
|
||
| GROUP_NAME = 'linstor_group' | ||
| STORAGE_POOL_NAME = f'{GROUP_NAME}/thin_device' | ||
|
|
@@ -171,3 +175,56 @@ def vm_on_linstor_sr(host: Host, linstor_sr: SR, vm_ref: str): | |
| yield vm | ||
| logging.info("<< Destroy VM") | ||
| vm.destroy(verify=True) | ||
|
|
||
| @pytest.fixture(scope='function') | ||
| def host_and_corrupted_vdi_on_linstor_sr(host: Host, linstor_sr: SR, vm_ref: str): | ||
| vm: VM = host.import_vm(vm_ref, sr_uuid=linstor_sr.uuid) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should be created in a fixture
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I need the VM lifetime to be directly tied to the fixture's lifetime and since we already have a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe worth a comment ?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will break when vm_ref is a UUID referring to an existing VM. This also totally bypasses local cache for imported VMs. If it's best to import a VM and you don't care which VM, that there's no advantage in , then you should import one of the mini VMs without depending on vm_ref, as we already do in some storage tests: On the contrary, if you want the test to run with any VM we can test (as part of a "multi" job), then maybe you should still rely on the imported_vm fixture, but then copy it to the SR where you need it. And if what you want is actually an empty VM with just a disk that you can break, then maybe it's time we implemented creating a VM from scratch, without relying on an XVA. Worth a discussion. |
||
| pool: Pool = host.pool | ||
| master: Host = pool.master | ||
|
|
||
| def get_vdi_volume_name_from_linstor() -> str: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are the helper functions local to the fixture definition. There's no chance we could reuse them in other tests? This makes the fixture definition rather big. |
||
| result = master.ssh([ | ||
| "linstor-kv-tool", | ||
| "--dump-volumes", | ||
| "-g", | ||
| f"xcp-sr-{GROUP_NAME}_thin_device" | ||
| ]) | ||
| volumes = json.loads(result) | ||
| for k, v in volumes.items(): | ||
| path = safe_split(k, "/") | ||
| if len(path) < 4: | ||
| continue | ||
| uuid = path[2] | ||
| data_type = path[3] | ||
| if uuid == vdi_uuid and data_type == "volume-name": | ||
| return v | ||
| raise FileNotFoundError(f"Could not find matching linstor volume for `{vdi_uuid}`") | ||
|
|
||
| def get_vdi_host(path: str) -> Host: | ||
| for h in pool.hosts: | ||
| result = h.ssh(["test", "-e", path], simple_output=False, check=False) | ||
| if result.returncode == 0: | ||
| return h | ||
| raise FileNotFoundError(f"Could not find matching host for `{vdi_uuid}`") | ||
|
|
||
| try: | ||
| vdi_uuid: str = next(( | ||
| vdi.uuid for vdi in vm.vdis if vdi.sr.uuid == linstor_sr.uuid | ||
| )) | ||
|
|
||
| volume_name = get_vdi_volume_name_from_linstor() | ||
| lv_path = f"/dev/{GROUP_NAME}/{volume_name}_00000" | ||
| vdi_host = get_vdi_host(lv_path) | ||
| logging.info("[%s]: corrupting `%s`", host, lv_path) | ||
| vdi_host.ssh([ | ||
| "dd", | ||
| "if=/dev/urandom", | ||
| f"of={lv_path}", | ||
| "bs=4096", | ||
| # Lower values seems to go undetected sometimes | ||
| "count=10000" # ~40MB | ||
| ]) | ||
| yield vm, vdi_host, volume_name | ||
| finally: | ||
| logging.info("<< Destroy corrupted VDI") | ||
| vm.destroy(verify=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.
May this function-scope fixture be reused in future tests? If yes, I think conftest.py is the right location for it. If not, it would be better defined close to the only function that currently requires it.