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 010281FF15D for ; Thu, 25 Jul 2024 17:32:56 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id D849D88C9; Thu, 25 Jul 2024 17:32:53 +0200 (CEST) Mime-Version: 1.0 Date: Thu, 25 Jul 2024 17:32:49 +0200 Message-Id: To: "Fiona Ebner" , "Proxmox VE development discussion" From: "Max Carrara" X-Mailer: aerc 0.17.0-72-g6a84f1331f1c References: <20240723095624.53621-1-f.ebner@proxmox.com> <20240723095624.53621-11-f.ebner@proxmox.com> <74a748f3-f7ff-4de5-aa1d-3378ae22c642@proxmox.com> In-Reply-To: <74a748f3-f7ff-4de5-aa1d-3378ae22c642@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.029 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_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 Subject: Re: [pve-devel] [RFC storage 10/23] plugin: introduce new_backup_provider() method 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" On Thu Jul 25, 2024 at 3:11 PM CEST, Fiona Ebner wrote: > Am 25.07.24 um 11:48 schrieb Max Carrara: > > On Tue Jul 23, 2024 at 11:56 AM CEST, Fiona Ebner wrote: > >> Signed-off-by: Fiona Ebner > > > > Some overall thoughts: > > > > 1. I'm really, really happy to see documentation in this module here, > > that's fantastic! :) > > > > While the contents of the docs seem fine, I would suggest you used > > POD instead. You can find an example in one of my recent series. [1] > > I mainly prefer POD solely because it's what Perl uses; it also > > indirectly makes sure we all use the same kind of format for > > documenting our Perl code. > > > > Of course, we've currently not decided on any particular format, but > > because the opportunity arose, I wanted to pitch POD here > > nevertheless. ;) > > > > I'll look into it for v2. Agreed, following a standard for documenting > an API module has its merits. Sweet! > > > 2. I would personally prefer a namespace like `PVE::Backup::Provider` > > instead of `PVE::BackupProvider`, simply because it leaves room for > > further packages and reduces churn in the long term, IMO. > > > > There's a risk though that PVE::Backup::Provider and PVE::Backup::Foo > are unrelated things that have no real business sharing a namespace. Hmm, fair point - on second thought, `PVE::Backup` indeed seems a bit too generic. > > > The same goes for backup provider plugins - IMO namespacing them > > like e.g. `PVE::Backup::Provider::Plugin::Foo` where `Foo` is a > > (concrete) plugin. > > > > The BackupProvider namespace is already intended for the plugins, adding > an extra level with "Plugin" would just bloat the module names, > especially if we decide to go the same route as for storage plugins and > have a "Custom" sub-namespace. I understand what you mean, yeah. Would perhaps something like `PVE::BackupProvider::Plugin::*` be better? The reason why I'm suggesting this is because in `PVE::Storage::*`, every plugin lives alongside `Plugin.pm`, even though the extra directory wouldn't really hurt IMO. E.g. `PVE::Storage::DirPlugin` would then be `PVE::Storage::Plugin::Dir`. The reason why I'm suggesting *something* like that here is to reduce some clutter and simply keep related things grouped. Also, IMO it's better to consider the overall package structure beforehand, simply to avoid any churn in the future - something I've noticed while poking around the storage API. Maybe I'm being a bit pedantic here (tbh I probably am), but it's just something that I wanted to mention anyhow. > > > While this seems long or somewhat excessive, I think it enforces a > > clear package / module hierarchy and keeps things tidier in the long > > term, and those couple extra keystrokes don't really hurt anyone. > > > > I get where you're coming from, I just feel like BackupProvider might be > better as its own separate thing, containing the plugins for the > specific purpose. But I don't have a strong opinion about it, and am > fine making such changes if other developers prefer it too :) I agree now that BackupProvider should remain on its own; I otherwise don't have any strong opinions about it either (though I would like to shove plugins one directory level deeper ;P). As I said, I'm probably just a bit pedantic here; feel free to disregard these suggestions if you think they're not applicable :) > > > The above two methods - `backup_nbd` and `backup_directory` - is there > > perhaps a way to merge them? I'm not sure if what I'm having in mind > > here is actually feasible, but what I mean is "making the method > > agnostic to the type of backup". As in, perhaps pass a hash that > > contains a `type` key for the type of backup being made, and instead of > > having long method signatures, include the remaining parameters as the > > remaining keys. For example: > > > > { > > 'type' => 'lxc-dir', # type names are just examples here > > 'directory' => '/foo/bar/baz', > > 'bandwidth_limit' => 42, > > ... > > } > > > > { > > 'type' => 'vm-nbd', > > 'device_name' => '...', > > 'nbd_path' => '...', > > ... > > } > > > > You get the point :P > > > > IMO it would make it easier to extend later, and also make it more > > straightforward to introduce new parameters / deprecate old ones, while > > the method signature stays stable otherwise. > > > > The same goes for the different cleanup methods further down below; > > instead of having a separate method for each "type of cleanup being > > performed", let the implementor handle it according to the data the > > method receives. > > > > IMHO I think it's best to be completely agnostic over VM / LXC backups > > (and their specific types) wherever possible and let the data describe > > what's going on instead. > > > > The point about extensibility is a good one. The API wouldn't need to > change even if we implement new mechanisms. But thinking about it some > more, is there anything really gained? Because we will not force plugins > to implement the methods for new mechanisms of course, they can just > continue supporting what they support. Each mechanism will have its own > specific set of parameters, so throwing everything into a catch-all > method and hash might make it too generic. The main point is indeed extensibility, but it also makes maintaining the API a bit easier. Should we (in the future) decide to add or remove any parameters, we don't need to touch the signature - and in turn, we don't need to tediously `grep` for every call site to ensure that they're updated accordingly. With hashes one could instead always just check if the required arguments have been provided. > > Or think about the documentation for the single backup method: it would > become super lengthy and describe all backup mechanisms, while a plugin > most likely only cares about a single one and would have an easier time > with a method that captures that mechanism's parameters explicitly. > Won't the end result be making the implementors life slightly harder, > because it first needs to extract the parameters for the specific mechanism? Yes, I agree - this does create a bit of a double-edged sword - implementors are responsible for handling the hash correctly; of course they could lob it all into one generic method and call it a day, or they could introduce a separate helper function for each `type`, for example. The up- and downside of a generic method would be that it's up to the implementor on how to deal with it. At the same time, it would allow their plugin to handle different API versions a bit easier as well, because the method signature wouldn't change - only the data changes. If they wanted their plugin to support multiple API versions all at once, they could certainly do it that way and aren't restricted by a fixed set of parameters. Now that I've given it some more thought, there are quite a bunch of ups and downs, though I'm personally still in favour of the more generic method, as it would reduce maintenance cost in the long run, IMO for both us and implementors. The initial cost of adding the parameter extraction / handling would be higher, I agree, but I feel like it's more worth in the long run. Also, IMO lengthy documentation is better than having a rigid API ;P > > > For the specific types we can always then provide helper functions that > > handle common cases that implementors can use. > > > > Extending on my namespace idea above, those helpers could then land in > > e.g. `PVE::Backup::Provider::Common`, `PVE::Backup::Provider::Common::LXC`, > > etc. > > > > Could you give an example for such a helper? I rather feel like things > like libnbd will be that, i.e. for accessing the NBD export, not sure if > we can create common helpers that would benefit multiple providers, each > has their own backend they'll need to talk to after all and might have > quite different needs. Fair point! I agree with you; disregard this, then. :P _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel