Re: [pve-devel] [PATCH v3 qemu-server 4/7] api2: add cloudinit config api

[Fabian Ebner <f.ebner@proxmox.com>]

Am 09.06.21 um 13:54 schrieb Alexandre Derumier:
> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $vmid = $param->{vmid};
> +	my $conf = PVE::QemuConfig->load_config($vmid);
> +
> +	if( defined($conf->{cipassword}) && 
> +	    defined($conf->{cloudinit}->{cipassword}) && 
> +	    $conf->{cipassword} ne $conf->{cloudinit}->{cipassword}) {

Style nit: trailing spaces and should be "if (defined..." at the beginning.

> +	    $conf->{cipassword} = '********** ';
> +	} elsif (defined($conf->{cipassword})) {
> +	    $conf->{cipassword} = '**********';
> +	}
> +
> +	$conf->{cloudinit}->{cipassword} = '**********' if defined($conf->{cloudinit}->{cipassword});
> +

The handling above is still from v1/v2 of the series? IIUC, there is no
cloudinit section anymore now.

> +	my $res = [];
> +	my $pending = PVE::QemuServer::Cloudinit::get_pending_config($conf, $vmid);
> +
> +	foreach my $opt (keys %{$pending}) {

Style nit: please use "for" instead of "foreach" for new code

> +	    push @$res, $pending->{$opt};
> +	}
> +
> +	return $res;
> +   }});
> +
>  # POST/PUT {vmid}/config implementation
>  #
>  # The original API used PUT (idempotent) an we assumed that all operations
> diff --git a/PVE/CLI/qm.pm b/PVE/CLI/qm.pm
> index 1c199b6..d16bf2c 100755
> --- a/PVE/CLI/qm.pm
> +++ b/PVE/CLI/qm.pm
> @@ -994,6 +994,7 @@ our $cmddef = {
>  		my $data = shift;
>  		print "$data\n";
>  	    }],
> +	pending => [ "PVE::API2::Qemu", 'cloudinit_pending', ['vmid'], { node => $nodename }, \&PVE::GuestHelpers::format_pending ]
>      },
>  
>  };
> diff --git a/PVE/QemuServer/Cloudinit.pm b/PVE/QemuServer/Cloudinit.pm
> index abc62b7..156e073 100644
> --- a/PVE/QemuServer/Cloudinit.pm
> +++ b/PVE/QemuServer/Cloudinit.pm
> @@ -607,4 +607,74 @@ sub dump_cloudinit_config {
>      }
>  }
>  
> +sub get_pending_config {
> +    my ($conf, $vmid) = @_;
> +
> +    my $newconf = { %{$conf} };

Might not matter right now, but using dclone() is more future-proof.

> +    my $cloudinit_current = $newconf->{cloudinit};

The next patch should be ordered before this one and this one, so this
one can use extract_cloudinit_config right away.

> +    my @cloudinit_opts = keys %{PVE::QemuServer::cloudinit_config_properties()};
> +    push @cloudinit_opts, 'name';
> +
> +    #add cloud-init drive 

Is there a reason to care about pending changes on the drive itself here?

> +    my $drives = {};
> +    PVE::QemuConfig->foreach_volume($newconf, sub {
> +	my ($ds, $drive) = @_;
> +	$drives->{$ds} = 1 if PVE::QemuServer::drive_is_cloudinit($drive);
> +    });
> +
> +    PVE::QemuConfig->foreach_volume($cloudinit_current, sub {
> +	my ($ds, $drive) = @_;
> +	$drives->{$ds} = 1 if PVE::QemuServer::drive_is_cloudinit($drive);
> +    });
> +    foreach my $ds (keys %{$drives}) {
> +	push @cloudinit_opts, $ds;
> +    }
> +
> +    $newconf->{name} = "VM$vmid" if !$newconf->{name};

Needs to also happen for the old config, or not having 'name' in both
configs will wrongly be detected as a change below.

> +
> +    my $print_net_addr = sub {
> +	my ($conf, $opt, $netid) = @_;
> +
> +	if (defined($conf->{$netid})) {
> +
> +	    my $net = PVE::QemuServer::parse_net($conf->{$netid});
> +	    if (defined($conf->{$opt})) {
> +		$conf->{$opt} .= ",macaddr=".$net->{macaddr} if $net->{macaddr};
> +	    } else {
> +		$conf->{$opt} = "";
> +	    }
> +	}
> +    };
> +
> +    my $res = {};

Could also construct the array already here instead of a hash.

> +    foreach my $opt (@cloudinit_opts) {
> +
> +	#add macaddr to ipconfig

Should we instead consider 'net\d+' to be cloudinit options in this
context (similar to 'name' above) and show the changes to those
directly? That would avoid adding macaddr to ipconfig, which after all
isn't part of its schema.

> +	if ($opt =~ m/^ipconfig(\d+)/) {
> +	    my $netid = "net$1";
> +	    next if !defined($newconf->{$netid}) && !defined($cloudinit_current->{$netid}) && !defined($newconf->{$opt}) && !defined($cloudinit_current->{$opt} );

Style nit: line too long

> +
> +	    &$print_net_addr($newconf, $opt, $netid);
> +	    &$print_net_addr($cloudinit_current, $opt, $netid);
> +	}
> +
> +	my $item = {
> +	    key => $opt,
> +	};
> +	if ($cloudinit_current->{$opt}) {
> +	    $item->{value} = $cloudinit_current->{$opt};
> +	    if ($newconf->{$opt}) {

Needs defined() or everything falsy will be detected as delete

> +		$item->{pending} = $newconf->{$opt} if $newconf->{$opt} ne $cloudinit_current->{$opt};
> +	    } else {
> +		$item->{delete} = 1;
> +	    }
> +	} else {
> +	    $item->{pending} = $newconf->{$opt} if $newconf->{$opt}
> +	}
> +
> +	$res->{$opt} = $item;
> +   }
> +   return $res;
> +}
> +
>  1;