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 A70AC1FF15E for ; Fri, 26 Jul 2024 11:52:17 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 2C12C1C16B; Fri, 26 Jul 2024 11:52:17 +0200 (CEST) Message-ID: Date: Fri, 26 Jul 2024 11:52:12 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Max Carrara , Proxmox VE development discussion References: <20240723095624.53621-1-f.ebner@proxmox.com> <20240723095624.53621-11-f.ebner@proxmox.com> <74a748f3-f7ff-4de5-aa1d-3378ae22c642@proxmox.com> Content-Language: en-US From: Fiona Ebner In-Reply-To: X-SPAM-LEVEL: Spam detection results: 0 AWL -0.060 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 URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [plugin.pm] 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" Am 25.07.24 um 17:32 schrieb Max Carrara: > On Thu Jul 25, 2024 at 3:11 PM CEST, Fiona Ebner wrote: >> Am 25.07.24 um 11:48 schrieb Max Carrara: >>> 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`. > I think it's fine to live alongside the base plugin (I'd prefer Plugin::Base if going for a dedicated directory). I agree, if we ever want to add something other than plugins to the top namespace, it is much nicer to have the dedicated directory. And it is made more explicit that things in there are plugins (and not having to name each one FooPlugin). However, I still feel like PVE::BackupProvider::Plugin::Custom::Bar is rather lengthy (should we go with the Custom directory again), but I'm not really opposed to doing it like this. >> >>> 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. > I'm all for going with a similar API age + version mechanism like for the storage plugins, so removing parameters should not be done except for major releases and adding will be backwards-compatible. I don't quite get your point about not needing to update the call sites. If you change the structure of the passed-in hash you still need to do that. I do see some benefit in not needing to add new methods for every mechanism, and there could also be a single backup_hook() method instead of a dedicated one for each phase while we're at it. But changes to the passed-in hashes will still fall under the same restrictions like changing a signature API-wise, so users will be informed via API age + version. >> >> 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. > I would like to keep methods for containers and VMs separate in any case, because they require different things and I don't see any benefit in merging them. For containers, you backup the whole filesystem structure in one go, for VMs you get each disk separately. There are certain parameters that are better fixed, i.e. are passed for every mechanism, e.g. info about idmap for containers, drive ID for VMs. So having them in the signature rather then inside a hash is better and merging won't work if you have different fixed ones. But I'm fine with having a mechanism-agnostic signature, one for VMs and one for containers. > 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 > I do prefer a rigid API. After all you don't want to make life for implementers hard by changing too much between versions. And it can still be a rigid API, even if there's a mechanism-agnostic signature, just keep the data backwards-compatible. _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel