Merge lp:~andreserl/maas/ipmi_usercreation_ilo_versions into lp:maas/1.2

[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)

[Julian Edwards (julian-edwards)]

The amount of untested code going into the preseeds is frankly frightening. We have to come up with a testing scenario for this code ASAP.

[Andres Rodriguez (andreserl)]

While I agree that the preseeds are untested, I don't agree that in this particular scenario tested code would have helped as your comment seems to imply. The reason of this bug/branch is far from being unstested or, is possibility of discovery is far from being that case.

This is present due to the implementation differences between iLO versions (which is this case applies for 1.X and 2.X), and which differs from the latest 3.X.

Now, in favor of having things like this tested, you would either have to test it in different proper hardware, or simulate the implementations to perform the accurate tests.

[Julian Edwards (julian-edwards)]

On Monday 11 Feb 2013 01:13:24 you wrote:
> While I agree that the preseeds are untested, I don't agree that in this
> particular scenario tested code would have helped as your comment seems to
> imply. The reason of this bug/branch is far from being unstested or, is
> possibility of discovery is far from being that case.
>
> This is present due to the implementation differences between iLO versions
> (which is this case applies for 1.X and 2.X), and which differs from the
> latest 3.X.
>
> Now, in favor of having things like this tested, you would either have to
> test it in different proper hardware, or simulate the implementations to
> perform the accurate tests.

I wasn't commenting about this particular scenario; it is indeed an
unfortunate bug.

However it's not an excuse to not have test coverage. Would could easily have
prevented bug 1103716, for example, with proper test coverage. We don't need
to test in real hardware situations (that's what QA is for), we just need unit
tests to make sure the preseeds are not broken in stupid ways.

[Julian Edwards (julian-edwards)]

Sorry for the IRC paste:

<roaksoax> bigjools: how are you guys testing preseeds? I need to get this asap:
<roaksoax> https://code.launchpad.net/~andreserl/maas/ipmi_usercreation_ilo_versions/+merge/147460
<bigjools> roaksoax: there is no preseed testing at all
<roaksoax> bigjools: howdy btw :)
<bigjools> yo :)
<roaksoax> bigjools: so what can be done with that? I'm just really worried about that to hit maas/1.2 since FF is next week :)
<roaksoax> and I wanted to upload maas this week
<bigjools> roaksoax: you could do what jtv did with the user data. He split out the python bits to separate files that are included in the main template
<bigjools> then make sure the python bits are unit-testable
<roaksoax> bigjools: right, that;'s for trunk though
<roaksoax> bigjools: i don't want to introduce that big of a change for maas/1.2
<bigjools> just splitting them out is good enough for me, we can add tests later
<bigjools> it's a tiny change
<roaksoax> bigjools: right, though the split you mention is not backported to maas/1.2
<roaksoax> bigjools: and jtv did that with commissioning only, not enlistment :)
<bigjools> I know
<roaksoax> since enlistment is not controlled by a metadataserver
<bigjools> oh wait, are our preseeds not templated?
<bigjools> crapola
<bigjools> ok yes it's not a small change then :)
<roaksoax> yeah the commissioning stuff is not templated in lp:maas/1.2
<roaksoax> bigjools: that's why I was suggesting to get that in as it is only for lp:maas/1.2 since it is really a bugfix
<bigjools> roaksoax: +1, although I hate it :(
<bigjools> roaksoax: I think you need someone like smoser to sanity check that, I don't really know enough about iLO

[Julian Edwards (julian-edwards)]

In summary: get a +1 from smoser and this can land. We'll fix tests in trunk.

[Scott Moser (smoser)]

Its sane enough.
We've verified it doesn't cause regression in lab and via other testing, so I think we need to go with it.

review: Approve

=== modified file 'contrib/preseeds_v2/enlist_userdata'
--- contrib/preseeds_v2/enlist_userdata	2012-11-30 17:34:43 +0000
+++ contrib/preseeds_v2/enlist_userdata	2013-02-27 15:46:21 +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 'etc/maas/commissioning-user-data'
--- etc/maas/commissioning-user-data	2013-02-06 16:17:19 +0000
+++ etc/maas/commissioning-user-data	2013-02-27 15:46:21 +0000
@@ -198,14 +198,6 @@
 lshw -xml
 END_LSHW
 
-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
@@ -256,9 +248,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)