Merge lp:~kiril-vladimiroff/cloud-init/cloudsigma-smbios-data-check into lp:~cloud-init-dev/cloud-init/trunk

[Kiril Vladimiroff]

Changes made to the CloudSigma DS:

- Add timeouts for reading/writting from/to the serial console
- Check if the cloud-init is running in CloudSigma's infrastructure before trying to fetch data from its serial console (Fixes #1316475)

[Scott Moser (smoser)]

This looks reasonable. I only have a few minor comments:
 a.) READ_TIMEOUT and WRITE_TIMEOUT of 60 seconds and 10 seconds seem a bit high to me. Is there a need for that to be so high? I suspect if the other end of your datasource did not return in < 5 seconds to a query that you'd be looking to fix that very quickly.
 b.) did you want to allow configuration of the string 'cloudsigma' and/or the 'is_running_in_clouds' check?
    doc/examples/cloud-config-datasources.txt has examples of other configuration that datasources have. I might just suggest:
    datasource:
      CloudSigma:
        system_product_name: (null|cloudsigma)

    If null then do not bother checking. If set to some other value (such as 'cloudsigma') then check and disable if not present.

I'd like the timeouts lowered unless you have a good reason for them to be so high( as we've seen negative fallout of this already). I'm fine to take it without the configuration though. That part is up to you.

review: Needs Fixing

[Scott Moser (smoser)]

one other thing. due to the unavailability (and general failure) on arm, you'll need to modify your tests similar to I did in revision 960. Basically, even if dmi-decode is available, we don't want to run it on arm.

[Kiril Vladimiroff (kiril-vladimiroff)]

a) When a virtual machine from our marketplace is cloned all the
metadata for it gets cloned, too (arch, os, distribution, version and
most all: **install notes**). And install notes could be huge. When
this data is a lot, a lot of time is spent on receiving it. So, since
we have the SMBIOS check now, we're sure such serial read/writes will
be executed only in our cloud. In other words: especially the read
timeout is completely reasonable.

b) We're going to deploy the new version of our cloud soon and it's
useless to put this as configuration option. Will use custom builds
until then.

> Basically, even if dmi-decode is available, we don't want to run it on arm.

Could you please explain why? I don't see a reason to filter by arch,
since the SMBIOS check is pretty straightforward.
Kiril Vladimiroff

Software Developer

CloudSigma

E: <email address hidden>

T: www.twitter.com/CloudSigma

W: www.cloudsigma.com

The first exchange for trading IaaS cloud resources has arrived:

http://cld.sg/14RyTYG.

CloudSigma is a proud launch partner with Deutsche Börse:

http://cld.sg/17Xcbo7.

CloudSigma named Cool Vendor 2013 by Gartner Research: http://cld.sg/10G5az6

====================

This email is from CLOUDSIGMA AG. The contents of this email and any
attachments are confidential to the intended recipient. They may not
be disclosed to or used by or copied in any way by anyone other than
the intended recipient. If this email is received in error, please
contact CLOUDSIGMA AG on [phone] quoting the name of the
sender and the email address to which it has been sent and then delete
it. Please note that neither CLOUDSIGMA AG nor the sender accepts any
responsibility for viruses and it is your responsibility to scan or
otherwise check this email and any attachments. CLOUDSIGMA AG is a
public limited company registered in Canton Zürich, Switzerland
(registered number CH-020.3.034.422-0) with registered offices at
Sägereistrasse 35, 8152 Glattbrugg, Switzerland. For further
information, please refer to www.cloudsigma.com .

====================

On Fri, May 30, 2014 at 5:15 PM, Scott Moser <email address hidden> wrote:
> one other thing. due to the unavailability (and general failure) on arm, you'll need to modify your tests similar to I did in revision 960. Basically, even if dmi-decode is available, we don't want to run it on arm.
>
> --
> https://code.launchpad.net/~kiril-vladimiroff/cloud-init/cloudsigma-smbios-data-check/+merge/221524
> You are the owner of lp:~kiril-vladimiroff/cloud-init/cloudsigma-smbios-data-check.

[Kiril Vladimiroff (kiril-vladimiroff)]

a) When a virtual machine from our marketplace is cloned all the
metadata for it gets cloned, too (arch, os, distribution, version and
most all: **install notes**). And install notes could be huge. When
this data is a lot, a lot of time is spent on receiving it. So, since
we have the SMBIOS check now, we're sure such serial read/writes will
be executed only in our cloud. In other words: especially the read
timeout is completely reasonable.

b) We're going to deploy the new version of our cloud soon and it's
useless to put this as configuration option. Will use custom builds
until then.

> Basically, even if dmi-decode is available, we don't want to run it on arm.

Could you please explain why? I don't see a reason to filter by arch,
since the SMBIOS check is pretty straightforward.

P.S.: It seems that replies to the received email from this merge proposal don't show up here.

[Scott Moser (smoser)]

dmi-decode can actually crash arm guests or hosts. yes, thats completely silly, but true.
https://bugs.launchpad.net/qemu/+bug/1243287

kvm guests will simply die (killing kvm), and it is reported to me that bare metal hosts can crash similarly.

[Kiril Vladimiroff (kiril-vladimiroff)]

Oh, well... that's a reason. Will take a look at it tomorrow and see how to do the architecture checks, too.

Should I push these new arch checks to this merge proposal or open a new one?

[Kiril Vladimiroff (kiril-vladimiroff)]

I've pushed changes that disables the whole data source on arm to this very same branch. I however don't see it appended to the current merge proposal. What should I do in order to achieve that?

The revision is 974 (https://bazaar.launchpad.net/~kiril-vladimiroff/cloud-init/cloudsigma-smbios-data-check/revision/974?start_revid=974) ?

[Scott Moser (smoser)]

Hey, I added some ifarch and merged branch yesterday. Please make sure trunk looks good now.

On May 31, 2014 4:20:08 AM EDT, Kiril Vladimiroff <email address hidden> wrote:
>I've pushed changes that disables the whole data source on arm to this
>very same branch. I however don't see it appended to the current merge
>proposal. What should I do in order to achieve that?
>
>The revision is 974
>(https://bazaar.launchpad.net/~kiril-vladimiroff/cloud-init/cloudsigma-smbios-data-check/revision/974?start_revid=974)
>?
>--
>https://code.launchpad.net/~kiril-vladimiroff/cloud-init/cloudsigma-smbios-data-check/+merge/221524
>You are reviewing the proposed merge of
>lp:~kiril-vladimiroff/cloud-init/cloudsigma-smbios-data-check into
>lp:cloud-init.

[Kiril Vladimiroff (kiril-vladimiroff)]

Well it's the same check I've pushed :)

Super. We can consider this one fixed.

=== modified file 'cloudinit/cs_utils.py'
--- cloudinit/cs_utils.py	2014-02-12 10:14:49 +0000
+++ cloudinit/cs_utils.py	2014-05-30 11:27:37 +0000
@@ -35,6 +35,8 @@
 
 import serial
 
+READ_TIMEOUT = 60
+WRITE_TIMEOUT = 10
 SERIAL_PORT = '/dev/ttyS1'
 if platform.system() == 'Windows':
     SERIAL_PORT = 'COM2'
@@ -76,7 +78,9 @@
         self.result = self._marshal(self.raw_result)
 
     def _execute(self):
-        connection = serial.Serial(SERIAL_PORT)
+        connection = serial.Serial(port=SERIAL_PORT,
+                                   timeout=READ_TIMEOUT,
+                                   writeTimeout=WRITE_TIMEOUT)
         connection.write(self.request)
         return connection.readline().strip('\x04\n')
 

=== modified file 'cloudinit/sources/DataSourceCloudSigma.py'
--- cloudinit/sources/DataSourceCloudSigma.py	2014-03-04 17:11:10 +0000
+++ cloudinit/sources/DataSourceCloudSigma.py	2014-05-30 11:27:37 +0000
@@ -20,6 +20,7 @@
 
 from cloudinit import log as logging
 from cloudinit import sources
+from cloudinit import util
 from cloudinit.cs_utils import Cepko
 
 LOG = logging.getLogger(__name__)
@@ -40,12 +41,33 @@
         self.ssh_public_key = ''
         sources.DataSource.__init__(self, sys_cfg, distro, paths)
 
+    def is_running_in_cloudsigma(self):
+        """
+        Uses dmidecode to detect if this instance of cloud-init is running
+        in the CloudSigma's infrastructure.
+        """
+        dmidecode_path = util.which('dmidecode')
+        if not dmidecode_path:
+            return False
+
+        LOG.debug("Determining hypervisor product name via dmidecode")
+        try:
+            system_product_name, _ = util.subp([dmidecode_path, "-s", "system-product-name"])
+            return 'cloudsigma' in system_product_name.lower()
+        except:
+            LOG.exception("Failed to get hypervisor product name")
+
+        return False
+
     def get_data(self):
         """
         Metadata is the whole server context and /meta/cloud-config is used
         as userdata.
         """
         dsmode = None
+        if not self.is_running_in_cloudsigma():
+            return False
+
         try:
             server_context = self.cepko.all().result
             server_meta = server_context['meta']

=== modified file 'tests/unittests/test_datasource/test_cloudsigma.py'
--- tests/unittests/test_datasource/test_cloudsigma.py	2014-02-19 08:45:53 +0000
+++ tests/unittests/test_datasource/test_cloudsigma.py	2014-05-30 11:27:37 +0000
@@ -35,6 +35,7 @@
 class DataSourceCloudSigmaTest(TestCase):
     def setUp(self):
         self.datasource = DataSourceCloudSigma.DataSourceCloudSigma("", "", "")
+        self.datasource.is_running_in_cloudsigma = lambda: True
         self.datasource.cepko = CepkoMock(SERVER_CONTEXT)
         self.datasource.get_data()