[virt-tools-list] [virt-manager PATCH][v2][RFC] Introduction of cloud-init configuration in virt-install

[Cole Robinson]

On 6/21/19 1:34 PM, Daniel P. Berrangé wrote:
> On Fri, Jun 21, 2019 at 08:18:45PM +0300, Athina Plaskasoviti wrote:
>> Triggered by:
>> --install is_cloud=yes ... --import
>>
>> Signed-off-by: Athina Plaskasoviti <athina.plaskasoviti at gmail.com>
>> ---
>>  virt-install                        |  9 ++++---
>>  virtinst/cli.py                     |  2 ++
>>  virtinst/install/cloudinit.py       | 41 +++++++++++++++++++++++++++++
>>  virtinst/install/installer.py       | 16 ++++++++++-
>>  virtinst/install/installerinject.py | 20 +++++++-------
>>  5 files changed, 75 insertions(+), 13 deletions(-)
>>  create mode 100644 virtinst/install/cloudinit.py
>>
>> diff --git a/virt-install b/virt-install
>> index ee2b9006..b3608662 100755
>> --- a/virt-install
>> +++ b/virt-install
>> @@ -399,6 +399,7 @@ def build_installer(options, guest, installdata):
>>      install_kernel_args = installdata.kernel_args
>>      install_os = installdata.os
>>      no_install = installdata.no_install
>> +    is_cloud = installdata.is_cloud
>>      if installdata.kernel_args:
>>          if installdata.kernel_args_overwrite:
>>              install_kernel_args = installdata.kernel_args
>> @@ -417,10 +418,11 @@ def build_installer(options, guest, installdata):
>>              no_install = True
>>      elif options.pxe:
>>          install_bootdev = "network"
>> +    elif options.import_install:
>> +        no_install = True
>>      elif installdata.is_set:
>>          pass
>> -    elif (options.import_install or
>> -          options.xmlonly or
>> +    elif (options.xmlonly or
>>            options.boot):
>>          no_install = True
>>  
>> @@ -433,7 +435,8 @@ def build_installer(options, guest, installdata):
>>              install_kernel=install_kernel,
>>              install_initrd=install_initrd,
>>              install_kernel_args=install_kernel_args,
>> -            no_install=no_install)
>> +            no_install=no_install,
>> +            is_cloud=is_cloud)
>>  
>>      if options.unattended:
>>          unattended_data = cli.parse_unattended(options.unattended)
>> diff --git a/virtinst/cli.py b/virtinst/cli.py
>> index 9a1fe2f6..a2a501a5 100644
>> --- a/virtinst/cli.py
>> +++ b/virtinst/cli.py
>> @@ -1580,6 +1580,7 @@ class ParserInstall(VirtCLIParser):
>>                  is_onoff=True)
>>          cls.add_arg("os", "os")
>>          cls.add_arg("no_install", "no_install", is_onoff=True)
>> +        cls.add_arg("is_cloud", "is_cloud", is_onoff=True)
>>  
>>  
>>  class InstallData:
>> @@ -1592,6 +1593,7 @@ class InstallData:
>>          self.os = None
>>          self.is_set = False
>>          self.no_install = None
>> +        self.is_cloud = None
>>  
>>  
>>  def parse_install(optstr):
>> diff --git a/virtinst/install/cloudinit.py b/virtinst/install/cloudinit.py
>> new file mode 100644
>> index 00000000..25b2a79b
>> --- /dev/null
>> +++ b/virtinst/install/cloudinit.py
>> @@ -0,0 +1,41 @@
>> +import tempfile
>> +from ..logger import log
>> +
>> +
>> +def create_metadata(scratchdir, hostname=None):
>> +    if hostname:
>> +        instance = hostname
>> +    else:
>> +        hostname = instance = "localhost"
>> +
>> +    fileobj = tempfile.NamedTemporaryFile(
>> +            prefix="virtinst-", suffix="-metadata",
>> +            dir=scratchdir, delete=False)
>> +    filename = fileobj.name
>> +
>> +    with open(filename, "w") as f:
>> +        log.debug("Writing instance-id and hostname to file meta-data")
>> +        f.writelines(['instance-id: %s\n' % instance,
>> +            'hostname: %s\n' % hostname])
>> +    return filename
> 

Just a general note, for now we are just trying to straighten out the
plumbing and get something testable. I'm not planning on pushing
anything without a broader discussion which I will start soon

> We should probably use the UUID for the newly to-be-created VM as
> the instance-id value, and probably not set hostname at all unless
> we know what it will end up being.
> 

What practical effect does instance-id have? It wasn't obvious to me
from reading the docs

> 
>> +
>> +
>> +def create_userdata(scratchdir, username=None, password=None):
>> +    if not password:
>> +        password = "password"
> 
> We must not do this - it is a secret flaw to have a hardcoded
> password.
> 
> If the user doesn't spply a password, we should simply not
> set one.
>

Yes, final commit will not have a hardcoded password. We couldn't find a
way with native cloud-init commands to unset the password though,
unclear if there's a way besides calling host commands. We can generate
a random password, print it to stdout, to it and set it to expire on
first login. virt-builder does the random password thing IIRC

FWIW though fedora cloud images already have a notion of a default
password, it's just 'fedora' if it finds cloud-init config that doesn't
overwrite the password. Something like that... I haven't dug enough into
the details to know how all the pieces fit together.

> As default behaviour it is probably more useful to 
> take $HOME/.ssh/authorized_keys and inject it into
> the guest. We should also allow an alterantive file
> in case they want different keys in some cases.
> 

Hmm interesting. We can't depend on that option as necessary for login
because not everyone will have ssh keys configured.

> 
>> +    fileobj = tempfile.NamedTemporaryFile(
>> +            prefix="virtinst-", suffix="-userdata",
>> +            dir=scratchdir, delete=False)
>> +    filename = fileobj.name
>> +
>> +    with open(filename, "w+") as f:
>> +        f.write("#cloud-config\n")
>> +        if username:
>> +            log.debug("Writing username to file user-data")
>> +            f.write("name: %s\n" % username)
>> +        log.debug("Writing password and cmd for disabling of cloud-init to file user-data")
>> +        f.writelines(["password: %s\n" % password,
>> +            "chpasswd: { expire: False }\n", "runcmd:\n",
>> +            "- [ sudo, touch, /etc/cloud/cloud-init.disabled ]"])
>> +    return filename
> 
> Regards,
> Daniel
> 


- Cole