Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add snapshot and mfssubfolder support; fix /proc/mounts regex #5

Merged
merged 3 commits into from
Jan 2, 2024

Conversation

anwright
Copy link

@anwright anwright commented Jan 2, 2024

On PVE 8.1/Debian Bookworm, /proc/mounts returns mfs\043mfsmaster... instead of mfs#mfsmaster.... The existing code thinks MFS is never mounted.

Also added support for snapshots for raw files.

Fixes #1 (if you add --shared 1 in your pvesm add command), and #4.

@Zorlin
Copy link
Owner

Zorlin commented Jan 2, 2024

Oh my god. 😁

I will need to review this carefully as it's a huge changeset (for me), but this looks amazing! Thank you so much! Snapshots is huge, I've been meaning to get around to them!

@Zorlin
Copy link
Owner

Zorlin commented Jan 2, 2024

@anwright One minor issue; I'm not a fan of the current style of the copyright file. Not because you're in it now, but because it will get out of control pretty quickly as contributors get involved. I also don't like the copyright year being included because it's doomed to be perpetually out of date. For one thing, it wouldn't include Piotr yet who helps me develop this plugin with advice and other help.

Would you be okay with this revised version?

Copyright (C) Benjamin Arntzen <[email protected]>

This software is maintained by Benjamin Arntzen <[email protected]> and consists of work contributed by themselves and others. A complete list can be found in README.md.
Based on plugin for NetApp by Dmitry Petuhov <[email protected]>
Based on plugin for cluster-mode NFS by Alexandre Derumier <[email protected]>

This program is free software: you can redistribute it and/or modify
it under the terms of the GNU Affero General Public License as published by
the Free Software Foundation, either version 3 of the License, or
(at your option) any later version.

This program is distributed in the hope that it will be useful,
but WITHOUT ANY WARRANTY; without even the implied warranty of
MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
GNU Affero General Public License for more details.

You should have received a copy of the GNU Affero General Public License
along with this program.  If not, see <http://www.gnu.org/licenses/>.

Along with prepending:

"Thanks to the following contributors:
* @anwright
* @pkonopelko

@Zorlin Zorlin merged commit 1060a1a into Zorlin:main Jan 2, 2024
@anwright
Copy link
Author

anwright commented Jan 2, 2024

Oh my god. 😁

I will need to review this carefully as it's a huge changeset (for me), but this looks amazing! Thank you so much! Snapshots is huge, I've been meaning to get around to them!

Please do review it carefully. It works on my machine, but I have a very limited understanding of perl, and of some of the things Proxmox is doing as it calls this plugin. I figured most of it out by just trying different things in the UI, and outputting a bunch of debug logs to figure out the contents of variables etc. And reviewing the existing storage plugins. I've been using MooseFS for years, but only started using Proxmox about a week ago.

I'd like to implement snapshot-based image copying (via the clone_image method) but as far as I can tell Proxmox never actually calls that method in the plugin. Instead it actually copies the images, which is very inefficient compared to MFS snapshots.

Would you be okay with this revised version?

No issues at all.

@Zorlin
Copy link
Owner

Zorlin commented Jan 2, 2024

@anwright May be a bug here:

root@coheed:~/pve-moosefs# pvesm add moosefs moosefs-vm-storage --path /mnt/mfs --mfssubfolder=/proxmox --mfspassword=REDACTED
Use of uninitialized value $mfsmaster in quotemeta at /usr/share/perl5/PVE/Storage/Custom/MooseFSPlugin.pm line 23.
mfsmaster accepted connection with parameters: read-write,restricted_ip,admin ; root mapped to root:root

I found a fix though :)

@anwright
Copy link
Author

anwright commented Jan 2, 2024

Makes sense, thanks for finding that. I spun up a test MFS instance just for this, and specified the mfsmaster during all of my testing as a result.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Filesystem does not appear as a "shared" filesystem in Proxmox
2 participants