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

Proposed by Andres Rodriguez
Status: Merged
Approved by: Raphaël Badin
Approved revision: 1406
Merged at revision: 1410
Proposed branch: lp:~andreserl/maas/lp1073462_fence_cdu_power_type
Merge into: lp:maas
Diff against target: 148 lines (+93/-0)
5 files modified
src/maasserver/models/node.py (+1/-0)
src/maasserver/power_parameters.py (+20/-0)
src/provisioningserver/enum.py (+4/-0)
src/provisioningserver/power/templates/fence_cdu.template (+54/-0)
src/provisioningserver/power/tests/test_poweraction.py (+14/-0)
To merge this branch: bzr merge lp:~andreserl/maas/lp1073462_fence_cdu_power_type
Reviewer Review Type Date Requested Status
Jeroen T. Vermeulen (community) Approve
Review via email: mp+140504@code.launchpad.net

Commit message

Add fence_cdu power type for SentrySwitch CDU's.

To post a comment you must log in.
Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

Looks very nice.

Two small notes:

1. In python, when we have to line-break the arguments to a function call, we start doing so right after the opening parenthesis, so that the arguments start on a line of their own.

So instead of:

    call_function(one_argument,
        another_argument, final_argument)

...we say:

    call_function(
        one_argument, another_argument,
        final_argument)

2. It would be nice if the test explained (in a comment, doesn't need to be very long) how the error condition happens. As I understand it, you substitute "echo" for the power command, and when that command gets called, you get something that isn't recognized as a power state.

I think I probably did this myself at some point. But reading it back now, I realize it's not obvious. So for the sake of whoever maintains the code next, it would be helpful to point out this subtlety.

Jeroen

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

Hi Jeroen,

Thanks for the review!

[1] I don't understand it :). The only place where a line-break would be required is inside the test_fence_cdu_checks_state function, in the line 134 which calls render_template.... However, this line-break is correctly place for such call... The whole diff follows the same code as to what the code currently is:

134 + script = action.render_template(
135 + action.get_template(), power_change='on',
136 + power_address='mysystem', power_id='system',
137 + power_user='me', power_pass='me', fence_cdu='echo')

[2]: Fixed.

Thanks!

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

I think Jeroen was referring to that piece of code:

24 + 'power_id',
25 + forms.CharField(label="Power ID",
26 + required=False)),
27 + (
28 + 'power_address',
29 + forms.CharField(label="IP Address or Hostname",
30 + required=False)),

Instead of:

forms.CharField(label="Power ID",
    required=False))

You should write:

forms.CharField(
    label="Power ID", required=False))

Revision history for this message
MAAS Lander (maas-lander) wrote :

The Jenkins job https://jenkins.qa.ubuntu.com/job/maas-merger-trunk/159/console reported an error when processing this lp:~andreserl/maas/lp1073462_fence_cdu_power_type branch.
Not merging it.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/maasserver/models/node.py'
2--- src/maasserver/models/node.py 2012-12-18 12:31:47 +0000
3+++ src/maasserver/models/node.py 2012-12-19 17:24:23 +0000
4@@ -768,6 +768,7 @@
5
6 power_params.setdefault('system_id', self.system_id)
7 power_params.setdefault('virsh', '/usr/bin/virsh')
8+ power_params.setdefault('fence_cdu', '/usr/sbin/fence_cdu')
9 power_params.setdefault('ipmipower', '/usr/sbin/ipmipower')
10 power_params.setdefault(
11 'ipmi_chassis_config', '/usr/sbin/ipmi-chassis-config')
12
13=== modified file 'src/maasserver/power_parameters.py'
14--- src/maasserver/power_parameters.py 2012-09-17 17:27:02 +0000
15+++ src/maasserver/power_parameters.py 2012-12-19 17:24:23 +0000
16@@ -64,6 +64,26 @@
17 ],
18 required=False,
19 skip_check=True),
20+ POWER_TYPE.CDU:
21+ DictCharField(
22+ [
23+ (
24+ 'power_id',
25+ forms.CharField(label="Power ID",
26+ required=False)),
27+ (
28+ 'power_address',
29+ forms.CharField(label="IP Address or Hostname",
30+ required=False)),
31+ (
32+ 'power_user',
33+ forms.CharField(label="Username", required=False)),
34+ (
35+ 'power_pass',
36+ forms.CharField(label="Password", required=False)),
37+ ],
38+ required=False,
39+ skip_check=True),
40 POWER_TYPE.IPMI:
41 DictCharField(
42 [
43
44=== modified file 'src/provisioningserver/enum.py'
45--- src/provisioningserver/enum.py 2012-09-17 17:27:02 +0000
46+++ src/provisioningserver/enum.py 2012-12-19 17:24:23 +0000
47@@ -33,6 +33,9 @@
48 # Network wake-up.
49 WAKE_ON_LAN = 'ether_wake'
50
51+ # Sentry Switch CDU's.
52+ CDU = 'fence_cdu'
53+
54 # IPMI (Intelligent Platform Management Interface).
55 IPMI = 'ipmi'
56
57@@ -40,6 +43,7 @@
58 POWER_TYPE_CHOICES = (
59 (POWER_TYPE.VIRSH, "virsh (virtual systems)"),
60 (POWER_TYPE.WAKE_ON_LAN, "Wake-on-LAN"),
61+ (POWER_TYPE.CDU, "Sentry Switch CDU"),
62 (POWER_TYPE.IPMI, "IPMI"),
63 )
64
65
66=== added file 'src/provisioningserver/power/templates/fence_cdu.template'
67--- src/provisioningserver/power/templates/fence_cdu.template 1970-01-01 00:00:00 +0000
68+++ src/provisioningserver/power/templates/fence_cdu.template 2012-12-19 17:24:23 +0000
69@@ -0,0 +1,54 @@
70+# -*- mode: shell-script -*-
71+#
72+# Control virtual system's "power" through virsh.
73+#
74+
75+# Parameters.
76+# Choose command for virsh to make the requested power change happen.
77+power_change={{power_change}}
78+power_address={{power_address}}
79+power_user={{power_user}}
80+power_pass={{power_pass}}
81+power_id={{power_id}}
82+fence_cdu={{fence_cdu}}
83+
84+
85+formulate_power_command() {
86+ if [ ${power_change} = 'on' ]
87+ then
88+ echo 'on'
89+ else
90+ echo 'off'
91+ fi
92+}
93+
94+
95+# Express system's current state as expressed by virsh as "on" or "off".
96+formulate_power_state() {
97+ case $2 in
98+ 'on'|'ON') echo 'on' ;;
99+ 'off'|'OFF') echo 'off' ;;
100+ *)
101+ echo "Got unknown power state from fence_cdu: '$1'" >&2
102+ exit 1
103+ esac
104+}
105+
106+
107+# Issue command to virsh, for the given system.
108+issue_fence_cdu_command() {
109+ ${fence_cdu} -a ${power_address} -n ${power_id} -l ${power_user} -p ${power_pass} -o "$@"
110+}
111+
112+
113+# Get the given system's power state: 'on' or 'off'.
114+get_power_state() {
115+ fence_cdu_state=$(issue_fence_cdu_command status)
116+ formulate_power_state ${fence_cdu_state}
117+}
118+
119+
120+if [ "$(get_power_state)" != "${power_change}" ]
121+then
122+ issue_fence_cdu_command $(formulate_power_command)
123+fi
124
125=== modified file 'src/provisioningserver/power/tests/test_poweraction.py'
126--- src/provisioningserver/power/tests/test_poweraction.py 2012-10-05 16:33:37 +0000
127+++ src/provisioningserver/power/tests/test_poweraction.py 2012-12-19 17:24:23 +0000
128@@ -159,6 +159,20 @@
129 stdout, stderr = action.run_shell(script)
130 self.assertIn("Got unknown power state from virsh", stderr)
131
132+ def test_fence_cdu_checks_state(self):
133+ # We can't test the fence_cdu template in detail (and it may be
134+ # customized), but by making it use "echo" instead of a real
135+ # fence_cdu we can make it get a bogus answer from its status check.
136+ # The bogus answer is actually the rest of the fence_cdu command
137+ # line. It will complain about this and fail.
138+ action = PowerAction(POWER_TYPE.CDU)
139+ script = action.render_template(
140+ action.get_template(), power_change='on',
141+ power_address='mysystem', power_id='system',
142+ power_user='me', power_pass='me', fence_cdu='echo')
143+ stdout, stderr = action.run_shell(script)
144+ self.assertIn("Got unknown power state from fence_cdu", stderr)
145+
146 def test_ipmi_checks_state(self):
147 action = PowerAction(POWER_TYPE.IPMI)
148 script = action.render_template(