Merge lp:~oddbloke/cloud-init/lp1460715 into lp:~cloud-init-dev/cloud-init/trunk

[Dan Watkins]

[cc_disk_setup] Use sectors to resize MBR disks

The version of sfdisk in wily (and onwards) only accepts sectors as a valid disk size. As such, this refactors the MBR code path in cc_disk_setup to use sectors.

X-CPC-Summary-Skip: 1

[Scott Moser (smoser)]

some comments inline.
other thing.. it seems like it would make sense to partition assuming sectors of 4096 bytes. just to ensure alignment or to round all partition sizes to 1MB or even 4MB.

if the user has just given us such course grained data as percentage, it seems we have some wiggle room to work with and should try to do same things.

for reference it seems that /sys/block/sda/queue/hw_sector_size has sector sizes. i'm not sure that i'd bother caring though wether there was 512 or 4096 there. I'm thinking just easiest to never partition in smaller units than 4096.

[Scott Moser (smoser)]

Dan and I discussed some and both of us were not sure if sfdisk's sectors were physical or logical sectors.

I did some testing with kvm and it turns out it uses logical sectors.
That unit is the same you'd get back from 'blockdev --getss' that value can also be read from /sys/

see http://paste.ubuntu.com/12864114/ for exampe (note, it didn't do one with 512/4096, but other tests do verify that logical is what is used).

[lp:~oddbloke/cloud-init/lp1460715]

1152. By Dan Watkins on 2015-10-20

Switch to using sectors for MBR disk setup.

[Dan Watkins (oddbloke)]

Right, I've updated the patch to use blockdev to find the size and sector size, which allows us to calculate the size of a disk in sectors.

I've tested this on wily and trusty, and it worked in both cases.

[Scott Moser (smoser)]

I'd like to read /sys if we can. blockdev is fine, but that costs us 2 subprocesses for information that we could conceivably read.

one last comment i should have added for reference later was that an example qemu command line where we provide all the sizes is like:

qemu-system-x86_64 -enable-kvm -drive file=/tmp/tmp.uCSmIFVSuA/disk.img,if=none,id=disk_img_1,format=qcow2 -device virtio-blk-pci,bootindex=1,scsi=off,drive=disk_img_1,id=virtio-disk-1, -drive file=/tmp/tmp.uCSmIFVSuA/extra.img,if=none,id=disk_img_2,format=raw -device virtio-blk-pci,bootindex=2,scsi=off,drive=disk_img_2,id=virtio-disk-2,physical_block_size=4096,logical_block_size=4096,min_io_size=4096,opt_io_size=4096, -device virtio-net-pci,netdev=net00 -netdev type=user,id=net00 -m 768 -echr 0x05 -nographic

[Scott Moser (smoser)]

Dan,
I'm ok to go with blockdev usage for now.
Could you set a nice commit message on this MP?

[Dan Watkins (oddbloke)]

Nice commit message set.

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:1152
https://server-team-jenkins.canonical.com/job/cloud-init-ci/44/
Executed test runs:
    None: https://server-team-jenkins.canonical.com/job/lp-vote-on-merge/18/console

Click here to trigger a rebuild:
https://server-team-jenkins.canonical.com/job/cloud-init-ci/44/rebuild

review: Needs Fixing (continuous-integration)

[Scott Moser (smoser)]

Merged with some updates at git commit 18203bf101dc04c28b53a92cd95c8be88959c428
https://git.launchpad.net/cloud-init/commit/18203bf101dc04c28b53a92cd95c8be88959c428

[Scott Moser (smoser)]

better link
 https://git.launchpad.net/cloud-init/commit/?id=18203bf101dc04c28b53a92cd95c8be88959c428

=== modified file 'cloudinit/config/cc_disk_setup.py'
--- cloudinit/config/cc_disk_setup.py	2015-07-22 19:14:33 +0000
+++ cloudinit/config/cc_disk_setup.py	2015-10-20 09:28:10 +0000
@@ -16,12 +16,13 @@
 #
 #    You should have received a copy of the GNU General Public License
 #    along with this program.  If not, see <http://www.gnu.org/licenses/>.
-from cloudinit.settings import PER_INSTANCE
-from cloudinit import util
 import logging
 import os
 import shlex
 
+from cloudinit import util
+from cloudinit.settings import PER_INSTANCE
+
 frequency = PER_INSTANCE
 
 # Define the commands to use
@@ -343,14 +344,13 @@
 
 
 def get_mbr_hdd_size(device):
-    size_cmd = [SFDISK_CMD, '--show-size', device]
-    size = None
     try:
-        size, _err = util.subp(size_cmd)
+        size_in_bytes, _ = util.subp([BLKDEV_CMD, '--getsize64', device])
+        sector_size, _ = util.subp([BLKDEV_CMD, '--getss', device])
     except Exception as e:
         raise Exception("Failed to get %s size\n%s" % (device, e))
 
-    return int(size.strip())
+    return int(size_in_bytes) / int(sector_size)
 
 
 def get_gpt_hdd_size(device):
@@ -492,7 +492,7 @@
                 raise Exception("Partition was incorrectly defined: %s" % part)
             percent, part_type = part
 
-        part_size = int((float(size) * (float(percent) / 100)) / 1024)
+        part_size = int(float(size) * (float(percent) / 100))
 
         if part_num == last_part_num:
             part_definition.append(",,%s" % part_type)
@@ -596,7 +596,7 @@
     types, i.e. gpt
     """
     # Create the partitions
-    prt_cmd = [SFDISK_CMD, "--Linux", "-uM", device]
+    prt_cmd = [SFDISK_CMD, device]
     try:
         util.subp(prt_cmd, data="%s\n" % layout)
     except Exception as e:

=== modified file 'tests/unittests/test_handler/test_handler_disk_setup.py'
--- tests/unittests/test_handler/test_handler_disk_setup.py	2015-03-18 13:32:54 +0000
+++ tests/unittests/test_handler/test_handler_disk_setup.py	2015-10-20 09:28:10 +0000
@@ -1,3 +1,5 @@
+import random
+
 from cloudinit.config import cc_disk_setup
 from ..helpers import ExitStack, mock, TestCase
 
@@ -28,3 +30,69 @@
         self.enumerate_disk.return_value = (mock.MagicMock() for _ in range(1))
         self.check_fs.return_value = (mock.MagicMock(), None, mock.MagicMock())
         self.assertFalse(cc_disk_setup.is_disk_used(mock.MagicMock()))
+
+
+class TestGetMbrHddSize(TestCase):
+
+    def setUp(self):
+        super(TestGetMbrHddSize, self).setUp()
+        self.patches = ExitStack()
+        self.subp = self.patches.enter_context(
+            mock.patch.object(cc_disk_setup.util, 'subp'))
+
+    def _configure_subp_mock(self, hdd_size_in_bytes, sector_size_in_bytes):
+        def _subp(cmd, *args, **kwargs):
+            self.assertEqual(3, len(cmd))
+            if '--getsize64' in cmd:
+                return hdd_size_in_bytes, None
+            elif '--getss' in cmd:
+                return sector_size_in_bytes, None
+            raise Exception('Unexpected blockdev command called')
+
+        self.subp.side_effect = _subp
+
+    def _test_for_sector_size(self, sector_size):
+        size_in_bytes = random.randint(10000, 10000000) * 512
+        size_in_sectors = size_in_bytes / sector_size
+        self._configure_subp_mock(size_in_bytes, sector_size)
+        self.assertEqual(size_in_sectors,
+                         cc_disk_setup.get_mbr_hdd_size('/dev/sda1'))
+
+    def test_size_for_512_byte_sectors(self):
+        self._test_for_sector_size(512)
+
+    def test_size_for_1024_byte_sectors(self):
+        self._test_for_sector_size(1024)
+
+    def test_size_for_2048_byte_sectors(self):
+        self._test_for_sector_size(2048)
+
+    def test_size_for_4096_byte_sectors(self):
+        self._test_for_sector_size(4096)
+
+
+class TestGetPartitionMbrLayout(TestCase):
+
+    def test_single_partition_using_boolean(self):
+        self.assertEqual('0,',
+                         cc_disk_setup.get_partition_mbr_layout(1000, True))
+
+    def test_single_partition_using_list(self):
+        disk_size = random.randint(1000000, 1000000000000)
+        self.assertEqual(
+            ',,83',
+            cc_disk_setup.get_partition_mbr_layout(disk_size, [100]))
+
+    def test_half_and_half(self):
+        disk_size = random.randint(1000000, 1000000000000)
+        expected_partition_size = int(float(disk_size) / 2)
+        self.assertEqual(
+            ',{0},83\n,,83'.format(expected_partition_size),
+            cc_disk_setup.get_partition_mbr_layout(disk_size, [50, 50]))
+
+    def test_thirds_with_different_partition_type(self):
+        disk_size = random.randint(1000000, 1000000000000)
+        expected_partition_size = int(float(disk_size) * 0.33)
+        self.assertEqual(
+            ',{0},83\n,,82'.format(expected_partition_size),
+            cc_disk_setup.get_partition_mbr_layout(disk_size, [33, [66, 82]]))