Merge lp:~jason-hobbs/maas/fix-ipmi-settings-order-lp-1289456 into lp:~maas-committers/maas/trunk

[Jason Hobbs]

Restore the original order used for applying IPMI user settings in maas-ipmi-autodetect.

The order changed during a refactor, because settings were moved from being applied in a sequence of function calls to being applied by iterating over a dict. Now, settings are applied by iterating over an OrderedDict with keys in the same order as the original sequence of function calls.

[Andres Rodriguez (andreserl)]

lgtm! Provided this has been tested, make lint passes and test pass. Land it!

review: Approve

=== modified file 'etc/maas/templates/commissioning-user-data/snippets/maas_ipmi_autodetect.py'
--- etc/maas/templates/commissioning-user-data/snippets/maas_ipmi_autodetect.py	2014-03-04 22:38:39 +0000
+++ etc/maas/templates/commissioning-user-data/snippets/maas_ipmi_autodetect.py	2014-03-07 18:25:04 +0000
@@ -37,6 +37,8 @@
 import subprocess
 import time
 
+from collections import OrderedDict
+
 
 class IPMIError(Exception):
     """An error related to IPMI."""
@@ -194,6 +196,29 @@
     verify_ipmi_user_settings(ipmi_user_number, user_settings)
 
 
+def make_ipmi_user_settings(username, password):
+    """Factory for IPMI user settings."""
+    # Some BMCs care about the order these settings are applied in,
+    # particulary what's done prior to the user being enabled. This
+    # behavior was seen on Dell Poweredge R240 systems, but may affect
+    # others too. Don't change the order of these without lots of
+    # testing.
+    user_settings = OrderedDict((
+        ('Username', username),
+        ('Password', password),
+        ('Enable_User', 'Yes'),
+        ('Lan_Enable_IPMI_Msgs', 'Yes'),
+        ('Lan_Privilege_Limit', 'Administrator'),
+    ))
+    return user_settings
+
+
+def configure_ipmi_user(username, password):
+    """Create or configure an IPMI user for remote use."""
+    user_settings = make_ipmi_user_settings(username, password)
+    apply_ipmi_user_settings(user_settings)
+
+
 def commit_ipmi_settings(config):
     run_command(('bmc-config', '--commit', '--filename', config))
 
@@ -251,17 +276,7 @@
     IPMI_MAAS_USER = "maas"
     IPMI_MAAS_PASSWORD = generate_random_password()
 
-    # Configure IPMI user/password
-
-    user_settings = {
-        'Username': IPMI_MAAS_USER,
-        'Password': IPMI_MAAS_PASSWORD,
-        'Enable_User': 'Yes',
-        'Lan_Enable_IPMI_Msgs': 'Yes',
-        'Lan_Privilege_Limit': 'Administrator'
-    }
-
-    apply_ipmi_user_settings(user_settings)
+    configure_ipmi_user(IPMI_MAAS_USER, IPMI_MAAS_PASSWORD)
 
     # Commit other IPMI settings
     if args.configdir:

=== modified file 'etc/maas/templates/commissioning-user-data/snippets/tests/test_maas_ipmi_autodetect.py'
--- etc/maas/templates/commissioning-user-data/snippets/tests/test_maas_ipmi_autodetect.py	2014-03-04 22:38:39 +0000
+++ etc/maas/templates/commissioning-user-data/snippets/tests/test_maas_ipmi_autodetect.py	2014-03-07 18:25:04 +0000
@@ -17,10 +17,13 @@
 from collections import OrderedDict
 import subprocess
 
+from mock import call
+
 from maastesting.factory import factory
 from maastesting.matchers import (
     MockAnyCall,
     MockCalledOnceWith,
+    MockCallsMatch,
     )
 from maastesting.testcase import MAASTestCase
 from snippets import maas_ipmi_autodetect
@@ -30,9 +33,11 @@
     bmc_supports_lan2_0,
     bmc_user_get,
     commit_ipmi_settings,
+    configure_ipmi_user,
     format_user_key,
     IPMIError,
     list_user_numbers,
+    make_ipmi_user_settings,
     pick_user_number,
     pick_user_number_from_list,
     run_command,
@@ -381,7 +386,7 @@
         """Ensure the user settings are committed and verified."""
         user_number = 'User2'
         self.patch(maas_ipmi_autodetect,
-                   'pick_user_number').return_value = 'User2'
+                   'pick_user_number').return_value = user_number
         bus_mock = self.patch(maas_ipmi_autodetect, 'bmc_user_set')
         vius_mock = self.patch(maas_ipmi_autodetect,
                                'verify_ipmi_user_settings')
@@ -395,6 +400,52 @@
             vius_mock,
             MockCalledOnceWith(user_number, user_settings))
 
+    def test_preserves_settings_order(self):
+        """Ensure user settings are applied in order of iteration."""
+        user_number = 'User2'
+        self.patch(maas_ipmi_autodetect,
+                   'pick_user_number').return_value = user_number
+        bus_mock = self.patch(maas_ipmi_autodetect, 'bmc_user_set')
+        self.patch(maas_ipmi_autodetect, 'verify_ipmi_user_settings')
+        user_settings = OrderedDict((('Username', 1), ('b', 2), ('c', 3)))
+        apply_ipmi_user_settings(user_settings)
+        expected_calls = (
+            call(user_number, key, value)
+            for key, value in user_settings.iteritems()
+        )
+        self.assertThat(bus_mock, MockCallsMatch(*expected_calls))
+
+
+class TestMakeIPMIUserSettings(MAASTestCase):
+    """Tests for make_ipmi_user_settings()."""
+
+    def test_settings_ordered_correctly(self):
+        """Ensure user settings are listed in the right order."""
+        settings = make_ipmi_user_settings('user', 'pass')
+        expected = ['Username', 'Password', 'Enable_User']
+        self.assertEqual(expected, settings.keys()[:3])
+
+    def test_uses_username_and_password(self):
+        """Ensure username and password supplied are used."""
+        username = 'user'
+        password = 'pass'
+        settings = make_ipmi_user_settings(username, password)
+        self.assertEquals(username, settings['Username'])
+        self.assertEquals(password, settings['Password'])
+
+
+class TestConfigureIPMIUser(MAASTestCase):
+    """Tests for configure_ipmi_user()."""
+
+    def test_preserves_setting_order(self):
+        """Ensure the order of user settings isn't modified."""
+        expected = OrderedDict((('a', 1), ('b', 2), ('c', 3)))
+        self.patch(maas_ipmi_autodetect,
+                   'make_ipmi_user_settings').return_value = expected.copy()
+        recorder = self.patch(maas_ipmi_autodetect, 'apply_ipmi_user_settings')
+        configure_ipmi_user('DC', 'DC')
+        self.assertThat(recorder, MockCalledOnceWith(expected))
+
 
 class TestCommitIPMISettings(MAASTestCase):
     """Test commit_ipmi_settings()."""