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

[Thomas Lamprecht]

Am 2/7/19 um 10:20 AM schrieb 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.
We already use CPAN::Meta::YAML from perl-modules in pve-common, so non-issue.
A basic syntax check may also be nice for a user, may allow fixing such things
earlier.

> It's probably a lot of work for little to no benefit.
with the parser available, the work amount depends on the capabillity of ci
(which I do not have fully in mind, atm, sorry), i.e., do you have much
interpendent options in different subtrees (YAML/JSON objects), or is it mostly
enough to replace things a the highest level, e.g., if internal represented as
hash just overwrite the values of everything below if the keys are the same,
because that would be easy.

> 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.
besides the obvious blocker of additional underscores in (sub) commands ( ;-)
), this could be nice in general as we basically can get this for free...  I'd
really use and alternative CLI syntax, though, maybe:

qm cloudinit config VMID

>>
>>> +    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,
>>>