cloud-init

Merge ~smoser/cloud-init:bug/1692093-sometimes-need-settle into cloud-init:master

  1. Git
  2. lp:~smoser/cloud-init
  3. bug/1692093-sometimes-need-settle
  4. Merge into master
Proposed by Scott Moser on 2017-05-25
Status: Merged
Merged at revision: 1815c6d801933c47a01f1a94a8e689824f6797b4
Proposed branch: ~smoser/cloud-init:bug/1692093-sometimes-need-settle
Merge into: cloud-init:master
Diff against target: 76 lines (+25/-3)
2 files modified
cloudinit/config/cc_disk_setup.py (+23/-3)
tests/unittests/test_handler/test_handler_disk_setup.py (+2/-0)
Reviewer Review Type Date Requested Status
Server Team CI bot continuous-integration Approve on 2017-05-26
Ryan Harper 2017-05-25 Approve on 2017-05-26
Paul Meyer (community) 2017-05-25 Approve on 2017-05-25
Review via email: mp+324639@code.launchpad.net

Commit Message

disk_setup: udev settle before attempting partitioning or fs creation.

This attempts to use udevadm settle to wait until devices have been
fully "realized". If a device exists, there may still be events in
the udev queue that would create its partition table entries.
We need to wait until those have been processed also.

LP: #1692093

To post a comment you must log in.
Server Team CI bot (server-team-bot) wrote :
review: Needs Fixing (continuous-integration)
Paul Meyer (paul-meyer) wrote :

LGTM

review: Approve
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Scott Moser (smoser) wrote :

I think i'm going to drop the blockdev --rereadpt
It should not strictly be needed during boot.
a udevadm settle shoudl be sufficient.

Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Ryan Harper (raharper) wrote :

It doesn't look like you dropped blockdev as you say in your comment.

Chad Smith (chad.smith) wrote :

> It doesn't look like you dropped blockdev as you say in your comment.
The drop was a drop from the previous commit

Scott Moser (smoser) :
Ryan Harper (raharper) wrote :

Other than trying to understand the comment about dropping blkdev command (which I think still is present at line 13) it looks good.

Scott Moser (smoser) wrote :

dropping blkid:
 https://git.launchpad.net/~smoser/cloud-init/commit/?id=6b0c90d33703ac075c42bb4a52d710f636d5a030

See that commit.
I *had* a blkid --rereadpt between the two udevadm calls in assert_and_settle_device. I dropped that.

the 'blkid --rereadpt' that you see is in 'read_parttbl', which is a function with the explicit purpose of making the call to blkid --rereadpt (for after wiping a disk or something).

Ryan Harper (raharper) wrote :

OK; it was an unfortunate diff; and the comment was referring to one inside assert_and_settle.
Thanks for clarifying.

review: Approve
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
1diff --git a/cloudinit/config/cc_disk_setup.py b/cloudinit/config/cc_disk_setup.py
2index e1505b3..c2b83ae 100644
3--- a/cloudinit/config/cc_disk_setup.py
4+++ b/cloudinit/config/cc_disk_setup.py
5@@ -680,14 +680,14 @@ def read_parttbl(device):
6 reliable way to probe the partition table.
7 """
8 blkdev_cmd = [BLKDEV_CMD, '--rereadpt', device]
9- udev_cmd = [UDEVADM_CMD, 'settle']
10+ udevadm_settle()
11 try:
12- util.subp(udev_cmd)
13 util.subp(blkdev_cmd)
14- util.subp(udev_cmd)
15 except Exception as e:
16 util.logexc(LOG, "Failed reading the partition table %s" % e)
17
18+ udevadm_settle()
19+
20
21 def exec_mkpart_mbr(device, layout):
22 """
23@@ -737,6 +737,24 @@ def exec_mkpart(table_type, device, layout):
24 return get_dyn_func("exec_mkpart_%s", table_type, device, layout)
25
26
27+def udevadm_settle():
28+ util.subp(['udevadm', 'settle'])
29+
30+
31+def assert_and_settle_device(device):
32+ """Assert that device exists and settle so it is fully recognized."""
33+ if not os.path.exists(device):
34+ udevadm_settle()
35+ if not os.path.exists(device):
36+ raise RuntimeError("Device %s did not exist and was not created "
37+ "with a udevamd settle." % device)
38+
39+ # Whether or not the device existed above, it is possible that udev
40+ # events that would populate udev database (for reading by lsdname) have
41+ # not yet finished. So settle again.
42+ udevadm_settle()
43+
44+
45 def mkpart(device, definition):
46 """
47 Creates the partition table.
48@@ -752,6 +770,7 @@ def mkpart(device, definition):
49 device: the device to work on.
50 """
51 # ensure that we get a real device rather than a symbolic link
52+ assert_and_settle_device(device)
53 device = os.path.realpath(device)
54
55 LOG.debug("Checking values for %s definition", device)
56@@ -852,6 +871,7 @@ def mkfs(fs_cfg):
57 overwrite = fs_cfg.get('overwrite', False)
58
59 # ensure that we get a real device rather than a symbolic link
60+ assert_and_settle_device(device)
61 device = os.path.realpath(device)
62
63 # This allows you to define the default ephemeral or swap
64diff --git a/tests/unittests/test_handler/test_handler_disk_setup.py b/tests/unittests/test_handler/test_handler_disk_setup.py
65index e322697..916a0d7 100644
66--- a/tests/unittests/test_handler/test_handler_disk_setup.py
67+++ b/tests/unittests/test_handler/test_handler_disk_setup.py
68@@ -168,6 +168,8 @@ class TestUpdateFsSetupDevices(TestCase):
69 }, fs_setup)
70
71
72+@mock.patch('cloudinit.config.cc_disk_setup.assert_and_settle_device',
73+ return_value=None)
74 @mock.patch('cloudinit.config.cc_disk_setup.find_device_node',
75 return_value=('/dev/xdb1', False))
76 @mock.patch('cloudinit.config.cc_disk_setup.device_type', return_value=None)

Subscribers

People subscribed via source and target branches

to all changes: