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

[David Limbeck]

On 2/7/19 10:32 AM, Thomas Lamprecht wrote:
> 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.
Wrong number, it's 2068 not 2038, will fix this as well in v2.
>>>>
>>>> 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.
>
There's not that little nesting and at the moment overwriting files 
allows the use of network config v2 for example. In addition people can 
set netplan specific configs like 'on-link: true' to get routes working 
(don't work in network config v1 on ubuntu) for gateways not in the 
subnet. Of course, with your suggestion of adding an addition parameter 
to enable/disable merging it might be possible. I will look into it.
>> 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,
>>>>