Merge lp:~andreserl/maas/ipmi_usercreation_ilo_versions_trunk into lp:maas

Proposed by Andres Rodriguez
Status: Merged
Approved by: Andres Rodriguez
Approved revision: 1444
Merged at revision: 1459
Proposed branch: lp:~andreserl/maas/ipmi_usercreation_ilo_versions_trunk
Merge into: lp:maas
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 :
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
1439. By Raphaël Badin

[r=allenap][bug=][author=rvb] Add 'owner' field to FileStorage.

1440. By Raphaël Badin

[r=allenap][bug=][author=rvb] Add randomly generated key to FileStorage objects.

1441. By Raphaël Badin

[r=allenap][bug=][author=rvb] Adapt the file storage API to the 'owner' field on FileStorage objects.

1442. By Raphaël Badin

[r=allenap][bug=][author=rvb] Add API endpoint to fetch the content of a file using its 'key'. Add 'anon_url' field when serializing file storage objects.

1443. By Raphaël Badin

[r=allenap][bug=][author=rvb] Add a header to the 404 response when the requested file is not present.

1444. By Andres Rodriguez

Refactor IPMI user creation to support different iLO versions

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
1=== modified file 'contrib/preseeds_v2/enlist_userdata'
2--- contrib/preseeds_v2/enlist_userdata 2012-11-30 17:35:04 +0000
3+++ contrib/preseeds_v2/enlist_userdata 2013-02-27 16:16:26 +0000
4@@ -40,14 +40,6 @@
5 chmod "${2:-644}" "${IPMI_CONFIG_D}/$1"
6 }
7
8- add_ipmi_config "01-user-privileges.ipmi" <<"END_IPMI_USER_PRIVILEGES"
9- Section User3
10- Enable_User Yes
11- Lan_Enable_IPMI_Msgs Yes
12- Lan_Privilege_Limit Administrator
13- EndSection
14- END_IPMI_USER_PRIVILEGES
15-
16 add_ipmi_config "02-global-config.ipmi" <<"END_IPMI_CONFIG"
17 Section Lan_Channel
18 Volatile_Access_Mode Always_Available
19@@ -99,9 +91,23 @@
20 res = show_re.search(output)
21 return res.group()
22
23+ def get_ipmi_user_number(user):
24+ for i in range(1, 17):
25+ ipmi_user_number = "User%s" % i
26+ (status, output) = commands.getstatusoutput('bmc-config --checkout --key-pair="%s:Username"' % ipmi_user_number)
27+ if user in output:
28+ return ipmi_user_number
29+ return None
30+
31 def commit_ipmi_user_settings(user, password):
32- (status, output) = commands.getstatusoutput('bmc-config --commit --key-pair="User3:Username=%s"' % user)
33- (status, output) = commands.getstatusoutput('bmc-config --commit --key-pair="User3:Password=%s"' % password)
34+ ipmi_user_number = get_ipmi_user_number(user)
35+ if ipmi_user_number is None:
36+ (status, output) = commands.getstatusoutput('bmc-config --commit --key-pair="User10:Username=%s"' % user)
37+ ipmi_user_number = get_ipmi_user_number(user)
38+ (status, output) = commands.getstatusoutput('bmc-config --commit --key-pair="%s:Password=%s"' % (ipmi_user_number, password))
39+ (status, output) = commands.getstatusoutput('bmc-config --commit --key-pair="%s:Enable_User=Yes"' % ipmi_user_number)
40+ (status, output) = commands.getstatusoutput('bmc-config --commit --key-pair="%s:Lan_Enable_IPMI_Msgs=Yes"' % ipmi_user_number)
41+ (status, output) = commands.getstatusoutput('bmc-config --commit --key-pair="%s:Lan_Privilege_Limit=Administrator"' % ipmi_user_number)
42
43 def commit_ipmi_settings(config):
44 (status, output) = commands.getstatusoutput('bmc-config --commit --filename %s' % config)
45@@ -147,10 +153,7 @@
46 time.sleep(120)
47
48 # create user/pass
49- if args.commission_creds:
50- IPMI_MAAS_USER="maas-commission"
51- else:
52- IPMI_MAAS_USER="maas"
53+ IPMI_MAAS_USER="maas"
54 IPMI_MAAS_PASSWORD=generate_random_password()
55
56 # Configure IPMI user/password
57
58=== modified file 'src/metadataserver/commissioning/user_data.template'
59--- src/metadataserver/commissioning/user_data.template 2013-02-06 13:59:35 +0000
60+++ src/metadataserver/commissioning/user_data.template 2013-02-27 16:16:26 +0000
61@@ -207,14 +207,6 @@
62
63 ### begin writing files ###
64
65-add_ipmi_config "01-user-privileges.ipmi" <<"END_IPMI_USER_PRIVILEGES"
66-Section User3
67- Enable_User Yes
68- Lan_Enable_IPMI_Msgs Yes
69- Lan_Privilege_Limit Administrator
70-EndSection
71-END_IPMI_USER_PRIVILEGES
72-
73 add_ipmi_config "02-global-config.ipmi" <<"END_IPMI_CONFIG"
74 Section Lan_Channel
75 Volatile_Access_Mode Always_Available
76@@ -265,9 +257,23 @@
77 res = show_re.search(output)
78 return res.group()
79
80+def get_ipmi_user_number(user):
81+ for i in range(1, 17):
82+ ipmi_user_number = "User%s" % i
83+ (status, output) = commands.getstatusoutput('bmc-config --checkout --key-pair="%s:Username"' % ipmi_user_number)
84+ if user in output:
85+ return ipmi_user_number
86+ return None
87+
88 def commit_ipmi_user_settings(user, password):
89- (status, output) = commands.getstatusoutput('bmc-config --commit --key-pair="User3:Username=%s"' % user)
90- (status, output) = commands.getstatusoutput('bmc-config --commit --key-pair="User3:Password=%s"' % password)
91+ ipmi_user_number = get_ipmi_user_number(user)
92+ if ipmi_user_number is None:
93+ (status, output) = commands.getstatusoutput('bmc-config --commit --key-pair="User10:Username=%s"' % user)
94+ ipmi_user_number = get_ipmi_user_number(user)
95+ (status, output) = commands.getstatusoutput('bmc-config --commit --key-pair="%s:Password=%s"' % (ipmi_user_number, password))
96+ (status, output) = commands.getstatusoutput('bmc-config --commit --key-pair="%s:Enable_User=Yes"' % ipmi_user_number)
97+ (status, output) = commands.getstatusoutput('bmc-config --commit --key-pair="%s:Lan_Enable_IPMI_Msgs=Yes"' % ipmi_user_number)
98+ (status, output) = commands.getstatusoutput('bmc-config --commit --key-pair="%s:Lan_Privilege_Limit=Administrator"' % ipmi_user_number)
99
100 def commit_ipmi_settings(config):
101 (status, output) = commands.getstatusoutput('bmc-config --commit --filename %s' % config)