MAAS

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

  1. ipmi_usercreation_ilo_versions_trunk
  2. Merge into trunk
Proposed by Andres Rodriguez
Status: Merged
Approved by: Andres Rodriguez
Approved revision: no longer in the source branch.
Merged at revision: 1459
Proposed branch: lp:~andreserl/maas/ipmi_usercreation_ilo_versions_trunk
Merge into: lp:~maas-committers/maas/trunk
Diff against target: 101 lines (+33/-24)
2 files modified
contrib/preseeds_v2/enlist_userdata (+17/-14)
src/metadataserver/commissioning/user_data.template (+16/-10)
To merge this branch: bzr merge lp:~andreserl/maas/ipmi_usercreation_ilo_versions_trunk
Reviewer Review Type Date Requested Status
Raphaël Badin (community) Approve
Julian Edwards (community) Needs Fixing
Scott Moser (community) Needs Fixing
Review via email: mp+148579@code.launchpad.net

Commit message

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)

To post a comment you must log in.
Revision history for this message
Scott Moser (smoser) wrote :

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
Revision history for this message
Andres Rodriguez (andreserl) wrote :

I think both a and b are reflected here:

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

Revision history for this message
Julian Edwards (julian-edwards) wrote :

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
Revision history for this message
Raphaël Badin (rvb) wrote :

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?

Revision history for this message
Andres Rodriguez (andreserl) wrote :

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.

Revision history for this message
Raphaël Badin (rvb) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
40 chmod "${2:-644}" "${IPMI_CONFIG_D}/$1"40 chmod "${2:-644}" "${IPMI_CONFIG_D}/$1"
41 }41 }
4242
43 add_ipmi_config "01-user-privileges.ipmi" <<"END_IPMI_USER_PRIVILEGES"
44 Section User3
45 Enable_User Yes
46 Lan_Enable_IPMI_Msgs Yes
47 Lan_Privilege_Limit Administrator
48 EndSection
49 END_IPMI_USER_PRIVILEGES
50
51 add_ipmi_config "02-global-config.ipmi" <<"END_IPMI_CONFIG"43 add_ipmi_config "02-global-config.ipmi" <<"END_IPMI_CONFIG"
52 Section Lan_Channel44 Section Lan_Channel
53 Volatile_Access_Mode Always_Available45 Volatile_Access_Mode Always_Available
@@ -99,9 +91,23 @@
99 res = show_re.search(output)91 res = show_re.search(output)
100 return res.group()92 return res.group()
10193
94 def get_ipmi_user_number(user):
95 for i in range(1, 17):
96 ipmi_user_number = "User%s" % i
97 (status, output) = commands.getstatusoutput('bmc-config --checkout --key-pair="%s:Username"' % ipmi_user_number)
98 if user in output:
99 return ipmi_user_number
100 return None
101
102 def commit_ipmi_user_settings(user, password):102 def commit_ipmi_user_settings(user, password):
103 (status, output) = commands.getstatusoutput('bmc-config --commit --key-pair="User3:Username=%s"' % user)103 ipmi_user_number = get_ipmi_user_number(user)
104 (status, output) = commands.getstatusoutput('bmc-config --commit --key-pair="User3:Password=%s"' % password)104 if ipmi_user_number is None:
105 (status, output) = commands.getstatusoutput('bmc-config --commit --key-pair="User10:Username=%s"' % user)
106 ipmi_user_number = get_ipmi_user_number(user)
107 (status, output) = commands.getstatusoutput('bmc-config --commit --key-pair="%s:Password=%s"' % (ipmi_user_number, password))
108 (status, output) = commands.getstatusoutput('bmc-config --commit --key-pair="%s:Enable_User=Yes"' % ipmi_user_number)
109 (status, output) = commands.getstatusoutput('bmc-config --commit --key-pair="%s:Lan_Enable_IPMI_Msgs=Yes"' % ipmi_user_number)
110 (status, output) = commands.getstatusoutput('bmc-config --commit --key-pair="%s:Lan_Privilege_Limit=Administrator"' % ipmi_user_number)
105111
106 def commit_ipmi_settings(config):112 def commit_ipmi_settings(config):
107 (status, output) = commands.getstatusoutput('bmc-config --commit --filename %s' % config)113 (status, output) = commands.getstatusoutput('bmc-config --commit --filename %s' % config)
@@ -147,10 +153,7 @@
147 time.sleep(120)153 time.sleep(120)
148154
149 # create user/pass155 # create user/pass
150 if args.commission_creds:156 IPMI_MAAS_USER="maas"
151 IPMI_MAAS_USER="maas-commission"
152 else:
153 IPMI_MAAS_USER="maas"
154 IPMI_MAAS_PASSWORD=generate_random_password()157 IPMI_MAAS_PASSWORD=generate_random_password()
155158
156 # Configure IPMI user/password159 # Configure IPMI user/password
157160
=== 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 @@
207207
208### begin writing files ###208### begin writing files ###
209209
210add_ipmi_config "01-user-privileges.ipmi" <<"END_IPMI_USER_PRIVILEGES"
211Section User3
212 Enable_User Yes
213 Lan_Enable_IPMI_Msgs Yes
214 Lan_Privilege_Limit Administrator
215EndSection
216END_IPMI_USER_PRIVILEGES
217
218add_ipmi_config "02-global-config.ipmi" <<"END_IPMI_CONFIG"210add_ipmi_config "02-global-config.ipmi" <<"END_IPMI_CONFIG"
219Section Lan_Channel211Section Lan_Channel
220 Volatile_Access_Mode Always_Available212 Volatile_Access_Mode Always_Available
@@ -265,9 +257,23 @@
265 res = show_re.search(output)257 res = show_re.search(output)
266 return res.group()258 return res.group()
267259
260def get_ipmi_user_number(user):
261 for i in range(1, 17):
262 ipmi_user_number = "User%s" % i
263 (status, output) = commands.getstatusoutput('bmc-config --checkout --key-pair="%s:Username"' % ipmi_user_number)
264 if user in output:
265 return ipmi_user_number
266 return None
267
268def commit_ipmi_user_settings(user, password):268def commit_ipmi_user_settings(user, password):
269 (status, output) = commands.getstatusoutput('bmc-config --commit --key-pair="User3:Username=%s"' % user)269 ipmi_user_number = get_ipmi_user_number(user)
270 (status, output) = commands.getstatusoutput('bmc-config --commit --key-pair="User3:Password=%s"' % password)270 if ipmi_user_number is None:
271 (status, output) = commands.getstatusoutput('bmc-config --commit --key-pair="User10:Username=%s"' % user)
272 ipmi_user_number = get_ipmi_user_number(user)
273 (status, output) = commands.getstatusoutput('bmc-config --commit --key-pair="%s:Password=%s"' % (ipmi_user_number, password))
274 (status, output) = commands.getstatusoutput('bmc-config --commit --key-pair="%s:Enable_User=Yes"' % ipmi_user_number)
275 (status, output) = commands.getstatusoutput('bmc-config --commit --key-pair="%s:Lan_Enable_IPMI_Msgs=Yes"' % ipmi_user_number)
276 (status, output) = commands.getstatusoutput('bmc-config --commit --key-pair="%s:Lan_Privilege_Limit=Administrator"' % ipmi_user_number)
271277
272def commit_ipmi_settings(config):278def commit_ipmi_settings(config):
273 (status, output) = commands.getstatusoutput('bmc-config --commit --filename %s' % config)279 (status, output) = commands.getstatusoutput('bmc-config --commit --filename %s' % config)