[Maas-devel] State machine and design reviews, was [jason.hobbs at canonical.com: [Merge] lp:~jason-hobbs/maas/expose-enable-disk-erasing-option into lp:maas]
Andres Rodriguez
andres.rodriguez at canonical.com
Wed Oct 1 00:10:52 UTC 2014
Hi Kiko,
We discussed this with Raphael and Jason already, and this are the meeting
notes:
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.
On Mon, Sep 29, 2014 at 3:11 PM, Christian Robottom Reis <kiko at canonical.com
> wrote:
> 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 <jason.hobbs at canonical.com> -----
>
> Date: Thu, 25 Sep 2014 22:09:23 -0000
> From: Jason Hobbs <jason.hobbs at canonical.com>
> To: mp+236039 at code.launchpad.net
> 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:
>
> https://code.launchpad.net/~jason-hobbs/maas/expose-enable-disk-erasing-option/+merge/236039
>
> You can enable disk erasing on release now via the API, but not via the
> Web UI.
> --
>
> https://code.launchpad.net/~jason-hobbs/maas/expose-enable-disk-erasing-option/+merge/236039
> 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 |
> http://launchpad.net/~kiko
> Canonical VP Hyperscale | [+55 16] 9 9112 6430 |
> http://async.com.br/~kiko
>
> --
> Mailing list: https://launchpad.net/~maas-devel
> Post to : maas-devel at lists.launchpad.net
> Unsubscribe : https://launchpad.net/~maas-devel
> More help : https://help.launchpad.net/ListHelp
>
--
Andres Rodriguez
Engineering Manager, HWE Team
Canonical USA, Inc.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/maas-devel/attachments/20140930/4a8d6da2/attachment.html>
More information about the Maas-devel
mailing list