Re: [pve-devel] [POC storage v3 15/34] WIP Borg plugin

[Fiona Ebner <f.ebner@proxmox.com>]

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