storage: add dynamic managed storage dispatcher#442
Conversation
sp-viktori
requested review from
aesteve-rh,
almusil,
mz-pdm and
tinez
as code owners
sp-viktori
force-pushed
the
managed-storage-dispatcher
branch
from
90d1601 to
394fa30
Compare
sp-viktori
force-pushed
the
managed-storage-dispatcher
branch
2 times, most recently
from
5cc41fc to
d0a34b5
Compare
sp-viktori
force-pushed
the
managed-storage-dispatcher
branch
from
d0a34b5 to
b026231
Compare
| description: Adapter vendor | ||
| name: adapter | ||
| type: string | ||
| added: '4.5.6' |
There was a problem hiding this comment.
4.5.8 it would be if merged now
There was a problem hiding this comment.
thanks for catching this, updated
| the default `cinderlib-client.py` helper. | ||
|
|
||
| The vendor packaging is expected to install a symlink in | ||
| `/usr/share/ovirt-engine/cinderlib/` named `{adapter}-adapter`. |
There was a problem hiding this comment.
Just in doubt if this is the right directory to put this in.
/usr/share/ovirt-engine/managedvolume/ perhaps and move cinderlib into it?
There was a problem hiding this comment.
Right, I've changed this to reflect the oVirt Engine PR (the updated proposal indeed uses a managedblock directory instead of cinderlib).
|
|
||
| try: | ||
| attachment = run_helper("attach", connection_info) | ||
| attachment = run_helper("attach", connection_info, connection_info.get("adapter")) |
There was a problem hiding this comment.
This is useless? As you pass connection_info, you can also get the adapter in the run_helper function then.
There was a problem hiding this comment.
In this case, yes, but the run_helper function has a union type: in this case it is connection_info but when calling for detach it is vol_info. Good news is that vol_info has connection_info as a field (which has adapter) but it becomes very convoluted to figure it out.
I'll think on simplifying the whole thing while keeping it robust, and push an update soon.
There was a problem hiding this comment.
I've pushed a different approach, and also split it in two:
- A change in how managedvolume-helper is called that makes the protocol a bit stricter (so there is no ambiguity what is sent as input: it is always volume_info struct).
- The adapter dispatch which now knows exactly where to look for the adapter field.
sp-viktori
force-pushed
the
managed-storage-dispatcher
branch
2 times, most recently
from
25159ca to
ce5b7f0
Compare
sp-viktori
force-pushed
the
managed-storage-dispatcher
branch
from
ce5b7f0 to
a87042d
Compare
| @@ -0,0 +1,48 @@ | |||
| <!-- | |||
| SPDX-FileCopyrightText: Red Hat, Inc. | |||
There was a problem hiding this comment.
You can change this to 'oVirt Developers'.
| helper = HELPER | ||
| if adapter: | ||
| helper = f"{HELPER}-{adapter}" | ||
| if not os.path.exists(helper): |
There was a problem hiding this comment.
What if the file exists but is non-executable?
There was a problem hiding this comment.
Added executable check as well, works with symlinks in my tests.
| f"Helper for adapter '{adapter}' not found" | ||
| f" at '{helper}'") | ||
| # This is the only sane way to run python scripts that work with both | ||
| # python2 and python3 in the tests. |
There was a problem hiding this comment.
Perhaps unrelated to this PR, but we dropped python2 support.
So if the execution can be improved, feel free :)
There was a problem hiding this comment.
Oof this is actually related now that you've pointed it out. My assumption (and the documentation) say that this helper can be anything but the way it's been run (for Python2 compatibility) is to hand it over to the Python executable as a script ($python helper basically). This means that it must have been a python script with the same version of python as Vdsm.
I'll remove the Python2 workaround and execute it directly, also checking for the executable bit. Need to run tests locally again to make sure it works (and the legacy cinderlib helper also). Will push when ready.
There was a problem hiding this comment.
Alright, did some more test this time and also ran OST locally with the RPMs from my Vdsm branch as well as the RPMs from my oVirt engine branch.
Removing the Python 2 workaround is OK and allows vendors to use non-Python adapters/helpers as well.
sp-viktori
force-pushed
the
managed-storage-dispatcher
branch
2 times, most recently
from
bec847e to
70a4280
Compare
Make the protocol used to communicate with the managed volume helper scripts stricter. Previously, the input could be: - when attaching: connection_info struct; - when detaching: volume_info struct which contains connection_info, attachment, path, etc.). Now it is always a volume_info struct, and in the case when we're attaching the struct only has the connection_info field populated. Signed-off-by: Viktor Ivanov <viktor.ivanov@storpool.com>
Add an optional "adapter" field in the ManagedVolumeConnection struct, allowing dispatching to vendor managedvolume-helper adapters. Drop the Python 2 workaround when running the helper, also dropping the requirement for it (and thus the vendor-provided adapters) to be a Python script. Signed-off-by: Viktor Ivanov <viktor.ivanov@storpool.com>
sp-viktori
force-pushed
the
managed-storage-dispatcher
branch
from
70a4280 to
e6c7c1e
Compare
|
/ost |
|
⏳ Running ost suite 'basic-suite-master' on distro 'centos9'. Follow the progress here. |
|
😎💪 ost suite 'basic-suite-master' on distro 'centos9' finished successfully. (details) |
2 participants
Dispatch mechanism that third-party storage vendors can use to redirect managed storage operations.
Add an optional
adapterfield in theManagedVolumeConnectionstruct.If there is an
adapterfield, direct storage requests tomanagedvolume-helper-{adapter}, otherwise use the default (os-brick) implementation.Works together with a simplar PR in ovirt-engine that allows redirection of the rest of the managed storage operations.