[pve-devel] [PATCH qemu-server] cloud-init: allow custom network/user data files via snippets

[David Limbeck]

On 2/7/19 8:23 AM, Thomas Lamprecht wrote:
> Am 2/6/19 um 1:35 PM schrieb David Limbeck:
>> Adds the 'cicustom' option to specify either or both network_data and
>> user_data options as property strings. Their parameters are files
>> in a snippets storage (e.g. local:snippets/network.yaml). If one or both
>> are specified they are used instead of their respective generated
>> configuration.
>> This allows the use of completely custom configurations and is also a
>> possible solution for bug #2038 by specifying a custom user_data file
>> that contains package_upgrade: false.
>>
>> Tested with Ubuntu 18.10
>>
>> Signed-off-by: David Limbeck <d.limbeck at proxmox.com>
>> ---
>>   PVE/API2/Qemu.pm            |  1 +
>>   PVE/QemuServer.pm           | 24 ++++++++++++++++++++++++
>>   PVE/QemuServer/Cloudinit.pm | 37 +++++++++++++++++++++++++++++++++----
>>   3 files changed, 58 insertions(+), 4 deletions(-)
>>
>> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
>> index 22f9f6a..49aaa48 100644
>> --- a/PVE/API2/Qemu.pm
>> +++ b/PVE/API2/Qemu.pm
>> @@ -292,6 +292,7 @@ my $diskoptions = {
>>   };
>>   
>>   my $cloudinitoptions = {
>> +    cicustom => 1,
>>       cipassword => 1,
>>       citype => 1,
>>       ciuser => 1,
>> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
>> index 4a903a6..7c39b97 100644
>> --- a/PVE/QemuServer.pm
>> +++ b/PVE/QemuServer.pm
>> @@ -623,6 +623,24 @@ EODESCR
>>       },
>>   };
>>   
>> +my $cicustom_fmt = {
>> +    network_data => {
>> +	type => 'string',
>> +	optional => 1,
>> +	description => 'Specify a custom file containing all network data passed to the VM via cloud-init.',
>> +	format => 'pve-volume-id',
>> +	format_description => 'volume',
>> +    },
>> +    user_data => {
>> +	type => 'string',
>> +	optional => 1,
>> +	description => 'Specify a custom file containing all user data passed to the VM via cloud-init.',
>> +	format => 'pve-volume-id',
>> +	format_description => 'volume',
>> +    },
>> +};
Maybe rename it to networkdata, userdata and metadata (v2)? Or keep it 
network_data, user_data, meta_data(v2)?
>> +PVE::JSONSchema::register_format('pve-qm-cicustom', $cicustom_fmt);
>> +
>>   my $confdesc_cloudinit = {
>>       citype => {
>>   	optional => 1,
>> @@ -640,6 +658,12 @@ my $confdesc_cloudinit = {
>>   	type => 'string',
>>   	description => 'cloud-init: Password to assign the user. Using this is generally not recommended. Use ssh keys instead. Also note that older cloud-init versions do not support hashed passwords.',
>>       },
>> +    cicustom => {
>> +	optional => 1,
>> +	type => 'string',
>> +	description => 'cloud-init: Specify custom files to replace the automatically generated ones at start.',
> "to replace and enhance" ? Could it make sense to merge both?
This would require a yaml parser and we would have to make sure 
indentation is compatible. It's probably a lot of work for little to no 
benefit. Maybe an additional option ('qm generate_cloud_config' or so) 
would make more sense that generates the files based on our 
configuration and people can then use it as a base or in a pre-start hook.
>
>> +	format => 'pve-qm-cicustom',
>> +    },
>>       searchdomain => {
>>   	optional => 1,
>>   	type => 'string',
>> diff --git a/PVE/QemuServer/Cloudinit.pm b/PVE/QemuServer/Cloudinit.pm
>> index 5be820c..9f36744 100644
>> --- a/PVE/QemuServer/Cloudinit.pm
>> +++ b/PVE/QemuServer/Cloudinit.pm
>> @@ -208,8 +208,9 @@ EOF
>>   sub generate_configdrive2 {
>>       my ($conf, $vmid, $drive, $volname, $storeid) = @_;
>>   
>> -    my $user_data = cloudinit_userdata($conf, $vmid);
>> -    my $network_data = configdrive2_network($conf);
>> +    my ($user_data, $network_data) = get_custom_cloudinit_files($conf);
>> +    $user_data = cloudinit_userdata($conf, $vmid) if !defined($user_data);
>> +    $network_data = configdrive2_network($conf) if !defined($network_data);
>>   
>>       my $digest_data = $user_data . $network_data;
>>       my $uuid_str = Digest::SHA::sha1_hex($digest_data);
>> @@ -378,8 +379,9 @@ sub nocloud_metadata {
>>   sub generate_nocloud {
>>       my ($conf, $vmid, $drive, $volname, $storeid) = @_;
>>   
>> -    my $user_data = cloudinit_userdata($conf, $vmid);
>> -    my $network_data = nocloud_network($conf);
>> +    my ($user_data, $network_data) = get_custom_cloudinit_files($conf);
>> +    $user_data = cloudinit_userdata($conf, $vmid) if !defined($user_data);
>> +    $network_data = nocloud_network($conf) if !defined($network_data);
> why not extend cloudinit_userdata and nocloud_network so that they merge
> an external script into the generated data? if you pull out the repeating
> parts (see below) this could be relatively straight forward and a nicer
> methond interface, the cloudinit_userdata and nocloud_network get all info
> needed ($conf) as parameter, so this could be a bit more transparent,
> a hunk above it looks the same.
>
> Merging may not be completely easy, but dooable. Maybe allow to switch it on/off
> by a cicustom format boolean?
>
>>   
>>       my $digest_data = $user_data . $network_data;
>>       my $uuid_str = Digest::SHA::sha1_hex($digest_data);
>> @@ -394,6 +396,33 @@ sub generate_nocloud {
>>       commit_cloudinit_disk($conf, $vmid, $drive, $volname, $storeid, $files, 'cidata');
>>   }
>>   
>> +sub get_custom_cloudinit_files {
>> +    my ($conf) = @_;
>> +
>> +    my $cicustom = $conf->{cicustom};
>> +    my $files = $cicustom ? PVE::JSONSchema::parse_property_string('pve-qm-cicustom', $cicustom) : {};
>> +
>> +    my $network_data_file = $files->{network_data};
>> +    my $user_data_file = $files->{user_data};
>> +
>> +    my $storage_conf = PVE::Storage::config();
>> +
>> +    my $network_data;
>> +    if ($network_data_file) {
>> +	my ($full_path, undef, $type) = PVE::Storage::path($storage_conf, $network_data_file);
>> +	die "$network_data_file is not in the snippets directory\n" if $type ne 'snippets';
>> +	$network_data = PVE::Tools::file_get_contents($full_path);
>> +    }
> hmm, this looks like it would fit good into its own submethod?
Yes, especially with the metadata as well which I will add in a v2.
>> +    my $user_data;
>> +    if ($user_data_file) {
>> +	my ($full_path, undef, $type) = PVE::Storage::path($storage_conf, $user_data_file);
>> +	die "$user_data_file is not in the snippets directory\n" if $type ne 'snippets';
>> +	$user_data = PVE::Tools::file_get_contents($full_path);
>> +    }
>> +
>> +    return ($user_data, $network_data);
>> +}
>> +
>>   my $cloudinit_methods = {
>>       configdrive2 => \&generate_configdrive2,
>>       nocloud => \&generate_nocloud,
>>