On 6/19/19 9:00 AM, Athina Plaskasoviti wrote:
> For now, virt-install responds to the --install is_cloud=yes,no_install=yes
> to initiate cloud image installation.
>
Thanks for this! It's a good start.
Make sure you run the test suite and ./setup.py pylint. If you are
getting skipped tests in the test suite, make sure to run against latest
libosinfo and osinfo-db (I think you already have a setup for that?)
This patch as is causes test breakage and some pylint warnings
Comments below
> Signed-off-by: Athina Plaskasoviti <athina.plaskasoviti@xxxxxxxxx>
> ---
> virt-install | 4 +++-
> virtinst/cli.py | 2 ++
> virtinst/install/cloudinit.py | 29 +++++++++++++++++++++++++++++
> virtinst/install/installer.py | 14 +++++++++++++-
> virtinst/install/installerinject.py | 13 ++++++++-----
> 5 files changed, 55 insertions(+), 7 deletions(-)
> create mode 100644 virtinst/install/cloudinit.py
>
> diff --git a/virt-install b/virt-install
> index ee2b9006..1be0b91c 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
> @@ -433,7 +434,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)
>
You can add this diff to make --import work instead of requiring
no_install. There's still issues, because is_cloud won't work with
xmlonly, but I think that needs a bigger change.
diff --git a/virt-install b/virt-install
index 1be0b91c..b3608662 100755
--- a/virt-install
+++ b/virt-install
@@ -418,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
> 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..76e11735
> --- /dev/null
> +++ b/virtinst/install/cloudinit.py
> @@ -0,0 +1,29 @@
> +import os
> +import subprocess
> +import yaml
> +import string
> +
Every import here is unused except yaml
yaml is an external dependency which I'd like to avoid depending on
unless it's really needed. Since we aren't actually reading yaml and
just generating it, we can get away with just generating string content
I think.
Do 'from .logger import log' and log the generated string content before
writing any file.
> +
> +def create_metadata(file_name, hostname=None):
> + if hostname:
> + instance = hostname
> + else:
> + hostname = instance = "localhost"
> +
> + with open(file_name, "w") as f:
> + yaml.dump({'instance-id': instance, 'hostname': hostname}, f)
> +
> +
> +def create_userdata(file_name, username=None, password=None):
> + if not password:
> + password = "password"
> +
> + userdata_dictionary = {'password': password}
> +
> + with open(file_name, "w+") as f:
> + f.write("#cloud-config\n")
> + if username:
> + yaml.dump({'name': username}, f)
> + yaml.dump(userdata_dictionary, f, allow_unicode=True,
> + default_flow_style=False, sort_keys=False)
> + f.writelines(["chpasswd: { expire: False }\n", "runcmd:\n", "- [ sudo, touch, /etc/cloud/cloud-init.disabled ]"])
As you mentioned in the cover letter this needs to be generating
temporary files. You can use the pattern we use in perform_initrd_injections
fileobj = tempfile.NamedTemporaryFile(
prefix="virtinst-", suffix="-metadata",
dir=scratchdir, delete=False)
filename = fileobj.name
The return the fileename from this function. The installer will append
it to self._tmpfiles which ensures it will get cleaned up when
virt-install exits
> diff --git a/virtinst/install/installer.py b/virtinst/install/installer.py
> index a5c7a708..3d9f2a7e 100644
> --- a/virtinst/install/installer.py
> +++ b/virtinst/install/installer.py
> @@ -16,6 +16,7 @@ from ..devices import DeviceDisk
> from ..osdict import OSDB
> from ..logger import log
> from .. import progress
> +from .cloudinit import create_metadata, create_userdata
>
>
> def _make_testsuite_path(path):
> @@ -49,7 +50,7 @@ class Installer(object):
> def __init__(self, conn, cdrom=None, location=None, install_bootdev=None,
> location_kernel=None, location_initrd=None,
> install_kernel=None, install_initrd=None, install_kernel_args=None,
> - no_install=None):
> + no_install=None, is_cloud=None):
> self.conn = conn
>
> # Entry point for virt-manager 'Customize' wizard to change autostart
> @@ -63,6 +64,7 @@ class Installer(object):
>
> self._install_bootdev = install_bootdev
> self._no_install = no_install
> + self._is_cloud = is_cloud
>
> self._treemedia = None
> self._treemedia_bootconfig = None
> @@ -266,6 +268,16 @@ class Installer(object):
> elif unattended_script:
> self._prepare_unattended_data(guest, unattended_script)
>
> + elif self._is_cloud:
> + metadata = "meta-data"
> + userdata = "user-data"
> + create_metadata(metadata)
> + create_userdata(userdata)
I think here we shouldn't pass down the meta-data and user-data names,
those functions should have them hardcoded. What we should pass down is
guest.conn.get_app_cache_dir() to use for the tempfile call
> + iso = perform_cdrom_injections([metadata, userdata],
What you will do here is:
[(metadata, "meta-data"), (userdata, "user-data")]
That tells perform_cdrom_injections to take the tmpfile generated by
cloudinit.py, and rename it to "meta-data"/"user-data" when adding it to
the generated ISO. So the end result is indepedent of the generated
tempfile names. This logic is in _perform_generic_injections
> + guest.conn.get_app_cache_dir(), label="cloud")
> + self._tmpfiles.append(iso)
> + self._add_unattended_install_cdrom_device(guest, iso)
> +
> def _cleanup(self, guest):
> if self._treemedia:
> self._treemedia.cleanup(guest)
> diff --git a/virtinst/install/installerinject.py b/virtinst/install/installerinject.py
> index d7cfcfb4..d34dfab9 100644
> --- a/virtinst/install/installerinject.py
> +++ b/virtinst/install/installerinject.py
> @@ -44,19 +44,22 @@ def _run_initrd_commands(initrd, tempdir):
> log.debug("gzip stderr=%s", gziperr)
>
>
> -def _run_iso_commands(iso, tempdir):
> +def _run_iso_commands(iso, tempdir, label=None):
> cmd = ["genisoimage",
> "-o", iso,
> "-J",
> "-input-charset", "utf8",
> "-rational-rock",
> tempdir]
> + if label is "cloud":
> + volid = ["-V", "cidata"]
> + cmd[3:1] = volid
I think maybe we can just passdown cloudinit=True|False And then do
cmd = ["genisoimage",
"-o", iso,
"-J",
"-input-charset", "utf8",
"-rational-rock"]
if cloudinit:
cmd.extend(["-V", "cidata"])
cmd.append(tmpdir)
That way we aren't comparing against the magic 'cloud' string, and you
don't need to insert the vol arguments into the cmd list
> log.debug("Running iso build command: %s", cmd)
> output = subprocess.check_output(cmd, stderr=subprocess.STDOUT)
> log.debug("cmd output: %s", output)
>
>
> -def _perform_generic_injections(injections, scratchdir, media, cb):
> +def _perform_generic_injections(injections, scratchdir, media, cb, label=None):
> if not injections:
> return
>
> @@ -74,7 +77,7 @@ def _perform_generic_injections(injections, scratchdir, media, cb):
> filename, dst, media)
> shutil.copy(filename, os.path.join(tempdir, dst))
>
> - return cb(media, tempdir)
> + return cb(media, tempdir, label)
This bit requires adding label=/cloudinit= to perform_initrd_injections
as well, otherwise the test suite has breakage after this patch
Thanks,
Cole
_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
