OpenStack Compute (nova)

Merge lp:~zulcss/nova/nova-iscsi-choice into lp:~hudson-openstack/nova/trunk

  1. nova-iscsi-choice
  2. Merge into trunk
Proposed by Chuck Short
Status: Work in progress
Proposed branch: lp:~zulcss/nova/nova-iscsi-choice
Merge into: lp:~hudson-openstack/nova/trunk
Diff against target: 195 lines (+94/-36)
3 files modified
nova/flags.py (+2/-0)
nova/tests/test_volume.py (+2/-2)
nova/volume/driver.py (+90/-34)
To merge this branch: bzr merge lp:~zulcss/nova/nova-iscsi-choice
Reviewer Review Type Date Requested Status
Brian Lamar (community) Needs Fixing
Review via email: mp+71188@code.launchpad.net

Description of the change

Allow the user to choose between iscsitarget and tgt for iscsi userland tools.

To post a comment you must log in.
lp:~zulcss/nova/nova-iscsi-choice updated
1418. By Chuck Short

Fix pep8 errors

Revision history for this message
Brian Lamar (blamar) wrote :

19 + self.volume.driver._execute("sudo", "%s" %FLAGS.iscsi_helper, "--op", "show",

This and there are a number of other PEP8 violations to fix.

169 + if FLAGS.iscsi_helper == 'tgtadm':
170 + mount_device = ("/dev/disk/by-path/ip-%s-iscsi-%s-lun-0" %
171 + (iscsi_properties['target_portal'],
172 + iscsi_properties['target_iqn']))
173 + else:
174 + mount_device = ("/dev/disk/by-path/ip-%s-iscsi-%s-lun-0" %
175 + (iscsi_properties['target_portal'],
176 + iscsi_properties['target_iqn']))

I don't think we need this if statement? Unless I'm missing something it's the same code. If you'd like you can put a note in the code indicating that the two supported iscsi implementations share the same strategy here but I'm not sure even that is necessary.

How are you currently testing this code? Manually? Have you tested both ietadm and tgtadm? I get very uneasy around code that's not only not tested, but isn't really testable.

review: Needs Fixing
Reply
Revision history for this message
Brian Lamar (blamar) wrote :

Also, can you delete your other branch if this is the way you're going to proceed? Thanks!

Reply
Revision history for this message
Ben McGraw (mcgrue) wrote :

needs fixing - please write tests for the new feature; In addition, are there tests for the old feature?

Reply
Revision history for this message
Mark McLoughlin (markmc) wrote :

More work on this at lp:~markmc/nova/iscsi-tgtadm-choice - I believe I've handled Ben and Brian's comments

Reply

Unmerged revisions

1418. By Chuck Short

Fix pep8 errors

1417. By Chuck Short

Fixup thinkos

1416. By Chuck Short

Allow the user to choose either ietadm or tgtadm

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'nova/flags.py'
2--- nova/flags.py 2011-08-04 18:50:20 +0000
3+++ nova/flags.py 2011-08-11 12:08:02 +0000
4@@ -392,3 +392,5 @@
5 'Whether to restart guests when the host reboots')
6 DEFINE_bool('resume_guests_state_on_host_boot', False,
7 'Whether to start guests, that was running before the host reboot')
8+DEFINE_string('iscsi_helper', 'ietadm',
9+ 'iscsi target user-land tool to use')
10
11=== modified file 'nova/tests/test_volume.py'
12--- nova/tests/test_volume.py 2011-07-08 03:07:58 +0000
13+++ nova/tests/test_volume.py 2011-08-11 12:08:02 +0000
14@@ -414,7 +414,7 @@
15 self.mox.StubOutWithMock(self.volume.driver, '_execute')
16 for i in volume_id_list:
17 tid = db.volume_get_iscsi_target_num(self.context, i)
18- self.volume.driver._execute("sudo", "ietadm", "--op", "show",
19+ self.volume.driver._execute("sudo", "%s" %FLAGS.iscsi_helper, "--op", "show",
20 "--tid=%(tid)d" % locals())
21
22 self.stream.truncate(0)
23@@ -433,7 +433,7 @@
24 # the first vblade process isn't running
25 tid = db.volume_get_iscsi_target_num(self.context, volume_id_list[0])
26 self.mox.StubOutWithMock(self.volume.driver, '_execute')
27- self.volume.driver._execute("sudo", "ietadm", "--op", "show",
28+ self.volume.driver._execute("sudo", "%s" %FLAGS.iscsi_helper, "--op", "show",
29 "--tid=%(tid)d" % locals()).AndRaise(
30 exception.ProcessExecutionError())
31
32
33=== modified file 'nova/volume/driver.py'
34--- nova/volume/driver.py 2011-06-24 12:01:51 +0000
35+++ nova/volume/driver.py 2011-08-11 12:08:02 +0000
36@@ -348,17 +348,35 @@
37
38 iscsi_name = "%s%s" % (FLAGS.iscsi_target_prefix, volume['name'])
39 volume_path = "/dev/%s/%s" % (FLAGS.volume_group, volume['name'])
40- self._sync_exec('sudo', 'ietadm', '--op', 'new',
41- "--tid=%s" % iscsi_target,
42- '--params',
43- "Name=%s" % iscsi_name,
44- check_exit_code=False)
45- self._sync_exec('sudo', 'ietadm', '--op', 'new',
46- "--tid=%s" % iscsi_target,
47- '--lun=0',
48- '--params',
49- "Path=%s,Type=fileio" % volume_path,
50- check_exit_code=False)
51+ if FLAGS.iscsi_helper == 'tgtadm':
52+ self._sync_exec('sudo', '%s' % FLAGS.iscsi_helper, '--op', 'new',
53+ '--lld=iscsi', '--mode=target',
54+ "--tid=%s" % iscsi_target,
55+ "--targetname=%s" % iscsi_name,
56+ check_exit_code=False)
57+ self._sync_exec('sudo', '%s' % FLAGS.iscsi_helper, '--op', 'bind'
58+ '--lld=iscsi', '--mode=target',
59+ '--initiator-address=ALL',
60+ "--tid=%s" % iscsi_target,
61+ check_exit_code=False)
62+ self._sync_exec('sudo', '%s' % FLAGS.iscsi_helper, '--op', 'new',
63+ '--lld=iscsi', '--mode=logicalunit',
64+ "--tid=%s" % iscsi_target,
65+ '--lun=1',
66+ "--backing-store=%s,Type=fileio" % volume_path,
67+ check_exit_code=False)
68+ else:
69+ self._sync_exec('sudo', '%s' %FLAGS.iscsi_helper, '--op', 'new',
70+ "--tid=%s" % iscsi_target,
71+ '--params',
72+ "Name=%s" % iscsi_name,
73+ check_exit_code=False)
74+ self._sync_exec('sudo', '%s' %FLAGS.iscsi_helper, '--op', 'new',
75+ "--tid=%s" % iscsi_target,
76+ '--lun=0',
77+ '--params',
78+ "Path=%s,Type=fileio" % volume_path,
79+ check_exit_code=False)
80
81 def _ensure_iscsi_targets(self, context, host):
82 """Ensure that target ids have been created in datastore."""
83@@ -378,13 +396,27 @@
84 volume['host'])
85 iscsi_name = "%s%s" % (FLAGS.iscsi_target_prefix, volume['name'])
86 volume_path = "/dev/%s/%s" % (FLAGS.volume_group, volume['name'])
87- self._execute('sudo', 'ietadm', '--op', 'new',
88- '--tid=%s' % iscsi_target,
89- '--params', 'Name=%s' % iscsi_name)
90- self._execute('sudo', 'ietadm', '--op', 'new',
91- '--tid=%s' % iscsi_target,
92- '--lun=0', '--params',
93- 'Path=%s,Type=fileio' % volume_path)
94+ if FLAGS.iscsi_helper == 'tgtadm':
95+ self._execute('sudo', '%s' %FLAGS.iscsi_helper, '--op', 'new',
96+ '--lld=iscsi', '--mode=target',
97+ '--tid=%s' % iscsi_target,
98+ '--params', 'Name=%s' % iscsi_name)
99+ self._execute('sudo', '%s' %FLAGS.iscsi_helper, '--op', 'bind',
100+ '--lld=iscsi', '--mode=target', '--initiator-address=ALL',
101+ "--tid=%s" % iscsi_target)
102+ self._execute('sudo', '%s' %FLAGS.iscsi_helper, '--op', 'new',
103+ '--ld=iscsi', '--mode=logicalunit',
104+ '--tid=%s' % iscsi_target,
105+ '--lun=0', '--params',
106+ 'Path=%s,Type=fileio' % volume_path)
107+ else:
108+ self._execute('sudo', '%s' %FLAGS.iscsi_helper, '--op', 'new',
109+ '--tid=%s' % iscsi_target,
110+ '--params', 'Name=%s' % iscsi_name)
111+ self._execute('sudo', '%s' %FLAGS.iscsi_helper, '--op', 'new',
112+ '--tid=%s' % iscsi_target,
113+ '--lun=0', '--params',
114+ 'Path=%s,Type=fileio' % volume_path)
115
116 def remove_export(self, context, volume):
117 """Removes an export for a logical volume."""
118@@ -399,18 +431,32 @@
119 try:
120 # ietadm show will exit with an error
121 # this export has already been removed
122- self._execute('sudo', 'ietadm', '--op', 'show',
123+ if FLAGS.iscsi_helper == 'tgtadm':
124+ self._execute('sudo', '%s' %FLAGS.iscsi_helper, '--op', 'show',
125+ '--lld=iscsi', '--mode=target',
126+ '--tid=%s' % iscsi_target)
127+ else:
128+ self._execute('sudo', '%s' %FLAGS.iscsi_helper, '--op', 'show',
129+ '--tid=%s' % iscsi_target)
130+ except Exception as e:
131+ LOG.info(_("Skipping remove_export. No iscsi_target " +
132+ "is presently exported for volume: %d"), volume['id'])
133+ return
134+
135+ if FLAGS.iscsi_helper == 'tgtadm':
136+ self._execute('sudo', '%s' %FLAGS.iscsi_helper, '--op', 'delete',
137+ '--lld=iscsi', '--mode=logicalunit',
138+ '--tid=%s' %iscsi_target,
139+ '--lun=1')
140+ self._execute('sudo', '%s' %FLAGS.iscsi_helper, '--op', 'delete',
141+ '--lld=iscsi', '--mode=target',
142 '--tid=%s' % iscsi_target)
143- except Exception as e:
144- LOG.info(_("Skipping remove_export. No iscsi_target " +
145- "is presently exported for volume: %d"), volume['id'])
146- return
147-
148- self._execute('sudo', 'ietadm', '--op', 'delete',
149- '--tid=%s' % iscsi_target,
150- '--lun=0')
151- self._execute('sudo', 'ietadm', '--op', 'delete',
152- '--tid=%s' % iscsi_target)
153+ else:
154+ self._execute('sudo', '%s' %FLAGS.iscsi_helper, '--op' 'delete',
155+ '--tid=%s' %iscsi_target,
156+ '--lun=0')
157+ self._execute('sudo', '%s' %FLAGS.iscsi_helper, '--op', 'delete',
158+ '--tid=%s' % iscsi_target)
159
160 def _do_iscsi_discovery(self, volume):
161 #TODO(justinsb): Deprecate discovery and use stored info
162@@ -518,9 +564,14 @@
163
164 self._iscsiadm_update(iscsi_properties, "node.startup", "automatic")
165
166- mount_device = ("/dev/disk/by-path/ip-%s-iscsi-%s-lun-0" %
167- (iscsi_properties['target_portal'],
168- iscsi_properties['target_iqn']))
169+ if FLAGS.iscsi_helper == 'tgtadm':
170+ mount_device = ("/dev/disk/by-path/ip-%s-iscsi-%s-lun-0" %
171+ (iscsi_properties['target_portal'],
172+ iscsi_properties['target_iqn']))
173+ else:
174+ mount_device = ("/dev/disk/by-path/ip-%s-iscsi-%s-lun-0" %
175+ (iscsi_properties['target_portal'],
176+ iscsi_properties['target_iqn']))
177
178 # The /dev/disk/by-path/... node is not always present immediately
179 # TODO(justinsb): This retry-with-delay is a pattern, move to utils?
180@@ -560,8 +611,13 @@
181
182 tid = self.db.volume_get_iscsi_target_num(context, volume_id)
183 try:
184- self._execute('sudo', 'ietadm', '--op', 'show',
185- '--tid=%(tid)d' % locals())
186+ if FLAGS.iscsi_helper == 'tgtadm':
187+ self._execute('sudo', '%s' %FLAGS.iscsi_helper, '--op', 'show'
188+ '--lld=iscsi', '--mode=target',
189+ '--tid=%(tid)d' % locals())
190+ else:
191+ self._execute('sudo', '%s' %FLAGS.iscsi_helper, '--op', 'show',
192+ '--tid=%(tid)d' % locals())
193 except exception.ProcessExecutionError, e:
194 # Instances remount read-only in this case.
195 # /etc/init.d/iscsitarget restart and rebooting nova-volume