comments inline
> On January 10, 2016 at 10:45 AM Gerrit Venema <[email protected]> wrote:
>
>
> This enables the container to use the lvmthin storage type if one is set up.
> It also fixes some logic in the handling of the freeze, errors on rollback
> and the handling of the config sections to keep a correct tree of snapshots.
>
> Signed-off-by: Gerrit Venema <[email protected]>
> ---
> src/PVE/LXC.pm | 54 +++++++++++++++++++++++++++---------------------------
> 1 file changed, 27 insertions(+), 27 deletions(-)
>
> diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
> index 6023334..6bd0d5d 100644
> --- a/src/PVE/LXC.pm
> +++ b/src/PVE/LXC.pm
> @@ -1795,11 +1795,12 @@ sub snapshot_create {
> my $rootinfo = parse_ct_mountpoint($conf->{rootfs});
> my $volid = $rootinfo->{volume};
>
> + PVE::Storage::volume_snapshot($storecfg, $volid, $snapname);
> +
> if ($running) {
> PVE::Tools::run_command(['/usr/bin/lxc-unfreeze', '-n', $vmid]);
> };
>
> - PVE::Storage::volume_snapshot($storecfg, $volid, $snapname);
> &$snapshot_commit($vmid, $snapname);
> };
> if(my $err = $@) {
I applied a similar patch, which additionally makes sure that
we unfreeze in case of errors.
> @@ -1840,17 +1841,17 @@ sub snapshot_delete {
> my $del_snap = sub {
>
> check_lock($conf);
> -
> - if ($conf->{parent} eq $snapname) {
> - if ($conf->{snapshots}->{$snapname}->{snapname}) {
> - $conf->{parent} = $conf->{snapshots}->{$snapname}->{parent};
> - } else {
> - delete $conf->{parent};
> + my $grand_parent = $conf->{snapshots}->{$snapname}->{parent};
> + foreach my $snapkey (keys %{$conf->{snapshots}}) {
> + if ($conf->{snapshots}->{$snapkey}->{parent} eq $snapname) {
> + if ($grand_parent) {
> + $conf->{snapshots}->{$snapkey}->{parent} = $grand_parent;
> + } else {
> + delete $conf->{snapshots}->{$snapkey}->{parent};
> + }
> }
> }
It is really hard to see the purpose of this code. I thought the current code
works? If not, would you mind to send a test case (example code) which triggers
the bug you want to fix?
> -
> delete $conf->{snapshots}->{$snapname};
> -
> write_config($vmid, $conf);
> };
>
> @@ -1904,30 +1905,25 @@ sub snapshot_rollback {
>
> $conf->{lock} = 'rollback';
>
> - my $forcemachine;
> -
> - # copy snapshot config to current config
> -
> - my $tmp_conf = $conf;
> - &$snapshot_copy_config($tmp_conf->{snapshots}->{$snapname}, $conf);
> - $conf->{snapshots} = $tmp_conf->{snapshots};
> - delete $conf->{snaptime};
> - delete $conf->{snapname};
> - $conf->{parent} = $snapname;
> -
> write_config($vmid, $conf);
> - };
>
> - my $unlockfn = sub {
> - delete $conf->{lock};
> - write_config($vmid, $conf);
> };
>
> lock_container($vmid, 10, $updatefn);
>
> - PVE::Storage::volume_snapshot_rollback($storecfg, $volid, $snapname);
> + eval {
> + PVE::Storage::volume_snapshot_rollback($storecfg, $volid, $snapname);
> + };
> +
> + die "Container stopped but the snapshot could not be rolled back.\n" . $@
> if $@;
> +
> + # copy snapshot config to current config
>
> - lock_container($vmid, 5, $unlockfn);
> + delete $conf->{parent};
> + $conf->{parent} = $snapname;
> + &$snapshot_copy_config($conf->{snapshots}->{$snapname}, $conf);
> + delete $conf->{lock};
> + write_config($vmid, $conf);
> }
same here - what bug does it fix?
>
> sub template_create {
> @@ -2189,7 +2185,7 @@ sub mountpoint_mount {
> if ($scfg->{path}) {
> push @extra_opts, '-o', 'loop';
> $use_loopdev = 1;
> - } elsif ($scfg->{type} eq 'drbd' || $scfg->{type} eq 'lvm' ||
> $scfg->{type} eq 'rbd') {
> + } elsif ($scfg->{type} eq 'lvmthin' || $scfg->{type} eq 'drbd' ||
> $scfg->{type} eq 'lvm' || $scfg->{type} eq 'rbd') {
> # do nothing
> } else {
> die "unsupported storage type '$scfg->{type}'\n";
> @@ -2321,6 +2317,10 @@ sub create_disks {
> undef, 0);
> push @$chown_vollist, $volid;
> }
> + } elsif ($scfg->{type} eq 'lvmthin') {
> + $volid = PVE::Storage::vdisk_alloc($storecfg, $storage,
> $vmid, 'raw',
> + undef, $size_kb);
> + format_disk($storecfg, $volid, $rootuid, $rootgid);
This code duplication is not really necessary, so I applied a modified version.
> } elsif ($scfg->{type} eq 'zfspool') {
>
> $volid = PVE::Storage::vdisk_alloc($storecfg, $storage,
> $vmid,
> 'subvol',
> --
I think this patch contains 3 different things:
1.) snapshot create bug fix
2.) snapshot config bug fix?
3.) enable lvmthin storage
It is usually better to split such patches.
_______________________________________________
pve-devel mailing list
[email protected]
http://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
