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

Proposed by Ryan Harper
Status: Merged
Approved by: Scott Moser
Approved revision: a79452b863516727eeae12ff6e41b77044accbf5
Merged at revision: 358fca4b96f0c6cea300c6a65d4350166fc8ff48
Proposed branch: ~raharper/curtin:fix/clear-holders-swap-partition
Merge into: curtin:master
Diff against target: 213 lines (+127/-0)
6 files modified
curtin/block/clear_holders.py (+16/-0)
curtin/swap.py (+15/-0)
examples/tests/basic.yaml (+9/-0)
examples/tests/dirty_disks_config.yaml (+26/-0)
tests/unittests/test_clear_holders.py (+22/-0)
tests/unittests/test_swap.py (+39/-0)
Reviewer Review Type Date Requested Status
Scott Moser Approve
Server Team CI bot continuous-integration Approve
Review via email: mp+336335@code.launchpad.net

Description of the change

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

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)
Revision history for this message
Chad Smith (chad.smith) :
Revision history for this message
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Scott Moser (smoser) :
review: Needs Fixing
Revision history for this message
Ryan Harper (raharper) wrote :

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.
>

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

Updated to address feedback.

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

these are nit picks.
generally this is fine.

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

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

ack,thanks.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/curtin/block/clear_holders.py b/curtin/block/clear_holders.py
2index cd18f14..88907db 100644
3--- a/curtin/block/clear_holders.py
4+++ b/curtin/block/clear_holders.py
5@@ -11,6 +11,7 @@ import os
6 import time
7
8 from curtin import (block, udev, util)
9+from curtin.swap import is_swap_device
10 from curtin.block import lvm
11 from curtin.block import mdadm
12 from curtin.block import zfs
13@@ -251,6 +252,9 @@ def wipe_superblock(device):
14 poolname = zfs.device_to_poolname(blockdev)
15 zfs.zpool_export(poolname)
16
17+ if is_swap_device(blockdev):
18+ shutdown_swap(blockdev)
19+
20 # some volumes will be claimed by the bcache layer but do not surface
21 # an actual /dev/bcacheN device which owns the parts (backing, cache)
22 # The result is that some volumes cannot be wiped while bcache claims
23@@ -322,6 +326,18 @@ def identify_partition(device):
24 return os.path.exists(path)
25
26
27+def shutdown_swap(path):
28+ """release swap device from kernel swap pool if present"""
29+ procswaps = util.load_file('/proc/swaps')
30+ for swapline in procswaps.splitlines():
31+ if swapline.startswith(path):
32+ msg = ('Removing %s from active use as swap device, '
33+ 'needed for storage config' % path)
34+ LOG.warning(msg)
35+ util.subp(['swapoff', path])
36+ return
37+
38+
39 def get_holders(device):
40 """
41 Look up any block device holders, return list of knames
42diff --git a/curtin/swap.py b/curtin/swap.py
43index 049c528..5f77b03 100644
44--- a/curtin/swap.py
45+++ b/curtin/swap.py
46@@ -1,6 +1,7 @@
47 # This file is part of curtin. See LICENSE file for copyright and license info.
48
49 import os
50+import resource
51
52 from .log import LOG
53 from . import util
54@@ -93,4 +94,18 @@ def setup_swapfile(target, fstab=None, swapfile=None, size=None, maxsize=None):
55 os.unlink(fpath)
56 raise
57
58+
59+def is_swap_device(path):
60+ """
61+ Determine if specified device is a swap device. Linux swap devices write
62+ a magic header value on kernel PAGESIZE - 10.
63+
64+ https://github.com/torvalds/linux/blob/master/include/linux/swap.h#L111
65+ """
66+ LOG.debug('Checking if %s is a swap device', path)
67+ swap_magic_offset = resource.getpagesize() - 10
68+ magic = util.load_file(path, read_len=10, offset=swap_magic_offset,
69+ decode=False)
70+ LOG.debug('Found swap magic: %s' % magic)
71+ return magic in [b'SWAPSPACE2', b'SWAP-SPACE']
72 # vi: ts=4 expandtab syntax=python
73diff --git a/examples/tests/basic.yaml b/examples/tests/basic.yaml
74index 4780d20..9b6bdd2 100644
75--- a/examples/tests/basic.yaml
76+++ b/examples/tests/basic.yaml
77@@ -21,6 +21,11 @@ storage:
78 number: 2
79 size: 1GB
80 device: sda
81+ - id: sda3
82+ type: partition
83+ number: 3
84+ size: 1GB
85+ device: sda
86 - id: sda1_root
87 type: format
88 fstype: ext4
89@@ -30,6 +35,10 @@ storage:
90 type: format
91 fstype: ext4
92 volume: sda2
93+ - id: sda3_swap
94+ type: format
95+ fstype: swap
96+ volume: sda3
97 - id: sda1_mount
98 type: mount
99 path: /
100diff --git a/examples/tests/dirty_disks_config.yaml b/examples/tests/dirty_disks_config.yaml
101index d8707cf..18d331d 100644
102--- a/examples/tests/dirty_disks_config.yaml
103+++ b/examples/tests/dirty_disks_config.yaml
104@@ -1,3 +1,28 @@
105+bucket:
106+ - &run_if_systemd |
107+ #!/bin/sh
108+ command -v systemctl && systemctl mask media-root\\x2dro.mount
109+ exit 0
110+ - &swapon |
111+ #!/bin/sh
112+ # enable any swap device or partition if present
113+ devices=""
114+ for device in /dev/[hsv]d[a-z][0-9]*; do
115+ if ! [ -b "$device" ]; then
116+ continue
117+ fi
118+ MOFF=$((`getconf PAGESIZE` - 10))
119+ magic=$(dd if="$device" bs=1 skip=$MOFF count=10 2>/dev/null) || continue
120+ if [ "$magic" = "SWAPSPACE2" -o "$magic" = "SWAP-SPACE" ]; then
121+ devices="$devices $device"
122+ fi
123+ done
124+ for device in $devices; do
125+ swapon $device || true
126+ done
127+ swapon --show
128+ exit 0
129+
130 early_commands:
131 # running block-meta custom from the install environment
132 # inherits the CONFIG environment, so this works to actually prepare
133@@ -8,3 +33,4 @@ early_commands:
134 TARGET_MOUNT_POINT=/tmp/my.bdir/target,
135 WORKING_DIR=/tmp/my.bdir/work.d,
136 curtin, --showtrace, -v, block-meta, --umount, custom]
137+ enable_swaps: [sh, -c, *swapon]
138diff --git a/tests/unittests/test_clear_holders.py b/tests/unittests/test_clear_holders.py
139index cc33268..f2acf21 100644
140--- a/tests/unittests/test_clear_holders.py
141+++ b/tests/unittests/test_clear_holders.py
142@@ -700,4 +700,26 @@ class TestClearHolders(CiTestCase):
143 self.assertNotIn(mock.call('zfs'),
144 mock_util.load_kernel_module.call_args_list)
145
146+ @mock.patch('curtin.block.clear_holders.util')
147+ def test_shutdown_swap_calls_swapoff(self, mock_util):
148+ """clear_holders.shutdown_swap() calls swapoff on active swap device"""
149+ blockdev = '/mydev/dummydisk'
150+ mock_util.load_file.return_value = textwrap.dedent("""\
151+ Filename Type Size Used Priority
152+ %s disk 4194300 53508 -1
153+ """ % blockdev)
154+ clear_holders.shutdown_swap(blockdev)
155+ mock_util.subp.assert_called_with(['swapoff', blockdev])
156+
157+ @mock.patch('curtin.block.clear_holders.util')
158+ def test_shutdown_swap_skips_nomatch(self, mock_util):
159+ """clear_holders.shutdown_swap() ignores unmatched swapdevices"""
160+ blockdev = '/mydev/dummydisk'
161+ mock_util.load_file.return_value = textwrap.dedent("""\
162+ Filename Type Size Used Priority
163+ /foobar/wark disk 4194300 53508 -1
164+ """)
165+ clear_holders.shutdown_swap(blockdev)
166+ self.assertEqual(0, mock_util.subp.call_count)
167+
168 # vi: ts=4 expandtab syntax=python
169diff --git a/tests/unittests/test_swap.py b/tests/unittests/test_swap.py
170new file mode 100644
171index 0000000..e12d12e
172--- /dev/null
173+++ b/tests/unittests/test_swap.py
174@@ -0,0 +1,39 @@
175+import mock
176+
177+from curtin import swap
178+from .helpers import CiTestCase
179+
180+
181+class TestSwap(CiTestCase):
182+ @mock.patch('curtin.swap.resource')
183+ @mock.patch('curtin.swap.util')
184+ def test_is_swap_device_read_offsets(self, mock_util, mock_resource):
185+ """swap.is_swap_device() checks offsets based on system pagesize"""
186+ path = '/mydev/dummydisk'
187+ # 4k and 64k page size
188+ for pagesize in [4096, 65536]:
189+ magic_offset = pagesize - 10
190+ mock_resource.getpagesize.return_value = pagesize
191+ swap.is_swap_device(path)
192+ mock_util.load_file.assert_called_with(path, read_len=10,
193+ offset=magic_offset,
194+ decode=False)
195+
196+ @mock.patch('curtin.swap.resource')
197+ @mock.patch('curtin.swap.util')
198+ def test_identify_swap_false(self, mock_util, mock_resource):
199+ """swap.is_swap_device() returns false on non swap magic"""
200+ mock_util.load_file.return_value = (
201+ b'\x00\x00c\x05\x00\x00\x11\x00\x19\x00')
202+ is_swap = swap.is_swap_device('ignored')
203+ self.assertFalse(is_swap)
204+
205+ @mock.patch('curtin.swap.resource')
206+ @mock.patch('curtin.swap.util')
207+ def test_identify_swap_true(self, mock_util, mock_resource):
208+ """swap.is_swap_device() returns true on swap magic strings"""
209+ path = '/mydev/dummydisk'
210+ for magic in [b'SWAPSPACE2', b'SWAP-SPACE']:
211+ mock_util.load_file.return_value = magic
212+ is_swap = swap.is_swap_device(path)
213+ self.assertTrue(is_swap)

Subscribers

People subscribed via source and target branches