Merge lp:~raharper/curtin/trunk.fix-iscsi-shutdown into lp:~curtin-dev/curtin/trunk

Proposed by Ryan Harper
Status: Merged
Merged at revision: 523
Proposed branch: lp:~raharper/curtin/trunk.fix-iscsi-shutdown
Merge into: lp:~curtin-dev/curtin/trunk
Diff against target: 298 lines (+227/-5)
4 files modified
curtin/block/iscsi.py (+35/-3)
curtin/commands/install.py (+7/-2)
tests/unittests/test_block_iscsi.py (+181/-0)
tests/vmtests/test_lvm_iscsi.py (+4/-0)
To merge this branch: bzr merge lp:~raharper/curtin/trunk.fix-iscsi-shutdown
Reviewer Review Type Date Requested Status
Server Team CI bot continuous-integration Approve
Chad Smith Approve
Nish Aravamudan Pending
Review via email: mp+329777@code.launchpad.net

Description of the change

iscsi: use curtin storage config to disconnect iscsi targets

When curtin disconnects from iscsi targets after an install, it does so
after unmounting the targets, however it passed the target dir into the
iscsi disconnect code which then silently failed to find any
session configurations.

We've been lucky as the existing shutdown iscsi service has been
logging out of sessions defined on the host; however on recent
Artful systems, the open-iscsi service is not restarted after
populating /etc/iscsi/nodes directory and since it is not running
in the ephemeral environment the service will not automatically
stop the iscsi devices, causing a hang on shutdown.

To resolve this parse the curtin storage config for iscsi disks,
construct IscsiDisk objects and then call their disconnect() method.
In addition to this logic fix, this branch contains:

- adds a missing Artful LVM Iscsi vmtest class
- Updated logging messages in iscsi paths, including
  a message when iscsi_disconnect doesn't find the
  /etc/iscsi/nodes directory.
- Updated IscsiDisk disconnect with similar logging

To post a comment you must log in.
Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
524. By Ryan Harper

iscsi: disconnect prior to unmounting target

Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
525. By Ryan Harper

Check if target is in active session, else skip disconnect

Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
526. By Ryan Harper

Add unittests for iscsi.disconnect_target_disks

Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
527. By Ryan Harper

Don't rely on ephemeral env, use target iscsi config, disconnect before unmount

Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Ryan Harper (raharper) wrote :
Revision history for this message
Scott Moser (smoser) wrote :

Nish,
could you think about this ?
the big change Ryan is making is moving disconnect to happen before unmount rather than after.

that would seem like it could be an issue to me.. if you disconnet a volume and then unmount it could reasonably expect to flush data to that volume.

528. By Ryan Harper

Switch back to unmounting all paths, and then disconnecting iscsi

In general, the typical cleanup of an in-use of block device involves
unmounting the filesystem/block device, and then perform any block
device specific actions. In this case, curtin will recursively unmount
any devices mounted under the install target directory (iscsi disks included)
And then we query the curtin iscsi layer for any connected iscsi devices it
created and invoke disconnect on each device. This should avoid shutting down
any other sessions which may be active but are not created/owned by curtin.

In addition, add some logging to some of the iscsi paths which previously
would silently exit without performing the requested action (disconnect for
example returns if the target iscsi node dir is not present).

529. By Ryan Harper

Don't use iscsi globals, not available across stages

The iscsi global is not available across stages due to use of
subprocess to run curtin commands. Revert back to finding
on-disk sessions that curtin configured to run iscsi disconnects.

Revision history for this message
Ryan Harper (raharper) wrote :
Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
530. By Ryan Harper

Parse storage config for reconstructing iscsi disk objects for disconnect operations

531. By Ryan Harper

config is a list

532. By Ryan Harper

Clean up logging messages

533. By Ryan Harper

Add unittest for parsing storage config for iscsi disks

Revision history for this message
Ryan Harper (raharper) wrote :

This branch ran through vmtest running iscsi tests (passes).

https://jenkins.ubuntu.com/server/view/curtin/job/curtin-vmtest-devel-amd64/38/console

Revision history for this message
Chad Smith (chad.smith) wrote :

Looks really good thanks for this (and all the unit tests give me warm fuzzies). +1

Revision history for this message
Ryan Harper (raharper) :
Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Chad Smith (chad.smith) wrote :

+1 on this thanks for the comments/discussion

review: Approve
Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'curtin/block/iscsi.py'
2--- curtin/block/iscsi.py 2017-05-18 23:54:21 +0000
3+++ curtin/block/iscsi.py 2017-08-31 19:22:49 +0000
4@@ -238,11 +238,35 @@
5 return _ISCSI_DISKS
6
7
8+def get_iscsi_disks_from_config(cfg):
9+ """Parse a curtin storage config and return a list
10+ of iscsi disk objects for each configuration present
11+ """
12+ if not cfg:
13+ cfg = {}
14+
15+ sconfig = cfg.get('storage', {}).get('config', {})
16+ if not sconfig:
17+ LOG.warning('Configuration dictionary did not contain'
18+ ' a storage configuration')
19+ return []
20+
21+ # Construct IscsiDisk objects for each iscsi volume present
22+ iscsi_disks = [IscsiDisk(disk['path']) for disk in sconfig
23+ if disk['type'] == 'disk' and
24+ disk.get('path', "").startswith('iscsi:')]
25+ LOG.debug('Found %s iscsi disks in storage config', len(iscsi_disks))
26+ return iscsi_disks
27+
28+
29 def disconnect_target_disks(target_root_path=None):
30 target_nodes_path = util.target_path(target_root_path, '/etc/iscsi/nodes')
31 fails = []
32 if os.path.isdir(target_nodes_path):
33 for target in os.listdir(target_nodes_path):
34+ if target not in iscsiadm_sessions():
35+ LOG.debug('iscsi target %s not active, skipping', target)
36+ continue
37 # conn is "host,port,lun"
38 for conn in os.listdir(
39 os.path.sep.join([target_nodes_path, target])):
40@@ -254,7 +278,9 @@
41 fails.append(target)
42 LOG.warn("Unable to logout of iSCSI target %s: %s",
43 target, e)
44-
45+ else:
46+ LOG.warning('Skipping disconnect: failed to find iscsi nodes path: %s',
47+ target_nodes_path)
48 if fails:
49 raise RuntimeError(
50 "Unable to logout of iSCSI targets: %s" % ', '.join(fails))
51@@ -414,9 +440,15 @@
52
53 def disconnect(self):
54 if self.target not in iscsiadm_sessions():
55+ LOG.warning('Iscsi target %s not in active iscsi sessions',
56+ self.target)
57 return
58
59- util.subp(['sync'])
60- iscsiadm_logout(self.target, self.portal)
61+ try:
62+ util.subp(['sync'])
63+ iscsiadm_logout(self.target, self.portal)
64+ except util.ProcessExecutionError as e:
65+ LOG.warn("Unable to logout of iSCSI target %s from portal %s: %s",
66+ self.target, self.portal, e)
67
68 # vi: ts=4 expandtab syntax=python
69
70=== modified file 'curtin/commands/install.py'
71--- curtin/commands/install.py 2017-07-25 16:24:07 +0000
72+++ curtin/commands/install.py 2017-08-31 19:22:49 +0000
73@@ -477,9 +477,14 @@
74 '/root/curtin-install.log')
75 if log_target_path:
76 copy_install_log(logfile, workingd.target, log_target_path)
77+ # unmount everything (including iscsi disks)
78 util.do_umount(workingd.target, recursive=True)
79- # need to do some processing on iscsi disks to disconnect?
80- iscsi.disconnect_target_disks(workingd.target)
81+ # disconnect configured iscsi disks
82+ LOG.info("Disconnecting iscsi targets (if present)")
83+ for iscsi_disk in iscsi.get_iscsi_disks_from_config(cfg):
84+ LOG.info("Attempting to disconnect %s", iscsi_disk)
85+ iscsi_disk.disconnect()
86+
87 shutil.rmtree(workingd.top)
88
89 apply_power_state(cfg.get('power_state'))
90
91=== modified file 'tests/unittests/test_block_iscsi.py'
92--- tests/unittests/test_block_iscsi.py 2017-08-03 18:14:51 +0000
93+++ tests/unittests/test_block_iscsi.py 2017-08-31 19:22:49 +0000
94@@ -1,6 +1,8 @@
95 import mock
96+import os
97
98 from curtin.block import iscsi
99+from curtin import util
100 from .helpers import CiTestCase
101
102
103@@ -557,4 +559,183 @@
104 with self.assertRaises(ValueError):
105 iscsi.volpath_is_iscsi(None)
106
107+
108+class TestBlockIscsiDiskFromConfig(CiTestCase):
109+ # Test iscsi parsing of storage config for iscsi configure disks
110+
111+ def setUp(self):
112+ super(TestBlockIscsiDiskFromConfig, self).setUp()
113+ self.add_patch('curtin.block.iscsi.util.subp', 'mock_subp')
114+
115+ def test_parse_iscsi_disk_from_config(self):
116+ """Test parsing iscsi volume path creates the same iscsi disk"""
117+ target = 'curtin-659d5f45-4f23-46cb-b826-f2937b896e09'
118+ iscsi_path = 'iscsi:10.245.168.20::20112:1:' + target
119+ cfg = {
120+ 'storage': {
121+ 'config': [{'type': 'disk',
122+ 'id': 'iscsidev1',
123+ 'path': iscsi_path,
124+ 'name': 'iscsi_disk1',
125+ 'ptable': 'msdos',
126+ 'wipe': 'superblock'}]
127+ }
128+ }
129+ expected_iscsi_disk = iscsi.IscsiDisk(iscsi_path)
130+ iscsi_disk = iscsi.get_iscsi_disks_from_config(cfg).pop()
131+ # utilize IscsiDisk str method for equality check
132+ self.assertEqual(str(expected_iscsi_disk), str(iscsi_disk))
133+
134+ def test_parse_iscsi_disk_from_config_no_iscsi(self):
135+ """Test parsing storage config with no iscsi disks included"""
136+ cfg = {
137+ 'storage': {
138+ 'config': [{'type': 'disk',
139+ 'id': 'ssd1',
140+ 'path': 'dev/slash/foo1',
141+ 'name': 'the-fast-one',
142+ 'ptable': 'gpt',
143+ 'wipe': 'superblock'}]
144+ }
145+ }
146+ expected_iscsi_disks = []
147+ iscsi_disks = iscsi.get_iscsi_disks_from_config(cfg)
148+ self.assertEqual(expected_iscsi_disks, iscsi_disks)
149+
150+ def test_parse_iscsi_disk_from_config_invalid_iscsi(self):
151+ """Test parsing storage config with no iscsi disks included"""
152+ cfg = {
153+ 'storage': {
154+ 'config': [{'type': 'disk',
155+ 'id': 'iscsidev2',
156+ 'path': 'iscsi:garbage',
157+ 'name': 'noob-city',
158+ 'ptable': 'msdos',
159+ 'wipe': 'superblock'}]
160+ }
161+ }
162+ with self.assertRaises(ValueError):
163+ iscsi.get_iscsi_disks_from_config(cfg)
164+
165+ def test_parse_iscsi_disk_from_config_empty(self):
166+ """Test parse_iscsi_disks handles empty/invalid config"""
167+ expected_iscsi_disks = []
168+ iscsi_disks = iscsi.get_iscsi_disks_from_config({})
169+ self.assertEqual(expected_iscsi_disks, iscsi_disks)
170+
171+ cfg = {'storage': {'config': []}}
172+ iscsi_disks = iscsi.get_iscsi_disks_from_config(cfg)
173+ self.assertEqual(expected_iscsi_disks, iscsi_disks)
174+
175+ def test_parse_iscsi_disk_from_config_none(self):
176+ """Test parse_iscsi_disks handles no config"""
177+ expected_iscsi_disks = []
178+ iscsi_disks = iscsi.get_iscsi_disks_from_config({})
179+ self.assertEqual(expected_iscsi_disks, iscsi_disks)
180+
181+ cfg = None
182+ iscsi_disks = iscsi.get_iscsi_disks_from_config(cfg)
183+ self.assertEqual(expected_iscsi_disks, iscsi_disks)
184+
185+
186+class TestBlockIscsiDisconnect(CiTestCase):
187+ # test that when disconnecting iscsi targets we
188+ # check that the target has an active session before
189+ # issuing a disconnect command
190+
191+ def setUp(self):
192+ super(TestBlockIscsiDisconnect, self).setUp()
193+ self.add_patch('curtin.block.iscsi.util.subp', 'mock_subp')
194+ self.add_patch('curtin.block.iscsi.iscsiadm_sessions',
195+ 'mock_iscsi_sessions')
196+ # fake target_root + iscsi nodes dir
197+ self.target_path = self.tmp_dir()
198+ self.iscsi_nodes = os.path.join(self.target_path, 'etc/iscsi/nodes')
199+ util.ensure_dir(self.iscsi_nodes)
200+
201+ def _fmt_disconnect(self, target, portal):
202+ return ['iscsiadm', '--mode=node', '--targetname=%s' % target,
203+ '--portal=%s' % portal, '--logout']
204+
205+ def _setup_nodes(self, sessions, connection):
206+ # setup iscsi_nodes dir (<fakeroot>/etc/iscsi/nodes) with content
207+ for s in sessions:
208+ sdir = os.path.join(self.iscsi_nodes, s)
209+ connpath = os.path.join(sdir, connection)
210+ util.ensure_dir(sdir)
211+ util.write_file(connpath, content="")
212+
213+ def test_disconnect_target_disk(self):
214+ """Test iscsi disconnecting multiple sessions, all present"""
215+
216+ sessions = [
217+ 'curtin-53ab23ff-a887-449a-80a8-288151208091',
218+ 'curtin-94b62de1-c579-42c0-879e-8a28178e64c5',
219+ 'curtin-556aeecd-a227-41b7-83d7-2bb471c574b4',
220+ 'curtin-fd0f644b-7858-420f-9997-3ea2aefe87b9'
221+ ]
222+ connection = '10.245.168.20,16395,1'
223+ self._setup_nodes(sessions, connection)
224+
225+ self.mock_iscsi_sessions.return_value = "\n".join(sessions)
226+
227+ iscsi.disconnect_target_disks(self.target_path)
228+
229+ expected_calls = []
230+ for session in sessions:
231+ (host, port, _) = connection.split(',')
232+ disconnect = self._fmt_disconnect(session, "%s:%s" % (host, port))
233+ calls = [
234+ mock.call(['sync']),
235+ mock.call(disconnect, capture=True, log_captured=True),
236+ mock.call(['udevadm', 'settle']),
237+ ]
238+ expected_calls.extend(calls)
239+
240+ self.mock_subp.assert_has_calls(expected_calls, any_order=True)
241+
242+ def test_disconnect_target_disk_skip_disconnected(self):
243+ """Test iscsi does not attempt to disconnect already closed sessions"""
244+ sessions = [
245+ 'curtin-53ab23ff-a887-449a-80a8-288151208091',
246+ 'curtin-94b62de1-c579-42c0-879e-8a28178e64c5',
247+ 'curtin-556aeecd-a227-41b7-83d7-2bb471c574b4',
248+ 'curtin-fd0f644b-7858-420f-9997-3ea2aefe87b9'
249+ ]
250+ connection = '10.245.168.20,16395,1'
251+ self._setup_nodes(sessions, connection)
252+ # Test with all sessions are already disconnected
253+ self.mock_iscsi_sessions.return_value = ""
254+
255+ iscsi.disconnect_target_disks(self.target_path)
256+
257+ self.mock_subp.assert_has_calls([], any_order=True)
258+
259+ @mock.patch('curtin.block.iscsi.iscsiadm_logout')
260+ def test_disconnect_target_disk_raises_runtime_error(self, mock_logout):
261+ """Test iscsi raises RuntimeError if we fail to logout"""
262+ sessions = [
263+ 'curtin-53ab23ff-a887-449a-80a8-288151208091',
264+ ]
265+ connection = '10.245.168.20,16395,1'
266+ self._setup_nodes(sessions, connection)
267+ self.mock_iscsi_sessions.return_value = "\n".join(sessions)
268+ mock_logout.side_effect = util.ProcessExecutionError()
269+
270+ with self.assertRaises(RuntimeError):
271+ iscsi.disconnect_target_disks(self.target_path)
272+
273+ expected_calls = []
274+ for session in sessions:
275+ (host, port, _) = connection.split(',')
276+ disconnect = self._fmt_disconnect(session, "%s:%s" % (host, port))
277+ calls = [
278+ mock.call(['sync']),
279+ mock.call(disconnect, capture=True, log_captured=True),
280+ mock.call(['udevadm', 'settle']),
281+ ]
282+ expected_calls.extend(calls)
283+
284+ self.mock_subp.assert_has_calls([], any_order=True)
285+
286 # vi: ts=4 expandtab syntax=python
287
288=== modified file 'tests/vmtests/test_lvm_iscsi.py'
289--- tests/vmtests/test_lvm_iscsi.py 2017-08-02 15:46:35 +0000
290+++ tests/vmtests/test_lvm_iscsi.py 2017-08-31 19:22:49 +0000
291@@ -61,3 +61,7 @@
292
293 class ZestyTestIscsiLvm(relbase.zesty, TestLvmIscsiAbs):
294 __test__ = True
295+
296+
297+class ArtfulTestIscsiLvm(relbase.artful, TestLvmIscsiAbs):
298+ __test__ = True

Subscribers

People subscribed via source and target branches