[pve-devel] [RFC pve-storage 00/36] Refactor / Cleanup of Storage Plugins
Max Carrara
m.carrara at proxmox.com
Wed Jul 17 11:39:58 CEST 2024
RFC: Refactor / Cleanup of Storage Plugins
==========================================
I know this looks like a lot of patches and a lot to read up front, but
the changes I want to discuss here are not that intricate. I will
elaborate more in the paragraphs below, which should make it all much
more digestible.
Preface
-------
These patches represent the "current state of affairs" regarding the
refactor and are by no means complete, which is why this is obviously an
RFC instead of an actual patch series.
A lot of commits could probably be merged or split, or perhaps even be
split into multiple smaller series in the future.
The point of this RFC is to get some early feedback in before diving too
deep into the refactor itself or prematurely deciding which solution(s)
to use.
The overall long term goal is to refactor and document the entirety of
the storage API so that third parties can develop their own plugins much
more easily. Once the API has been polished and documented, a separate
plugin is set to be developed and put into its own Debian package that
ought to serve as an example (or perhaps starting point) for plugin
authors.
This RFC is basically the first step towards this goal.
Goals of This Refactor
----------------------
There are three main goals of this refactor / cleanup:
1. Ensure storage plugins only do "plugin stuff" and are completely
independent of one another - no plugin should use features,
helpers, methods, etc. of another plugin. The only exempt of this
is SectionConfig-related functionality, like using properties that
another plugin provides.
The point of this is to ensure that our plugins themselves can be
changed or adapted at will without possibly affecting other plugins. In
other words, plugins should have *high cohesion and low coupling*.
This also means that plugins that inherit from another plugin should be
changed to inherit from the base plugin. The only such case currently is
the ZFSPlugin, which uses ZFSPoolPlugin as base.
2. Expose and document existing helper subroutines and methods so that
they may be reused where desired, so that no code has to depend on
a specific plugin anymore. This applies to the more "useful" / less
niche helpers.
There are a couple instances where other code relies directly on certain
plugins' helpers, thus making those helpers implicitly and
unintentionally part of a public interface.
Exposing and documenting those helpers has the benefit that our code
doesn't need to rely on the existence of a specific plugin anymore,
while at the same time making it easier for ourselves to develop further
plugins with similar functionalities. On top of that, these helpers can
then also be used by third party plugin authors for their own purposes.
3. Make other "less important" helpers, helper methods, etc.
private, so that they do not unintentionally become part of an
implicit public interface.
This is mainly done to further prevent plugin-specific code from
accidentally being used in other places.
Goal 2 and 3 can be summed up as: "What's inside a plugin stays inside
the plugin, but if something is used elsewhere or potentially useful for
others, put it in a separate location and expose it explicitly."
Current Approaches & Reasonings
-------------------------------
The goals above are reflected rather obviously in the series, with a lot
of helpers either being made private or factored into a separate module.
One example of a plugin's functionalities being used by other plugins is
PVE::Storage::DirPlugin, whose methods are also refactored into helper
subroutines. The new helpers are given proper documentation as well as
subroutine prototypes and are substituted into places where the
DirPlugin's methods were being called.
The original helpers still remain in the plugin and instead only wrap
the new ones while also emitting a deprecation warning.
Similar refactorings are done for LVM- and ISCSI-related plugins, as
some of their code is used outside of the plugin it belongs to (or in
the case of LVM, some helpers are even used in `pve-manager`, for
example).
The ZFS-related plugins are only partially refactored at the moment,
mainly because refactoring them specifically is a lot more involved, and
as stated in the preface, I deemed it better to get some feedback in
first before "going all in" on a solution.
Nevertheless, the patches 34, 35, 36 demonstrate how some of the ZFS
helpers could be factored out into a separate module.
The only plugin currently not touched at all is the PBSPlugin - as
mentioned off list, some of the helpers there are also present in
`pve-common` and should go into a separate package in the future.
All helpers that are factored out are put into a new module / namespace:
PVE::Storage::Common. Helpers that can be grouped more specifically by
their functionalities are put into separate sub-packages. In total, the
following new modules are introduced so far:
* PVE::Storage::Common
* PVE::Storage::Common::ISCSI
* PVE::Storage::Common::LVM
* PVE::Storage::Common::Path
* PVE::Storage::Common::ZFS
Each module comes with documentation written in Perl's POD format, which
can be rendered via the `perldoc` CLI or be viewed via an LSP.
Open Questions
--------------
This is a non-exhaustive list of questions that arose during
development:
1. Instead of making some helpers private and some explicitly public
in a new module, should all helpers be made public instead? There
could be third-party plugins out there using some of the helpers
that were made private that we do not know of.
2. Because the helpers of the PBSPlugin (which are also in
`pve-common`) are expected to be put into a separate package in the
future, should the helpers that are factored out into
PVE::Storage::Common and its sub-packages also be put into a
separate package or moved to an existing one?
3. What would be the best way to keep track of deprecated helpers so
that we know when to remove them?
4. Because the refactored helpers (those in PVE::Storage::Common etc.)
are public, should they fall under the same API guarantees as
PVE::Storage? Should there be a different mechanism instead?
5. What would be the best way to split these changes up into multiple
smaller series (so that e.g. reviewing becomes easier)?
My current idea would be to put the following changes into separate
series:
* Making other plugins independent of the DirPlugin
* Factoring out LVM-related helpers
* Factoring out ISCSI-related helpers
* Factoring out ZFS-related helpers
6. Are there any alternatives to the current approach(es) that might
be better than what's currently in this RFC?
Closing Thoughts
----------------
Please don't hesitate to let me know what you think - and thank you very
much for reading this far :) Even if it's just a minor thing, any
feedback is greatly appreciated.
Summary of Changes
------------------
Max Carrara (36):
plugin: base: remove old fixme comments
plugin: btrfs: make plugin-specific helpers private
plugin: cephfs: make plugin-specific helpers private
api: remove unused import of CIFS storage plugin
plugin: cifs: make plugin-specific helpers private
api: remove unused import of LVM storage plugin
common: introduce common module
plugin: dir: move helper subs of directory plugin to common modules
plugin: lvm: move LVM helper subroutines into separate common module
api: replace usages of deprecated LVM helper subs with new ones
plugin: lvmthin: replace usages of deprecated LVM helpers with new
ones
plugin: lvmthin: move helper that lists thinpools to common LVM module
common: lvm: update code style
api: replace usages of deprecated LVM thin pool helper sub
plugin: btrfs: replace deprecated helpers from directory plugin
plugin: dir: factor storage methods into separate common subs
plugin: dir: factor path validity check into helper methods
plugin: btrfs: remove dependency on directory plugin
plugin: cifs: remove dependency on directory plugin
plugin: cephfs: remove dependency on directory plugin
plugin: nfs: remove dependency on directory plugin
plugin: btrfs: make helper methods private
plugin: esxi: make helper subroutines private
plugin: esxi: remove unused helper subroutine `query_vmdk_size`
plugin: esxi: make helper methods private
plugin: gluster: make helper subroutines private
plugin: iscsi-direct: make helper subroutine `iscsi_ls` private
plugin: iscsi: factor helper functions into common module
plugin: iscsi: make helper subroutine `iscsi_session` private
plugin: lvm: update definition of subroutine `check_tags`
plugin: lvmthin: update definition of subroutine `activate_lv`
plugin: nfs: make helper subroutines private
plugin: rbd: update private sub signatures and make helpers private
common: zfs: introduce module for common ZFS helpers
plugin: zfspool: move helper `zfs_parse_zvol_list` to common module
plugin: zfspool: refactor method `zfs_request` into helper subroutine
src/PVE/API2/Disks/LVM.pm | 12 +-
src/PVE/API2/Disks/LVMThin.pm | 14 +-
src/PVE/API2/Storage/Config.pm | 2 -
src/PVE/API2/Storage/Scan.pm | 6 +-
src/PVE/Storage.pm | 4 +-
src/PVE/Storage/BTRFSPlugin.pm | 105 +++---
src/PVE/Storage/CIFSPlugin.pm | 53 ++-
src/PVE/Storage/CephFSPlugin.pm | 47 ++-
src/PVE/Storage/Common.pm | 303 ++++++++++++++++
src/PVE/Storage/Common/ISCSI.pm | 475 ++++++++++++++++++++++++
src/PVE/Storage/Common/LVM.pm | 515 +++++++++++++++++++++++++++
src/PVE/Storage/Common/Makefile | 10 +
src/PVE/Storage/Common/Path.pm | 124 +++++++
src/PVE/Storage/Common/ZFS.pm | 196 ++++++++++
src/PVE/Storage/DirPlugin.pm | 146 +++-----
src/PVE/Storage/ESXiPlugin.pm | 46 +--
src/PVE/Storage/GlusterfsPlugin.pm | 16 +-
src/PVE/Storage/ISCSIDirectPlugin.pm | 2 +-
src/PVE/Storage/ISCSIPlugin.pm | 257 +------------
src/PVE/Storage/LVMPlugin.pm | 258 ++++----------
src/PVE/Storage/LvmThinPlugin.pm | 51 ++-
src/PVE/Storage/Makefile | 1 +
src/PVE/Storage/NFSPlugin.pm | 45 ++-
src/PVE/Storage/Plugin.pm | 19 -
src/PVE/Storage/RBDPlugin.pm | 84 ++---
src/PVE/Storage/ZFSPoolPlugin.pm | 157 ++++----
26 files changed, 2107 insertions(+), 841 deletions(-)
create mode 100644 src/PVE/Storage/Common.pm
create mode 100644 src/PVE/Storage/Common/ISCSI.pm
create mode 100644 src/PVE/Storage/Common/LVM.pm
create mode 100644 src/PVE/Storage/Common/Makefile
create mode 100644 src/PVE/Storage/Common/Path.pm
create mode 100644 src/PVE/Storage/Common/ZFS.pm
--
2.39.2
More information about the pve-devel
mailing list