Merge lp:~andreserl/maas/ipmi_usercreation_ilo_versions_trunk into lp:~maas-committers/maas/trunk

[Andres Rodriguez]

This branch creates a new approach of generating maas related IPMI user. The IPMI user creation is done by slots. However, in the newest iLO versions, even though you try to create a user in SlotX, it will be created in the first available slot. This branch handles that. (This is backwards compatible with older versions)

[Scott Moser (smoser)]

Andres,
  Thanks for looking at this. I think the implementation seems generally correct, and I suspect you've tested.
  However, I'd like 2 things
a.) since this is non straight forward when you look at it, could you add some comments explaining why you're doing what you're doing?
b.) this really probably should have a bug associated with it (sorry for that nit pick, but in absence of comments, I'd go looking for a bug to read about what was going on).

review: Needs Fixing

[Andres Rodriguez (andreserl)]

I think both a and b are reflected here:

https://code.launchpad.net/~andreserl/maas/ipmi_usercreation_ilo_versions/+merge/147460

[Julian Edwards (julian-edwards)]

enlist_userdata and user_data.template are turning into (in fact have already largely turned into) a wall of unmaintainable untested code. More specifically in this case:

 - the same code is copy and pasted in two files
 - there are no unit tests for any of the code in the user data

This means that if there is a subtle bug, it won't get seen until production time, and then when someone fixes it they will very likely miss fixing it in the duplicated code.

I can't in good conscience keep approving this kind of change. Please talk to one of us about a testing strategy for embedded code in user data.

review: Needs Fixing

[Raphaël Badin (rvb)]

Looks like all the changes from this branch have been landed in https://code.launchpad.net/~andreserl/maas/ipmi_usercreation_ilo_versions/+merge/147460 right?

[Andres Rodriguez (andreserl)]

HI Raphael,

These changes have been landed for 1.2, since we both agreed (bigjools and I) that it didn't make much sense to integrate testing for it, but we needed it for trunk, which is why this is not merged.

Cheers.

[Raphaël Badin (rvb)]

As discussed with Julian, we don't really have time to add the testing so let's land this and add a bug about the lack of testing.

review: Approve

=== modified file 'contrib/preseeds_v2/enlist_userdata'
--- contrib/preseeds_v2/enlist_userdata	2012-11-30 17:35:04 +0000
+++ contrib/preseeds_v2/enlist_userdata	2013-02-27 16:16:26 +0000
@@ -40,14 +40,6 @@
       chmod "${2:-644}" "${IPMI_CONFIG_D}/$1"
    }
 
-   add_ipmi_config "01-user-privileges.ipmi" <<"END_IPMI_USER_PRIVILEGES"
-   Section User3
-        Enable_User                             Yes
-        Lan_Enable_IPMI_Msgs                    Yes
-        Lan_Privilege_Limit                     Administrator
-   EndSection
-   END_IPMI_USER_PRIVILEGES
-
    add_ipmi_config "02-global-config.ipmi" <<"END_IPMI_CONFIG"
    Section Lan_Channel
         Volatile_Access_Mode                    Always_Available
@@ -99,9 +91,23 @@
        res = show_re.search(output)
        return res.group()
 
+   def get_ipmi_user_number(user):
+       for i in range(1, 17):
+           ipmi_user_number = "User%s" % i
+           (status, output) = commands.getstatusoutput('bmc-config --checkout --key-pair="%s:Username"' % ipmi_user_number)
+           if user in output:
+               return ipmi_user_number
+       return None
+
    def commit_ipmi_user_settings(user, password):
-       (status, output) = commands.getstatusoutput('bmc-config --commit --key-pair="User3:Username=%s"' % user)
-       (status, output) = commands.getstatusoutput('bmc-config --commit --key-pair="User3:Password=%s"' % password)
+       ipmi_user_number = get_ipmi_user_number(user)
+       if ipmi_user_number is None:
+           (status, output) = commands.getstatusoutput('bmc-config --commit --key-pair="User10:Username=%s"' % user)
+           ipmi_user_number = get_ipmi_user_number(user)
+       (status, output) = commands.getstatusoutput('bmc-config --commit --key-pair="%s:Password=%s"' % (ipmi_user_number, password))
+       (status, output) = commands.getstatusoutput('bmc-config --commit --key-pair="%s:Enable_User=Yes"' % ipmi_user_number)
+       (status, output) = commands.getstatusoutput('bmc-config --commit --key-pair="%s:Lan_Enable_IPMI_Msgs=Yes"' % ipmi_user_number)
+       (status, output) = commands.getstatusoutput('bmc-config --commit --key-pair="%s:Lan_Privilege_Limit=Administrator"' % ipmi_user_number)
 
    def commit_ipmi_settings(config):
        (status, output) = commands.getstatusoutput('bmc-config --commit --filename %s' % config)
@@ -147,10 +153,7 @@
            time.sleep(120)
 
        # create user/pass
-       if args.commission_creds:
-           IPMI_MAAS_USER="maas-commission"
-       else:
-           IPMI_MAAS_USER="maas"
+       IPMI_MAAS_USER="maas"
        IPMI_MAAS_PASSWORD=generate_random_password()
 
        # Configure IPMI user/password

=== modified file 'src/metadataserver/commissioning/user_data.template'
--- src/metadataserver/commissioning/user_data.template	2013-02-06 13:59:35 +0000
+++ src/metadataserver/commissioning/user_data.template	2013-02-27 16:16:26 +0000
@@ -207,14 +207,6 @@
 
 ### begin writing files ###
 
-add_ipmi_config "01-user-privileges.ipmi" <<"END_IPMI_USER_PRIVILEGES"
-Section User3
-	Enable_User				Yes
-	Lan_Enable_IPMI_Msgs			Yes
-	Lan_Privilege_Limit			Administrator
-EndSection
-END_IPMI_USER_PRIVILEGES
-
 add_ipmi_config "02-global-config.ipmi" <<"END_IPMI_CONFIG"
 Section Lan_Channel
 	Volatile_Access_Mode			Always_Available
@@ -265,9 +257,23 @@
     res = show_re.search(output)
     return res.group()
 
+def get_ipmi_user_number(user):
+    for i in range(1, 17):
+        ipmi_user_number = "User%s" % i
+        (status, output) = commands.getstatusoutput('bmc-config --checkout --key-pair="%s:Username"' % ipmi_user_number)
+        if user in output:
+            return ipmi_user_number
+    return None
+
 def commit_ipmi_user_settings(user, password):
-    (status, output) = commands.getstatusoutput('bmc-config --commit --key-pair="User3:Username=%s"' % user)
-    (status, output) = commands.getstatusoutput('bmc-config --commit --key-pair="User3:Password=%s"' % password)
+    ipmi_user_number = get_ipmi_user_number(user)
+    if ipmi_user_number is None:
+        (status, output) = commands.getstatusoutput('bmc-config --commit --key-pair="User10:Username=%s"' % user)
+        ipmi_user_number = get_ipmi_user_number(user)
+    (status, output) = commands.getstatusoutput('bmc-config --commit --key-pair="%s:Password=%s"' % (ipmi_user_number, password))
+    (status, output) = commands.getstatusoutput('bmc-config --commit --key-pair="%s:Enable_User=Yes"' % ipmi_user_number)
+    (status, output) = commands.getstatusoutput('bmc-config --commit --key-pair="%s:Lan_Enable_IPMI_Msgs=Yes"' % ipmi_user_number)
+    (status, output) = commands.getstatusoutput('bmc-config --commit --key-pair="%s:Lan_Privilege_Limit=Administrator"' % ipmi_user_number)
 
 def commit_ipmi_settings(config):
     (status, output) = commands.getstatusoutput('bmc-config --commit --filename %s' % config)