Merge ~paelzer/uvtool:per-arch-templates-onmaster into uvtool:master

[Christian Ehrhardt ]

Hi,
as discussed the re-written and tested contribution for better multi-arch support.

Suggestion for the debian/changelog when you carry this from master:

  * Select the default template per host architecture (LP: #1452016)
    - Provide and auto-select per arch templates for aarch64, ppc64le and s390x
    - reject console redirection on s390x due to sclp console.
    - Track UNDEF_FLAGS to handle nvram being used and properly removed in
      aarch64 use cases.
    - create: add guest-arch option to override default template arch
    - Document template architecture changes in man pages

A matching commit for the changelog would be at https://git.launchpad.net/~paelzer/uvtool/commit/?id=c4679bb197635fb5ee54061282f83ab33cb82a09
Those two branches are identical other than this commit to debian/changelog.

A testable build is in https://launchpad.net/~ci-train-ppa-service/+archive/ubuntu/2972

[Christian Ehrhardt  (paelzer)]

Hmm, all spawning was fine, but the nvram cleanup on arm does need an improvement.
Expect a follow up fix into this branch rather soon.

[Christian Ehrhardt  (paelzer)]

In fact the destroy now works (as I tested before), but it throws an error messages.
Maybe it does a try to remove before where the flag I added is still missing.

[Christian Ehrhardt  (paelzer)]

Ok, now also the destroy action works as intended.

[Robie Basak (racb)]

Looks good, thanks! Comments requesting some minor changes inline.

review: Needs Fixing

[Christian Ehrhardt  (paelzer)]

All minor changes agreed, pushing follow up fixes to this MP now

[Robie Basak (racb)]

review: Approve

diff --git a/contrib/README b/contrib/README
index f0bfd07..7fb3968 100644
--- a/contrib/README
+++ b/contrib/README
@@ -1,11 +1,6 @@
 Here are some pieces contributed by others that haven't made it into uvtool
 proper yet. They may change or disappear between releases.
 
-Additional architecture support:
-
-arch/arm64.xml - contributed by Ali in LP: #1349854
-arch/ppc64el.xml - contributed by Christian Ehrhardt in LP: #1573820
-
 Others:
 
 ussh/ - contributed by Scott Moser in https://gist.github.com/smoser/88a5a77ab0debf268b945d46314ea447
diff --git a/man/uvt-kvm.1 b/man/uvt-kvm.1
index 9db5c33..e6fae4f 100644
--- a/man/uvt-kvm.1
+++ b/man/uvt-kvm.1
@@ -372,6 +372,8 @@ This options enables retrospective examination of VM console output, but
 breaks
 .B virsh\ console
 for interactive use.
+On s390x this option is currently not supported due to incompatibilties with
+the sclp console used.
 
 .SH CLOUD-INIT CONFIGURATION OPTIONS
 
@@ -452,7 +454,16 @@ a new VM's definition. This is dynamically altered before domain
 creation; see LIBVIRT DOMAIN DEFINITION OPTIONS.
 
 Default:
-.IR /usr/share/uvtool/default.xml .
+.IR /usr/share/uvtool/libvirt/template.xml .
+
+.TP
+.BI --guest-arch\  architecture
+Specify the architecture of the guest template file that will be selected.
+If an explicit \fB--template\fR is given then \fB--guest-arch\fR has no effect.
+If neither \fB--template\fR nor \fB--guest-arch\fR are set the hosts
+architecture will be used to determine the default xml template.
+
+Default: The architecture of the host system.
 
 .TP
 .BI --user-data\  user_data_file
diff --git a/setup.py b/setup.py
index 59404b7..9f7d8eb 100644
--- a/setup.py
+++ b/setup.py
@@ -29,6 +29,9 @@ distutils.core.setup(
     packages=['uvtool.libvirt'],
     scripts=glob.glob('bin/*'),
     data_files=[
-        ('/usr/share/uvtool/libvirt', ['template.xml', 'remote-wait.sh'])
+        ('/usr/share/uvtool/libvirt', ['template.xml', 'remote-wait.sh',
+                                       'template-aarch64.xml',
+                                       'template-ppc64le.xml',
+                                       'template-s390x.xml'])
     ],
 )
diff --git a/contrib/arch/arm64.xml b/template-aarch64.xml
index fa57a2a..bef38ff 100644
--- a/contrib/arch/arm64.xml
+++ b/template-aarch64.xml
@@ -1,22 +1,26 @@
-<domain type='kvm' xmlns:qemu='http://libvirt.org/schemas/domain/qemu/1.0'>
+<domain type='kvm'>
   <os>
     <type arch='aarch64' machine='virt'>hvm</type>
-    <loader readonly='yes' type='rom'>/usr/share/AAVMF/AAVMF_CODE.fd</loader>
-    <boot dev='hd' />
+    <loader readonly='yes' type='pflash'>/usr/share/AAVMF/AAVMF_CODE.fd</loader>
+    <nvram template='/usr/share/AAVMF/AAVMF_CODE.fd'>/tmp/AAVMF_CODE.fd</nvram>
+    <boot dev='hd'/>
   </os>
+  <features>
+    <acpi/>
+    <apic/>
+    <pae/>
+  </features>
   <cpu mode='custom' match='exact'>
     <model fallback='allow'>host</model>
   </cpu>
-  <clock offset='utc'/>
   <devices>
-    <emulator>/usr/bin/qemu-system-aarch64</emulator>
-    <console type='pty'/>
-      <console type='pty'>
-      <target type='virtio' port='0'/>
-    </console>
     <interface type='network'>
       <source network='default'/>
       <model type='virtio'/>
     </interface>
-   </devices>
+    <serial type='pty'>
+      <source path='/dev/pts/3'/>
+      <target port='0'/>
+    </serial>
+  </devices>
 </domain>
diff --git a/contrib/arch/ppc64el.xml b/template-ppc64le.xml
index e861561..e861561 100644
--- a/contrib/arch/ppc64el.xml
+++ b/template-ppc64le.xml
diff --git a/template-s390x.xml b/template-s390x.xml
new file mode 100644
index 0000000..ac65e6a
--- /dev/null
+++ b/template-s390x.xml
@@ -0,0 +1,22 @@
+<domain type='kvm'>
+  <os>
+    <type>hvm</type>
+    <boot dev='hd'/>
+  </os>
+  <features>
+    <acpi/>
+    <apic/>
+    <pae/>
+  </features>
+  <devices>
+    <interface type='network'>
+      <source network='default'/>
+      <model type='virtio'/>
+    </interface>
+    <console type='pty' tty='/dev/pts/3'>
+      <source path='/dev/pts/3'/>
+      <target type='sclp' port='0'/>
+      <alias name='console0'/>
+    </console>
+  </devices>
+</domain>
diff --git a/uvtool/libvirt/kvm.py b/uvtool/libvirt/kvm.py
index ac24ad3..ba1d971 100755
--- a/uvtool/libvirt/kvm.py
+++ b/uvtool/libvirt/kvm.py
@@ -27,6 +27,7 @@ import errno
 import functools
 import itertools
 import os
+import platform
 import shutil
 import signal
 import string
@@ -47,7 +48,10 @@ import uvtool.libvirt.simplestreams
 import uvtool.ssh
 import uvtool.wait
 
+
+ARCH = platform.machine()
 DEFAULT_TEMPLATE = '/usr/share/uvtool/libvirt/template.xml'
+
 DEFAULT_REMOTE_WAIT_SCRIPT = '/usr/share/uvtool/libvirt/remote-wait.sh'
 POOL_NAME = 'uvtool'
 
@@ -63,6 +67,21 @@ class InsecureError(RuntimeError):
     pass
 
 
+def get_template_path(arch):
+    if arch == 'aarch64':
+        return '/usr/share/uvtool/libvirt/template-aarch64.xml'
+    elif arch == 'ppc64le':
+        return '/usr/share/uvtool/libvirt/template-ppc64le.xml'
+    elif arch == 's390x':
+        return '/usr/share/uvtool/libvirt/template-s390x.xml'
+    elif arch == 'x86_64' or arch == 'i686':
+        return DEFAULT_TEMPLATE
+    else:
+        print("Warning: unknown architecture '%s' using defaults" % arch,
+              file=sys.stderr)
+        return DEFAULT_TEMPLATE
+
+
 # From: http://www.chiark.greenend.org.uk/ucgi/~cjwatson/blosxom/2009-07-02-python-sigpipe.html
 def subprocess_setup():
     # Python installs a SIGPIPE handler by default. This is usually not what
@@ -285,8 +304,8 @@ def create_cow_volume_by_path(backing_volume_path, new_volume_name,
     return pool.createXML(etree.tostring(new_vol), 0)
 
 
-def compose_domain_xml(name, volumes, cpu=1, memory=512, unsafe_caching=False,
-        template_path=DEFAULT_TEMPLATE, log_console_output=False, bridge=None,
+def compose_domain_xml(name, volumes, template_path, cpu=1, memory=512,
+        unsafe_caching=False, log_console_output=False, bridge=None,
         ssh_known_hosts=None):
     tree = etree.parse(template_path)
     domain = tree.getroot()
@@ -339,14 +358,18 @@ def compose_domain_xml(name, volumes, cpu=1, memory=512, unsafe_caching=False,
                       )
 
     if log_console_output:
-        print(
-            "Warning: logging guest console output introduces a DoS " +
+        if ARCH == 's390x':
+            raise CLIError("logging guest console output is currently"
+                           "not supported on s390x.")
+        else:
+            print(
+                "Warning: logging guest console output introduces a DoS " +
                 "security problem on the host and should not be used in " +
                 "production.",
-            file=sys.stderr
-        )
-        etree.strip_elements(devices, 'serial')
-        devices.append(E.serial(E.target(port='0'), type='stdio'))
+                file=sys.stderr
+            )
+            etree.strip_elements(devices, 'serial')
+            devices.append(E.serial(E.target(port='0'), type='stdio'))
 
     if ssh_known_hosts:
         metadata = domain.find('metadata')
@@ -373,8 +396,8 @@ def get_base_image(filters):
     return result[0]
 
 
-def create(hostname, filters, user_data_fobj, meta_data_fobj, memory=512,
-           cpu=1, disk=2, unsafe_caching=False, template_path=DEFAULT_TEMPLATE,
+def create(hostname, filters, user_data_fobj, meta_data_fobj, template_path,
+           memory=512, cpu=1, disk=2, unsafe_caching=False,
            log_console_output=False, bridge=None, backing_image_file=None,
            start=True, ssh_known_hosts=None, ephemeral_disks=None):
     if backing_image_file is None:
@@ -425,7 +448,12 @@ def create(hostname, filters, user_data_fobj, meta_data_fobj, memory=512,
             try:
                 domain.create()
             except:
-                domain.undefine()
+                if ARCH == 'aarch64':
+                    # aarch runs with nvram per our default template, flag
+                    # needed to be able to remove those on undefine
+                    domain.undefineFlags(libvirt.VIR_DOMAIN_UNDEFINE_NVRAM)
+                else:
+                    domain.undefine()
                 raise
     except:
         for vol in undo_volume_creation:
@@ -463,7 +491,12 @@ def destroy(hostname):
 
     delete_domain_volumes(conn, domain)
 
-    domain.undefine()
+    if ARCH == 'aarch64':
+        # aarch runs with nvram per our default template, flag
+        # needed to be able to remove those on undefine
+        domain.undefineFlags(libvirt.VIR_DOMAIN_UNDEFINE_NVRAM)
+    else:
+        domain.undefine()
 
 
 def get_lts_series():
@@ -607,6 +640,14 @@ def main_create(parser, args):
     meta_data_fobj = apply_default_fobj(
         args, 'meta_data', create_default_meta_data
     )
+
+    template = get_template_path(ARCH)
+    if args.guest_arch:
+        template = get_template_path(args.guest_arch)
+    # an explicit template overrides general and guest-arch defaults
+    if args.template:
+        template = args.template
+
     if args.backing_image_file:
         abs_image_backing_file = os.path.abspath(args.backing_image_file)
     else:
@@ -619,7 +660,7 @@ def main_create(parser, args):
         disk=args.disk,
         log_console_output=args.log_console_output,
         memory=args.memory,
-        template_path=args.template,
+        template_path=template,
         unsafe_caching=args.unsafe_caching,
         start=not args.no_start,
         ssh_known_hosts=ssh_known_hosts,
@@ -746,7 +787,7 @@ def main(args):
     create_subparser.set_defaults(func=main_create)
     create_subparser.add_argument(
         '--developer', '-d', nargs=0, action=DeveloperOptionAction)
-    create_subparser.add_argument('--template', default=DEFAULT_TEMPLATE)
+    create_subparser.add_argument('--template', default=None)
     create_subparser.add_argument('--memory', default=512, type=int)
     create_subparser.add_argument('--cpu', default=1, type=int)
     create_subparser.add_argument('--disk', default=8, type=int)
@@ -760,6 +801,8 @@ def main(args):
     create_subparser.add_argument(
         '--meta-data', type=argparse.FileType('rb'))
     create_subparser.add_argument('--password')
+    create_subparser.add_argument('--guest-arch',
+        help='guest arch to select template, default is the host architecture')
     create_subparser.add_argument('--log-console-output', action='store_true')
     create_subparser.add_argument('--backing-image-file')
     create_subparser.add_argument('--run-script-once', action='append')