[pve-devel] [PATCH v1 pve-common 00/18] Introduction of libproxmox-backup-client-perl

Fri Aug 2 15:26:38 CEST 2024

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