Merge lp:~jeffmarcom/checkbox/kvm-cloud_data into lp:checkbox

[Jeff Marcom]

This fixes an issue where a recent change in cloud-init in the latest 12.04.2 precise cloud images prevented the VM from booting successfully if cloud config data was not supplied.

The virtualization test now creates a cloud data disk that includes generic user data and meta data files to assist during cloud-init stages of boot process. Supplying this data is dependent on whether the "cloud" string is part of the image to test.

Output from run:
DEBUG:root:Downloading precise-server-cloudimg-i386-disk1.img, from http://cloud-images.ubuntu.com
DEBUG:root:Creating cloud user-data
DEBUG:root:Creating cloud meta-data
DEBUG:root:Attempting boot for:precise-server-cloudimg-i386-disk1.img
DEBUG:root:Attaching Cloud config disk
DEBUG:root:Using params:kvm -m 256 -net nic -net user,net=10.0.0.0/8,host=10.0.0.1,hostfwd=tcp::2222-:22 -drive file=precise-server-cloudimg-i386-disk1.img,if=virtio -drive file=seed.iso,if=virtio -display none -nographic

Cloud-init v. 0.7.2 running 'modules:final' at Mon, 18 Mar 2013 17:21:40 +0000. Up 201.27 seconds.
========= CERTIFICATION TEST =========
ci-info: no authorized ssh keys fingerprints found for user ubuntu.ci-info: no authorized ssh keys fingerprints found for user ubuntu.ec2:
ec2: #############################################################
ec2: -----BEGIN SSH HOST KEY FINGERPRINTS-----
ec2: 1024 19:d3:e4:39:17:27:f2:c0:67:50:58:1b:f8:5c:72:0e root@ubuntu (DSA)
ec2: 256 13:74:68:e2:0c:fd:2a:55:e5:af:97:12:0e:69:32:75 root@ubuntu (ECDSA)
ec2: 2048 6c:f1:e8:36:3e:76:b7:a6:1f:5f:58:a1:b9:f6:58:02 root@ubuntu (RSA)
ec2: -----END SSH HOST KEY FINGERPRINTS-----
ec2: #############################################################
-----BEGIN SSH HOST KEY KEYS-----
ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBLVPgiCLVPYPvQJCf5ZpDkceNFt4pFXYael3DCMu23Nm6MM1IDLr+75jic834O4TEK7laYSHu6WJ/yb+i+/zzJg= root@ubuntu
ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDCpAF4MBa3N+L7UF5tgb0BPBUu1RutdaEnZLuSI2MwpJCIeqv57H/haysbZmVGo+T+irIUa2DQgDdQLDCJyEvBbLZFD356oV34lT5JoC20+h9xv9WjCQnxw0s5YoOsMO284j0/acCBKMAufMhe7CXVxcJGd/HE7feH2c8oaZult1yZSREqEBJoUj88TFxrEUZB7oda9Uvs1W1FWeaSr1vYRnPkXzWQRgJj2sRnpqJ4Qv6X5tAbsykeIpfuSd8uvW9qQnN50xwLkP17dJL1qSyX1XC1RBx0Uqq3rfUoAc4I8cACL+zVMinxad5KGxxhsVqK7rx/Ci5Lt6FrajYOql4j root@ubuntu
-----END SSH HOST KEY KEYS-----
Cloud-init v. 0.7.2 finished at Mon, 18 Mar 2013 17:21:44 +0000. Datasource DataSourceNoCloudNet [seed=/dev/vdb][dsmode=net]. Up 205.73 seconds

[Zygmunt Krynicki (zyga)]

32 - logging.error("Failed download {}: {}".format(image_url, exception))
33 + logging.error("Failed download: %s", exception)

You may want to replace that with:

logging.exception("Failed to download image: %s", image_url)

This has the advantage of knowing everything there is to know about the exception _and_ displaying a traceback.

89 +#cloud-config
90 +
91 +# run commands
92 +# default: none
93 +# runcmd contains a list of either lists or a string
94 +# each item will be executed in order at rc.local like level with
95 +# output to the console
96 +# - if the item is a list, the items will be properly executed as if
97 +# passed to execve(3) (with the first arg as the command).
98 +# - if the item is a string, it will be simply written to the file and
99 +# will be interpreted by 'sh'
100 +#
101 +# Note, that the list has to be proper yaml, so you have to escape
102 +# any characters yaml would eat (':' can be problematic)

Perhaps the comment in the embedded data section is not that interesting, this is an automated test after all. I would move that comment to the method, indented it as with everything else.

113 + with open(file, "wt") as data_file:
114 + os.fchmod(data_file.fileno(), 0o777)

I would use mode=0o777 there, otherwise there is a race condition where an attacker can open the file before you change the permission. After that the attacker can read or write anything.

Still, what's the point of using 0o777?

111 + for file in ['user-data', 'meta-data']:
112 + logging.debug("Creating cloud %s", file)
113 + with open(file, "wt") as data_file:
114 + os.fchmod(data_file.fileno(), 0o777)
115 + data_file.write(vars()[file.replace("-", "_")])
116 +
117 + # Create Data ISO hosting user & meta cloud config data
118 + iso_cmd = \
119 + '''
120 + genisoimage -output seed.iso -volid
121 + cidata -joliet -rock user-data meta-data
122 + '''
123 +
124 + iso_build = Popen(
125 + shlex.split(iso_cmd), stderr=PIPE, stdout=PIPE,
126 + universal_newlines=True)

I think it's much better to just use ['genisoimage', '-output', 'seed.iso', '-volid', ...] and not use shlex for that.

128 + error, output = iso_build.communicate()
129 +
130 + if iso_build.returncode != 0:
131 + logging.error("Cloud data disk creation failed")

Perhaps what you want instead is subprocess.check_output() it does everything above in one call

None of that is a critical problem though, I'm not -1 this

[Jeff Marcom (jeffmarcom)]

> 32 - logging.error("Failed download {}: {}".format(image_url, exception))
> 33 + logging.error("Failed download: %s", exception)
>
> You may want to replace that with:
>
> logging.exception("Failed to download image: %s", image_url)

Fixed
>
> This has the advantage of knowing everything there is to know about the
> exception _and_ displaying a traceback.
>
> 89 +#cloud-config
> 90 +
> 91 +# run commands
> 92 +# default: none
> 93 +# runcmd contains a list of either lists or a string
> 94 +# each item will be executed in order at rc.local like level with
> 95 +# output to the console
> 96 +# - if the item is a list, the items will be properly executed as if
> 97 +# passed to execve(3) (with the first arg as the command).
> 98 +# - if the item is a string, it will be simply written to the file
> and
> 99 +# will be interpreted by 'sh'
> 100 +#
> 101 +# Note, that the list has to be proper yaml, so you have to escape
> 102 +# any characters yaml would eat (':' can be problematic)
>
> Perhaps the comment in the embedded data section is not that interesting, this
> is an automated test after all. I would move that comment to the method,
> indented it as with everything else.

Removed extraneous comments. I believe the cloud-data comment at the top MUST be there.
>
> 113 + with open(file, "wt") as data_file:
> 114 + os.fchmod(data_file.fileno(), 0o777)
>
> I would use mode=0o777 there, otherwise there is a race condition where an
> attacker can open the file before you change the permission. After that the
> attacker can read or write anything.
K, fixed
>
> Still, what's the point of using 0o777?
Has to be executable. It's a temporary file so I thought it would be the simplest way. I don't see the race condition ever being an issue.
.
>
>
> 111 + for file in ['user-data', 'meta-data']:
> 112 + logging.debug("Creating cloud %s", file)
> 113 + with open(file, "wt") as data_file:
> 114 + os.fchmod(data_file.fileno(), 0o777)
> 115 + data_file.write(vars()[file.replace("-", "_")])
> 116 +
> 117 + # Create Data ISO hosting user & meta cloud config data
> 118 + iso_cmd = \
> 119 + '''
> 120 + genisoimage -output seed.iso -volid
> 121 + cidata -joliet -rock user-data meta-data
> 122 + '''
> 123 +
> 124 + iso_build = Popen(
> 125 + shlex.split(iso_cmd), stderr=PIPE, stdout=PIPE,
> 126 + universal_newlines=True)
>
> I think it's much better to just use ['genisoimage', '-output', 'seed.iso',
> '-volid', ...] and not use shlex for that.
>
> 128 + error, output = iso_build.communicate()
> 129 +
> 130 + if iso_build.returncode != 0:
> 131 + logging.error("Cloud data disk creation failed")

I changed it but I'm curious why exactly? Shlex was made for this kind of argument processing correct? I can't promise I won't make the same mistake again... :(
>
> Perhaps what you want instead is subprocess.check_output() it does everything
> above in one call

Actually used the error and output grabbed from communicate this time.
>
> None of that is a critical problem though, I'm not -1 this

[Zygmunt Krynicki (zyga)]

> Removed extraneous comments. I believe the cloud-data comment at the top MUST
> be there.

Sure, I meant that the comment could be in the _function_ and not in the generated _file_

> > 128 + error, output = iso_build.communicate()
> > 129 +
> > 130 + if iso_build.returncode != 0:
> > 131 + logging.error("Cloud data disk creation failed")
>
> I changed it but I'm curious why exactly? Shlex was made for this kind of
> argument processing correct? I can't promise I won't make the same mistake
> again... :(

19:28 < zyga> spideyman: as for shlex, it's just redundant: compre: data = json.loads("{'a': 5}") # data = {"a": 5}
19:28 < zyga> spideyman: so shlex is just a wrapper around the data you wanted anyway
19:30 < spideyman> zyga: understood, doing ['var', 'var', 'var', 'var', 'var', 'var', 'var', 'var' ,'var'] is tiring
19:30 < spideyman> zyga: that's why I use it, and with me at least, typing all that is error prone.

standard syntax checkers should alert you about any mistakes, hopefully you won't have to type _too_ much shell :)

> > Perhaps what you want instead is subprocess.check_output() it does
> everything
> > above in one call
>
> Actually used the error and output grabbed from communicate this time.

19:27 < zyga> spideyman: so check_output() is still shorter
19:27 < zyga> spideyman: it returns stdout
19:27 < zyga> spideyman: and raises CalledProcessErorr if retval != 0
19:27 < zyga> spideyman: that's the point I was trying to make there
19:28 < zyga> spideyman: as for shlex, it's just redundant: compre: data = json.loads("{'a': 5}") # data = {"a": 5}
19:28 < zyga> spideyman: so shlex is just a wrapper around the data you wanted anyway

If you want to land this +1, I'm fine with the code as before

[lp:~jeffmarcom/checkbox/kvm-cloud_data]

1994. By Jeff Marcom on 2013-03-18

Add classmethod for generating cloud config data based on ISO in use

Signed-off-by: Jeff Marcom <email address hidden>

1995. By Jeff Marcom on 2013-03-18

pep8 fixes

1996. By Jeff Marcom on 2013-03-18

Updated Changelog

[Zygmunt Krynicki (zyga)]

Looks good +1

review: Approve

=== modified file 'debian/changelog'
--- debian/changelog	2013-03-15 20:31:24 +0000
+++ debian/changelog	2013-03-18 18:48:19 +0000
@@ -20,8 +20,10 @@
   * jobs/input.txt.in: Added manual check job for accelerometer hardware
   * scripts/virtualization: Fixed issue where specifying test timeout via 
     vitualization.cfg was pulled in as a string rather than integer.
+  * scripts/virtualization: Added classmethod for generating cloud 
+    config data based on ISO in use
 
- -- Jeff Marcom <[e-mail]>  Fri, 15 Mar 2013 16:04:46 +0000
+ -- Jeff Marcom <[e-mail]>  Mon, 18 Mar 2013 15:04:46 +0000
 
 checkbox (0.15.4) raring; urgency=low
 

=== modified file 'scripts/virtualization'
--- scripts/virtualization	2013-03-15 15:26:35 +0000
+++ scripts/virtualization	2013-03-18 18:48:19 +0000
@@ -28,7 +28,7 @@
 import logging
 import lsb_release
 import signal
-from subprocess import Popen, PIPE
+from subprocess import Popen, PIPE, CalledProcessError, check_output
 import sys
 import tempfile
 import time
@@ -67,7 +67,7 @@
         try:
             resp = urllib.request.urlretrieve(image_url, cloud_iso)
         except urllib.error.HTTPError as exception:
-            logging.error("Failed download {}: {}".format(image_url, exception))
+            logging.exception("Failed download of image from:", image_url)
             return False
 
         if not os.path.isfile(cloud_iso):
@@ -90,22 +90,28 @@
         hostfwd = "tcp::2222-:22"
 
         params = \
-        '''
-        kvm -m {0} -net nic -net user,net={1},host={2},
-        hostfwd={3} -drive file={4},if=virtio -display none -nographic
-        '''.format(
+            '''
+            kvm -m {0} -net nic -net user,net={1},host={2},
+            hostfwd={3} -drive file={4},if=virtio -display none -nographic
+            '''.format(
             "256",
             netrange,
             image_ip,
             hostfwd,
             data_disk).replace("\n", "").replace("  ", "")
 
+        if os.path.isfile("seed.iso"):
+            logging.debug("Attaching Cloud config disk")
+
+            params = params.replace(
+                "if=virtio", "if=virtio -drive file=seed.iso,if=virtio")
+
         logging.debug("Using params:{}".format(params))
 
         # Default file location for log file is in checkbox output directory
         checkbox_dir = os.getenv("CHECKBOX_DATA")
 
-        if checkbox_dir != None:
+        if checkbox_dir is not None:
             self.debug_file = os.path.join(checkbox_dir, self.debug_file)
 
         # Open VM STDERR/STDOUT log file for writing
@@ -116,22 +122,63 @@
             return False
 
         # Start Virtual machine
-        self.process = Popen(params, stderr=file,
-            stdout=file, universal_newlines=True, shell=True)
+        self.process = Popen(
+            params, stderr=file, stdout=file,
+            universal_newlines=True, shell=True)
+
+    @classmethod
+    def create_cloud_disk(cls):
+        """
+        Generate Cloud meta data and creates an iso object
+        to be mounted as virtual device to instance during boot.
+        """
+
+        user_data = """\
+#cloud-config
+
+runcmd:
+ - [ sh, -c, echo "========= CERTIFICATION TEST =========" ]
+"""
+
+        meta_data = """\
+{ echo instance-id: iid-local01; echo local-hostname, certification; }
+"""
+
+        for file in ['user-data', 'meta-data']:
+            logging.debug("Creating cloud %s", file)
+            with open(file, "wt") as data_file:
+                os.fchmod(data_file.fileno(), 0o777)
+                data_file.write(vars()[file.replace("-", "_")])
+
+        # 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'],
+                universal_newlines=True)
+        except CalledProcessError as exception:
+            logging.exception("Cloud data disk creation failed")
+
 
     def start(self):
         status = 1
         # Create temp directory:
-        with tempfile.TemporaryDirectory("_kvm_test", 
-        time.strftime("%b_%d_%Y_")) as temp_dir:
-            
+
+        date = time.strftime("%b_%d_%Y_")
+        with tempfile.TemporaryDirectory("_kvm_test", date) as temp_dir:
+
             os.chdir(temp_dir)
             if self.image is None:
                 # Download cloud image
                 self.image = self.download_image()
 
             if os.path.isfile(self.image):
- 
+
+                if "cloud" in self.image:
+                    # Will assume we need to supply cloud meta data
+                    # for instance boot to be successful
+                    self.create_cloud_disk()
+
                 # Boot Virtual Machine
                 instance = self.boot_image(self.image)
 
@@ -148,8 +195,8 @@
             else:
                 print("Could not find: {}".format(self.image), file=sys.stderr)
 
-        return status  
-     
+        return status
+
 
 def test_kvm(args):
     print("Executing KVM Test", file=sys.stderr)
@@ -164,7 +211,7 @@
     try:
         config.readfp(open(config_file))
         timeout = int(config.get("KVM", "timeout"))
-        image = config.get("KVM", "image") 
+        image = config.get("KVM", "image")
     except IOError:
         logging.warn("No config file found")
     except Exception as exception:
@@ -187,19 +234,19 @@
     subparsers = parser.add_subparsers()
 
     # Main cli options
-    kvm_test_parser = subparsers.add_parser('kvm',
-        help=("Run kvm virtualization test"))
+    kvm_test_parser = subparsers.add_parser(
+        'kvm', help=("Run kvm virtualization test"))
 
     #xen_test_parser = subparsers.add_parser('xen',
     #    help=("Run xen virtualization test"))
 
     # Sub test options
-    kvm_test_parser.add_argument('-i', '--image',
-        type=str, default=None)
-    kvm_test_parser.add_argument('-t', '--timeout',
-        type=int, default=500)
-    kvm_test_parser.add_argument('--debug',
-        action="store_true")
+    kvm_test_parser.add_argument(
+        '-i', '--image', type=str, default=None)
+    kvm_test_parser.add_argument(
+        '-t', '--timeout', type=int, default=500)
+    kvm_test_parser.add_argument(
+        '--debug', action="store_true")
     kvm_test_parser.set_defaults(func=test_kvm)
 
     args = parser.parse_args()
@@ -209,8 +256,5 @@
 
     args.func(args)
 
-    
-    
-
 if __name__ == "__main__":
-    main();
+    main()