Merge lp:~alexandru-sirbu/cloud-init/bigstep-datasource-improvements into lp:~cloud-init-dev/cloud-init/trunk

[Alex Sirbu]

Based on the proposal of smoser:

<smoser> Odd_Bloke, when you dont have that file there..
<smoser> i dontrecall. does cloud-init warn that the ds raised exception ?
<smoser> can you just try and catch the load_file and return false if not there ?

+ Added Bigstep as one of the default datasources in the settings.py file

[Dan Watkins (daniel-thewatkins)]

review: Needs Fixing

[Alex Sirbu (alexandru-sirbu)]

Commented on reviews.

[Dan Watkins (daniel-thewatkins)]

On 07/03/16 12:18, Alex Sirbu wrote:
>> === modified file 'cloudinit/sources/DataSourceBigstep.py'
>> --- cloudinit/sources/DataSourceBigstep.py 2016-03-02 08:53:47 +0000
>> +++ cloudinit/sources/DataSourceBigstep.py 2016-03-07 09:36:05 +0000
>> @@ -22,7 +23,13 @@
>> self.userdata_raw = ""
>>
>> def get_data(self, apply_filter=False):
>> - url = get_url_from_file()
>> + try:
>
> So, have the try catch there and return a NULL value or something similar and then return False if the url returned by the function has that value?

Yeah, I think that makes sense.

>> + url = get_url_from_file()
>> + except IOError as e:
>> + if e.errno == errno.ENOENT:
>
> Exactly, if the file does not exist (the case covered by the ENOENT) then it is ok, as we probably are on a non-Bigstep server. However, if another error is raised, then it is probably worthwhile to see and investigate the issue and is probably not caused by the inexistance of the file. The same error catching is also used in the load_config function in the read_conf from the util file -> the inexistance of the file is treated as an empty config, any other error is thrown anyway. Does an explanation need to be added in this case?

It's not immediately obvious to me, so I would still like one. Just
something like "If the file doesn't exist, this isn't Bigstep;
otherwise, this is broken Bigstep".

[lp:~alexandru-sirbu/cloud-init/bigstep-datasource-improvements]

1179. By Alex Sirbu on 2016-03-07

Implemented review concerning position of try and more information about the caught exception.

[Dan Watkins (daniel-thewatkins)]

review: Approve

[Dan Watkins (daniel-thewatkins)]

Thanks Alex, this looks good to me; I've added smoser to confirm that this addresses his concern.

=== modified file 'cloudinit/settings.py'
--- cloudinit/settings.py	2015-03-04 17:42:34 +0000
+++ cloudinit/settings.py	2016-03-07 12:31:21 +0000
@@ -42,6 +42,7 @@
         'CloudSigma',
         'CloudStack',
         'SmartOS',
+        'Bigstep',
         # At the end to act as a 'catch' when none of the above work...
         'None',
     ],

=== modified file 'cloudinit/sources/DataSourceBigstep.py'
--- cloudinit/sources/DataSourceBigstep.py	2016-03-02 08:53:47 +0000
+++ cloudinit/sources/DataSourceBigstep.py	2016-03-07 12:31:21 +0000
@@ -5,6 +5,7 @@
 #
 
 import json
+import errno
 
 from cloudinit import log as logging
 from cloudinit import sources
@@ -23,6 +24,8 @@
 
     def get_data(self, apply_filter=False):
         url = get_url_from_file()
+        if url is None:
+            return False
         response = url_helper.readurl(url)
         decoded = json.loads(response.contents)
         self.metadata = decoded["metadata"]
@@ -32,7 +35,15 @@
 
 
 def get_url_from_file():
-    content = util.load_file("/var/lib/cloud/data/seed/bigstep/url")
+    try:
+        content = util.load_file("/var/lib/cloud/data/seed/bigstep/url")
+    except IOError as e:
+        # If the file doesn't exist, then the server probably isn't a Bigstep
+        # instance; otherwise, another problem exists which needs investigation
+        if e.errno == errno.ENOENT:
+            return None
+        else:
+            raise
     return content
 
 # Used to match classes to dependencies