Merge lp:~jamesodhunt/ubuntu/vivid/ubuntu-core-upgrader/implement-format-command into lp:ubuntu/vivid/ubuntu-core-upgrader

Proposed by James Hunt on 2015-04-07
Status: Merged
Merged at revision: 23
Proposed branch: lp:~jamesodhunt/ubuntu/vivid/ubuntu-core-upgrader/implement-format-command
Merge into: lp:ubuntu/vivid/ubuntu-core-upgrader
Diff against target: 215 lines (+131/-7)
3 files modified
debian/changelog (+7/-0)
ubuntucoreupgrader/tests/test_upgrader.py (+45/-0)
ubuntucoreupgrader/upgrader.py (+79/-7)
To merge this branch: bzr merge lp:~jamesodhunt/ubuntu/vivid/ubuntu-core-upgrader/implement-format-command
Reviewer Review Type Date Requested Status
Ubuntu branches 2015-04-07 Pending
Review via email: mp+255359@code.launchpad.net

Description of the change

* Implement s-i format command to handle full-image (non-delta) updates
  correctly.

Details:

* ubuntucoreupgrader/tests/test_upgrader.py:
  - test_format(): New test to ensure mkfs called appropriately.
* ubuntucoreupgrader/upgrader.py:
  - get_mount_details(): New function, called by _cmd_format(), introduced to make testing easier.

To post a comment you must log in.
Michael Vogt (mvo) wrote :

Thanks for working on this! I like the tests!

I put some comments inline.

A note about the tests that is too big to fit into the inline comments. The tests can be simplified by using the automatic mocking features from unittest.mock. It provides automatic construction of mock function and asserts to test if the mocks go called (or got called with the right parameters). So something like:
"""
=== modified file 'ubuntucoreupgrader/tests/test_upgrader.py'
--- ubuntucoreupgrader/tests/test_upgrader.py 2015-04-07 13:30:32 +0000
+++ ubuntucoreupgrader/tests/test_upgrader.py 2015-04-08 14:18:03 +0000
@@ -195,18 +195,6 @@
     return l

-def mock_get_mount_details(target):
- return [True, True, True]
-
-
-def mock_mount(source, target, options=None):
- pass
-
-
-def mock_unmount(target, options=None):
- pass
-
-
 class UbuntuCoreUpgraderObectTestCase(UbuntuCoreUpgraderTestCase):

     def mock_get_cache_dir(self):
@@ -255,20 +243,16 @@

         shutil.rmtree(root_dir)

- @patch('ubuntucoreupgrader.upgrader.get_mount_details',
- mock_get_mount_details)
- @patch('ubuntucoreupgrader.upgrader.mount', mock_mount)
- @patch('ubuntucoreupgrader.upgrader.unmount', mock_unmount)
- def test_format(self):
-
- self.mkfs_called = False
-
- def mock_mkfs(device, fs_type, label):
- self.mkfs_called = True
-
+ @patch('ubuntucoreupgrader.upgrader.get_mount_details')
+ @patch('ubuntucoreupgrader.upgrader.mount')
+ @patch('ubuntucoreupgrader.upgrader.unmount')
+ def test_format(self, mock_umount, mock_mount, mock_mount_details):
+ MOCK_FS_TUPLE = ("device", "fstype", "label")
+ mock_mount_details.return_value = MOCK_FS_TUPLE
+
         # If the command file does not contain the format command, mkfs
         # should not be called.
- with patch('ubuntucoreupgrader.upgrader.mkfs', mock_mkfs):
+ with patch('ubuntucoreupgrader.upgrader.mkfs') as mock_mkfs:
             args = ['cmdfile']
             options = parse_args(args=args)
             commands = make_commands([self.TARFILE])
@@ -279,11 +263,11 @@

         # No format command in command file, so should not have been
         # called.
- self.assertFalse(self.mkfs_called)
+ self.assertFalse(mock_mkfs.called)

         # mkfs should be called if the format command is specified in
         # the command file.
- with patch('ubuntucoreupgrader.upgrader.mkfs', mock_mkfs):
+ with patch('ubuntucoreupgrader.upgrader.mkfs') as mock_mkfs:
             args = ['cmdfile']
             options = parse_args(args=args)
             commands = make_commands([self.TARFILE])
@@ -296,8 +280,7 @@
             upgrader.MOUNTPOINT_CMD = "true"
             upgrader.run()

- self.assertTrue(self.mkfs_called)
-
+ mock_mkfs.assert_called_with(*MOCK_FS_TUPLE)

 if __name__ == "__main__":
     unittest.main()

"""

27. By James Hunt on 2015-04-08

* Review changes.

James Hunt (jamesodhunt) wrote :

I've applied most of the suggestions, but see responses below.

Michael Vogt (mvo) wrote :

Thanks for updating the branch. I still feel uneasy about the sys.exit() but as you pointed out this is already done in other places in the code.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'debian/changelog'
2--- debian/changelog 2015-03-13 08:14:06 +0000
3+++ debian/changelog 2015-04-08 15:51:51 +0000
4@@ -1,3 +1,10 @@
5+ubuntu-core-upgrader (0.7.8) UNRELEASED; urgency=medium
6+
7+ * Implement s-i format command to handle full-image (non-delta) updates
8+ correctly.
9+
10+ -- James Hunt <james.hunt@ubuntu.com> Tue, 07 Apr 2015 14:29:29 +0100
11+
12 ubuntu-core-upgrader (0.7.7) vivid; urgency=low
13
14 * fix entry point for s-i 3.0
15
16=== modified file 'ubuntucoreupgrader/tests/test_upgrader.py'
17--- ubuntucoreupgrader/tests/test_upgrader.py 2015-03-04 16:50:08 +0000
18+++ ubuntucoreupgrader/tests/test_upgrader.py 2015-04-08 15:51:51 +0000
19@@ -27,6 +27,7 @@
20 import tarfile
21 import tempfile
22 import unittest
23+from unittest.mock import patch
24 import os
25 import shutil
26 import sys
27@@ -242,6 +243,50 @@
28
29 shutil.rmtree(root_dir)
30
31+ @patch('ubuntucoreupgrader.upgrader.get_mount_details')
32+ @patch('ubuntucoreupgrader.upgrader.mount')
33+ @patch('ubuntucoreupgrader.upgrader.unmount')
34+ def test_no_format_in_cmd(self, mock_umount, mock_mount,
35+ mock_mount_details):
36+
37+ # If the command file does not contain the format command, mkfs
38+ # should not be called.
39+ with patch('ubuntucoreupgrader.upgrader.mkfs') as mock_mkfs:
40+ args = ['cmdfile']
41+ options = parse_args(args=args)
42+ commands = make_commands([self.TARFILE])
43+ upgrader = Upgrader(options, commands, [])
44+ upgrader.TIMESTAMP_FILE = '/dev/null'
45+ upgrader.MOUNTPOINT_CMD = "true"
46+ upgrader.run()
47+
48+ # No format command in command file, so should not have been
49+ # called.
50+ self.assertFalse(mock_mkfs.called)
51+
52+ @patch('ubuntucoreupgrader.upgrader.get_mount_details')
53+ @patch('ubuntucoreupgrader.upgrader.mount')
54+ @patch('ubuntucoreupgrader.upgrader.unmount')
55+ def test_format(self, mock_umount, mock_mount, mock_mount_details):
56+ MOCK_FS_TUPLE = ("device", "fstype", "label")
57+ mock_mount_details.return_value = MOCK_FS_TUPLE
58+
59+ # mkfs should be called if the format command is specified in
60+ # the command file.
61+ with patch('ubuntucoreupgrader.upgrader.mkfs') as mock_mkfs:
62+ args = ['cmdfile']
63+ options = parse_args(args=args)
64+ commands = make_commands([self.TARFILE])
65+
66+ # add format command to command file
67+ commands.insert(0, 'format system')
68+
69+ upgrader = Upgrader(options, commands, [])
70+ upgrader.TIMESTAMP_FILE = '/dev/null'
71+ upgrader.MOUNTPOINT_CMD = "true"
72+ upgrader.run()
73+
74+ mock_mkfs.assert_called_with(*MOCK_FS_TUPLE)
75
76 if __name__ == "__main__":
77 unittest.main()
78
79=== modified file 'ubuntucoreupgrader/upgrader.py'
80--- ubuntucoreupgrader/upgrader.py 2015-03-05 13:24:40 +0000
81+++ ubuntucoreupgrader/upgrader.py 2015-04-08 15:51:51 +0000
82@@ -203,6 +203,56 @@
83 sys.exit(1)
84
85
86+def mkfs(device, fs_type, label):
87+ '''
88+ Run mkfs(8) on specified device.
89+ '''
90+ assert (os.path.exists(device))
91+
92+ args = ['/sbin/mkfs',
93+ '-v'
94+ '-t', fs_type,
95+ '-L', label,
96+ device]
97+
98+ log.debug('running: {}'.format(args))
99+
100+ proc = subprocess.Popen(args,
101+ stdout=subprocess.PIPE,
102+ stderr=subprocess.PIPE,
103+ universal_newlines=True)
104+
105+ ret = proc.wait()
106+
107+ if ret == 0:
108+ return
109+
110+ stdout, stderr = proc.communicate()
111+
112+ log.error('{} returned {}: {}, {}'
113+ .format(args,
114+ ret,
115+ stdout,
116+ stderr))
117+ sys.exit(1)
118+
119+
120+def get_mount_details(target):
121+ '''
122+ Returns a list comprising device, filesystem type and filesystem
123+ label for the mount target specified.
124+ '''
125+ args = ['findmnt', '-o', 'SOURCE,FSTYPE,LABEL', '-n', target]
126+ output = subprocess.check_output(args, universal_newlines=True)
127+ output = output.strip().split()
128+
129+ if len(output) != 3:
130+ sys.exit('Failed to determine mount details for {}'
131+ .format(target))
132+
133+ return output
134+
135+
136 def remount(mountpoint, options):
137 """
138 Remount mountpoint using the specified options string (which is
139@@ -468,10 +518,11 @@
140 # If True, the other partition is considered empty. In this
141 # scenario, prior to unpacking the latest image, the _current_
142 # rootfs image is copied to the other partition.
143- #
144- # Note: Only used by UPGRADE_IN_PLACE.
145 self.other_is_empty = False
146
147+ # True if the upgrader has reformatted the "other" partition.
148+ self.other_has_been_formatted = False
149+
150 def update_timestamp(self):
151 '''
152 Update the timestamp file to record the time the last upgrade
153@@ -566,9 +617,17 @@
154 if self.options.dry_run or self.options.root_dir != '/':
155 return
156
157- log.warning('ingoring format target {} (unsupported operation)'
158- .format(target))
159- return
160+ other = self.get_mount_target()
161+
162+ source, fstype, label = get_mount_details(other)
163+
164+ unmount(other)
165+ mkfs(source, fstype, label)
166+
167+ # leave the mount as it was initially.
168+ mount(source, other, "ro")
169+
170+ self.other_has_been_formatted = True
171
172 def _cmd_load_keyring(self, args):
173 try:
174@@ -609,6 +668,19 @@
175 .format(target_type))
176 return
177
178+ if self.other_is_empty and not self.other_has_been_formatted:
179+ # We believe "other" is empty. However, it's possible that a
180+ # previous attempt at upgrading failed due to a power
181+ # outage. In that scenario, "other" may contain most of a
182+ # rootfs image, but just be missing the files used to
183+ # determine that the partition is empty. As such, if we
184+ # believe the partition is empty, it should forcibly be made
185+ # empty since it may contain detritus from a
186+ # previously-failed unpack (possibly caused by a power
187+ # outage).
188+ log.warning('reformatting other (no system image)')
189+ self._cmd_format("system")
190+
191 self.remount_rootfs(writable=True)
192
193 if self.other_is_empty:
194@@ -739,6 +811,7 @@
195 else:
196 to_remove = []
197
198+ # by definition, full images don't have 'removed' files.
199 if not self.full_image:
200
201 if found_removed_file:
202@@ -816,7 +889,7 @@
203 Copy all rootfs data from the current partition to the other
204 partition.
205
206- XXX: Assumes that the other rootfs is already mounted to
207+ XXX: Assumes that the other rootfs is already mounted r/w to
208 mountpoint get_mount_target().
209 '''
210 target = self.get_mount_target()
211@@ -846,4 +919,3 @@
212
213 unmount(bindmount_rootfs_dir)
214 os.rmdir(bindmount_rootfs_dir)
215- self.other_is_empty = False

Subscribers

People subscribed via source and target branches

to all changes: