On October 8, 2019 11:25 am, Alwin Antreich wrote:
> On Tue, Oct 08, 2019 at 08:36:57AM +0200, Fabian Grünbichler wrote:
>> On October 7, 2019 2:41 pm, Alwin Antreich wrote:
>> > Machine states that were created on snapshots with memory could not be
>> > restored on rollback. The state volume was not activated so KVM couldn't
>> > load the state.
>> >
>> > This patch moves the path generation into vm_start and de-/activates the
>> > state volume.
>>
>> alternatively, the following could also work and re-use more code so
>> that we don't miss the next special handling of some corner case.
>> rolling back from a snapshot with state is just like resuming, but we
>> want to keep the statefile instead of deleting it.
> I will send another version with your alternative.
>
>>
>> (untested):
>>
>> diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
>> index edbf1a7..b70c276 100644
>> --- a/PVE/QemuConfig.pm
>> +++ b/PVE/QemuConfig.pm
>> @@ -358,9 +358,7 @@ sub __snapshot_rollback_vm_stop {
>> sub __snapshot_rollback_vm_start {
>> my ($class, $vmid, $vmstate, $data) = @_;
>>
>> - my $storecfg = PVE::Storage::config();
>> - my $statefile = PVE::Storage::path($storecfg, $vmstate);
>> - PVE::QemuServer::vm_start($storecfg, $vmid, $statefile, undef, undef,
>> undef, $data->{forcemachine});
>> + PVE::QemuServer::vm_start($storecfg, $vmid, $vmstate, undef, undef,
>> undef, $data->{forcemachine});
>> }
>>
>> sub __snapshot_rollback_get_unused {
>> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
>> index 8376260..f2d19e1 100644
>> --- a/PVE/QemuServer.pm
>> +++ b/PVE/QemuServer.pm
>> @@ -5418,6 +5418,11 @@ sub vm_start {
>> print "Resuming suspended VM\n";
>> }
>>
>> + if ($statefile && $statefile ne 'tcp' && $statefile ne 'unix') {
>> + # re-use resume code
>> + $conf->{vmstate} = $statefile;
>> + }
>> +
>> my ($cmd, $vollist, $spice_port) = config_to_command($storecfg, $vmid,
>> $conf, $defaults, $forcemachine);
>>
>> my $migrate_port = 0;
>> @@ -5465,8 +5470,6 @@ sub vm_start {
>> push @$cmd, '-incoming', $migrate_uri;
>> push @$cmd, '-S';
>>
>> - } else {
>> - push @$cmd, '-loadstate', $statefile;
>> }
>> } elsif ($paused) {
>> push @$cmd, '-S';
>> @@ -5616,11 +5619,16 @@ sub vm_start {
>> property => "guest-stats-polling-interval",
>> value => 2) if (!defined($conf->{balloon}) ||
>> $conf->{balloon});
>>
>> - if ($is_suspended && (my $vmstate = $conf->{vmstate})) {
>> - print "Resumed VM, removing state\n";
>> - delete $conf->@{qw(lock vmstate runningmachine)};
>> + if (my $vmstate = $conf->{vmstate}) {
>> PVE::Storage::deactivate_volumes($storecfg, [$vmstate]);
>> - PVE::Storage::vdisk_free($storecfg, $vmstate);
>> + delete $conf->{vmstate};
>> +
>> + if ($is_suspended) {
>> + print "Resumed VM, removing state\n";
>> + delete $conf->@{qw(lock runningmachine)};
>> + PVE::Storage::vdisk_free($storecfg, $vmstate);
>> + }
>> +
>> PVE::QemuConfig->write_config($vmid, $conf);
>> }
>>
>>
>> >
>> > Signed-off-by: Alwin Antreich <a.antre...@proxmox.com>
>> > ---
>> > PVE/QemuConfig.pm | 3 +--
>> > PVE/QemuServer.pm | 10 +++++++++-
>> > 2 files changed, 10 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
>> > index edbf1a7..e9796a3 100644
>> > --- a/PVE/QemuConfig.pm
>> > +++ b/PVE/QemuConfig.pm
>> > @@ -359,8 +359,7 @@ sub __snapshot_rollback_vm_start {
>> > my ($class, $vmid, $vmstate, $data) = @_;
>> >
>> > my $storecfg = PVE::Storage::config();
>> > - my $statefile = PVE::Storage::path($storecfg, $vmstate);
>> > - PVE::QemuServer::vm_start($storecfg, $vmid, $statefile, undef, undef,
>> > undef, $data->{forcemachine});
>> > + PVE::QemuServer::vm_start($storecfg, $vmid, $vmstate, undef, undef,
>> > undef, $data->{forcemachine});
>> > }
>> >
>> > sub __snapshot_rollback_get_unused {
>> > diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
>> > index 8376260..39315b3 100644
>> > --- a/PVE/QemuServer.pm
>> > +++ b/PVE/QemuServer.pm
>> > @@ -5420,6 +5420,7 @@ sub vm_start {
>> >
>> > my ($cmd, $vollist, $spice_port) = config_to_command($storecfg, $vmid,
>> > $conf, $defaults, $forcemachine);
>> >
>> > +
>> > my $migrate_port = 0;
>> > my $migrate_uri;
>> > if ($statefile) {
>> > @@ -5466,7 +5467,12 @@ sub vm_start {
>> > push @$cmd, '-S';
>> >
>> > } else {
>> > - push @$cmd, '-loadstate', $statefile;
>> > + my $sfile = $statefile;
>> > + if (!-e $statefile) {
>> > + PVE::Storage::activate_volumes($storecfg, [$statefile]);
>> > + $sfile = PVE::Storage::path($storecfg, $statefile);
>> > + }
>>
>> why only activate if the file does not exist? $statefile should be a
>> full volume ID including storage at this point, it does not make sense
>> to check for existence?
> You can pass a stateuri on vm start (qm start <ID> --stateuri) and this
> can be a file path. I added the if to not break this.
true, I missed that (we use it for migration, but only with 'tcp' /
'unix' as values). but in this case, we need to actually check whether
it is an absolute path, or a PVE-managed volume. in the latter case, we
can just push it to $vollist to get activation/deactivation handled like
the other volumes?
>
>>
>> > + push @$cmd, '-loadstate', $sfile;
>> > }
>> > } elsif ($paused) {
>> > push @$cmd, '-S';
>> > @@ -5622,6 +5628,8 @@ sub vm_start {
>> > PVE::Storage::deactivate_volumes($storecfg, [$vmstate]);
>> > PVE::Storage::vdisk_free($storecfg, $vmstate);
>> > PVE::QemuConfig->write_config($vmid, $conf);
>> > + } elsif ($statefile && (!-e $statefile)) {
>> > + PVE::Storage::deactivate_volumes($storecfg, [$statefile]);
>>
>> this check looks wrong? we should always deactivate, no need to check
>> whether it does not exist? (and like above, it always does not exist
>> AFAICT?)
> True, besides that stateuri. :/
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
