Merge lp:~ltrager/maas/validate_storage into lp:~maas-committers/maas/trunk
- validate_storage
- Merge into trunk
Status: | Merged |
---|---|
Approved by: | Blake Rouse |
Approved revision: | no longer in the source branch. |
Merged at revision: | 4506 |
Proposed branch: | lp:~ltrager/maas/validate_storage |
Merge into: | lp:~maas-committers/maas/trunk |
Diff against target: |
411 lines (+212/-13) 11 files modified
src/maasserver/models/node.py (+74/-1) src/maasserver/models/tests/test_filesystem.py (+4/-1) src/maasserver/models/tests/test_node.py (+33/-2) src/maasserver/models/tests/test_virtualblockdevice.py (+14/-0) src/maasserver/models/virtualblockdevice.py (+34/-0) src/maasserver/static/js/angular/controllers/node_details_storage.js (+8/-0) src/maasserver/static/js/angular/controllers/tests/test_node_details_storage.js (+14/-0) src/maasserver/static/partials/node-details.html (+9/-0) src/maasserver/testing/factory.py (+5/-1) src/maasserver/websockets/handlers/node.py (+1/-0) src/maasserver/websockets/handlers/tests/test_node.py (+16/-8) |
To merge this branch: | bzr merge lp:~ltrager/maas/validate_storage |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Gavin Panella (community) | Approve | ||
Review via email: mp+277402@code.launchpad.net |
Commit message
Validate a node has a boot device and / is mounted before deploying
Description of the change
This patch validates that the node has a boot device and has / mounted somewhere before deploying. This also adds an error message to the UI when this is not the case. I modified the make_Node method to also create a partition and mount it at '/' since we now require it to be a valid bootable node.
I also made two small fixes for the previous validate node on network patch
* I was raising a validation error with just the error message. If we don't return a dictionary the error doesn't display in the UI.
* Rename the on_network tests to not include 'managed'
Blake Rouse (blake-rouse) wrote : | # |
Lee Trager (ltrager) wrote : | # |
We now check if / is on a bcache block device, if we are we check if /boot is mounted not on a bcache device. I also made the error message more specific to why the storage layout isn't valid.
Gavin Panella (allenap) wrote : | # |
Lots of comments. I think there are a few things that need fixing before it can be landed. I'm worried about how the new methods will perform. I won't block landing based on that but it would be good if you could do something about it.
Lee Trager (ltrager) wrote : | # |
Thanks for the review. I've gone over and responded to your comments below. The performance issues you are seeing are all due to validating that / isn't on bcache. In order to check if a partition/block device is on bcache I need to check the parent devices. There isn't currently a relation between child and parent so I have to iterate over all the blockdevices the node has until I find the partition/block device in question.
There are only two ways I can think of that will improve performance
1. Modify the schema so we have a relation between child(virtual block devices, and partitions), and parent(block devices, partitions)
2. Don't check if / is on bcache.
Lee Trager (ltrager) wrote : | # |
I stand corrected. A partition/block device can have two file systems. One is the filesystem the administrator sets another is the one a user sets. I now use the get_effective_
The only place I use the first() method is getting the filesystem_group for a cache_set.
Gavin Panella (allenap) wrote : | # |
Replies to diff comments.
Gavin Panella (allenap) wrote : | # |
This seems sane to me. Just change storage_
Actually, hold up, there's no test for the change to node-details.html. Better get your Angular boots on. If you really want to land this now, undo the change to node-details.html and propose it separately with a test.
Lee Trager (ltrager) wrote : | # |
I took your suggestion and renamed storage_
MAAS Lander (maas-lander) wrote : | # |
The attempt to merge lp:~ltrager/maas/validate_storage into lp:maas failed. Below is the output from the failed tests.
Hit http://
Get:1 http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Get:2 http://
Get:3 http://
Get:4 http://
Get:5 http://
Get:6 http://
Get:7 http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Fetched 22.7 MB in 13s (1,725 kB/s)
Reading package lists...
sudo DEBIAN_
--no-
Reading package lists...
Building dependency tree...
Reading state information...
apache2 is...
Preview Diff
1 | === modified file 'src/maasserver/models/node.py' |
2 | --- src/maasserver/models/node.py 2015-11-13 20:01:04 +0000 |
3 | +++ src/maasserver/models/node.py 2015-11-17 23:04:09 +0000 |
4 | @@ -63,6 +63,7 @@ |
5 | from maasserver.enum import ( |
6 | ALLOCATED_NODE_STATUSES, |
7 | FILESYSTEM_FORMAT_TYPE_CHOICES_DICT, |
8 | + FILESYSTEM_TYPE, |
9 | INTERFACE_LINK_TYPE, |
10 | INTERFACE_TYPE, |
11 | IPADDRESS_FAMILY, |
12 | @@ -760,6 +761,73 @@ |
13 | event_action=action, |
14 | event_description=description) |
15 | |
16 | + def storage_layout_issues(self): |
17 | + """Return any errors with the storage layout. |
18 | + |
19 | + Checks that the node has / mounted. If / is mounted on bcache check |
20 | + that /boot is mounted and is not on bcache.""" |
21 | + def on_bcache(obj): |
22 | + if obj.type == "physical": |
23 | + return False |
24 | + elif obj.type == "partition": |
25 | + return on_bcache(obj.partition_table.block_device) |
26 | + for parent in obj.virtualblockdevice.get_parents(): |
27 | + if((parent.type != "physical" and on_bcache(parent)) or |
28 | + (parent.get_effective_filesystem().fstype in |
29 | + [FILESYSTEM_TYPE.BCACHE_CACHE, |
30 | + FILESYSTEM_TYPE.BCACHE_BACKING])): |
31 | + return True |
32 | + return False |
33 | + |
34 | + has_boot = False |
35 | + root_mounted = False |
36 | + root_on_bcache = False |
37 | + boot_mounted = False |
38 | + |
39 | + for block_device in self.blockdevice_set.all(): |
40 | + if block_device.is_boot_disk(): |
41 | + has_boot = True |
42 | + pt = block_device.get_partitiontable() |
43 | + if pt is not None: |
44 | + for partition in pt.partitions.all(): |
45 | + fs = partition.get_effective_filesystem() |
46 | + if fs is None: |
47 | + continue |
48 | + if fs.mount_point == '/': |
49 | + root_mounted = True |
50 | + if on_bcache(block_device): |
51 | + root_on_bcache = True |
52 | + else: |
53 | + # If / is mounted and its not on bcache the system |
54 | + # is bootable |
55 | + return [] |
56 | + elif (fs.mount_point == '/boot' and |
57 | + not on_bcache(block_device)): |
58 | + boot_mounted = True |
59 | + else: |
60 | + fs = block_device.get_effective_filesystem() |
61 | + if fs is None: |
62 | + continue |
63 | + if fs.mount_point == '/': |
64 | + root_mounted = True |
65 | + if on_bcache(block_device): |
66 | + root_on_bcache = True |
67 | + else: |
68 | + # If / is mounted and its not on bcache the system |
69 | + # is bootable. |
70 | + return [] |
71 | + elif fs.mount_point == '/boot' and not on_bcache(block_device): |
72 | + boot_mounted = True |
73 | + issues = [] |
74 | + if not has_boot: |
75 | + issues.append("Node must have boot disk.") |
76 | + if not root_mounted: |
77 | + issues.append("Node must have / mounted.") |
78 | + if root_mounted and root_on_bcache and not boot_mounted: |
79 | + issues.append("Because / is on a bcache volume you must create " |
80 | + "/boot on a non-bcache volume") |
81 | + return issues |
82 | + |
83 | def on_network(self): |
84 | """Return true if the node is connected to a managed network.""" |
85 | for interface in self.interface_set.all(): |
86 | @@ -772,7 +840,12 @@ |
87 | def _start_deployment(self): |
88 | """Mark a node as being deployed.""" |
89 | if self.on_network() is False: |
90 | - raise ValidationError("Node must be configured to use a network") |
91 | + raise ValidationError( |
92 | + {"network": |
93 | + ["Node must be configured to use a network"]}) |
94 | + storage_layout_issues = self.storage_layout_issues() |
95 | + if len(storage_layout_issues) > 0: |
96 | + raise ValidationError({"storage": storage_layout_issues}) |
97 | self.status = NODE_STATUS.DEPLOYING |
98 | self.save() |
99 | |
100 | |
101 | === modified file 'src/maasserver/models/tests/test_filesystem.py' |
102 | --- src/maasserver/models/tests/test_filesystem.py 2015-10-27 20:53:16 +0000 |
103 | +++ src/maasserver/models/tests/test_filesystem.py 2015-11-17 23:04:09 +0000 |
104 | @@ -29,6 +29,9 @@ |
105 | |
106 | def test_filter_by_node(self): |
107 | node = factory.make_Node() |
108 | + boot_bd = node.blockdevice_set.first() |
109 | + root_partition = boot_bd.partitiontable_set.first().partitions.first() |
110 | + root_fs = root_partition.filesystem_set.first() |
111 | block_device = factory.make_PhysicalBlockDevice(node=node) |
112 | bd_for_partitions = factory.make_PhysicalBlockDevice(node=node) |
113 | partition_table = factory.make_PartitionTable( |
114 | @@ -37,7 +40,7 @@ |
115 | filesystem_on_bd = factory.make_Filesystem(block_device=block_device) |
116 | filesystem_on_partition = factory.make_Filesystem(partition=partition) |
117 | self.assertItemsEqual( |
118 | - [filesystem_on_bd, filesystem_on_partition], |
119 | + [root_fs, filesystem_on_bd, filesystem_on_partition], |
120 | Filesystem.objects.filter_by_node(node)) |
121 | |
122 | |
123 | |
124 | === modified file 'src/maasserver/models/tests/test_node.py' |
125 | --- src/maasserver/models/tests/test_node.py 2015-11-10 18:12:57 +0000 |
126 | +++ src/maasserver/models/tests/test_node.py 2015-11-17 23:04:09 +0000 |
127 | @@ -3774,15 +3774,46 @@ |
128 | |
129 | self.assertThat(release_auto_ips, MockCalledOnceWith()) |
130 | |
131 | - def test_connected_to_managed_network_true_when_connected(self): |
132 | + def test_on_network_returns_true_when_connected(self): |
133 | node = factory.make_Node_with_Interface_on_Subnet( |
134 | status=NODE_STATUS.ALLOCATED) |
135 | self.assertTrue(node.on_network()) |
136 | |
137 | - def test_connected_to_managed_network_false_when_not_connected(self): |
138 | + def test_on_network_returns_false_when_not_connected(self): |
139 | node = factory.make_Node(status=NODE_STATUS.ALLOCATED) |
140 | self.assertFalse(node.on_network()) |
141 | |
142 | + def test_storage_layout_issues_is_valid_when_flat(self): |
143 | + node = factory.make_Node() |
144 | + self.assertEquals([], node.storage_layout_issues()) |
145 | + |
146 | + def test_storage_layout_issues_returns_valid_with_boot_and_bcache(self): |
147 | + node = factory.make_Node(with_boot_disk=False) |
148 | + boot_partition = factory.make_Partition(node=node) |
149 | + factory.make_Filesystem(partition=boot_partition, mount_point='/boot') |
150 | + fs_group = factory.make_FilesystemGroup( |
151 | + node=node, group_type=FILESYSTEM_GROUP_TYPE.BCACHE) |
152 | + bcache = fs_group.virtual_device |
153 | + factory.make_Filesystem(block_device=bcache, mount_point="/") |
154 | + self.assertEquals([], node.storage_layout_issues()) |
155 | + |
156 | + def test_storage_layout_issues_returns_invalid_when_no_disk(self): |
157 | + node = factory.make_Node(with_boot_disk=False) |
158 | + self.assertEquals(["Node must have boot disk.", |
159 | + "Node must have / mounted."], |
160 | + node.storage_layout_issues()) |
161 | + |
162 | + def test_storage_layout_issues_returns_invalid_when_root_on_bcache(self): |
163 | + node = factory.make_Node(with_boot_disk=False) |
164 | + factory.make_Partition(node=node) |
165 | + fs_group = factory.make_FilesystemGroup( |
166 | + node=node, group_type=FILESYSTEM_GROUP_TYPE.BCACHE) |
167 | + bcache = fs_group.virtual_device |
168 | + factory.make_Filesystem(block_device=bcache, mount_point="/") |
169 | + self.assertEquals(["Because / is on a bcache volume you must create " |
170 | + "/boot on a non-bcache volume"], |
171 | + node.storage_layout_issues()) |
172 | + |
173 | |
174 | class TestNode_Stop(MAASServerTestCase): |
175 | """Tests for Node.stop().""" |
176 | |
177 | === modified file 'src/maasserver/models/tests/test_virtualblockdevice.py' |
178 | --- src/maasserver/models/tests/test_virtualblockdevice.py 2015-08-14 13:48:59 +0000 |
179 | +++ src/maasserver/models/tests/test_virtualblockdevice.py 2015-11-17 23:04:09 +0000 |
180 | @@ -159,3 +159,17 @@ |
181 | uuid = uuid4() |
182 | block_device = factory.make_VirtualBlockDevice(uuid=uuid) |
183 | self.assertEquals('%s' % uuid, block_device.uuid) |
184 | + |
185 | + def test_get_parents_finds_devices(self): |
186 | + node = factory.make_Node() |
187 | + factory.make_FilesystemGroup( |
188 | + node=node, |
189 | + group_type=factory.pick_enum( |
190 | + FILESYSTEM_GROUP_TYPE, but_not=FILESYSTEM_GROUP_TYPE.LVM_VG)) |
191 | + fs_group_disks = [block_device.blockdevice_ptr |
192 | + for block_device in |
193 | + node.physicalblockdevice_set.all() |
194 | + if not block_device.is_boot_disk()] |
195 | + virtualblockdevice = node.virtualblockdevice_set.first() |
196 | + self.assertEqual( |
197 | + len(fs_group_disks), len(virtualblockdevice.get_parents())) |
198 | |
199 | === modified file 'src/maasserver/models/virtualblockdevice.py' |
200 | --- src/maasserver/models/virtualblockdevice.py 2015-11-16 06:26:17 +0000 |
201 | +++ src/maasserver/models/virtualblockdevice.py 2015-11-17 23:04:09 +0000 |
202 | @@ -137,3 +137,37 @@ |
203 | if not self.uuid: |
204 | self.uuid = uuid4() |
205 | return super(VirtualBlockDevice, self).save(*args, **kwargs) |
206 | + |
207 | + def get_parents(self): |
208 | + """Return the blockdevices and partition which make up this device.""" |
209 | + def check_fs_group(obj): |
210 | + fs = obj.get_effective_filesystem() |
211 | + if fs is None: |
212 | + return False |
213 | + if fs.filesystem_group is not None: |
214 | + fs_group = fs.filesystem_group |
215 | + elif fs.cache_set is not None: |
216 | + # A block device/partition can only have one cache_set |
217 | + fs_group = fs.cache_set.filesystemgroup_set.first() |
218 | + else: |
219 | + return False |
220 | + for virtual_device in fs_group.virtual_devices.all(): |
221 | + if virtual_device.id == self.id: |
222 | + return True |
223 | + return False |
224 | + |
225 | + parents = [] |
226 | + # We need to check all of the nodes block devices incase |
227 | + # we have nested virtual block devices. |
228 | + for block_device in self.node.blockdevice_set.all(): |
229 | + if block_device.id == self.id: |
230 | + continue |
231 | + if check_fs_group(block_device): |
232 | + parents.append(block_device) |
233 | + pt = block_device.get_partitiontable() |
234 | + if pt is None: |
235 | + continue |
236 | + for partition in pt.partitions.all(): |
237 | + if check_fs_group(partition): |
238 | + parents.append(partition) |
239 | + return parents |
240 | |
241 | === modified file 'src/maasserver/static/js/angular/controllers/node_details_storage.js' |
242 | --- src/maasserver/static/js/angular/controllers/node_details_storage.js 2015-11-10 17:36:38 +0000 |
243 | +++ src/maasserver/static/js/angular/controllers/node_details_storage.js 2015-11-17 23:04:09 +0000 |
244 | @@ -1913,4 +1913,12 @@ |
245 | return false; |
246 | } |
247 | }; |
248 | + |
249 | + // Returns true if there are storage layout errors |
250 | + $scope.hasStorageLayoutIssues = function() { |
251 | + if(angular.isObject($scope.node)) { |
252 | + return $scope.node.storage_layout_issues.length > 0; |
253 | + } |
254 | + return false; |
255 | + }; |
256 | }]); |
257 | |
258 | === modified file 'src/maasserver/static/js/angular/controllers/tests/test_node_details_storage.js' |
259 | --- src/maasserver/static/js/angular/controllers/tests/test_node_details_storage.js 2015-11-10 17:36:38 +0000 |
260 | +++ src/maasserver/static/js/angular/controllers/tests/test_node_details_storage.js 2015-11-17 23:04:09 +0000 |
261 | @@ -4744,4 +4744,18 @@ |
262 | expect($scope.isAllStorageDisabled()).toBe(true); |
263 | }); |
264 | }); |
265 | + |
266 | + describe("hasStorageLayoutIssues", function() { |
267 | + it("true when node.storage_layout_issues has issues", function() { |
268 | + var controller = makeController(); |
269 | + $scope.node.storage_layout_issues = [makeName("issue")]; |
270 | + expect($scope.hasStorageLayoutIssues()).toBe(true); |
271 | + }); |
272 | + |
273 | + it("false when node.storage_layout_issues has no issues", function() { |
274 | + var controller = makeController(); |
275 | + $scope.node.storage_layout_issues = []; |
276 | + expect($scope.hasStorageLayoutIssues()).toBe(false); |
277 | + }); |
278 | + }); |
279 | }); |
280 | |
281 | === modified file 'src/maasserver/static/partials/node-details.html' |
282 | --- src/maasserver/static/partials/node-details.html 2015-11-14 00:32:02 +0000 |
283 | +++ src/maasserver/static/partials/node-details.html 2015-11-17 23:04:09 +0000 |
284 | @@ -772,6 +772,15 @@ |
285 | </li> |
286 | </ul> |
287 | </div> |
288 | + <div class="twelve-col error ng-hide" data-ng-show="hasStorageLayoutIssues()"> |
289 | + <div data-ng-repeat="issue in node.storage_layout_issues"> |
290 | + <ul class="flash-messages"> |
291 | + <li class="flash-messages__item error"> |
292 | + {$ issue $} |
293 | + </li> |
294 | + </ul> |
295 | + </div> |
296 | + </div> |
297 | <div class="twelve-col padding-bottom no-margin"> |
298 | <h3>File systems</h3> |
299 | <section class="table"> |
300 | |
301 | === modified file 'src/maasserver/testing/factory.py' |
302 | --- src/maasserver/testing/factory.py 2015-11-12 19:57:04 +0000 |
303 | +++ src/maasserver/testing/factory.py 2015-11-17 23:04:09 +0000 |
304 | @@ -32,6 +32,7 @@ |
305 | from django.utils import timezone |
306 | from maasserver.clusterrpc.power_parameters import get_power_types |
307 | from maasserver.enum import ( |
308 | + ALLOCATED_NODE_STATUSES, |
309 | BOOT_RESOURCE_FILE_TYPE, |
310 | BOOT_RESOURCE_TYPE, |
311 | CACHE_MODE_TYPE, |
312 | @@ -330,7 +331,10 @@ |
313 | self.make_Interface( |
314 | INTERFACE_TYPE.PHYSICAL, node=node, vlan=vlan, fabric=fabric) |
315 | if installable and with_boot_disk: |
316 | - self.make_PhysicalBlockDevice(node=node) |
317 | + root_partition = self.make_Partition(node=node) |
318 | + acquired = node.status in ALLOCATED_NODE_STATUSES |
319 | + self.make_Filesystem( |
320 | + partition=root_partition, mount_point='/', acquired=acquired) |
321 | |
322 | # Update the 'updated'/'created' fields with a call to 'update' |
323 | # preventing a call to save() from overriding the values. |
324 | |
325 | === modified file 'src/maasserver/websockets/handlers/node.py' |
326 | --- src/maasserver/websockets/handlers/node.py 2015-11-10 23:41:49 +0000 |
327 | +++ src/maasserver/websockets/handlers/node.py 2015-11-17 23:04:09 +0000 |
328 | @@ -313,6 +313,7 @@ |
329 | {'key': key, 'ui': ui} |
330 | for key, ui in FILESYSTEM_FORMAT_TYPE_CHOICES |
331 | ] |
332 | + data["storage_layout_issues"] = obj.storage_layout_issues() |
333 | |
334 | # Events |
335 | data["events"] = self.dehydrate_events(obj) |
336 | |
337 | === modified file 'src/maasserver/websockets/handlers/tests/test_node.py' |
338 | --- src/maasserver/websockets/handlers/tests/test_node.py 2015-11-12 23:20:54 +0000 |
339 | +++ src/maasserver/websockets/handlers/tests/test_node.py 2015-11-17 23:04:09 +0000 |
340 | @@ -165,6 +165,7 @@ |
341 | "disable_ipv4": node.disable_ipv4, |
342 | "physical_disk_count": node.physicalblockdevice_set.count(), |
343 | "disks": disks, |
344 | + "storage_layout_issues": node.storage_layout_issues(), |
345 | "supported_filesystems": [ |
346 | {'key': key, 'ui': ui} |
347 | for key, ui in FILESYSTEM_FORMAT_TYPE_CHOICES], |
348 | @@ -1471,20 +1472,23 @@ |
349 | interface=True, |
350 | architecture=architecture, |
351 | status=NODE_STATUS.ALLOCATED) |
352 | - partition_table = factory.make_PartitionTable(node=node) |
353 | + block_device = factory.make_BlockDevice(node=node) |
354 | + partition_table = factory.make_PartitionTable( |
355 | + block_device=block_device, node=node) |
356 | size = partition_table.block_device.size / 2 |
357 | handler.create_partition({ |
358 | 'system_id': node.system_id, |
359 | 'block_id': partition_table.block_device_id, |
360 | 'partition_size': size |
361 | }) |
362 | + partition = partition_table.partitions.first() |
363 | self.assertEquals( |
364 | - 1, Partition.objects.count()) |
365 | + 2, Partition.objects.count()) |
366 | self.assertEquals( |
367 | human_readable_bytes( |
368 | round_size_to_nearest_block( |
369 | size, PARTITION_ALIGNMENT_SIZE, False)), |
370 | - human_readable_bytes(Partition.objects.first().size)) |
371 | + human_readable_bytes(partition.size)) |
372 | |
373 | def test_create_partition_with_filesystem(self): |
374 | user = factory.make_admin() |
375 | @@ -1494,7 +1498,10 @@ |
376 | interface=True, |
377 | architecture=architecture, |
378 | status=NODE_STATUS.ALLOCATED) |
379 | - partition_table = factory.make_PartitionTable(node=node) |
380 | + block_device = factory.make_BlockDevice(node=node) |
381 | + partition_table = factory.make_PartitionTable( |
382 | + block_device=block_device, node=node) |
383 | + partition = partition_table.partitions.first() |
384 | size = partition_table.block_device.size / 2 |
385 | fstype = factory.pick_choice(FILESYSTEM_FORMAT_TYPE_CHOICES) |
386 | mount_point = factory.make_absolute_path() |
387 | @@ -1505,19 +1512,20 @@ |
388 | 'fstype': fstype, |
389 | 'mount_point': mount_point, |
390 | }) |
391 | + partition = partition_table.partitions.first() |
392 | self.assertEquals( |
393 | - 1, Partition.objects.count()) |
394 | + 2, Partition.objects.count()) |
395 | self.assertEquals( |
396 | human_readable_bytes( |
397 | round_size_to_nearest_block( |
398 | size, PARTITION_ALIGNMENT_SIZE, False)), |
399 | - human_readable_bytes(Partition.objects.first().size)) |
400 | + human_readable_bytes(partition.size)) |
401 | self.assertEquals( |
402 | fstype, |
403 | - Partition.objects.first().get_effective_filesystem().fstype) |
404 | + partition.get_effective_filesystem().fstype) |
405 | self.assertEquals( |
406 | mount_point, |
407 | - Partition.objects.first().get_effective_filesystem().mount_point) |
408 | + partition.get_effective_filesystem().mount_point) |
409 | |
410 | def test_create_cache_set_for_partition(self): |
411 | user = factory.make_admin() |
Haven't reviewed the code but please add one more check.
Make sure that a /boot partition exists not on a bcache device if the / partition is on a bcache devices.