Am 13.11.24 um 11:52 schrieb Fabian Grünbichler:
> On November 7, 2024 5:51 pm, Fiona Ebner wrote:
>> +my sub prepare_run_dir {
>> + my ($archive, $operation) = @_;
>> +
>> + my $run_dir = "/run/pve-storage-borg-plugin/${archive}.${operation}";
>> + _remove_tree($run_dir);
>> + make_path($run_dir);
>> + die "unable to create directory $run_dir\n" if !-d $run_dir;
>
> this is used as part of restoring - what if I restore the same archive
> in parallel into two different VMIDs?
Right, I'll suffix the run_dir path with the current PID.
---snip 8<---
>> +my sub file_contents_from_archive {
>> + my ($self, $archive, $file) = @_;
>> +
>> + my $run_dir = prepare_run_dir($archive, "file-contents");
>> +
>> + my $raw;
>> +
>> + eval {
>> + local $CWD = $run_dir;
>> +
>> + $self->{'storage-plugin'}->borg_cmd_extract(
>> + $self->{scfg},
>> + $self->{storeid},
>> + $archive,
>> + [$file],
>> + );
>
> borg extract has `--stdout`, which would save writing to the FS here
> (since this is only used to extract config file, it should be okay)?
Will change it in v5.
---snip 8<---
>> +sub job_hook {
>> + my ($self, $phase, $info) = @_;
>> +
>> + if ($phase eq 'start') {
>> + $self->{'job-id'} = $info->{'start-time'};
>> + $self->{password} = $self->{'storage-plugin'}->borg_get_password(
>> + $self->{scfg}, $self->{storeid});
>> + $self->{'ssh-key-fh'} = $self->{'storage-plugin'}->borg_open_ssh_key(
>> + $self->{scfg}, $self->{storeid});
>> + } else {
>> + delete $self->{password};
>
> why do we delete this, but don't close the ssh-key-fh ?
Will close it in v5.
>
>> + }
>> +
>> + return;
>> +}
>> +
>> +sub backup_hook {
>> + my ($self, $phase, $vmid, $vmtype, $info) = @_;
>> +
>> + if ($phase eq 'start') {
>> + $self->{$vmid}->{'task-size'} = 0;
>> + } elsif ($phase eq 'prepare') {
>> + if ($vmtype eq 'lxc') {
>> + my $archive = $self->{$vmid}->{archive};
>> + my $run_dir = prepare_run_dir($archive, "backup-container");
>> + $self->{$vmid}->{'run-dir'} = $run_dir;
>> +
>> + my $create_dir = sub {
>> + my $dir = shift;
>> + make_path($dir);
>> + die "unable to create directory $dir\n" if !-d $dir;
>> + chown($info->{'backup-user-id'}, -1, $dir)
>> + or die "unable to change owner for $dir\n";
>> + };
>> +
>> + $create_dir->("${run_dir}/backup/");
>> + $create_dir->("${run_dir}/backup/filesystem");
>> + $create_dir->("${run_dir}/ssh");
>> + $create_dir->("${run_dir}/.config");
>> + $create_dir->("${run_dir}/.cache");
>
> so this is a bit tricky.. we need unpriv access (to do the backup), but
> we store sensitive things here that we don't actually want to hand out
> to everyone..
I'll change the run dir and other dirs to be 0700 in v5.
>
>> +
>> + for my $subdir ($info->{sources}->@*) {
>> + PVE::Tools::run_command([
>> + 'mount',
>> + '-o', 'bind,ro',
>> + "$info->{directory}/${subdir}",
>> + "${run_dir}/backup/filesystem/${subdir}",
>> + ]);
>> + }
>> + }
>> + } elsif ($phase eq 'end' || $phase eq 'abort') {
>> + if ($vmtype eq 'lxc') {
>> + my $run_dir = $self->{$vmid}->{'run-dir'};
>> + eval {
>> + eval { PVE::Tools::run_command(['umount', "${run_dir}/ssh"]); };
>
> this might warrant a comment ;) a tmpfs is mounted there in
> backup_container..
Will add a comment in v5.
---snip 8<---
>> +sub restore_vm_init {
>> + my ($self, $volname, $storeid) = @_;
>> +
>> + my $res = {};
>> +
>> + my (undef, $archive, $vmid) =
>> $self->{'storage-plugin'}->parse_volname($volname);
>> + my $mount_point = prepare_run_dir($archive, "restore-vm");
>> +
>> + $self->{'storage-plugin'}->borg_cmd_mount(
>> + $self->{scfg},
>> + $self->{storeid},
>> + $archive,
>> + $mount_point,
>> + );
>
> haven't actually tested this code, but what are the permissions like for
> this mounted backup archive contents? we don't want to expose guest
> volumes as world-readable either..
>
From a quick test, while it shows as world-readable when looking at it
via the root user, it's a fuse mount not actually accessible by other
users. Still, I decided to do the mount in a subdirectory in v5 just to
be sure.
---snip 8<---
>> +sub properties {
>> + return {
>> + 'repository-path' => {
>> + description => "Path to the backup repository",
>> + type => 'string',
>> + },
>> + 'ssh-key' => {
>> + description => "FIXME", # FIXME
>> + type => 'string',
>> + },
>> + 'ssh-fingerprint' => {
>> + description => "FIXME", # FIXME
>> + type => 'string',
>> + },
>
> these should probably get descriptions and formats, but this is titled
> WIP :)
As discussed of list, will fix these up, using better names and using
'pem-string' format and PVE::Tools::validate_ssh_public_keys()
---snip 8<---
>> diff --git a/src/PVE/Storage/Makefile b/src/PVE/Storage/Makefile
>> index acd37f4..9fe2c66 100644
>> --- a/src/PVE/Storage/Makefile
>> +++ b/src/PVE/Storage/Makefile
>> @@ -14,6 +14,7 @@ SOURCES= \
>> PBSPlugin.pm \
>> BTRFSPlugin.pm \
>> LvmThinPlugin.pm \
>> + BorgBackupPlugin.pm \
>
> do we want this one here, while the other one is in Custom?
>
Ah no, or at least not yet. Moved to Custom in v5.
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
