feat(storage/linstor): Enhance multi-disk support and provisioning flexibility in Linstor SR tests

[rushikeshjadhav]

• Added sr_disks_for_all_hosts fixture to support multiple disks, ensuring availability across all hosts and handling "auto" selection dynamically.
• Updated lvm_disk fixture to provision multiple disks collectively, refining vgcreate and pvcreate logic.
• Introduced provisioning_type and storage_pool_name fixtures to dynamically configure storage provisioning (thin or thick).
• Refactored Linstor SR test cases to use the new fixtures, improving test coverage across provisioning types.
• Optimized Linstor installation and cleanup using concurrent execution, reducing setup time.
• Enhanced validation and logging for disk selection.

[stormi]

Adding a new CLI parameter such as --provisioning is a last resort change, because it moves the responsibility for defining it to the caller, when we probably want that our tests, by default, provide both testing for thick and thin provisioning, as their definition.

Couldn't we have test definitions that do both thick and thin, instead?

[rushikeshjadhav]

Couldn't we have test definitions that do both thick and thin, instead?

We can do that. Since this is XOSTOR level, one full cycle (create-test-destroy) with thin and then another full cycle with thick can be added.

Can we retain the CLI option for those callers who want to selectively run in one of the mode?

[stormi]

Can we retain the CLI option for those callers who want to selectively run in one of the mode?

In theory pytest already offers enough selectors to execute a set of tests and not others.

[Wescoeur]

Is there a specific reason to set 2 in your tests?

[rushikeshjadhav]

There is a strange issue with thick SR and redundancy set as 1. The error looks like below upon trying to create a VM (VDI). So, keeping it as 2 for now, so that other tests on thick SR can pass.

***** LINSTOR resources on XCP-ng: EXCEPTION <class 'xs_errors.SROSError'>, VDI Creation failed [opterr=error cmd: `['/usr/bin/vhd-util', 'create', '--debug', '-n', '/dev/drbd/by-res/xcp-volume-d21c6a82-ed29-4106-ace6-7542c334254c/0', '-s', '2048', '-S', '2097152']`, code: `30`, reason: `/dev/drbd/by-res/xcp-volume-d21c6a82-ed29-4106-ace6-7542c334254c/0: failed to create: -30` (openers: {'xcp-ng-xs1': {}})]
Feb 18 19:02:59 xcp-ng-xs1 SM: [46577]   File "/opt/xensource/sm/LinstorSR", line 1743, in create

[Wescoeur]

I think we must handle this error in the driver itself. In theory a simple busy loop should fix that.

[rushikeshjadhav]

Thanks, will keep it as 1.

[stormi]

Will this PR involve changes in linstor job definitions in jobs.py for the extended test coverage to be effective in Jenkins once this is merged?

[rushikeshjadhav]

Will this PR involve changes in linstor job definitions in jobs.py for the extended test coverage to be effective in Jenkins once this is merged?

After review of jobs.py it seems no changes are needed. Including a snippet for reference on "params" below.

Earlier we had --provisioning but that was removed and now the tests will run for both thin and thick as both should be tested.

    "linstor-main": {
        "description": "tests the linstor storage driver, but avoids migrations and reboots",
        "requirements": [
            "A pool with at least 3 hosts.",
            "An additional free disk on every host.",
            "A small VM that can be imported on the SR.",
        ],
        "nb_pools": 1,
        "params": {
            "--vm": "single/small_vm",
            "--sr-disk": "auto",
        },
        "paths": ["tests/storage/linstor"],
        "markers": "(small_vm or no_vm) and not reboot and not quicktest",
        "name_filter": "not migration",
    },
    "linstor-migrations": {
        "description": "tests migrations with the linstor storage driver",
        "requirements": [
            "A pool with at least 3 hosts.",
            "An additional free disk on every host.",
            "A second pool with at least 1 host and a SR to receive VMs.",
            "A small VM that can be imported on the SRs.",
        ],
        "nb_pools": 2,
        "params": {
            "--vm": "single/small_vm",
            "--sr-disk": "auto",
        },
        "paths": ["tests/storage/linstor"],
        "markers": "",
        "name_filter": "migration",
    },
    "linstor-reboots": {
        "description": "linstor storage driver tests that involve rebooting hosts",
        "requirements": [
            "A pool with at least 3 hosts, whose master host can be rebooted (best if reboots fast).",
            "An additional free disk on every host.",
            "A small VM that can be imported on the SRs.",
        ],
        "nb_pools": 1,
        "params": {
            "--vm": "single/small_vm",
            "--sr-disk": "auto",
        },
        "paths": ["tests/storage/linstor"],
        "markers": "reboot",
    },
    "linstor-quicktest": {
        "description": "runs `quicktest` on the linstor storage driver`",
        "requirements": [
            "A pool with at least 3 hosts.",
            "An additional free disk on every host.",
        ],
        "nb_pools": 1,
        "params": {
            "--sr-disk": "auto",
        },
        "paths": ["tests/storage/linstor"],
        "markers": "quicktest",
    },

[Nambrok]

You use disks as a list (which it is) but it only give multiple members when called independently.
Meaning --sr-disk=nvme1n1 --sr-disk=nvme2n1 result in ["nvme1n1", "nvme2n1"].
But a call like --sr-disk=nvme1n1,nvme2n1 result in ["nvme1n1,nvme2n1"].
We expect the list on the command line to be given in the latter way, you will need to split the member of disks.
You can look at the hosts fixture that do the same things with its input.

[rushikeshjadhav]

Yes, we can update code and handle disks for --sr-disk=nvme1n1,nvme2n1 scenario.

However, it seemed sr-disk is used in other places as well and did not want to break/alter that for compatibility.
Also, if we are handling multiple disks in one option then the option will become --sr-disks instead of --sr-disk on same line as hosts. And this will cause ripple effect at other places.. The current code will gracefully handle both cases, so can I put a different PR for this (--sr-disks) or you prefer to change in current one?

In jobs.py, "--sr-disk": "auto" is used which lets the current code pick multiple available disks and stay intact.

[Nambrok]

It's mostly used in auto, it's just that if we are expected to give multiple disks to a test, using --sr-disk with comma separated value make more sense.
If we use multiple value, we expect the test to be run multiple time using the values one after the other like it is doing with --vm.
For this case, we don't have to go further than having comma-separated value working on sr-disk for your new fixture and (maybe) ignore the other value after the first disk for the already existing fixtures using sr-disk expecting only one disk.
For renaming the parameter completely, I'm summoning @Wescoeur and @stormi to give their idea on the matter.

[ydirson]

What's the benefit of parsing a comma-separated list, whereas pytest already gives us a way to specify --sr-disk=nvme1n1 --sr-disk=nvme2n1 ?

[ydirson]

The lvm_disks part looks like a good candidate to move into a separate first commit