virt-install: Split out --wait handling into a helper class

And add much more clitest coverage

man/virt-install.pod

@@ -1648,9 +1648,8 @@ Amount of time to wait (in minutes) for a VM to complete its install.
 Without this option, virt-install will wait for the console to close (not
 necessarily indicating the guest has shutdown), or in the case of
 --noautoconsole, simply kick off the install and exit. Any negative
-value will make virt-install wait indefinitely, a value of 0 triggers the
+value will make virt-install wait indefinitely, If the time limit is exceeded,
-same results as noautoconsole. If the time limit is exceeded, virt-install
+virt-install simply exits, leaving the virtual machine in its current state.
-simply exits, leaving the virtual machine in its current state.
 
 =item B<--dry-run>
 

tests/clitest.py

@@ -795,7 +795,7 @@ c.add_invalid("--features smm=on --machine pc")  # smm=on doesn't work for machi
 
 c = vinst.add_category("nodisk-install", "--nographics --noautoconsole --nodisks")
 c.add_valid("--hvm --cdrom %(EXISTIMG1)s")  # Simple cdrom install
-c.add_valid("--wait 0 --os-variant winxp --cdrom %(EXISTIMG1)s")  # Windows (2 stage) install
+c.add_valid("--os-variant winxp --cdrom %(EXISTIMG1)s")  # Windows (2 stage) install
 c.add_valid("--pxe --virt-type test")  # Explicit virt-type
 c.add_valid("--arch i686 --pxe")  # Explicitly fullvirt + arch
 c.add_valid("--location location=%(TREEDIR)s")  # Directory tree URL install
@@ -804,8 +804,8 @@ c.add_valid("--hvm --location %(TREEDIR)s --extra-args console=ttyS0")  # Direct
 c.add_valid("--paravirt --location %(TREEDIR)s")  # Paravirt location
 c.add_valid("--paravirt --location %(TREEDIR)s --os-variant none")  # Paravirt location with --os-variant none
 c.add_valid("--location %(TREEDIR)s --os-variant fedora12")  # URL install with manual os-variant
-c.add_valid("--cdrom %(EXISTIMG2)s --os-variant win2k3 --wait 0")  # HVM windows install with disk
+c.add_valid("--cdrom %(EXISTIMG2)s --os-variant win2k3")  # HVM windows install with disk
-c.add_valid("--cdrom %(EXISTIMG2)s --os-variant win2k3 --wait 0 --print-step 2")  # HVM windows install, print 3rd stage XML
+c.add_valid("--cdrom %(EXISTIMG2)s --os-variant win2k3 --print-step 2")  # HVM windows install, print 3rd stage XML
 c.add_valid("--pxe --autostart")  # --autostart flag
 c.add_compare("--cdrom http://example.com/path/to/some.iso", "cdrom-url")
 c.add_compare("--pxe --print-step all", "simple-pxe")  # Diskless PXE install
@@ -831,7 +831,11 @@ c.add_valid("--hvm --import")  # FV Import install
 c.add_valid("--hvm --import --prompt --force")  # Working scenario w/ prompt shouldn't ask anything
 c.add_valid("--paravirt --import")  # PV Import install
 c.add_valid("--paravirt --print-xml 1")  # print single XML, implied import install
-c.add_compare("-c %(EXISTIMG2)s --os-variant win2k3 --wait 0 --vcpus cores=4 --controller usb,model=none", "w2k3-cdrom")  # HVM windows install with disk
+c.add_compare("-c %(EXISTIMG2)s --os-variant win2k3 --vcpus cores=4 --controller usb,model=none", "w2k3-cdrom")  # HVM windows install with disk
+c.add_invalid("--hvm --import --wait 2", grep="exceeded specified time limit")  # --wait positive number, but test suite hack
+c.add_invalid("--hvm --import --wait 0", grep="exceeded specified time limit")  # --wait 0, but test suite hack
+c.add_invalid("--hvm --import --wait -1", grep="exceeded specified time limit")  # --wait -1, but test suite hack
+c.add_invalid("--connect test:///default --name foo --ram 64 --disk none --sdl --hvm --import", use_default_args=False, grep="exceeded specified time limit")  # --sdl doesn't have a console callback, triggers implicit --wait -1
 c.add_invalid("--paravirt --import --print-xml 2")  # PV Import install, no second XML step
 c.add_invalid("--paravirt --import --print-xml 7")  # Invalid --print-xml arg
 c.add_invalid("--location kernel=foo,initrd=bar")  # location kernel/initrd without any url
@@ -894,7 +898,7 @@ c = vinst.add_category("kvm-generic", "--connect %(URI-KVM)s --noautoconsole")
 c.add_compare("--os-variant fedora-unknown --file %(EXISTIMG1)s --location %(TREEDIR)s --extra-args console=ttyS0 --cpu host --channel none --console none --sound none --redirdev none --boot cmdline='foo bar baz'", "kvm-fedoralatest-url", prerun_check=has_old_osinfo)  # Fedora Directory tree URL install with extra-args
 c.add_compare("--test-media-detection %(TREEDIR)s --arch x86_64 --hvm", "test-url-detection")  # --test-media-detection
 c.add_compare("--os-variant full_id=http://fedoraproject.org/fedora/20 --disk %(EXISTIMG1)s,device=floppy --disk %(NEWIMG1)s,size=.01,format=vmdk --location %(TREEDIR)s --extra-args console=ttyS0 --quiet", "quiet-url", prerun_check=has_old_osinfo)  # Quiet URL install should make no noise
-c.add_compare("--cdrom %(EXISTIMG2)s --file %(EXISTIMG1)s --os-variant win2k3 --wait 0 --sound --controller usb", "kvm-win2k3-cdrom")  # HVM windows install with disk
+c.add_compare("--cdrom %(EXISTIMG2)s --file %(EXISTIMG1)s --os-variant win2k3 --sound --controller usb", "kvm-win2k3-cdrom")  # HVM windows install with disk
 c.add_compare("--os-variant ubuntusaucy --nodisks --boot cdrom --virt-type qemu --cpu Penryn --input tablet", "qemu-plain")  # plain qemu
 c.add_compare("--os-variant fedora20 --nodisks --boot network --nographics --arch i686", "qemu-32-on-64", prerun_check=has_old_osinfo)  # 32 on 64
 
@@ -1061,7 +1065,6 @@ c.add_valid("--nographics --cdrom %(EXISTIMG1)s")  # console warning about cdrom
 c.add_valid("--nographics --console none --location %(TREEDIR)s")  # console warning about nographics + --console none
 c.add_valid("--nographics --console none --location %(TREEDIR)s")  # console warning about nographics + --console none
 c.add_valid("--nographics --location %(TREEDIR)s")  # console warning about nographics + missing extra args
-c.add_invalid("--pxe --noautoconsole --wait 1", grep="Installation has exceeded specified time limit")  # --wait 1 is converted to 1 second if we are in the test suite, so this should actually touch the wait machinery. however in this case it exits with failure
 c.add_valid("--pxe --nographics --transient", grep="testsuite console command: ['virsh'")  # --transient handling
 
 

virt-install

@@ -562,38 +562,74 @@ def build_guest_instance(conn, options):
 # Install process helpers #
 ###########################
 
+def _sleep(secs):
+    if not cli.in_testsuite():
+        time.sleep(secs)  # pragma: no cover
+
+
+class WaitHandler:
+    """
+    Helper class for handling the --wait option sleeping and time tracking
+    """
+    def __init__(self, wait):
+        self.wait_is_requested = False
+        self._wait_mins = 0
+        self._start_time = 0
+
+        if wait is not None:
+            self.wait_is_requested = True
+            self._wait_mins = wait
+
+    @property
+    def wait_for_console_to_exit(self):
+        # If --wait specified, we don't want the default behavior of waiting
+        # for virt-viewer to exit, we want to launch it, then manually count
+        # down time for ourselves
+        return not self.wait_is_requested
+    @property
+    def _wait_forever(self):
+        return self._wait_mins < 0
+    @property
+    def _wait_secs(self):
+        return self._wait_mins * 60
+
+    def start(self):
+        self._start_time = time.time()
+
+    def get_time_string(self):
+        timestr = _(" %d minutes") % self._wait_secs
+        if self._wait_forever:
+            timestr = ""
+        ret = _("Waiting%(time_string)s for installation to complete.") % {
+                "time_string": timestr}
+        return ret
+
+    def wait(self):
+        """
+        sleep 1 second, then teturn True if wait time has expired
+        """
+        _sleep(1)
+        if self._wait_forever:
+            if cli.in_testsuite():
+                return True
+            return False  # pragma: no cover
+
+        time_elapsed = (time.time() - self._start_time)
+        return (time_elapsed >= self._wait_secs) or cli.in_testsuite()
+
+
 def start_install(guest, installer, options):
-    if options.wait is not None:
-        wait_on_install = True
-        wait_time = options.wait * 60
-    else:
-        wait_on_install = False
-        wait_time = -1
-
-    # If --wait specified, we don't want the default behavior of waiting
-    # for virt-viewer to exit, since then we can't exit the app when time
-    # expires
-    wait_on_console = not wait_on_install
-
-    if wait_time == 0:
-        # --wait 0 implies --noautoconsole
-        autoconsole = False
-    else:
-        autoconsole = options.autoconsole
-
     conscb = None
-    if autoconsole:
+    if options.autoconsole:
         conscb = cli.get_console_cb(guest)
-        if not conscb:
+        if not conscb and options.wait is None:
             # If there isn't any console to actually connect up,
             # default to --wait -1 to get similarish behavior
-            autoconsole = False
+            logging.warning(_("No console to launch for the guest, "
-            if options.wait is None:
+                "defaulting to --wait -1"))
-                logging.warning(_("No console to launch for the guest, "
+            options.wait = -1
-                    "defaulting to --wait -1"))
-                wait_on_install = True
-                wait_time = -1
 
+    waithandler = WaitHandler(options.wait)
     meter = cli.get_meter()
     logging.debug("Guest.has_install_phase: %s",
                   installer.has_install_phase())
@@ -603,7 +639,7 @@ def start_install(guest, installer, options):
 
     domain = None
     try:
-        start_time = time.time()
+        waithandler.start()
 
         domain = installer.start_install(guest, meter=meter,
                 doboot=not options.noreboot,
@@ -612,10 +648,10 @@ def start_install(guest, installer, options):
         if options.destroy_on_exit:
             atexit.register(_destroy_on_exit, domain)
 
-        cli.connect_console(guest, domain, conscb, wait_on_console,
+        cli.connect_console(guest, domain, conscb,
+                waithandler.wait_for_console_to_exit,
                 options.destroy_on_exit)
-        check_domain(installer, domain, conscb, options.transient,
+        check_domain(installer, domain, conscb, options.transient, waithandler)
-                     wait_on_install, wait_time, start_time)
 
         print_stdout(_("Domain creation completed."))
         if not options.transient and not domain.isActive():
@@ -644,8 +680,7 @@ def start_install(guest, installer, options):
         _destroy_on_exit(domain)
 
 
-def check_domain(installer, domain, conscb, transient,
+def check_domain(installer, domain, conscb, transient, waithandler):
-                 wait_for_install, wait_time, start_time):
     """
     Make sure domain ends up in expected state, and wait if for install
     to complete if requested
@@ -674,16 +709,14 @@ def check_domain(installer, domain, conscb, transient,
         # just closed the console and the VM is still running. In the
         # the former case, libvirt may not have caught up yet with the
         # VM having exited, so wait a bit and check again
-        if not cli.in_testsuite():
+        _sleep(2)
-            time.sleep(2)  # pragma: no cover
         if check_domain_inactive():
             return  # pragma: no cover
 
     # If we reach here, the VM still appears to be running.
-    if not wait_for_install or wait_time == 0:
+    if not waithandler.wait_is_requested:
         # User either:
         #   used --noautoconsole
-        #   used --wait 0
         #   killed console and guest is still running
         if not installer.has_install_phase():
             return
@@ -693,13 +726,8 @@ def check_domain(installer, domain, conscb, transient,
               " to \nthe console to complete the installation process."))
         sys.exit(0)
 
-    wait_forever = (wait_time < 0)
+    print_stdout(_("Domain installation still in progress."))
-    timestr = (not wait_forever and
+    print_stdout(waithandler.get_time_string())
-               _(" %d minutes") % (int(wait_time) / 60) or "")
-    print_stdout(
-        _("Domain installation still in progress. Waiting"
-          "%(time_string)s for installation to complete.") %
-        {"time_string": timestr})
 
     # Wait loop
     while True:
@@ -707,20 +735,12 @@ def check_domain(installer, domain, conscb, transient,
             print_stdout(_("Domain has shutdown. Continuing."))
             break
 
-        if not cli.in_testsuite():  # pragma: no cover
+        done = waithandler.wait()
-            time.sleep(1)
+        if done:
-
+            print_stdout(
-        time_elapsed = (time.time() - start_time)
+                _("Installation has exceeded specified time limit. "
-        if not cli.in_testsuite():  # pragma: no cover
+                  "Exiting application."))
-            if wait_forever:
+            sys.exit(1)
-                continue
-            if time_elapsed < wait_time:
-                continue
-
-        print_stdout(
-            _("Installation has exceeded specified time limit. "
-              "Exiting application."))
-        sys.exit(1)
 
 
 ########################