<div dir="ltr">Hi Kiko,<div> </div><div>We discussed this with Raphael and Jason already, and this are the meeting notes:</div><div> </div><div>Notes from this morning's call - please add/correct if I missed something. - Jason described the current design and implementation of the disk erasing feature in a little more detail than previously. - Jason clarified that there is no separate action/api for starting disk erasing - it's done as part of the releasing action, so the user's workflow isn't complicated by the change. - Raphael agreed that erasing is basically a substate of releasing, and that this was ok and that the design and implementation are fine - it's not necessarily a negative thing to introduce more states, but everyone agreed it should be discussed more ahead of time next time. - Jason brought up the possibility of changing 'Failed Disk Erasing' to 'Failed Releasing', but Raphael suggested that the failed erasing state is good because it allows people to pin point exactly where the failure was. - Discussed a per node or per release option to override the global default option for disk erasing - Raphael would prefer a per-release option so we don't add additional boolean properties to nodes. Need to decide if we want to allow users to override the admin set global default or not. </div></div><div class="gmail_extra"> <div class="gmail_quote">On Mon, Sep 29, 2014 at 3:11 PM, Christian Robottom Reis <<a href="mailto:kiko@canonical.com" target="_blank">kiko@canonical.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I made a comment on this MP which I wanted to note more widely: In the future, changes to the state machine require explicit discussion and review before we commit to them. The exposure of complex state machines often kills good software -- we're not going to make that mistake in MAAS. I'd like us to have a process where a lightweight discussion and sign-off happens with the core team on infrastructural changes. We used to have pre-implementation calls for features in Launchpad that we could reuse, or perhaps you want to suggest your favorite approach, I'm happy either way, and I don't want to do any hard enforcement of this at this time. But let's ensure that as new features are mooted and designs started, that we have clarity on the design and fix it before the code is sealed. Think about this as you work on new features -- communication is key to the success of any project assembled by a team, and software even more so (and Fred Brooks be the man). Thanks, ----- Forwarded message from Jason Hobbs <<a href="mailto:jason.hobbs@canonical.com">jason.hobbs@canonical.com</a>> ----- Date: Thu, 25 Sep 2014 22:09:23 -0000 From: Jason Hobbs <<a href="mailto:jason.hobbs@canonical.com">jason.hobbs@canonical.com</a>> To: <a href="mailto:mp%2B236039@code.launchpad.net">mp+236039@code.launchpad.net</a> Subject: [Merge] lp:~jason-hobbs/maas/expose-enable-disk-erasing-option into lp:maas X-Launchpad-Message-Rationale: Reviewer @maas-maintainers X-Launchpad-Notification-Type: code-review X-Launchpad-Branch: ~jason-hobbs/maas/expose-enable-disk-erasing-option X-Launchpad-Project: maas X-Launchpad-Hash: 3f95510c33553bf1cac8d8113198718fe47c7cfe X-Spambayes-Classification: ham; 0.00 Jason Hobbs has proposed merging lp:~jason-hobbs/maas/expose-enable-disk-erasing-option into lp:maas. Commit message: Add a new config option - 'enable_disk_erasing_on_release'. Requested reviews: MAAS Maintainers (maas-maintainers) For more details, see: <a href="https://code.launchpad.net/~jason-hobbs/maas/expose-enable-disk-erasing-option/+merge/236039" target="_blank">https://code.launchpad.net/~jason-hobbs/maas/expose-enable-disk-erasing-option/+merge/236039</a> You can enable disk erasing on release now via the API, but not via the Web UI. -- <a href="https://code.launchpad.net/~jason-hobbs/maas/expose-enable-disk-erasing-option/+merge/236039" target="_blank">https://code.launchpad.net/~jason-hobbs/maas/expose-enable-disk-erasing-option/+merge/236039</a> Your team MAAS Maintainers is requested to review the proposed merge of lp:~jason-hobbs/maas/expose-enable-disk-erasing-option into lp:maas. === modified file 'src/maasserver/api/nodes.py' --- src/maasserver/api/nodes.py 2014-09-22 05:33:12 +0000 +++ src/maasserver/api/nodes.py 2014-09-25 21:59:28 +0000 @@ -324,7 +324,7 @@ # postcondition is achieved, so call this success. pass elif node.status in RELEASABLE_STATUSES: - node.release() + node.release_or_erase() else: raise NodeStateViolation( "Node cannot be released in its current state ('%s')." === modified file 'src/maasserver/forms.py' --- src/maasserver/forms.py 2014-09-23 14:16:33 +0000 +++ src/maasserver/forms.py 2014-09-25 21:59:28 +0000 @@ -26,6 +26,7 @@ "get_node_edit_form", "get_node_create_form", "list_all_usable_architectures", + "DiskErasingOnReleaseForm", "MAASAndNetworkForm", "MACAddressForm", "NetworkConnectMACsForm", @@ -1085,6 +1086,12 @@ enable_third_party_drivers = get_config_field('enable_third_party_drivers') +class DiskErasingOnReleaseForm(ConfigForm): + """Settings page, Disk Erasing on Release section.""" + enable_disk_erasing_on_release = get_config_field( + 'enable_disk_erasing_on_release') + + class CommissioningForm(ConfigForm): """Settings page, Commissioning section.""" check_compatibility = get_config_field('check_compatibility') === modified file 'src/maasserver/forms_settings.py' --- src/maasserver/forms_settings.py 2014-09-22 14:47:49 +0000 +++ src/maasserver/forms_settings.py 2014-09-25 21:59:28 +0000 @@ -257,6 +257,15 @@ "using KMS activation.)") } }, + 'enable_disk_erasing_on_release': { + 'default': False, + 'form': forms.BooleanField, + 'form_kwargs': { + 'required': False, + 'label': ( + "Erase nodes' disks prior to releasing.") + } + }, } === modified file 'src/maasserver/models/config.py' --- src/maasserver/models/config.py 2014-07-03 07:01:15 +0000 +++ src/maasserver/models/config.py 2014-09-25 21:59:28 +0000 @@ -58,6 +58,7 @@ 'rpc_region_certificate': None, # Third Party 'enable_third_party_drivers': True, + 'enable_disk_erasing_on_release': False, ## /settings } === modified file 'src/maasserver/models/node.py' --- src/maasserver/models/node.py 2014-09-25 18:08:05 +0000 +++ src/maasserver/models/node.py 2014-09-25 21:59:28 +0000 @@ -1240,6 +1240,17 @@ self.license_key = '' self.save() + def release_or_erase(self): + """Either release the node or erase the node then release it, depending + on settings.""" + erase_on_release = Config.objects.get_config( + 'enable_disk_erasing_on_release') + if erase_on_release: + self.start_disk_erasing(self.owner) + return + + self.release() + def set_netboot(self, on=True): """Set netboot on or off.""" maaslog.debug("%s: Turning on netboot for node", self.hostname) === modified file 'src/maasserver/models/tests/test_node.py' --- src/maasserver/models/tests/test_node.py 2014-09-25 17:25:08 +0000 +++ src/maasserver/models/tests/test_node.py 2014-09-25 21:59:28 +0000 @@ -80,6 +80,7 @@ from maastesting.matchers import ( MockAnyCall, MockCalledOnceWith, + MockNotCalled, ) from maastesting.testcase import MAASTestCase from metadataserver.enum import RESULT_TYPE @@ -1695,6 +1696,28 @@ node.set_netboot(False) self.assertFalse(node.netboot) + def test_release_or_erase_erases_when_enabled(self): + owner = factory.make_User() + node = factory.make_Node(status=NODE_STATUS.ALLOCATED, owner=owner) + Config.objects.set_config( + 'enable_disk_erasing_on_release', True) + erase_mock = self.patch_autospec(node, 'start_disk_erasing') + release_mock = self.patch_autospec(node, 'release') + node.release_or_erase() + self.assertThat(erase_mock, MockCalledOnceWith(owner)) + self.assertThat(release_mock, MockNotCalled()) + + def test_release_or_erase_releases_when_disabled(self): + owner = factory.make_User() + node = factory.make_Node(status=NODE_STATUS.ALLOCATED, owner=owner) + Config.objects.set_config( + 'enable_disk_erasing_on_release', False) + erase_mock = self.patch_autospec(node, 'start_disk_erasing') + release_mock = self.patch_autospec(node, 'release') + node.release_or_erase() + self.assertThat(release_mock, MockCalledOnceWith()) + self.assertThat(erase_mock, MockNotCalled()) + class NodeManagerTest_StartNodes(MAASServerTestCase): === modified file 'src/maasserver/node_action.py' --- src/maasserver/node_action.py 2014-09-25 17:25:08 +0000 +++ src/maasserver/node_action.py 2014-09-25 21:59:28 +0000 @@ -330,7 +330,7 @@ def execute(self, allow_redirect=True): """See `NodeAction.execute`.""" - self.node.release() + self.node.release_or_erase() return dedent("""\ This node is no longer allocated to you. """) ----- End forwarded message ----- -- Christian Robottom Reis | [+1] 612 888 4935 | <a href="http://launchpad.net/~kiko" target="_blank">http://launchpad.net/~kiko</a> Canonical VP Hyperscale | [+55 16] 9 9112 6430 | <a href="http://async.com.br/~kiko" target="_blank">http://async.com.br/~kiko</a> -- Mailing list: <a href="https://launchpad.net/~maas-devel" target="_blank">https://launchpad.net/~maas-devel</a> Post to : <a href="mailto:maas-devel@lists.launchpad.net">maas-devel@lists.launchpad.net</a> Unsubscribe : <a href="https://launchpad.net/~maas-devel" target="_blank">https://launchpad.net/~maas-devel</a> More help : <a href="https://help.launchpad.net/ListHelp" target="_blank">https://help.launchpad.net/ListHelp</a> </blockquote></div> <div> </div>-- <div dir="ltr">Andres Rodriguez<div>Engineering Manager, HWE Team</div><div>Canonical USA, Inc.</div></div> </div>