[pve-devel] [PATCH v2 storage 1/5] fix #3030: always activate volumes in storage_migrate

[pve-devel] [PATCH v2 storage 1/5] fix #3030: always activate volumes in storage_migrate

[Fabian Ebner]

AFAICT the snapshot activation is not necessary for our plugins at the moment,
but it doesn't really hurt and might be relevant in the future or for external
plugins.

Deactivating volumes is up to the caller, because for example, for replication
on a running guest, we obviously don't want to deactivate volumes.

Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/Storage.pm | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index cd7b5ff..69c373b 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -697,6 +697,13 @@ sub storage_migrate {
     };
 
     volume_snapshot($cfg, $volid, $snapshot) if $migration_snapshot;
+
+    if (defined($snapshot)) {
+	activate_volumes($cfg, [$volid], $snapshot);
+    } else {
+	activate_volumes($cfg, [$volid]);
+    }
+
     eval {
 	if ($insecure) {
 	    my $input = IO::File->new();
-- 
2.20.1

[pve-devel] [PATCH v2 container 2/5] adapt to new storage_migrate activation behavior

[Fabian Ebner]

Every local volume is migrated via storage_migrate and activated there,
so there is no need to do it in prepare() anymore.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

dependency bump needed

I only found run_replication as a potential place that might need
active local volumes, but that also uses storage_migrate in the end.

 src/PVE/LXC/Migrate.pm | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/src/PVE/LXC/Migrate.pm b/src/PVE/LXC/Migrate.pm
index 90d74b4..94a78c5 100644
--- a/src/PVE/LXC/Migrate.pm
+++ b/src/PVE/LXC/Migrate.pm
@@ -45,7 +45,6 @@ sub prepare {
     $self->{was_running} = $running;
 
     my $force = $self->{opts}->{force} // 0;
-    my $need_activate = [];
 
     PVE::LXC::Config->foreach_volume($conf, sub {
 	my ($ms, $mountpoint) = @_;
@@ -80,9 +79,6 @@ sub prepare {
 	    warn "Used shared storage '$storage' is not online on source node!\n"
 		if !$plugin->check_connection($storage, $scfg);
 	} else {
-	    # only activate if not shared
-	    push @$need_activate, $volid;
-
 	    # unless in restart mode because we shut the container down
 	    die "unable to migrate local mount point '$volid' while CT is running"
 		if $running && !$restart;
@@ -90,8 +86,6 @@ sub prepare {
 
     });
 
-    PVE::Storage::activate_volumes($self->{storecfg}, $need_activate);
-
     # todo: test if VM uses local resources
 
     # test ssh connection
-- 
2.20.1

[pve-devel] applied: [PATCH v2 container 2/5] adapt to new storage_migrate activation behavior

[Thomas Lamprecht]

On 06.11.20 15:30, Fabian Ebner wrote:
> Every local volume is migrated via storage_migrate and activated there,
> so there is no need to do it in prepare() anymore.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> dependency bump needed
> 
> I only found run_replication as a potential place that might need
> active local volumes, but that also uses storage_migrate in the end.
> 
>  src/PVE/LXC/Migrate.pm | 6 ------
>  1 file changed, 6 deletions(-)
> 
>

applied, thanks!

[pve-devel] [RFC v2 container 3/5] deactivate volumes after storage_migrate

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

This is probably not worth it, for two reasons:
1. only local unused volumes are not already deactivated by the existing code
2. if nothing else goes wrong, the volumes migrated with storage_migrate
will be deleted anyways

 src/PVE/LXC/Migrate.pm | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/src/PVE/LXC/Migrate.pm b/src/PVE/LXC/Migrate.pm
index 94a78c5..7c3536f 100644
--- a/src/PVE/LXC/Migrate.pm
+++ b/src/PVE/LXC/Migrate.pm
@@ -294,6 +294,11 @@ sub phase1 {
 	if (my $err = $@) {
 	    die "storage migration for '$volid' to storage '$sid' failed - $err\n";
 	}
+
+	eval { PVE::Storage::deactivate_volumes($self->{storecfg}, [$volid]); };
+	if (my $err = $@) {
+	    $self->log('warn', $err);
+	}
     }
 
     my $conffile = PVE::LXC::Config->config_file($vmid);
-- 
2.20.1

[pve-devel] applied: [RFC v2 container 3/5] deactivate volumes after storage_migrate

[Fabian Grünbichler]

it can't hurt either, and makes it more uniform with qemu migration

On November 6, 2020 3:30 pm, Fabian Ebner wrote:
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> This is probably not worth it, for two reasons:
> 1. only local unused volumes are not already deactivated by the existing code
> 2. if nothing else goes wrong, the volumes migrated with storage_migrate
> will be deleted anyways
> 
>  src/PVE/LXC/Migrate.pm | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/src/PVE/LXC/Migrate.pm b/src/PVE/LXC/Migrate.pm
> index 94a78c5..7c3536f 100644
> --- a/src/PVE/LXC/Migrate.pm
> +++ b/src/PVE/LXC/Migrate.pm
> @@ -294,6 +294,11 @@ sub phase1 {
>  	if (my $err = $@) {
>  	    die "storage migration for '$volid' to storage '$sid' failed - $err\n";
>  	}
> +
> +	eval { PVE::Storage::deactivate_volumes($self->{storecfg}, [$volid]); };
> +	if (my $err = $@) {
> +	    $self->log('warn', $err);
> +	}
>      }
>  
>      my $conffile = PVE::LXC::Config->config_file($vmid);
> -- 
> 2.20.1
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

[pve-devel] [RFC v2 qemu-server 4/5] adapt to new storage_migrate activation behavior

[Fabian Ebner]

Offline migrated volumes are now activated within storage_migrate.
Online migrated volumes can be assumed to be already active.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

dependency bump needed

Sent as RFC, because I'm not completly sure if this is fine here.
Is the assumption about online volumes correct or is there some weird
edge case I'm missing?
I only found run_replication as a potential place that might need active
local volumes, but that also uses storage_migrate in the end.

 PVE/QemuMigrate.pm | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index 2f4eec3..f2c2b07 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -251,7 +251,6 @@ sub prepare {
 
     my $vollist = PVE::QemuServer::get_vm_volumes($conf);
 
-    my $need_activate = [];
     foreach my $volid (@$vollist) {
 	my ($sid, $volname) = PVE::Storage::parse_volume_id($volid, 1);
 
@@ -266,16 +265,9 @@ sub prepare {
 	    my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
 	    warn "Used shared storage '$sid' is not online on source node!\n"
 		if !$plugin->check_connection($sid, $scfg);
-	} else {
-	    # only activate if not shared
-	    next if ($volid =~ m/vm-\d+-cloudinit/);
-	    push @$need_activate, $volid;
 	}
     }
 
-    # activate volumes
-    PVE::Storage::activate_volumes($self->{storecfg}, $need_activate);
-
     # test ssh connection
     my $cmd = [ @{$self->{rem_ssh}}, '/bin/true' ];
     eval { $self->cmd_quiet($cmd); };
-- 
2.20.1

[pve-devel] applied: [RFC v2 qemu-server 4/5] adapt to new storage_migrate activation behavior

[Fabian Grünbichler]

On November 6, 2020 3:30 pm, Fabian Ebner wrote:
> Offline migrated volumes are now activated within storage_migrate.
> Online migrated volumes can be assumed to be already active.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> dependency bump needed
> 
> Sent as RFC, because I'm not completly sure if this is fine here.
> Is the assumption about online volumes correct or is there some weird
> edge case I'm missing?
> I only found run_replication as a potential place that might need active
> local volumes, but that also uses storage_migrate in the end.
> 
>  PVE/QemuMigrate.pm | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
> index 2f4eec3..f2c2b07 100644
> --- a/PVE/QemuMigrate.pm
> +++ b/PVE/QemuMigrate.pm
> @@ -251,7 +251,6 @@ sub prepare {
>  
>      my $vollist = PVE::QemuServer::get_vm_volumes($conf);
>  
> -    my $need_activate = [];
>      foreach my $volid (@$vollist) {
>  	my ($sid, $volname) = PVE::Storage::parse_volume_id($volid, 1);
>  
> @@ -266,16 +265,9 @@ sub prepare {
>  	    my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
>  	    warn "Used shared storage '$sid' is not online on source node!\n"
>  		if !$plugin->check_connection($sid, $scfg);
> -	} else {
> -	    # only activate if not shared
> -	    next if ($volid =~ m/vm-\d+-cloudinit/);
> -	    push @$need_activate, $volid;
>  	}
>      }
>  
> -    # activate volumes
> -    PVE::Storage::activate_volumes($self->{storecfg}, $need_activate);
> -
>      # test ssh connection
>      my $cmd = [ @{$self->{rem_ssh}}, '/bin/true' ];
>      eval { $self->cmd_quiet($cmd); };
> -- 
> 2.20.1
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

[pve-devel] [RFC v2 qemu-server 5/5] deactivate volumes after storage_migrate

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

same comment as for the corresponding LXC patch

 PVE/QemuMigrate.pm | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index f2c2b07..10cf31a 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -569,6 +569,11 @@ sub sync_disks {
 
 		$self->{volume_map}->{$volid} = $new_volid;
 		$self->log('info', "volume '$volid' is '$new_volid' on the target\n");
+
+		eval { PVE::Storage::deactivate_volumes($storecfg, [$volid]); };
+		if (my $err = $@) {
+		    $self->log('warn', $err);
+		}
 	    }
 	}
     };
-- 
2.20.1

[pve-devel] applied: [RFC v2 qemu-server 5/5] deactivate volumes after storage_migrate

[Fabian Grünbichler]

On November 6, 2020 3:30 pm, Fabian Ebner wrote:
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> same comment as for the corresponding LXC patch
> 
>  PVE/QemuMigrate.pm | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
> index f2c2b07..10cf31a 100644
> --- a/PVE/QemuMigrate.pm
> +++ b/PVE/QemuMigrate.pm
> @@ -569,6 +569,11 @@ sub sync_disks {
>  
>  		$self->{volume_map}->{$volid} = $new_volid;
>  		$self->log('info', "volume '$volid' is '$new_volid' on the target\n");
> +
> +		eval { PVE::Storage::deactivate_volumes($storecfg, [$volid]); };
> +		if (my $err = $@) {
> +		    $self->log('warn', $err);
> +		}
>  	    }
>  	}
>      };
> -- 
> 2.20.1
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

[pve-devel] applied: [PATCH v2 storage 1/5] fix #3030: always activate volumes in storage_migrate

[Thomas Lamprecht]

On 06.11.20 15:30, Fabian Ebner wrote:
> AFAICT the snapshot activation is not necessary for our plugins at the moment,
> but it doesn't really hurt and might be relevant in the future or for external
> plugins.
> 
> Deactivating volumes is up to the caller, because for example, for replication
> on a running guest, we obviously don't want to deactivate volumes.
> 
> Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  PVE/Storage.pm | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
>

applied, thanks!