public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [RFC pve-storage 00/36] Refactor / Cleanup of Storage Plugins
@ 2024-07-17  9:39 Max Carrara
  2024-07-17  9:39 ` [pve-devel] [RFC pve-storage 01/36] plugin: base: remove old fixme comments Max Carrara
                   ` (35 more replies)
  0 siblings, 36 replies; 39+ messages in thread
From: Max Carrara @ 2024-07-17  9:39 UTC (permalink / raw)
  To: pve-devel

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



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 39+ messages in thread

end of thread, other threads:[~2024-07-18  7:43 UTC | newest]

Thread overview: 39+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-07-17  9:39 [pve-devel] [RFC pve-storage 00/36] Refactor / Cleanup of Storage Plugins Max Carrara
2024-07-17  9:39 ` [pve-devel] [RFC pve-storage 01/36] plugin: base: remove old fixme comments Max Carrara
2024-07-17 16:02   ` Thomas Lamprecht
2024-07-18  7:43     ` Max Carrara
2024-07-17  9:40 ` [pve-devel] [RFC pve-storage 02/36] plugin: btrfs: make plugin-specific helpers private Max Carrara
2024-07-17  9:40 ` [pve-devel] [RFC pve-storage 03/36] plugin: cephfs: " Max Carrara
2024-07-17  9:40 ` [pve-devel] [RFC pve-storage 04/36] api: remove unused import of CIFS storage plugin Max Carrara
2024-07-17  9:40 ` [pve-devel] [RFC pve-storage 05/36] plugin: cifs: make plugin-specific helpers private Max Carrara
2024-07-17  9:40 ` [pve-devel] [RFC pve-storage 06/36] api: remove unused import of LVM storage plugin Max Carrara
2024-07-17  9:40 ` [pve-devel] [RFC pve-storage 07/36] common: introduce common module Max Carrara
2024-07-17  9:40 ` [pve-devel] [RFC pve-storage 08/36] plugin: dir: move helper subs of directory plugin to common modules Max Carrara
2024-07-17  9:40 ` [pve-devel] [RFC pve-storage 09/36] plugin: lvm: move LVM helper subroutines into separate common module Max Carrara
2024-07-17  9:40 ` [pve-devel] [RFC pve-storage 10/36] api: replace usages of deprecated LVM helper subs with new ones Max Carrara
2024-07-17  9:40 ` [pve-devel] [RFC pve-storage 11/36] plugin: lvmthin: replace usages of deprecated LVM helpers " Max Carrara
2024-07-17  9:40 ` [pve-devel] [RFC pve-storage 12/36] plugin: lvmthin: move helper that lists thinpools to common LVM module Max Carrara
2024-07-17  9:40 ` [pve-devel] [RFC pve-storage 13/36] common: lvm: update code style Max Carrara
2024-07-17  9:40 ` [pve-devel] [RFC pve-storage 14/36] api: replace usages of deprecated LVM thin pool helper sub Max Carrara
2024-07-17  9:40 ` [pve-devel] [RFC pve-storage 15/36] plugin: btrfs: replace deprecated helpers from directory plugin Max Carrara
2024-07-17  9:40 ` [pve-devel] [RFC pve-storage 16/36] plugin: dir: factor storage methods into separate common subs Max Carrara
2024-07-17  9:40 ` [pve-devel] [RFC pve-storage 17/36] plugin: dir: factor path validity check into helper methods Max Carrara
2024-07-17  9:40 ` [pve-devel] [RFC pve-storage 18/36] plugin: btrfs: remove dependency on directory plugin Max Carrara
2024-07-17  9:40 ` [pve-devel] [RFC pve-storage 19/36] plugin: cifs: " Max Carrara
2024-07-17  9:40 ` [pve-devel] [RFC pve-storage 20/36] plugin: cephfs: " Max Carrara
2024-07-17  9:40 ` [pve-devel] [RFC pve-storage 21/36] plugin: nfs: " Max Carrara
2024-07-17  9:40 ` [pve-devel] [RFC pve-storage 22/36] plugin: btrfs: make helper methods private Max Carrara
2024-07-17  9:40 ` [pve-devel] [RFC pve-storage 23/36] plugin: esxi: make helper subroutines private Max Carrara
2024-07-17  9:40 ` [pve-devel] [RFC pve-storage 24/36] plugin: esxi: remove unused helper subroutine `query_vmdk_size` Max Carrara
2024-07-17  9:40 ` [pve-devel] [RFC pve-storage 25/36] plugin: esxi: make helper methods private Max Carrara
2024-07-17  9:40 ` [pve-devel] [RFC pve-storage 26/36] plugin: gluster: make helper subroutines private Max Carrara
2024-07-17  9:40 ` [pve-devel] [RFC pve-storage 27/36] plugin: iscsi-direct: make helper subroutine `iscsi_ls` private Max Carrara
2024-07-17  9:40 ` [pve-devel] [RFC pve-storage 28/36] plugin: iscsi: factor helper functions into common module Max Carrara
2024-07-17  9:40 ` [pve-devel] [RFC pve-storage 29/36] plugin: iscsi: make helper subroutine `iscsi_session` private Max Carrara
2024-07-17  9:40 ` [pve-devel] [RFC pve-storage 30/36] plugin: lvm: update definition of subroutine `check_tags` Max Carrara
2024-07-17  9:40 ` [pve-devel] [RFC pve-storage 31/36] plugin: lvmthin: update definition of subroutine `activate_lv` Max Carrara
2024-07-17  9:40 ` [pve-devel] [RFC pve-storage 32/36] plugin: nfs: make helper subroutines private Max Carrara
2024-07-17  9:40 ` [pve-devel] [RFC pve-storage 33/36] plugin: rbd: update private sub signatures and make helpers private Max Carrara
2024-07-17  9:40 ` [pve-devel] [RFC pve-storage 34/36] common: zfs: introduce module for common ZFS helpers Max Carrara
2024-07-17  9:40 ` [pve-devel] [RFC pve-storage 35/36] plugin: zfspool: move helper `zfs_parse_zvol_list` to common module Max Carrara
2024-07-17  9:40 ` [pve-devel] [RFC pve-storage 36/36] plugin: zfspool: refactor method `zfs_request` into helper subroutine Max Carrara

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal