public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: Thomas Lamprecht <t.lamprecht@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
	Fiona Ebner <f.ebner@proxmox.com>,
	Max Carrara <m.carrara@proxmox.com>
Subject: Re: [pve-devel] [PATCH v2 pve-common 00/12] Introduce and Package PVE::Path & PVE::Filesystem
Date: Fri, 3 Jan 2025 11:41:10 +0100	[thread overview]
Message-ID: <66a4dca4-d3d7-4def-8f25-e23a2fcaefe5@proxmox.com> (raw)
In-Reply-To: <c7698f04-5e13-413c-90bc-1332327bd779@proxmox.com>

On 03/01/2025 10:49, Fiona Ebner wrote:
> Am 02.01.25 um 16:54 schrieb Max Carrara:
> That is a good point. I agree this is also worth discussing. Do we want
> to put all bindings for PVE there or do we want to have multiple
> libraries for binding? Moving forward, more and more Perl packages will
> need to depend on pve-rs or the other binding libraries in any case, so
> I wouldn't consider that to be a blocker for the path library here.

That we'll depending solely on pve-rs as central dependency is IMO not set
in stone, having one huge library, even if just temporarily for a transition
period, is IME a rather big PITA fast, besides, the period won't be short
but rather a few years I think, so just because it's temporary doesn't mean
we should ignore costs or potential tech debt.

>> 4. Should we decide to use perlmod here eventually, individual functions
>> can still be implemented in Rust separately. Right now, there wasn't
>> really a need to use Rust, because PVE::Path works at most with strings
>> and a couple arrays here and there; there are no complex data structures
>> that need to be made typesafe.
> 
> The motivation for my suggestion is not about type-safety, but about
> doing the groundwork for the future. Since we can call Rust from Perl
> but not the other way around, we need to start with the low-level
> modules to use Rust. If we go for it, I'd rather have all functions in
> the new libraries be wrappers and as transparent as possible, so that a
> module that uses the path functions can be converted to Rust more easily
> in the future.

I'm not sure if replacing from low-level helper upwards is the best choice,
as for one those helpers exist since a long time and are for most pretty
stable nowadays, replacing that means a lot of churn for almost no benefit.
Especially as you add extra cost now with a new perlmod backed interface
for code that won't then be used in the future when we actually do not
require 

This is not thought out 100% yet, but as of now I'd favor rewriting those
parts that actually provide value, and that can be plugged in in various
ways into the existing perl code:

- reusing our existing proxying for the API to route to a new fully rust
  based daemon. As of now that could already hosts a lot of the shared API
  and implementations parts that we got for PBS and PDM already anyway;
  albeit, PVE is sometimes a bit special (longer grown history) and might
  need more care to ensure backward compat, but that's details.
  This is basically a replacement for perlmod for whole API endpoints or
  even directories; it certainly is a bit bigger w.r.t. initial work
  required to get permission handling and locking right, but as most of
  that uses file locks or pmxcfs and works already for multiple processes,
  not just multiple threads, it should be doable.

- using IPC to call from perl into rust, through, e.g., UNIX sockets or
  more elaborate existing RPC methods. The storage system would be a prime
  candidate here. As there we will require rather permanent backwards
  compat for the perl based plugins, so we require a system that can handle
  that already; a storage daemon that calls into perl plugins via a wrapper
  executable and allows rewriting ours one by one would be nice, and it
  would provide one of the internal core APIs in rust, which would unlock
  rewriting code where rust could bring actually a big benefit by having
  modern language features and rust guarantees. And once the storage client
  system (not the disk management system that has been wrongly mixed into
  the same code basis) has a ABI available from rust we unlock a lot of
  things to rewrite in modules, one by one.

- perlmod. Yes, for some things it will be still fine, but IMO it should
  be rather used to plug in a smaller set of methods for bigger pieces of
  rust code/rewrites, not dozens of wrappers for trivial (helper, utility)
  methods; at the moment I just do not see much value in there.

With that we are much more modular in terms of switching out stuff compared
to preparing some low-level stuff first. Having code with side-effects
shared can be nice to ensure implementations match though, but that's mostly
locking and permissions and owner on file creation, but lots of that is
handled by pmxcfs already on PVE side and the rest really should be testable,
as it just has to be implemented correctly once.

>>>> You state that you (also) took inspiration from Rust's `std::path` so
>>>> could we just use that itself, wrapping via perlmod? Or would the
>>>> wrapping be too ugly here or lead to performance issues?
>>
>> 5. I'm not sure about the performance overhead, but it would certainly
>> be somewhat ugly, because all which PVE::Path essentially does consists
>> of string and array operations. If we used perlmod here hypothetically,
>> all that we'd be doing is give the Rust side a string or an array,
>> convert that to a PathBuf / Path or an iterable, perform the requested
>> operation and give the result back to Perl. It just seems a little
>> unnecessary to me.
>>
>> 6. I reckon that the places in which those two little libraries here
>> will be used will most likely be replaced by a pure Rust implementation
>> as a whole -- IMO there's no need to use perlmod for every single
>> smaller library if the Perl code using them gets replaced by Rust.
> 
> The question is how much easier does converting modules using these get
> if we start out with wrappers? Because sure, right now it is limited in
> scope, but if we start out with Perl, things will get added on top and
> this might result in more work in the future.

IMO not much _most_ of the time. Stefan's firewall approach is a good
example where it would have hurt much more to go that way, albeit that
was not only switching to rust but also switching to nft, so might not
be applicable in general.

Besides that, Perl is still our prime language for the PVE backend and
will quite probably stay so for at least a year, probably the whole
future PVE 9 release, not unlikely even the 10 one. During that time it
definitively must not become a second class citizen, it must be OK to
continue feature development there during transition period; forcing a
language switch to undergoing development is far from ideal. That's also
why I think doing _some_ clean-up and refactoring work on perl now is
still worth it now (stares at qemu-server) 
>> In other words, IMO a top-down approach such as replacing higher-level
>> subroutines or entire API calls would probably yield better results
>> rather than a bottom-up approach. (I believe there's a pattern for this
>> -- strangler pattern? I'd have to look it up tbh)
I agree with above.
> See above, we can't too easily do that. We need to have all the parts an
> API call needs already in Rust or we won't be able to implement it. Of
> course path modification could be converted to Rust "on-the-fly", but if
> we already have transparent wrappers it could even be trivial.
It might not be that easy than throwing in perlmod now, but it would
IMO bring more benefits and less (intermediate/transitional) churn.

>>> Or depending on whether it's nicer, also wrapping helpers from
>>> proxmox-sys and friends where we already have similar functionality in
>>> our Rust code.
>>
>> 7. While I'm a big fan of re-using existing code, I don't think it
>> applies here -- I think it's fine to keep *certain* things separate and
>> decoupled from one another until we actually find that there's a lot of
>> common functionality between two or more things (speaking generally
>> here). For PVE::Path and PVE::Filesystem in particular, we can always
>> bridge over to Rust via perlmod for individual functions if needed (4.)
>> nevertheless, if that even ends up being necessary (6.).

+1

>> With all that being said, I hope I could convey my reasoning here and
>> shine some light on my design decisions -- please let me know what you
>> think! And thanks for having a look :)
>>
>> [1]: https://github.com/pyca/cryptography/tree/7fd5f95354e33d9ca90ba854e9cbda958968043a/src
> 
> All that said, yes, it might be a too small library in this case. OTOH,
> it might be a good place to start.

IMO not really, a good place to start is to split up and rework pve-common's
into more modules with a clearer scope and also into more binary packages
and with some tests added.

That should happen in perl first, and would already bring us a lot of gains
there for the next few years where the transition is going on.
Also other stuff like making the guest configs "real" section configs,
which the would (or should) be in rust then too.

Then we might even switch some more complicated or finicky helpers over to
rust, but not something I'd start out with now.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


  reply	other threads:[~2025-01-03 10:41 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-12-20 18:51 Max Carrara
2024-12-20 18:51 ` [pve-devel] [PATCH v2 pve-common 01/12] introduce PVE::Path Max Carrara
2025-01-08 14:05   ` Wolfgang Bumiller
2025-01-09  9:56     ` Max Carrara
2025-01-09 11:06       ` Wolfgang Bumiller
2025-01-09 12:56         ` Max Carrara
2024-12-20 18:51 ` [pve-devel] [PATCH v2 pve-common 02/12] test: add directory for tests of PVE::Path module Max Carrara
2024-12-20 18:51 ` [pve-devel] [PATCH v2 pve-common 03/12] test: add tests for path_is_absolute and path_is_relative of PVE::Path Max Carrara
2024-12-20 18:51 ` [pve-devel] [PATCH v2 pve-common 04/12] test: add tests for path_components " Max Carrara
2024-12-20 18:52 ` [pve-devel] [PATCH v2 pve-common 05/12] test: add tests for path_join " Max Carrara
2024-12-20 18:52 ` [pve-devel] [PATCH v2 pve-common 06/12] test: add tests for path_push " Max Carrara
2024-12-20 18:52 ` [pve-devel] [PATCH v2 pve-common 07/12] test: add tests for path_parent " Max Carrara
2024-12-20 18:52 ` [pve-devel] [PATCH v2 pve-common 08/12] test: add tests for path_starts_with, path_ends_with, path_equals Max Carrara
2024-12-20 18:52 ` [pve-devel] [PATCH v2 pve-common 09/12] test: add tests for file path ops functions of PVE::Path Max Carrara
2024-12-20 18:52 ` [pve-devel] [PATCH v2 pve-common 10/12] test: add tests for path_normalize " Max Carrara
2024-12-20 18:52 ` [pve-devel] [PATCH v2 pve-common 11/12] introduce PVE::Filesystem Max Carrara
2024-12-20 18:52 ` [pve-devel] [PATCH v2 pve-common 12/12] debian: introduce package libproxmox-fs-path-utils-perl Max Carrara
2025-01-02 13:46 ` [pve-devel] [PATCH v2 pve-common 00/12] Introduce and Package PVE::Path & PVE::Filesystem Fiona Ebner
2025-01-02 13:53   ` Fiona Ebner
2025-01-02 15:54     ` Max Carrara
2025-01-03  9:49       ` Fiona Ebner
2025-01-03 10:41         ` Thomas Lamprecht [this message]
2025-01-03 12:37           ` Fiona Ebner

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=66a4dca4-d3d7-4def-8f25-e23a2fcaefe5@proxmox.com \
    --to=t.lamprecht@proxmox.com \
    --cc=f.ebner@proxmox.com \
    --cc=m.carrara@proxmox.com \
    --cc=pve-devel@lists.proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal