[pve-devel] [PATCH 5/5] DRBD: Implement snapshots.

[Dietmar Maurer]

First, code looks reasonable to me, but I am unable to test
without a working rollback. Some comments inline:

> On October 8, 2015 at 10:24 AM Philipp Marek <philipp.marek at linbit.com> wrote:
> 
> 
> ---
>  PVE/Storage/DRBDPlugin.pm | 88
> ++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 84 insertions(+), 4 deletions(-)
> 
> diff --git a/PVE/Storage/DRBDPlugin.pm b/PVE/Storage/DRBDPlugin.pm
> index 14f232b..3cc7f0b 100644
> --- a/PVE/Storage/DRBDPlugin.pm
> +++ b/PVE/Storage/DRBDPlugin.pm
> @@ -5,6 +5,8 @@ use warnings;
>  use IO::File;
>  use Net::DBus;
>  use Data::Dumper;
> +use Digest::SHA;
> +use Time::HiRes;
>  
>  use PVE::Tools qw(run_command trim);
>  use PVE::INotify;
> @@ -17,6 +19,10 @@ use base qw(PVE::Storage::Plugin);
>  
>  my $default_redundancy = 2;
>  
> +# constants to use as DRBDmanage property keys
> +our $PN_IS_TEMP_RES = "aux:proxmox:is-temp-resource";
> +
> +
>  sub type {
>      return 'drbd';
>  }
> @@ -298,7 +304,7 @@ sub deactivate_storage {
>  sub activate_volume {
>      my ($class, $storeid, $scfg, $volname, $snapname, $cache) = @_;
>  
> -    die "Snapshot not implemented on DRBD\n" if $snapname;
> +    return activate_volume_from_snapshot(@_) if $snapname;

I would prefer to list all parameters by name (instead of using @_)

>      my $path = $class->path($scfg, $volname);
>      
> @@ -340,7 +346,7 @@ sub activate_volume {
>  sub deactivate_volume {
>      my ($class, $storeid, $scfg, $volname, $snapname, $cache) = @_;
>  
> -    die "Snapshot not implemented on DRBD\n" if $snapname;
> +    return deactivate_volume_from_snapshot(@_) if $snapname;

same here

>  
>      return undef; # fixme: should we unassign ?
>  
> @@ -358,6 +364,58 @@ sub deactivate_volume {
>      return undef;    
>  }
>  
> +sub activate_volume_from_snapshot {
> +    my ($class, $storeid, $scfg, $volname, $snapname, $cache) = @_;
> +
> +    my $hdl = connect_drbdmanage_service();
> +
> +# create a new temporary resource, with an optional DRBD client on this node.
> +
> +# If the snapshot would be activated multiple times, and perhaps even
> +# concurrently, we might benefit from a "shared" resource.
> +# (That might need to do some reference counting and/or make the DRBD
> +# device read-only, to avoid data divergence for multiple accessing nodes
> +# that might all try to replay the journals, etc.)
> +
> +# start with volume- and snapshotname, but get a unique ID.
> +    my $new_name = sprintf("%.32s-%.12s",
> +		    sprintf("%.24s-%.24s", $volname, $snapname),
> +		    Digest::SHA::sha1_base64($volname, $snapname,
> +			    $$, Time::HiRes::gettimeofday()));
> +
> +    my $r_prop = { $PN_IS_TEMP_RES => '1', };
> +
> +    my ($rc) = $hdl->restore_snapshot($new_name, $volname, $snapname,
> $r_prop, []);
> +    check_drbd_res($rc);
> +
> +# now get a local assignment...
> +    return activate_volume($class, $storeid, $scfg, $new_name, undef,
> $cache);
> +}
> +
> +sub deactivate_volume_from_snapshot {
> +    my ($class, $storeid, $scfg, $volname, $snapname, $cache) = @_;
> +
> +    my $hdl = connect_drbdmanage_service();
> +    my $nodename = PVE::INotify::nodename();
> +
> +    my ($rc, $res) = $hdl->list_resources([$volname], 0, {}, {});
> +    check_drbd_res($rc);
> +
> +# TODO: should we throw an error if that doesn't exist anymore?
> +    warn("Resource '$volname' doesn't exist anymore?"),return undef if !$res;
> +    die("Multiple resources with name '$volname'?"),return undef if @$res !=
> 1;
> +
> +    my $prop = $res->[0][1];
> +
> +    if ($prop->{$PN_IS_TEMP_RES}) {
> +        my ($rc) = $hdl->remove_resource($volname, 0);
> +        check_drbd_res($rc);
> +    }
> +
> +
> +    return undef;
> +}
> +
>  sub volume_resize {
>      my ($class, $scfg, $storeid, $volname, $size, $running) = @_;
>  
> @@ -374,19 +432,40 @@ sub volume_resize {
>  sub volume_snapshot {
>      my ($class, $scfg, $storeid, $volname, $snap, $running) = @_;
>  
> -    die "drbd snapshot is not implemented";
> +    my $hdl = connect_drbdmanage_service();
> +
> +    my ($rc) = $hdl->create_snapshot($volname, $snap, [], {});
> +    check_drbd_res($rc);
> +    return undef;
>  }
>  
>  sub volume_snapshot_rollback {
>      my ($class, $scfg, $storeid, $volname, $snap) = @_;
>  
>      die "drbd snapshot rollback is not implemented";
> +# FIXME: inplace?
> +}
> +
> +sub volume_rollback_is_possible {
> +    my ($class, $scfg, $storeid, $volname, $snap) = @_; 
> +
> +    my $hdl = connect_drbdmanage_service();
> +
> +# just check for existance, no other restrictions
> +    my ($rc) = $hdl->list_snapshots([$volname], [], [$snap], [], {});
> +    check_drbd_res($rc);
> +
> +    return 1;
>  }
>  
>  sub volume_snapshot_delete {
>      my ($class, $scfg, $storeid, $volname, $snap) = @_;
>  
> -    die "drbd snapshot delete is not implemented";
> +    my $hdl = connect_drbdmanage_service();
> +
> +    my ($rc) = $hdl->remove_snapshot($volname, $snap, 0);
> +    check_drbd_res($rc);
> +    return undef;
>  }
>  
>  sub volume_has_feature {
> @@ -394,6 +473,7 @@ sub volume_has_feature {
>  
>      my $features = {
>          copy => { base => 1, current => 1},
> +        snapshot => { current => 1, snap => 1}, ## what's the difference?

just use

+  snapshot => { current => 1 },