Merge lp:~roadmr/checkbox/1260507-config-files into lp:checkbox

[Daniel Manrique]

scripts/network and scripts/virtualization can now be configured via environment variables, in addition to the existing config file mechanism and command-line parameters.

This is most useful with canonical-certification-server or other plainbox-based testing tools (i.e. in the absence of checkbox proper). In this case all configuration is centralized in the plainbox config file (/etc/xdg/plainbox.conf). These variables can be defined in the [environment] section with the familiar config file syntax.

For virtualization, the KVM_IMAGE and KVM_TIMEOUT variables are picked up.

For network, it's TEST_TARGET_FTP, TEST_TARGET_IPERF, TEST_USER and TEST_PASS. Separate variables are used for iperf and ftp to mimic the existing config file behavior (but note that a single --target command-line parameter is used in all cases).

Across the board, the preference order is command line -> environment -> config file.

scripts/network and scripts/virtualization can now be configured via environment variables, in addition to the existing config file mechanism and command-line parameters.

This is most useful with canonical-certification-server or other plainbox-based testing tools (i.e. in the absence of checkbox proper). In this case all configuration is centralized in the plainbox config file (/etc/xdg/plainbox.conf). These variables can be defined in the [environment] section with the familiar config file syntax.

For virtualization, the KVM_IMAGE and KVM_TIMEOUT variables are picked up.

For network, it's TEST_TARGET_FTP, TEST_TARGET_IPERF, TEST_USER and TEST_PASS. Separate variables are used for iperf and ftp to mimic the existing config file behavior (but note that a single --target command-line parameter is used in all cases).

Across the board, the preference order is command line -> environment -> config file.

[Zygmunt Krynicki (zyga)]

-checkbox (0.17.4) UNRELEASED; urgency=low
6 +checkbox (0.17.4ubuntu1) UNRELEASED; urgency=low

I'm sure you didn't want to send that change

review: Needs Fixing

[lp:~roadmr/checkbox/1260507-config-files]

2628. By Daniel Manrique on 2014-01-30

scripts/virtualization: refactoring of option management.

The logic and the preference sequence are a bit clearer.

It also adds support for environment variables to specify configuration
values (image and timout for KVM testing).

Note that the behavior when more than one configuration mechanism is
specified has changed; previously, command-line values would only be
used if they weren't defined in the config file. Now, command-line
always overrides environment variable, which in turn always overrides
config file.

2629. By Daniel Manrique on 2014-01-30

Pep8 fixes, save for a couple lines I'm too afraid to touch.

[Daniel Manrique (roadmr)]

Ah, the wonders of dch.

Fixed! thanks!

review: Needs Resubmitting

[Zygmunt Krynicki (zyga)]

+1 thanks

review: Approve

[Jeff Lane  (bladernr)]

Heh... so...
1: If I read that right, now the configs for virtualization and network will live in /etc/xdg/plainbox.conf. Did you add the default configs there? (I don't see it in this diff)
2: Personally, I think the separate files is a bit more intuitive as it says (Hey Tester, edit these files here for network and virtualization tests)
3: /etc/xdg/plainbox.conf is harder to find than /etc/checkbox.d was but that's a minor quibble
4: What's the order of precedence for environment variables now? they don't override command line, yes, but do pre-set env vars override the ones read from plainbox.conf? (e.g. if I have TARGET set already via the shell, will running plainbox overwrite TARGET with whatever is in plainbox.conf?)
5: has this been tested with the contents of the network.cfg and virtualization.cfg included in plainbox.conf?
6: the pep8 fixes made the diff rather hard to read, it's hard to tell what is related to this fix and what is pep8 adding spaces or doing something that doesn't really affect the script.

[Daniel Manrique (roadmr)]

>Heh... so...
>1: If I read that right, now the configs for virtualization and network will live in /etc/xdg/plainbox.conf. Did you add the default configs there? (I don't see it in this diff)

We don't create plainbox.conf by default in any of our packages, but this is a good point. I'll see if it makes sense to ship this file in one of the packages (I'm thinking plainbox-provider-checkbox).

>2: Personally, I think the separate files is a bit more intuitive as it says (Hey Tester, edit these files here for network and virtualization tests)

I agree in principle, we decided against having script-specific config files because it felt a bit kludgy, but we can also try to think of a way to do this. The naive way is to just have the scripts search in /etc/xdg as well and have plainbox-provider-checkbox install the existing configs in there, but that's just relocating and doesn't feel like it solves the issue entirely.

>3: /etc/xdg/plainbox.conf is harder to find than /etc/checkbox.d was but that's a minor quibble

that's just because the tool was called checkbox :D. If the new tool is called canonical-certification-server the equivalent would be some sort of /etc/canonical-certification or some other such.

>4: What's the order of precedence for environment variables now? they don't override command line, yes, but do pre-set env vars override the ones read from plainbox.conf? (e.g. if I have TARGET set already via the shell, will running plainbox overwrite TARGET with whatever is in plainbox.conf?)

PLainbox's env-setting magic will always give precedence to the actual environment, so in your example, no, whatever you set in the shell for TARGET would be authoritative.

>5: has this been tested with the contents of the network.cfg and virtualization.cfg included in plainbox.conf?

I didn't :/ I tested with some dummy values, I checked all possible sequences (setting env in the shell, setting env in the config file, using command-line) and in all cases it picked up the correct values. But again, I didn't just translate the existing values to equivalents in plainbox.conf :( sorry, my bad.

>6: the pep8 fixes made the diff rather hard to read, it's hard to tell what is related to this fix and what is pep8 adding spaces or doing something that doesn't really affect the script.

Yes, that's always the case when reading the complete diff on launchpad :( The pep8 fixes are in a separate commit though, so you could bzr branch and review locally, or check each individual commit in the branch (https://code.launchpad.net/~roadmr/checkbox/1260507-config-files). The meat is in commits 2626 and 2628; 2627 is just documentation and licensing stuff, while 2629 are the pep fixes.

OK, actions that will result from this comment:

- Find a good place/way to create an example/prepopulated plainbox.conf.
- Retest this mechanism with the existing values from checkbox.d to ensure they work and make sense as they did before.

We need to decide:
- Do we want to somehow split again these config files? If so, how to do it without simply relocating them, which would be a bit kludgy?
- Do we want to somehow place these files in a more-intuitive config directory that refer...

=== modified file 'checkbox-old/debian/changelog'
--- checkbox-old/debian/changelog	2014-01-28 02:06:31 +0000
+++ checkbox-old/debian/changelog	2014-01-30 14:28:16 +0000
@@ -19,11 +19,13 @@
     PCI subclass "2", like newer NVidia GPUs (LP: #1270139)
   * checkbox/scripts/audio_settings: Place/read active_profiles in an
     absolute, known-writable location (LP: #1271707)
+  * scripts/virtualization, scripts/network: Use specific environment
+    variables for configuration, if available (LP: #1260507)
 
   [ Zygmunt Krynicki ]
   * Incremented changelog
 
- -- Zygmunt Krynicki <[e-mail]>  Tue, 28 Jan 2014 03:06:27 +0100
+ -- Daniel Manrique <[e-mail]>  Wed, 29 Jan 2014 17:07:13 -0500
 
 checkbox (0.17.2) trusty; urgency=low
 

=== modified file 'checkbox-old/scripts/network'
--- checkbox-old/scripts/network	2014-01-15 22:21:57 +0000
+++ checkbox-old/scripts/network	2014-01-30 14:28:16 +0000
@@ -1,9 +1,10 @@
 #!/usr/bin/env python3
 """
-Copyright (C) 2012 Canonical Ltd.
+Copyright (C) 2012-2014 Canonical Ltd.
 
 Authors
   Jeff Marcom <[e-mail]>
+  Daniel Manrique <[e-mail]>
 
 This program is free software: you can redistribute it and/or modify
 it under the terms of the GNU General Public License version 3,
@@ -267,6 +268,7 @@
             print("Failed sending file via ftp")
             return 1
 
+
 class StressPerformanceTest:
 
     def __init__(self, interface, target):
@@ -281,7 +283,7 @@
         ping_cmd = 'ping -I {} {}'.format(self.interface, self.target)
         ping = subprocess.Popen(shlex.split(ping_cmd), stdout=subprocess.PIPE)
         iperf.communicate()
-        
+
         ping.terminate()
         (out, err) = ping.communicate()
 
@@ -301,9 +303,10 @@
             if 'unreachable' in line.lower():
                 print(line)
                 result = 1
-        
+
         return result
-        
+
+
 class Interface(socket.socket):
     """
     Simple class that provides network interface information.
@@ -367,10 +370,81 @@
         return self._read_data("device/label")
 
 
+def get_test_parameters(args, environ, config_filename):
+    # Decide the actual values for test parameters, which can come
+    # from one of three possible sources: a config file, command-line
+    # arguments, or environment variables.
+    # - If command-line args were given, they take precedence
+    # - Next come environment variables, if set.
+    # - Last, values in the config file are used if present.
+
+    params = {"test_target_ftp": None,
+              "test_user": None,
+              "test_pass": None,
+              "test_target_iperf": None}
+
+    #First (try to) load values from config file
+    config = configparser.SafeConfigParser()
+
+    try:
+        with open(config_filename) as config_file:
+            config.readfp(config_file)
+            params["test_target_ftp"] = config.get("FTP", "Target")
+            params["test_user"] = config.get("FTP", "User")
+            params["test_pass"] = config.get("FTP", "Pass")
+            params["test_target_iperf"] = config.get("IPERF", "Target")
+    except FileNotFoundError as err:
+        pass  # No biggie, we can still get configs from elsewhere
+
+    # Next see if we have environment variables to override the config file
+    # "partial" overrides are not allowed; if an env variable is missing,
+    # we won't use this at all.
+    if all([param.upper() in os.environ for param in params.keys()]):
+        for key in params.keys():
+            params[key] = os.environ[key.upper()]
+
+    # Finally, see if we have the command-line arguments that are the ultimate
+    # override. Again, we will only override if we have all of them.
+    if args.target and args.username and args.password:
+        params["test_target_ftp"] = args.target
+        params["test_user"] = args.username
+        params["test_pass"] = args.password
+        params["test_target_iperf"] = args.target
+
+    return params
+
+
 def interface_test(args):
     if not "test_type" in vars(args):
         return
 
+    # Determine whether to use the default or user-supplied config
+    # file name.
+    DEFAULT_CFG = "/etc/checkbox.d/network.cfg"
+    if not "config" in vars(args):
+        config_filename = DEFAULT_CFG
+    else:
+        config_filename = args.config
+
+    # Get the actual test data from one of three possible sources
+    test_parameters = get_test_parameters(args, os.environ, config_filename)
+
+    test_user = test_parameters["test_user"]
+    test_pass = test_parameters["test_pass"]
+    if (args.test_type.lower() == "iperf" or
+            args.test_type.lower() == "stress"):
+        test_target = test_parameters["test_target_iperf"]
+    else:
+        test_target = test_parameters["test_target_ftp"]
+
+    # Validate that we got reasonable values
+    if "example.com" in test_target:
+        # Default values found in config file
+        logging.error("Please supply target via: %s", config_filename)
+        sys.exit(1)
+
+    # Testing begins here!
+    #
     # Check and make sure that interface is indeed connected
     try:
         cmd = "ip link set dev %s up" % args.interface
@@ -382,37 +456,6 @@
     # Give interface enough time to get DHCP address
     time.sleep(10)
 
-    # Open Network config file
-    DEFAULT_CFG = "/etc/checkbox.d/network.cfg"
-    if not "config" in vars(args):
-        config_file = DEFAULT_CFG
-    else:
-        config_file = args.config
-
-    config = configparser.SafeConfigParser()
-    config.readfp(open(config_file))
-
-    # Set default network config options
-    test_target = args.target
-    test_user = args.username
-    test_pass = args.password
-
-    if test_target is None:
-        # Set FTP parameters based on config file
-        test_target = config.get("FTP", "Target")
-        test_user = config.get("FTP", "User")
-        test_pass = config.get("FTP", "Pass")
-
-        if (args.test_type.lower() == "iperf" or
-            args.test_type.lower() == "stress"):
-            test_target = config.get("IPERF", "Target")
-
-        if "example.com" in test_target:
-            # Default values found in config file
-            logging.error("Please supply target via: %s", config_file)
-            sys.exit(1)
-
-
     result = 0
     # Stop all other interfaces
     extra_interfaces = \
@@ -443,9 +486,10 @@
             result = iperf_benchmark.run()
 
         elif args.test_type.lower() == "stress":
-            stress_benchmark = StressPerformanceTest(args.interface, test_target)
+            stress_benchmark = StressPerformanceTest(args.interface,
+                                                     test_target)
             result = stress_benchmark.run()
-    
+
     for iface in extra_interfaces:
         logging.debug("Restoring interface:%s", iface)
         try:
@@ -457,6 +501,7 @@
 
     return result
 
+
 def interface_info(args):
 
     info_set = ""
@@ -476,17 +521,53 @@
 
 def main():
 
-    intro_message = "Network module\n\nThis script provides benchmarking " \
-    + "and information for a specified network interface.\n\n\n" \
-    + "Example NIC information usage:\nnetwork info -i eth0 --max-speed " \
-    + "\n\nFor running ftp benchmark test: \nnetwork test -i eth0 -t ftp " \
-    + "--target 192.168.0.1 --username USERID --password PASSW0RD " \
-    + "--filesize-2\n\nPlease note that this script can use configuration " \
-    + "values supplied via a config file.\nExample config file:\n[FTP]\n" \
-    + "Target: 192.168.1.23\nUser: FTPUser\nPass:PassW0Rd\n" \
-    + "[IPERF]\nTarget: 192.168.1.45\n**NOTE**\nDefault config location " \
-    + "is /etc/checkbox.d/network.cfg"
-
+    intro_message = """
+Network module
+
+This script provides benchmarking and information for a specified network
+interface.
+
+Example NIC information usage:
+network info -i eth0 --max-speed
+
+For running ftp benchmark test:
+network test -i eth0 -t ftp
+--target 192.168.0.1 --username USERID --password PASSW0RD
+--filesize-2
+
+Configuration
+=============
+
+Configuration can be supplied in three different ways, with the following
+priorities:
+
+1- Command-line parameters (see above).
+2- Environment variables (example will follow).
+3- Configuration file (example will follow).
+   Default config location is /etc/checkbox.d/network.cfg
+
+Environment variables
+=====================
+ALL environment variables must be defined, even if empty, for them to be
+picked up. The variables are:
+TEST_TARGET_FTP
+TEST_USER
+TEST_PASS
+TEST_TARGET_IPERF
+
+example config file
+===================
+[FTP]
+
+Target: 192.168.1.23
+User: FTPUser
+Pass:PassW0Rd
+
+[IPERF]
+Target: 192.168.1.45
+**NOTE**
+
+"""
 
     parser = ArgumentParser(
         description=intro_message, formatter_class=RawTextHelpFormatter)
@@ -502,7 +583,7 @@
     test_parser.add_argument(
         '-i', '--interface', type=str, required=True)
     test_parser.add_argument(
-        '-t', '--test_type', type=str, 
+        '-t', '--test_type', type=str,
         choices=("ftp", "iperf", "stress"), default="ftp",
         help=("[FTP *Default*]"))
     test_parser.add_argument('--target', type=str)
@@ -518,7 +599,6 @@
         default="/etc/checkbox.d/network.cfg",
         help="Supply config file for target/host network parameters")
 
-
     # Sub info options
     info_parser.add_argument(
         '-i', '--interface', type=str, required=True)

=== modified file 'checkbox-old/scripts/virtualization'
--- checkbox-old/scripts/virtualization	2013-12-12 19:35:38 +0000
+++ checkbox-old/scripts/virtualization	2014-01-30 14:28:16 +0000
@@ -3,10 +3,11 @@
 """
 Script to test virtualization functionality
 
-Copyright (C) 2013 Canonical Ltd.
+Copyright (C) 2013, 2014 Canonical Ltd.
 
 Authors
   Jeff Marcom <[e-mail]>
+  Daniel Manrique <[e-mail]>
 
 This program is free software: you can redistribute it and/or modify
 it under the terms of the GNU General Public License version 3,
@@ -31,9 +32,9 @@
 import shlex
 import signal
 from subprocess import (
-    Popen, 
-    PIPE, 
-    CalledProcessError, 
+    Popen,
+    PIPE,
+    CalledProcessError,
     check_output,
     call
 )
@@ -43,6 +44,8 @@
 import time
 import urllib.request
 
+DEFAULT_TIMEOUT = 500
+
 
 class XENTest(object):
     pass
@@ -106,19 +109,18 @@
         netrange = "10.0.0.0/8"
         image_ip = "10.0.0.1"
         hostfwd = "tcp::2222-:22"
-        cloud_disk = ""        
+        cloud_disk = ""
 
         # Should we attach the cloud config disk
         if os.path.isfile("seed.iso"):
             logging.debug("Attaching Cloud config disk")
             cloud_disk = "-drive file=seed.iso,if=virtio"
 
-
         if re.match("(arm.*|aarch64)", self.arch):
-            uname = check_output(['uname','-r'], universal_newlines=True)
+            uname = check_output(['uname', '-r'], universal_newlines=True)
             cloud_disk = cloud_disk.replace("virtio", "sd")
-            params = 'qemu-system-arm -machine vexpress-a15 -cpu cortex-a15 -enable-kvm -m {} -kernel /boot/vmlinuz -append "console=ttyAMA0 earlyprintk=serial root=/dev/mmcblk0 ro rootfstype=ext4" -serial stdio -dtb /lib/firmware/{}/device-tree/vexpress-v2p-ca15-tc1.dtb -initrd /boot/initrd.img -net nic -net user,net={},host={},hostfwd={} -drive file={},if=sd,cache=writeback {} -display none -nographic'.format(uname, "256", netrange,image_ip, hostfwd, data_disk, cloud_disk)
-        else:    
+            params = 'qemu-system-arm -machine vexpress-a15 -cpu cortex-a15 -enable-kvm -m {} -kernel /boot/vmlinuz -append "console=ttyAMA0 earlyprintk=serial root=/dev/mmcblk0 ro rootfstype=ext4" -serial stdio -dtb /lib/firmware/{}/device-tree/vexpress-v2p-ca15-tc1.dtb -initrd /boot/initrd.img -net nic -net user,net={},host={},hostfwd={} -drive file={},if=sd,cache=writeback {} -display none -nographic'.format(uname, "256", netrange, image_ip, hostfwd, data_disk, cloud_disk)
+        else:
             params = \
                 '''
                 qemu-system-x86_64 -machine accel=kvm:tcg -m {0} -net nic 
@@ -185,13 +187,12 @@
         # Create Data ISO hosting user & meta cloud config data
         try:
             iso_build = check_output(
-                ['genisoimage', '-output', 'seed.iso', '-volid', 
-                'cidata', '-joliet', '-rock', 'user-data', 'meta-data'],
+                ['genisoimage', '-output', 'seed.iso', '-volid',
+                 'cidata', '-joliet', '-rock', 'user-data', 'meta-data'],
                 universal_newlines=True)
         except CalledProcessError as exception:
             logging.exception("Cloud data disk creation failed")
 
-
     def start(self):
         logging.debug('Starting KVM Test')
         status = 1
@@ -246,6 +247,8 @@
     image = ""
     timeout = ""
 
+    # Configuration data can come from three sources.
+    # Lowest priority is the config file.
     config_file = DEFAULT_CFG
     config = configparser.SafeConfigParser()
 
@@ -254,22 +257,35 @@
     except IOError:
         logging.warn("No config file found")
     else:
-        try:    
+        try:
             timeout = config.getfloat("KVM", "timeout")
-        except ValueError: 
+        except ValueError:
             logging.warning('Invalid or Empty timeout in config file. '
-                          'Falling back to default')
+                            'Falling back to default')
         except configparser.NoSectionError as e:
             logging.exception(e)
-        
+
         try:
             image = config.get("KVM", "image")
         except configparser.NoSectionError:
             logging.exception('Invalid or Empty image in config file.')
 
-    if timeout == "":
+    # Next in priority are environment variables.
+    if 'KVM_TIMEOUT' in os.environ:
+        try:
+            timeout = float(os.environ['KVM_TIMEOUT'])
+        except ValueError as err:
+            logging.warning("TIMEOUT env variable: %s" % err)
+            timeout = DEFAULT_TIMEOUT
+    if 'KVM_IMAGE' in os.environ:
+        image = os.environ['KVM_IMAGE']
+
+    # Finally, highest-priority are command line arguments.
+    if args.timeout:
         timeout = args.timeout
-    if image == "":
+    elif not timeout:
+        timeout = DEFAULT_TIMEOUT
+    if args.image:
         image = args.image
 
     kvm_test = KVMTest(image, timeout)
@@ -294,24 +310,24 @@
     kvm_test_parser.add_argument(
         '-i', '--image', type=str, default=None)
     kvm_test_parser.add_argument(
-        '-t', '--timeout', type=int, default=500)
+        '-t', '--timeout', type=int)
     kvm_test_parser.add_argument(
         '--debug', action="store_true")
     kvm_test_parser.set_defaults(func=test_kvm)
 
     args = parser.parse_args()
-    
+
     try:
         if args.debug:
             logging.basicConfig(level=logging.DEBUG)
     except AttributeError:
-        pass #avoids exception when trying to run without specifying 'kvm'
-    
-    #to check if not len(sys.argv) > 1
+        pass  # avoids exception when trying to run without specifying 'kvm'
+
+    # to check if not len(sys.argv) > 1
     if len(vars(args)) == 0:
         parser.print_help()
         return False
-    
+
     args.func(args)
 
 if __name__ == "__main__":