From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 08A401FF177 for ; Fri, 2 Aug 2024 15:26:59 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id F0085D976; Fri, 2 Aug 2024 15:27:01 +0200 (CEST) From: Max Carrara To: pve-devel@lists.proxmox.com Date: Fri, 2 Aug 2024 15:26:38 +0200 Message-Id: <20240802132656.270077-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.373 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 RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. 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. [pbsclient.pm, proxmox.com] Subject: [pve-devel] [PATCH v1 pve-common 00/18] Introduction of libproxmox-backup-client-perl 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" Introduction of libproxmox-backup-client-perl - v1 ================================================== Description ----------- This series' main goal is to split the `PVE::PBSClient` Perl module from the `libpve-common-perl` Debian package while improving and documenting its code. The new `libproxmox-backup-client-perl` Debian package is introduced at the end of this series. While this looks like a lot, most patches, except patch 08 which documents the `PVE::PBSClient` module, are rather small (tiny, even) and rather localized (for the most part). Code Style Changes: Patches 01 - 07 *********************************** The patches 01 - 07 contain a handful of smaller changes regarding code style and readability. Most of them, except patch 01, should be able to be dropped if they're not desired or deemed unnecessary / too pedantic (though I haven't tested this). If desired, those patches can also be squashed into one or more larger commits. Documentation: Patch 08 *********************** Patch 08 provides some basic documentation on the `PVE::PBSClient` module overall and adds docstrings for each public subroutine / method. This is done mostly because a lot of the module's capabilities / functionalities / invariants aren't really obvious - at best, some of the behaviour is mentioned in a comment here or there, or has to be deciphered from the private method's code itself. Consumers of this module, in my opinion, shouldn't have to figure all of these things out themselves, so patch 08 was made to address this. After all, a couple minutes of reading documentation can save hours of debugging and discovering things through trial-and-error. Functional Changes: Patches 09 - 16 *********************************** The patches 09 - 16 contain various improvements regarding the module's functionalities themselves, aiming to make *some* things here and there a little more ergonimic. For example, the password and encryption key file setter methods currently don't create the directory in which the files are stored in recursively (`mkdir` vs `mkdir -p`), which is addressed in patch 09. Methods that shouldn't actually return anything (and yet return `0` if the underlying `run_command` call was successful) are also made to explicitly not return anything. While changes like these might also seem somewhat pedantic, they prevent consumers of the module from accidentally / inadvertedly rely on behaviour that isn't documented or even intended, especially in the case of Perl's implicit returns. Note that because none of our code uses any of the methods (only the `get_repository` function is used), some of these changes deliberately "break API" here - figured it would be a good opportunity to while we still have the chance. Any recommendations in that regard are therefore welcome. Package Split: Patches 17 and 18 ******************************** Finally, patches 17 and 18 make it possible to move `PVE::PBSClient` into its own Debian package. Patch 17 introduces the underlying support for building multiple Debian packages, updating the root `Makefile` to support / be aware of multiple binary packages. Patch 18 then updates `debian/control` and adds a `.install` file for both the old and the new package. Note Regarding Version Bumps ---------------------------- The very last patch, patch 18, contains a placeholder version `X.Y.Z` in the changes added to the `debian/control` file. This placeholder must be substituted with the desired version when applying the patch or bumping the package. Note that the same version must be used for all `X.Y.Z` placeholders, though I assume that this is known already. There is only one package that uses `PVE::PBSClient`, which is `qemu-server`. This package requires `libproxmox-backup-client-perl` as dependency, should this series be applied. Note Regarding API Breakage --------------------------- As mentioned in the section regarding patches 09 - 16 above, some methods' behaviours are deliberately changed in order to be more developer-friendly. Because almost none of our code depends on the `PVE::PBSClient` module, this package split is a good opportunity for further changes in that regard. A lot of the module's functionality also exists in similar (or even duplicate) helpers in `libpve-storage` [1], so a lot of changes and additional features could be implemented before replacing said helpers with this module. That way we'd have a mostly convenient (for lack of a better term) way to decouple other packages' dependency on "internals" of the `PVE::Storage::PBSPlugin` module. ("Internals" that have now become part of an implicit or perhaps inadvertedly created public API.) Because all this also directly ties into #3186 [2], I can also provide any such changes in a separate series or incorporate them all in another version of this series - I'm happy for any feedback or suggestions in that regard. Testing ------- Module Methods ************** The module's methods were smoke-tested with a local script that I executed on my dev VM. I created a new user on my PBS VM that the script uses. What was tested: * Creating new password and encryption key files in a separate secret directory (in order to not mess with my VM's installation) * Querying the datastore's status * Listing snapshots * Creating and uploading a new PXAR backup from a random directory I had laying around * Querying the snapshots again and filtering out the newly created snapshot of the PXAR backup, then extracting the backed up archive to a new directory again * Forgetting the newly snapshot after its extraction * Downloading files from specific backups via the `file_restore_extract_prepare` and `file_restore_extract` methods This does however not cover all methods / cases, so I'd be grateful for anyone testing this series. The docs should hopefully be enough to get you started - if they're not, please let me know, because that means they need improvements ;) Package Split ************* The package split was tested by substituting the `X.Y.Z` placeholder in `debian/control` with a higher minor version and adding a corresponding bump in `debian/changelog` with `dch -i`. The two packages built successfully and were installed on my dev VM. * Trying to install only the `libproxmox-backup-client-perl` package results in `apt` refusing to perform the installation, as expected. * Installing both packages succeeds, as expected, and did not caus any noticable issues. * Downgrading to the previous version of `libpve-common-perl` also causes `libproxmox-backup-client-perl` to be uninstalled, as expected. For good measure I also double-checked if the Perl module was correctly moved to the new package by comparing both `.deb` files via `dpkg -c` and running `dpkg -S /usr/share/perl5/PVE/PBSClient.pm` on my dev VM. References ---------- [1]: https://git.proxmox.com/?p=pve-storage.git;a=blob;f=src/PVE/Storage/PBSPlugin.pm;h=0808bccd79a75a6ace1c7194b30e894f5db54659;hb=refs/heads/master#l75 [2]: https://bugzilla.proxmox.com/show_bug.cgi?id=3186 Summary of Changes ------------------ Max Carrara (18): pbsclient: rename 'sdir' parameter of constructor to 'secret_dir' pbsclient: use parentheses when calling most inbuilts pbsclient: use post-if definedness checks instead of '//=' operator pbsclient: pull variable out of long post-if definedness check pbsclient: use cond. statements instead of chained 'or' operators pbsclient: use spaces around list braces and parens around ternaries pbsclient: s/foreach/for pbsclient: document package and its public functions & methods pbsclient: create secret dir with `mkdir -p` and mode `700` pbsclient: use `File::Spec->catfile` to concatenate file paths pbsclient: let `status` method return a hash instead of an array pbsclient: throw exception if username of client has no realm pbsclient: make method `password_file_name` public pbsclient: prohibit implicit return pbsclient: don't return anything in PXAR methods pbsclient: don't return anything in `forget_snapshot` make: support building multiple packages from the same source deb: split PBSClient.pm into new package libproxmox-backup-client-perl Makefile | 27 +- debian/control | 12 + debian/libproxmox-backup-client-perl.install | 1 + debian/libpve-common-perl.install | 28 + src/PVE/PBSClient.pm | 670 ++++++++++++++++--- 5 files changed, 651 insertions(+), 87 deletions(-) create mode 100644 debian/libproxmox-backup-client-perl.install create mode 100644 debian/libpve-common-perl.install -- 2.39.2 _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel