Merge ~raharper/curtin:fix/clear-holders-swap-partition into curtin:master

[Ryan Harper]

clear-holders: detect and remove devices from kernel swap as needed

During testing of subiquity the ephemeral environment may activate
existing swap partitions which then prevent curtin from using a device during
storage configuration. This branch introduces a change to TestBasic to add
a swap partition and a command to the dirty-disk mode to forcibly enable
the swap device to reproduce the error. The fix involves having
clear_holders detect and remove the device (disk or partition) from
the kernel swap via use of swapoff.

LP: #1743643

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

PASSED: Continuous integration, rev:a67f0f460dc92794a4986e0043adb3256c9962a2
https://jenkins.ubuntu.com/server/job/curtin-ci/760/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/760
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/760
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/760
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/760

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/760/rebuild

review: Approve (continuous-integration)

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

PASSED: Continuous integration, rev:6a05f91070388003d2602c5383e914429fb33135
https://jenkins.ubuntu.com/server/job/curtin-ci/768/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/768
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/768
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/768
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/768

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/768/rebuild

review: Approve (continuous-integration)

[Scott Moser (smoser)]

review: Needs Fixing

[Ryan Harper (raharper)]

On Thu, Jan 25, 2018 at 7:17 PM, Scott Moser <email address hidden>
wrote:

> Review: Needs Fixing
>
>
>
> Diff comments:
>
> > diff --git a/curtin/swap.py b/curtin/swap.py
> > index 049c528..c77117c 100644
> > --- a/curtin/swap.py
> > +++ b/curtin/swap.py
> > @@ -93,4 +94,20 @@ def setup_swapfile(target, fstab=None, swapfile=None,
> size=None, maxsize=None):
> > os.unlink(fpath)
> > raise
> >
> > +<<<<<<< curtin/swap.py
> > # vi: ts=4 expandtab syntax=python
> > +=======
> > +
> > +def is_swap_device(path):
> > + """
> > + Determine if specified device is a swap device. Linux swap devices
> write
> > + a magic header value on kernel PAGESIZE - 10.
> > +
> > + https://github.com/torvalds/linux/blob/master/include/
> linux/swap.h#L111
> > + """
> > + LOG.debug('Checking if %s is a swap device', path)
> > + swap_magic_offset = resource.getpagesize() - 10
> > + magic = util.load_file(path, read_len=10, offset=swap_magic_offset)
>
> You're basically opening a device and reading from it.
> It very likely has non-utf-8 data on it, so you need to pass decode=False
> or it could raise a unicodedecode error.
>

OK.

>
> > + LOG.debug('Found swap magic: %s' % magic)
> > + return magic in ["SWAPSPACE2", "SWAP-SPACE"]
> > +>>>>>>> curtin/swap.py
> > diff --git a/examples/tests/dirty_disks_config.yaml
> b/examples/tests/dirty_disks_config.yaml
> > index 58d7713..3f22c24 100644
> > --- a/examples/tests/dirty_disks_config.yaml
> > +++ b/examples/tests/dirty_disks_config.yaml
> > @@ -3,6 +3,25 @@ bucket:
> > #!/bin/sh
> > command -v systemctl && systemctl mask media-root\\x2dro.mount
> > exit 0
> > + - &swapon |
> > + #!/bin/sh
> > + # enable any swap device or partition if present
> > + devices=""
> > + for device in /dev/[hsv]d[a-z][0-9]*; do
> > + if ! [ -b "$device" ]; then
> > + continue
> > + fi
> > + /sbin/blkid -o udev -p ${device%%[0-9]*} | grep -q
> "^ID_FS_USAGE=raid" && continue
> > + magic=$(/bin/dd if="$device" bs=4086 skip=1 count=1 2>/dev/null
> | /bin/dd bs=10 count=1 2>/dev/null) || continue
>
> drop /bin/dd just 'dd'.
> this reads bytes 4086 to 4096 ?
> sudo dd if=/swap.img bs=1 count=10 skip=4086
>
> also, can't you just use blkid ?
> $ sudo blkid /swap.img
> /swap.img: UUID="e7498b09-7346-4d64-9371-9f1ba738686d" TYPE="swap"
>

Well, the read is tempting; if I can get kernel PAGE_SIZE easly, then I'll
use that to
find PAGE_SIZE - 10; otherwise blkid will work but heavier.

> > + if [ "$magic" = "SWAPSPACE2" -o "$magic" = "SWAP-SPACE" ]; then
> > + devices="$devices $device"
> > + fi
> > + done
> > + for device in $devices; do
> > + swapon $device || true
> > + done
> > + swapon --show
> > + exit 0
> >
> > early_commands:
> > # systemd sucks; automatic mount and fsck services clash with curtin
>
>
> --
> https://code.launchpad.net/~raharper/curtin/+git/curtin/+merge/336335
> You are the owner of ~raharper/curtin:fix/clear-holders-swap-partition.
>

[Ryan Harper (raharper)]

Updated to address feedback.

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

PASSED: Continuous integration, rev:dd8d38f953b67fa9ea15f8532e7c3cc3b195dd1e
https://jenkins.ubuntu.com/server/job/curtin-ci/790/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/790
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/790
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/790
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/790

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/790/rebuild

review: Approve (continuous-integration)

[Scott Moser (smoser)]

these are nit picks.
generally this is fine.

i just like to avoid having code that I dont know why it is there.

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

PASSED: Continuous integration, rev:a79452b863516727eeae12ff6e41b77044accbf5
https://jenkins.ubuntu.com/server/job/curtin-ci/804/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/804
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/804
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/804
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/804

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/804/rebuild

review: Approve (continuous-integration)

[Scott Moser (smoser)]

ack,thanks.

review: Approve

diff --git a/curtin/block/clear_holders.py b/curtin/block/clear_holders.py
index cd18f14..88907db 100644
--- a/curtin/block/clear_holders.py
+++ b/curtin/block/clear_holders.py
@@ -11,6 +11,7 @@ import os
 import time
 
 from curtin import (block, udev, util)
+from curtin.swap import is_swap_device
 from curtin.block import lvm
 from curtin.block import mdadm
 from curtin.block import zfs
@@ -251,6 +252,9 @@ def wipe_superblock(device):
             poolname = zfs.device_to_poolname(blockdev)
             zfs.zpool_export(poolname)
 
+        if is_swap_device(blockdev):
+            shutdown_swap(blockdev)
+
         # some volumes will be claimed by the bcache layer but do not surface
         # an actual /dev/bcacheN device which owns the parts (backing, cache)
         # The result is that some volumes cannot be wiped while bcache claims
@@ -322,6 +326,18 @@ def identify_partition(device):
     return os.path.exists(path)
 
 
+def shutdown_swap(path):
+    """release swap device from kernel swap pool if present"""
+    procswaps = util.load_file('/proc/swaps')
+    for swapline in procswaps.splitlines():
+        if swapline.startswith(path):
+            msg = ('Removing %s from active use as swap device, '
+                   'needed for storage config' % path)
+            LOG.warning(msg)
+            util.subp(['swapoff', path])
+            return
+
+
 def get_holders(device):
     """
     Look up any block device holders, return list of knames
diff --git a/curtin/swap.py b/curtin/swap.py
index 049c528..5f77b03 100644
--- a/curtin/swap.py
+++ b/curtin/swap.py
@@ -1,6 +1,7 @@
 # This file is part of curtin. See LICENSE file for copyright and license info.
 
 import os
+import resource
 
 from .log import LOG
 from . import util
@@ -93,4 +94,18 @@ def setup_swapfile(target, fstab=None, swapfile=None, size=None, maxsize=None):
         os.unlink(fpath)
         raise
 
+
+def is_swap_device(path):
+    """
+    Determine if specified device is a swap device.  Linux swap devices write
+    a magic header value on kernel PAGESIZE - 10.
+
+    https://github.com/torvalds/linux/blob/master/include/linux/swap.h#L111
+    """
+    LOG.debug('Checking if %s is a swap device', path)
+    swap_magic_offset = resource.getpagesize() - 10
+    magic = util.load_file(path, read_len=10, offset=swap_magic_offset,
+                           decode=False)
+    LOG.debug('Found swap magic: %s' % magic)
+    return magic in [b'SWAPSPACE2', b'SWAP-SPACE']
 # vi: ts=4 expandtab syntax=python
diff --git a/examples/tests/basic.yaml b/examples/tests/basic.yaml
index 4780d20..9b6bdd2 100644
--- a/examples/tests/basic.yaml
+++ b/examples/tests/basic.yaml
@@ -21,6 +21,11 @@ storage:
         number: 2
         size: 1GB
         device: sda
+      - id: sda3
+        type: partition
+        number: 3
+        size: 1GB
+        device: sda
       - id: sda1_root
         type: format
         fstype: ext4
@@ -30,6 +35,10 @@ storage:
         type: format
         fstype: ext4
         volume: sda2
+      - id: sda3_swap
+        type: format
+        fstype: swap
+        volume: sda3
       - id: sda1_mount
         type: mount
         path: /
diff --git a/examples/tests/dirty_disks_config.yaml b/examples/tests/dirty_disks_config.yaml
index d8707cf..18d331d 100644
--- a/examples/tests/dirty_disks_config.yaml
+++ b/examples/tests/dirty_disks_config.yaml
@@ -1,3 +1,28 @@
+bucket:
+  - &run_if_systemd |
+    #!/bin/sh
+    command -v systemctl && systemctl mask media-root\\x2dro.mount
+    exit 0
+  - &swapon |
+    #!/bin/sh
+    # enable any swap device or partition if present
+    devices=""
+    for device in /dev/[hsv]d[a-z][0-9]*; do
+        if ! [ -b "$device" ]; then
+            continue
+        fi
+        MOFF=$((`getconf PAGESIZE` - 10))
+        magic=$(dd if="$device" bs=1 skip=$MOFF count=10 2>/dev/null) || continue
+        if [ "$magic" = "SWAPSPACE2" -o "$magic" = "SWAP-SPACE" ]; then
+            devices="$devices $device"
+        fi
+    done
+    for device in $devices; do
+        swapon $device || true
+    done
+    swapon --show
+    exit 0
+
 early_commands:
   # running block-meta custom from the install environment
   # inherits the CONFIG environment, so this works to actually prepare
@@ -8,3 +33,4 @@ early_commands:
               TARGET_MOUNT_POINT=/tmp/my.bdir/target,
               WORKING_DIR=/tmp/my.bdir/work.d, 
               curtin, --showtrace, -v, block-meta, --umount, custom]
+  enable_swaps: [sh, -c, *swapon]
diff --git a/tests/unittests/test_clear_holders.py b/tests/unittests/test_clear_holders.py
index cc33268..f2acf21 100644
--- a/tests/unittests/test_clear_holders.py
+++ b/tests/unittests/test_clear_holders.py
@@ -700,4 +700,26 @@ class TestClearHolders(CiTestCase):
             self.assertNotIn(mock.call('zfs'),
                              mock_util.load_kernel_module.call_args_list)
 
+    @mock.patch('curtin.block.clear_holders.util')
+    def test_shutdown_swap_calls_swapoff(self, mock_util):
+        """clear_holders.shutdown_swap() calls swapoff on active swap device"""
+        blockdev = '/mydev/dummydisk'
+        mock_util.load_file.return_value = textwrap.dedent("""\
+            Filename				Type		Size	Used	Priority
+            %s        disk		4194300	53508	-1
+        """ % blockdev)
+        clear_holders.shutdown_swap(blockdev)
+        mock_util.subp.assert_called_with(['swapoff', blockdev])
+
+    @mock.patch('curtin.block.clear_holders.util')
+    def test_shutdown_swap_skips_nomatch(self, mock_util):
+        """clear_holders.shutdown_swap() ignores unmatched swapdevices"""
+        blockdev = '/mydev/dummydisk'
+        mock_util.load_file.return_value = textwrap.dedent("""\
+            Filename				Type		Size	Used	Priority
+            /foobar/wark            disk		4194300	53508	-1
+        """)
+        clear_holders.shutdown_swap(blockdev)
+        self.assertEqual(0, mock_util.subp.call_count)
+
 # vi: ts=4 expandtab syntax=python
diff --git a/tests/unittests/test_swap.py b/tests/unittests/test_swap.py
new file mode 100644
index 0000000..e12d12e
--- /dev/null
+++ b/tests/unittests/test_swap.py
@@ -0,0 +1,39 @@
+import mock
+
+from curtin import swap
+from .helpers import CiTestCase
+
+
+class TestSwap(CiTestCase):
+    @mock.patch('curtin.swap.resource')
+    @mock.patch('curtin.swap.util')
+    def test_is_swap_device_read_offsets(self, mock_util, mock_resource):
+        """swap.is_swap_device() checks offsets based on system pagesize"""
+        path = '/mydev/dummydisk'
+        # 4k and 64k page size
+        for pagesize in [4096, 65536]:
+            magic_offset = pagesize - 10
+            mock_resource.getpagesize.return_value = pagesize
+            swap.is_swap_device(path)
+            mock_util.load_file.assert_called_with(path, read_len=10,
+                                                   offset=magic_offset,
+                                                   decode=False)
+
+    @mock.patch('curtin.swap.resource')
+    @mock.patch('curtin.swap.util')
+    def test_identify_swap_false(self, mock_util, mock_resource):
+        """swap.is_swap_device() returns false on non swap magic"""
+        mock_util.load_file.return_value = (
+            b'\x00\x00c\x05\x00\x00\x11\x00\x19\x00')
+        is_swap = swap.is_swap_device('ignored')
+        self.assertFalse(is_swap)
+
+    @mock.patch('curtin.swap.resource')
+    @mock.patch('curtin.swap.util')
+    def test_identify_swap_true(self, mock_util, mock_resource):
+        """swap.is_swap_device() returns true on swap magic strings"""
+        path = '/mydev/dummydisk'
+        for magic in [b'SWAPSPACE2', b'SWAP-SPACE']:
+            mock_util.load_file.return_value = magic
+            is_swap = swap.is_swap_device(path)
+            self.assertTrue(is_swap)