Merge lp:~javier.collado/utah/provisioned_machine into lp:utah

[Javier Collado]

This branch creates a new class named ProvisionedMachine class that takes care
of running tests on a provisioned machine (physical or virtual).

The way to run test cases in such a machine with the implementation in this
branch would be as follows:
PYTHONPATH=. ./examples/run_utah_tests.py --skip-provisioning --name
<machine_name> ./utah/client/examples/pass.run

I don't consider this merge request yet ready to be committed, but I'd like to
get some feedback with regard the following:
- Flag
What do you think about using --skip-provisioning flag? I believe there are too
many flags already in run_utah_tests.py, so if you any suggestion would be
welcomed.

- Inventory
For now, no inventory is used to get the machine object. However, one must be
used to prevent jenkins jobs trying to make use of the same hardware
simultaneously. What kind of inventory do you suggest? I see there's a separate
inventory for cobbler machines and for VMs so probably it makes sense to have
one for provisioned machines, but I think that it can be confusing to have so
many inventories. I know there's a plan to use PostgreSQL, but I'm thinking
about having all inventories in a common database in MongoDB. This way we could
use a different schema for the machines collection depending on how it's
expected to be used.

- Class hierarchy
The workaround I had to implement to skip the initialization code in the
Machine class probably means that some refactoring is needed to have a simpler
generic machine class from which ProvisionedMachine can inherit and another one
used by all the machine classes that need an image to install the system before
running any test. Do you think this refactoring would make sense?

[Max Brustkern (nuclearbob)]

I'm going to comment on your questions before I actually look at the merge.

I think that flag is fine. I also think that script is overloaded with options, but I think at this point we're going to need a more thorough design overhaul to fix that, so adding another flag for now is probably the best thing to do.

I think at this point, an inventory that could handle physical machines, virtual machines, already installed machines, reinstalling existing machines, ARM boards, etc. is becoming a priority. It's something we've wanted for a long time. The PS team, in particular, would like to be able to request machines based on hardware info/tags rather than just always pulling one by name. I think that will help hardware testing scale much better as well. I've used SQL because that's what I know, but we do need different information for machines depending on whether they're virtual, physical, already installed, etc. If you want to put together something in Mongo, I'd enjoy looking at it, and I'm sure it'd be a good learning experience for me.

I think up until we did automatic ISO downloading, the base Machine class could run fine without an image, so I would be in favor of refactoring things so it can work that way again. If you want to prepare something for that, I'd be happy to test it, but if you're working on other things, that's something I can try to tackle at some point as well.

[Max Brustkern (nuclearbob)]

Now for my thoughts on the code. I think since this class requires SSH, it would make sense to put it in the same file as SSHMixin. Right now that's ssh.py, but I'm not sure if we should have that in a separate file or if it should be moved back into provisioning.py. It was moved out for modularity, but we're still not using any machine that isn't SSH-based, so maybe it should just live in the main provisioning file. Either way, I'd be in favor of ProvisionedMachine living in the same file as SSHMixin.

Have you done any testing without your __del__ method in place? If the cleanup functions are written correctly, then if we haven't defined any cleanup actions for an instance of the class, nothing will happen when we call the cleanup functions, and we shouldn't need to skip them. If they are causing problems when we don't have any items to be cleaned up, I think that's probably something that should be fixed there.

[lp:~javier.collado/utah/provisioned_machine]

815. By Javier Collado on 2013-02-01

Moved ProvisionedMachine class to utah.provisioning.ssh

Aside from this, timeout values have been set to more sensible values when
network connectivity fails.

816. By Javier Collado on 2013-02-01

Updated {ssh,ping}check methods to sleep only after a failure.

In the old implementation the sleep took place before any connection attempt
which was good when failures happened, but wasn't so good on successes because
of all the time wasted waiting for a check that finally succeeded. To fix this,
both methods now run the check right away and sleep only if they fail to
guarantee that the retry attempt doesn't happen immediately.

Besides this, the documentation strings for both methods have been updated to
describe the new behavior.

817. By Javier Collado on 2013-02-01

Added clean attribute and removed __del__ method implementation

[Javier Collado (javier.collado)]

Max, I've applied the following changes based on your comments:

- ProvisionedMachine class implementation has been moved to ssh module as
  suggested. provisioning.py is already quite large, so I prefer to do that
  rather than moving ssh mixin back.

- self.clean instance attribute has been added to the ProvisionedMachine
  implementation to fix the problem that I had when calling the cleanup
  methods. Indeed, there was no need to overwrite __del__ implementation.

Besides this, I've changed a little bit the timeout behavior of {ping,ssh}check
methods (have a look at rev. 816 commit message or the updated documentation in
the code).

[Max Brustkern (nuclearbob)]

This looks reasonable to me. I'm going to check it out and run a test on physical hardware.

[Max Brustkern (nuclearbob)]

So when I tested this on physical hardware I got this:

Traceback (most recent call last):
  File "./examples/run_utah_tests.py", line 115, in <module>
    run_utah_tests()
  File "./examples/run_utah_tests.py", line 101, in run_utah_tests
    function(args=args)
  File "./examples/run_utah_tests.py", line 90, in run_provisioned_tests
    run_tests(args, machine)
  File "/home/max/provisioned_machine/utah/run.py", line 203, in run_tests
    shutil.copyfile(machine.finalpreseed, preseedfile)
AttributeError: 'ProvisionedMachine' object has no attribute 'finalpreseed'

I made it work with this patch:

=== modified file 'utah/run.py'
--- utah/run.py 2013-01-23 12:26:38 +0000
+++ utah/run.py 2013-02-01 15:43:56 +0000
@@ -193,7 +193,8 @@
             except Exception as err:
                 logging.warning('Failed to download files: ' + str(err))

- if args.outputpreseed or config.outputpreseed:
+ if ((args.outputpreseed or config.outputpreseed) and
+ hasattr(machine, 'finalpreseed')):
         if args.outputpreseed:
             logging.debug('Capturing preseed due to command line option')
         elif config.outputpreseed:

But everywhere I read about hasattr seems to indicate we may as well just try to use it and catch an AttributeError, so maybe that's more pythonic.

Also, it runs, and copies the test log, but it doesn't output the copied test log the way a normal run does.

Finally, it installs the client every time. This is really a design artifact of the main function in run.py assuming a new run is always happening, and it won't really hurt anything (it does make sure we always have the latest version of the client) but I wonder if at some point maybe installing the client should be considered part of provisioning the system. Maybe not, since this method does ensure we always have the latest client even if a machine was installed a while ago.

review: Needs Fixing

[Javier Collado (javier.collado)]

Max, I haven't been able to reproduce the problem and I guess it's because
we're using different command lines. What I've been using is:

PYTHONPATH=. ./examples/run_utah_tests.py --skip-provisioning --name <name>
./utah/client/examples/pass.run -d

I've tried with a dell mini9 and with a nexus 7 and it worked fine on both.

Regarding the log behavior, could you elaborate on this? I believe what you're
seeing is the behavior we have after the latest logging changes. If we want to
get the old behavior, then we probably need to set the console log level to
logging.INFO in the configuration file.

Regarding the client installation, probably this could to be improved to update
the package only when needed, but I think keeping the client in sync is
important. I also needed to install `gdebi` in my tests, but I think we can
ignore this, since the idea is to reuse systems provisioned by UTAH not
manually as I did in my tests.

[Max Brustkern (nuclearbob)]

I've got some long logs, so I'm going to send them in an email.

[lp:~javier.collado/utah/provisioned_machine]

818. By Javier Collado on 2013-02-05

Added check to writing the preseed to a file for provisioned systems

[Javier Collado (javier.collado)]

Max, thanks for the logs.

I finally realized that I wasn't seeing the preseed problem because of a
configuration option in the system you're using that isn't enabled by default.
Anyway, using --outputpreseed I got to the same result.

Regarding how to fix the problem, in my opinion we shouldn't catch exceptions
for something that can be checked upfront, but also using hasattr is not a very
good solution because it doesn't commnicate much about the design. The change
I've pushed checks if machine is a ProvisionedMachine object which I feel makes
clearer what kind of machine isn't expected to have a preseed file available.
If you disagree, then we can talk about alternative solutions.

I'm looking now at the problem regarding the log output.

[lp:~javier.collado/utah/provisioned_machine]

819. By Javier Collado on 2013-02-05

Added statement to print log file names to stdout

The log file names are needed by external processes (for example a jenkins job)
to gather them in a single location.

[Javier Collado (javier.collado)]

I've added a try/except/finally block similar to the one in other run_* scripts
and a print statement to make sure log file names are printed as expected.

With this change, both issues should have been addressed properly. Please let
me know if something else is missing.

review: Needs Resubmitting

[Max Brustkern (nuclearbob)]

This just reminds me that we could use more unit testing for all the options we have at this point. This is what I get now:

jenkins@magners-orchestra:/home/max/provisioned_machine$ PYTHONPATH=. ./examples/run_utah_tests.py --skip-provisioning --name acer-veriton-03 /usr/share/utah/client/examples/pass.run
Running on machine: acer-veriton-03
Traceback (most recent call last):
  File "./examples/run_utah_tests.py", line 126, in <module>
    run_utah_tests()
  File "./examples/run_utah_tests.py", line 112, in run_utah_tests
    function(args=args)
  File "./examples/run_utah_tests.py", line 97, in run_provisioned_tests
    if len(locallogs) != 0:
UnboundLocalError: local variable 'locallogs' referenced before assignment

I think we probably just need to define locallogs (and maybe exitstatus) before we try to set them to the return values on run_test, in case run_test has an exception.

[lp:~javier.collado/utah/provisioned_machine]

820. By Javier Collado on 2013-02-05

Added default values for locallogs and exitstatus

[Javier Collado (javier.collado)]

Thanks for detecting that problem. It should be fixed in the latest commit.
Yes, I'd like to have more unit tests everywhere.

[Max Brustkern (nuclearbob)]

I got this working using this change:

- args.outputpreseed or config.outputpreseed):
+ (args.outputpreseed or config.outputpreseed)):

Without that, if config.outputpreseed is True, that's good enough for the if statement to be True, and we get the same AttributeError as before.

review: Needs Fixing

[lp:~javier.collado/utah/provisioned_machine]

821. By Javier Collado on 2013-02-05

Fixed logic as suggested by Max

[Javier Collado (javier.collado)]

Thanks again Max. This merge certainly required to be reviewed.

review: Needs Resubmitting

[Max Brustkern (nuclearbob)]

Could you tell me why line 232 is indented the way it is? It doesn't actually matter, I'm just curious so I can be sure I'm doing the right thing in the future.

Other than that, it looks good. The only reason I found most of these was that I was testing in the lab, so I guess we should continue to test in the lab, since that represents a common configuration that we need to support.

review: Approve

[Javier Collado (javier.collado)]

To explain the indentation of line 232, let me paste the output of pep8 when
the code is aligned as usual:

if (not isinstance(machine, ProvisionedMachine) and
    (args.outputpreseed or config.outputpreseed)):
    if args.outputpreseed:

----------------------------------
$ pep8 --show-pep8 utah/run.py
utah/run.py:200:9: E125 continuation line does not distinguish itself from next logical line
    Continuation lines should align wrapped elements either vertically using
    Python's implicit line joining inside parentheses, brackets and braces, or
    using a hanging indent.

    When using a hanging indent the following considerations should be applied:

    - there should be no arguments on the first line, and

    - further indentation should be used to clearly distinguish itself as a
      continuation line.
----------------------------------

In particular, the problem the extra indentation in the merge request fixes is
the one explained in the last bullet. Before using pep8, I would have used the
version above, but now I find it makes sense to make visually clear the
separation between the if condition and the body.

[Max Brustkern (nuclearbob)]

That makes sense, I wasn't looking at the line below. I'll remember that in the future, thanks.

=== modified file 'examples/run_utah_tests.py'
--- examples/run_utah_tests.py	2013-01-23 12:26:38 +0000
+++ examples/run_utah_tests.py	2013-02-05 17:24:22 +0000
@@ -26,10 +26,13 @@
     file_arguments,
     name_argument,
     virtual_arguments,
-    configure_logging
+    configure_logging,
+    run_tests
 )
 from utah.timeout import timeout, UTAHTimeout
 from run_install_test import run_install_test
+from utah.provisioning.ssh import ProvisionedMachine
+from utah.exceptions import UTAHException
 
 
 def get_parser():
@@ -50,6 +53,9 @@
                         help='Type of machine to provision (%(choices)s)')
     parser.add_argument('-v', '--variant',
                         help='Variant of architecture, i.e., armel, armhf')
+    parser.add_argument('--skip-provisioning', action='store_true',
+                        help=('Reuse a system that is already provisioned '
+                              '(name argument must be passed)'))
     parser = common_arguments(parser)
     parser = custom_arguments(parser)
     parser = file_arguments(parser)
@@ -76,6 +82,26 @@
     # Default is now CustomVM
     function = run_install_test
 
+    if args.skip_provisioning:
+        def run_provisioned_tests(args):
+            """Run test cases in a provisioned machine."""
+            locallogs = []
+            exitstatus = 0
+            try:
+                # TBD: Inventory should be used to verify machine
+                # is not running other tests
+                machine = ProvisionedMachine(name=args.name)
+                exitstatus, locallogs = run_tests(args, machine)
+            except UTAHException as error:
+                sys.stderr.write('Exception: ' + str(error))
+                exitstatus = 2
+            finally:
+                if len(locallogs) != 0:
+                    print('Test logs copied to the following files:')
+                    print("\t" + "\n\t".join(locallogs))
+            sys.exit(exitstatus)
+
+        function = run_provisioned_tests
     if args.arch is not None and 'arm' in args.arch:
         # If arch is arm, use BambooFeederMachine
         from run_test_bamboo_feeder import run_test_bamboo_feeder
 
=== modified file 'utah/provisioning/provisioning.py'
--- utah/provisioning/provisioning.py	2013-01-24 16:31:58 +0000
+++ utah/provisioning/provisioning.py	2013-02-05 17:24:22 +0000
@@ -302,15 +302,29 @@
             self._start()
 
     def pingcheck(self, timeout=config.checktimeout):
-        """Check network connectivity using ping."""
-        self.logger.info('Sleeping {timeout} seconds'
-                         .format(timeout=timeout))
-        time.sleep(timeout)
+        """Check network connectivity using ping.
+
+        :param timeout: Amount of time in seconds to sleep after a failure
+        :type timeout: int
+        :raises: UTAHProvisioningException
+
+        If there's a network connectivity failure, then sleep ``timeout``
+        seconds and raise a retriable exception.
+
+        .. seealso:: :func:`utah.retry.retry`, :meth:`pingpoll`
+
+        """
         self.logger.info('Checking network connectivity (ping)')
         returncode = \
             self._runargs(['ping', '-c1', '-w5', self.name])['returncode']
         if returncode != 0:
             err = 'Ping returned {0}'.format(returncode)
+
+            if timeout > 0:
+                self.logger.info('Sleeping {timeout} seconds'
+                                 .format(timeout=timeout))
+                time.sleep(timeout)
+
             raise UTAHProvisioningException(err, retry=True)
 
     def pingpoll(self,
 
=== modified file 'utah/provisioning/ssh.py'
--- utah/provisioning/ssh.py	2013-01-23 09:10:16 +0000
+++ utah/provisioning/ssh.py	2013-02-05 17:24:22 +0000
@@ -14,7 +14,8 @@
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 """
-Provide a mixin class for machines with SSH support.
+SSH based machine class for a provisioned system
+and SSHMixin for every machine class that needs SSH support.
 """
 
 import logging
@@ -29,6 +30,7 @@
 
 from utah import config
 from utah.provisioning.exceptions import UTAHProvisioningException
+from utah.provisioning.provisioning import Machine
 from utah.retry import retry
 
 
@@ -40,6 +42,15 @@
         # Note: Since this is a mixin it doesn't expect any argument
         # However, it calls super to initialize any other mixins in the mro
         super(SSHMixin, self).__init__(*args, **kwargs)
+        self.initialize()
+
+    def initialize(self):
+        """SSH mixin initialization
+
+        Use this method when it isn't appropriate to follow the MRO as in
+        __init__
+
+        """
         ssh_client = paramiko.SSHClient()
         ssh_client.set_missing_host_key_policy(paramiko.AutoAddPolicy())
         self.ssh_client = ssh_client
@@ -223,20 +234,29 @@
         super(SSHMixin, self).destroy(*args, **kw)
 
     def sshcheck(self, timeout=config.checktimeout):
-        """
-        Sleep for a while and check if the machine is available via ssh.
-        Return a retryable exception if it is not.
-        Intended for use with retry.
-        """
-        self.ssh_logger.info('Sleeping {timeout} seconds'
-                             .format(timeout=timeout))
-        time.sleep(timeout)
+        """Check if the machine is available via ssh.
+
+        :param timeout: Amount of time in seconds to sleep after a failure
+        :type timeout: int
+        :raises: UTAHProvisioningException
+
+        If there's a network connectivity failure, then sleep ``timeout``
+        seconds and raise a retriable exception.
+
+        .. seealso:: :func:`utah.retry.retry`, :meth:`sshpoll`
+
+        """
         self.ssh_logger.info('Checking for ssh availability')
         try:
             self.ssh_client.connect(self.name,
                                     username=config.user,
                                     key_filename=config.sshprivatekey)
         except socket.error as err:
+            if timeout > 0:
+                self.ssh_logger.info('Sleeping {timeout} seconds'
+                                     .format(timeout=timeout))
+                time.sleep(timeout)
+
             raise UTAHProvisioningException(str(err), retry=True)
 
     def sshpoll(self, timeout=None,
@@ -260,3 +280,41 @@
         if not self.active:
             self._start()
             self.sshcheck()
+
+
+class ProvisionedMachine(SSHMixin, Machine):
+    """A machine that is provisioned and can be accessed through ssh."""
+    def __init__(self, name, installtype=None):
+        SSHMixin.initialize(self)
+        self.name = name
+        self._loggersetup()
+
+        # No cleanup needed for systems that are already provisioned
+        self.clean = False
+
+        # System is expected to be available already, so there's no need to
+        # wait before trying to connect through ssh
+        self.check_timeout = 3
+        self.connectivity_timeout = 60
+
+        # TBD: Figure out install type by getting information through ssh
+        if installtype is None:
+            self.installtype = config.installtype
+
+    def activecheck(self):
+        """Check if machine is active.
+
+        Given that the machine is already provisioned, it's considered to be
+        active as long as it's reachable through ssh
+
+        """
+        try:
+            self.pingpoll(timeout=self.connectivity_timeout,
+                          checktimeout=self.check_timeout)
+        except utah.timeout.UTAHTimeout:
+            # Ignore timeout for ping, since depending on the network
+            # configuration ssh might still work despite of the ping failure.
+            self.logger.warning('Network connectivity (ping) failure')
+
+        self.sshpoll(timeout=self.connectivity_timeout,
+                     checktimeout=self.check_timeout)
 
=== modified file 'utah/run.py'
--- utah/run.py	2013-01-23 12:26:38 +0000
+++ utah/run.py	2013-02-05 17:24:22 +0000
@@ -26,6 +26,7 @@
 from utah import config
 from utah.exceptions import UTAHException
 from utah.url import url_argument
+from utah.provisioning.ssh import ProvisionedMachine
 
 
 def common_arguments(parser):
@@ -193,7 +194,10 @@
             except Exception as err:
                 logging.warning('Failed to download files: ' + str(err))
 
-    if args.outputpreseed or config.outputpreseed:
+    # Provisioned systems have an image already installed
+    # and the preseed file is no longer available
+    if (not isinstance(machine, ProvisionedMachine) and
+            (args.outputpreseed or config.outputpreseed)):
         if args.outputpreseed:
             logging.debug('Capturing preseed due to command line option')
         elif config.outputpreseed: