From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 232E11FF2C8 for ; Wed, 17 Jul 2024 11:40:40 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id ED83C38026; Wed, 17 Jul 2024 11:41:08 +0200 (CEST) From: Max Carrara To: pve-devel@lists.proxmox.com Date: Wed, 17 Jul 2024 11:39:58 +0200 Message-Id: <20240717094034.124857-1-m.carrara@proxmox.com> X-Mailer: git-send-email 2.39.2 MIME-Version: 1.0 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.622 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_ASCII_DIVIDERS 0.8 Email that uses ascii formatting dividers and possible spam tricks KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [storage.pm, cifsplugin.pm, esxiplugin.pm, path.pm, lvmplugin.pm, common.pm, config.pm, lvmthinplugin.pm, zfspoolplugin.pm, nfsplugin.pm, lvm.pm, iscsi.pm, zfs.pm, iscsiplugin.pm, rbdplugin.pm, btrfsplugin.pm, cephfsplugin.pm, plugin.pm, dirplugin.pm, iscsidirectplugin.pm] URI_NOVOWEL 0.5 URI hostname has long non-vowel sequence Subject: [pve-devel] [RFC pve-storage 00/36] Refactor / Cleanup of Storage Plugins X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "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