On February 6, 2020 9:42 am, Fabian Ebner wrote:
> On 2/5/20 10:29 AM, Fabian Grünbichler wrote:
>> On January 29, 2020 2:29 pm, Fabian Ebner wrote:
>>> This function is intened to be used after doing a migration where some
>>> of the volume IDs changed.
>>>
>>> Signed-off-by: Fabian Ebner <f.eb...@proxmox.com>
>>> ---
>>> PVE/AbstractConfig.pm | 61 +++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 61 insertions(+)
>>>
>>> diff --git a/PVE/AbstractConfig.pm b/PVE/AbstractConfig.pm
>>> index a94a379..fb833cb 100644
>>> --- a/PVE/AbstractConfig.pm
>>> +++ b/PVE/AbstractConfig.pm
>>> @@ -366,6 +366,67 @@ sub get_replicatable_volumes {
>>> die "implement me - abstract method\n";
>>> }
>>>
>>> +sub foreach_volume {
>>> + my ($class, $conf, $func, @param) = @_;
>>> +
>>> + die "abstract method - implement me\n";
>>> +}
>>> +
>>> +sub print_volume {
>>> + my ($class, $volume) = @_;
>>> +
>>> + die "abstract method - implement me\n";
>>> +}
>>
>> if we do this, we probably also want a parse_volume here? see comments
>> on qemu-server #12
>> >> +
>>> +# $volume_map is a hash of 'old_volid' => 'new_volid' pairs.
>>> +# This method replaces 'old_volid' by 'new_volid' throughout
>>> +# the config including snapshots, both for volumes appearing in
>>> +# foreach_volume as well as vmstate and unusedN values.
>>> +sub update_volume_ids {
>>> + my ($class, $conf, $volume_map) = @_;
>>> +
>>> + my $newconf = {};
>>
>> why not modify the config in place? you replace the old one with the
>> returned new one anyway in the single caller, and write it out directly
>> afterwards ;) it would make the code shorter, and more inline with how
>> we usually modify $conf
>>
>
> Ok, I'll do that.
>
>>> +
>>> + my $do_replace = sub {
>>> + my ($key, $volume, $newconf, $volume_map) = @_;
>>
>> no need to pass $newconf and $volume_map as parameter, the one from
>> update_volume_ids is accessible here anyway.
>>
>>> +
>>> + my $old_volid = $volume->{file} // $volume->{volume};
>>
>> it might make sense to expose this (under which key the volid is stored)
>> via AbstractConfig/QemuConfig/LXC::Config, Aaron's backup status patch
>> series also
>> needs this information in pve-manager.
>>
>
> You suggested creating new helper modules for Qemu drives in the reply
> to #12. Would it make sense to go all the way and create a base
> AbstractVolume.pm and then QemuDrive.pm and LXCMountpoint.pm? Initially
> it might only contain parse_volume, print_volume, foreach_volume and
> this key (or maybe get_volid and set_volid?) and in the long term more
> code could be moved there.
I think just for those 4 it would be a bit much to have their own
abstraction. if while investigating you find more stuff that actually
benefits from a common interface, or even better, lots of duplicate code
that could be moved to a common top layer, then please propose such a
change!
my guess would be that most stuff we actually do with the volumes is
pretty different in qemu-server and pve-container. the former is more
concerned with how to present the volumes as VM hardware, as well as
managing various blackjobs. the latter is more concerned with
formatting, mounting, pulling/pushing files, namespaces.
high-level interfaces that are on the config layer can go into
AbstractConfig + its implementations anyway, and utilize helpers that
are in specific modules that don't share a common interface if it does
not make sense.
>
>>> + if (my $new_volid = $volume_map->{$old_volid}) {
>>> + $volume->{file} = $new_volid if defined($volume->{file});
>>> + $volume->{volume} = $new_volid if defined($volume->{volume});
>>
>> which would make this a single line
>>
>>> + $newconf->{$key} = $class->print_volume($volume);
>>> + }
>>> + };
>>> +
>>> + my $replace_volids = sub {
>>> + my ($conf) = @_;
>>> +
>>> + my $newconf = {};
>>> + foreach my $key (keys %{$conf}) {
>>> + next if $key =~ m/^snapshots$/;
>>> + # these keys are not handled by foreach_volume
>>
>> would it make sense to include them optionally?
>>
>> we have lots of use cases where we really want to iterate over ALL the
>> currently referenced volumes.. I know both of them only have a volid,
>> but we could just set that into $parsed->{file} or $parsed->{volume} and
>> leave the rest empty. if you call foreach_volume with
>> $opts->{include_vmstate} or $opts->{include_unused} you of course need
>> to be able to handle them properly, and we'd need to look whether
>> pve-container can easily support such an interface as well.
>>
>
> Yes, I'll look into it.
>
>>> + if ($key =~ m/^(vmstate)|(unused\d+)$/) {
>>> + my $old_volid = $conf->{$key};
>>> + $newconf->{$key} = $volume_map->{$old_volid};
>>
>> in-place, this could become
>>
>> $conf->{$key} = $volume_map->{$conf->{$key}};
>>
>>> + }
>>> + $newconf->{$key} = $conf->{$key} if !defined($newconf->{$key});
>>
>> not needed for in-place
>>
>>> + }
>>> +
>>> + $class->foreach_volume($conf, $do_replace, $newconf, $volume_map);
>>> + return $newconf;
>>> + };
>>> +
>>> + $newconf = $replace_volids->($conf);
>>> + foreach my $snap (keys %{$conf->{snapshots}}) {
>>> + my $newsnap = $replace_volids->($conf->{snapshots}->{$snap});
>>> + foreach my $k (keys %{$newsnap}) {
>>> + $newconf->{snapshots}->{$snap}->{$k} = $newsnap->{$k};
>>> + }
>>
>> this second foreach should not be needed:
>>
>> foreach my $snap (keys %${$conf->{snapshots}}) {
>> $newconf->{snapshots}->{$snap} =
>> $replace_volids->($conf->{snapshots}->{$snap});
>> }
>>
>> (or $conf-> if we drop $newconf)
>>
>>> + }
>>> +
>>> + return $newconf;
>>> +}
>>> +
>>> # Internal snapshots
>>>
>>> # NOTE: Snapshot create/delete involves several non-atomic
>>> --
>>> 2.20.1
>>>
>>>
>>> _______________________________________________
>>> 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
>>
>
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
