Merge lp:~fwereade/pyjuju/provider-base-launch-machine into lp:~fwereade/pyjuju/provider-base

[William Reade]

Does this cover what you need from https://code.launchpad.net/~fwereade/ensemble/provider-base/+merge/71272 point [1]?

I'm not totally sold on the bootstrap argument to start_machine, but it does simplify the code, and I think it's a good candidate for inclusion in the forthcoming-as-soon-as-I-get-around-to-it fix for lp:820892. That is to say: the act of starting a machine isn't really connected to the concept of bootstrapping, it's just that when we *are* bootstrapping we want the incidentally-started machine to play a couple of extra roles, which would go nicely in whatever we end up replacing machine_data with.

[Gustavo Niemeyer (niemeyer)]

Please just rename the bootstrap=True parameter on start_machine to
master=True, and +1. Thanks for separating it out.

review: Approve

[lp:~fwereade/pyjuju/provider-base-launch-machine]

326. By William Reade on 2011-08-17

renamed start_machine bootstrap kwarg (and LaunchMachine.__init__ bootstrap kwarg, and LaunchMachine._bootstrap) to master

=== modified file 'ensemble/providers/common/base.py'
--- ensemble/providers/common/base.py	2011-08-11 22:04:38 +0000
+++ ensemble/providers/common/base.py	2011-08-17 12:06:25 +0000
@@ -16,6 +16,7 @@
 
         * connect
         * get_file_storage
+        * start_machine
         * get_machines
         * shutdown_machines
         * open_port
@@ -32,7 +33,7 @@
     You probably shouldn't override anything else.
     """
 
-    def __init__(self, environment_name, config, launch_machine_class):
+    def __init__(self, environment_name, config):
         if ("authorized-keys-path" in config and
             "authorized-keys" in config):
             raise EnvironmentsConfigError(
@@ -41,7 +42,6 @@
 
         self.environment_name = environment_name
         self.config = config
-        self.launch_machine_class = launch_machine_class
 
     def get_serialization_data(self):
         """Get provider configuration suitable for serialization."""
@@ -70,6 +70,17 @@
         """Retrieve the provider C{FileStorage} abstraction."""
         raise NotImplementedError()
 
+    def start_machine(self, machine_data, master=False):
+        """Start a machine in the provider.
+
+        @param machine_data: a dictionary of data to pass along to the newly
+           launched machine.
+
+        @param master: if True, machine will initialize the ensemble admin
+            and run a provisioning agent.
+        """
+        raise NotImplementedError()
+
     def get_machines(self, instance_ids=()):
         """List machines running in the provider.
 
@@ -119,16 +130,6 @@
         """Bootstrap an ensemble server in the provider."""
         return Bootstrap(self).run()
 
-    def start_machine(self, machine_data):
-        """Start a machine in the provider.
-
-        @param machine_data: a dictionary of data to pass along to the newly
-               launched machine.
-        """
-        # XXX something's a bit inelegant here; also see Bootstrap operation
-        launch = self.launch_machine_class(self)
-        return launch.run(machine_data=machine_data)
-
     def get_machine(self, instance_id):
         """Retrieve a provider machine by instance id.
 

=== modified file 'ensemble/providers/common/bootstrap.py'
--- ensemble/providers/common/bootstrap.py	2011-08-04 09:50:57 +0000
+++ ensemble/providers/common/bootstrap.py	2011-08-17 12:06:25 +0000
@@ -41,11 +41,10 @@
         return storage.put(_VERIFY_PATH, StringIO("storage is writable"))
 
     def _cannot_write(self, failure):
-        raise ProviderError("Bootstrap aborted: file storage not writable (%s)"
-                            % str(failure.value))
+        raise ProviderError(
+            "Bootstrap aborted because file storage is not writable: %s"
+            % str(failure.value))
 
     def _launch_machine(self, unused):
         log.debug("Launching Ensemble bootstrap instance.")
-        launch_class = self._provider.launch_machine_class
-        launch = launch_class(self._provider, bootstrap=True)
-        return launch.run({"machine-id": "0"})
+        return self._provider.start_machine({"machine-id": "0"}, master=True)

=== modified file 'ensemble/providers/common/launch.py'
--- ensemble/providers/common/launch.py	2011-08-10 18:12:47 +0000
+++ ensemble/providers/common/launch.py	2011-08-17 12:06:25 +0000
@@ -55,7 +55,7 @@
 class LaunchMachine(object):
     """Abstract class with generic instance-launching logic.
 
-    Constructing with bootstrap=True will cause the run method to
+    Constructing with master=True will cause the run method to
     construct a machine which is also running a zookeeper for the
     cluster, and a provisioning agent, as well as the usual machine
     agent.
@@ -65,9 +65,9 @@
     convenient to override C{get_machine_variables} as well.
     """
 
-    def __init__(self, provider, bootstrap=False):
+    def __init__(self, provider, master=False):
         self._provider = provider
-        self._bootstrap = bootstrap
+        self._master = master
 
     @inlineCallbacks
     def run(self, machine_data):
@@ -85,8 +85,8 @@
         machine_variables = yield self.get_machine_variables(machine_data)
         provider_machines = yield self.start_machine(
             machine_variables, machine_data["machine-id"])
-        if self._bootstrap:
-            yield self._on_bootstrap_launched(provider_machines)
+        if self._master:
+            yield self._on_master_launched(provider_machines)
         returnValue(provider_machines)
 
     def start_machine(self, machine_variables, machine_id):
@@ -127,7 +127,7 @@
 
     @inlineCallbacks
     def _get_zookeeper_hosts(self):
-        if self._bootstrap:
+        if self._master:
             returnValue("localhost:2181")
         machines = yield self._provider.get_zookeeper_machines()
         hosts = [m.private_dns_name for m in machines]
@@ -136,7 +136,7 @@
 
     def _get_packages(self):
         result = list(DEFAULT_PACKAGES)
-        if self._bootstrap:
+        if self._master:
             result.extend(BOOTSTRAP_PACKAGES)
         return result
 
@@ -149,7 +149,7 @@
             "sudo mkdir -p /var/lib/ensemble",
             "sudo mkdir -p /var/log/ensemble"])
 
-        if self._bootstrap:
+        if self._master:
             launch_scripts.append(self._get_initialize_script())
 
         # Every machine has its own agent.
@@ -161,7 +161,7 @@
             "--pidfile=/var/run/ensemble/machine-agent.pid")
         launch_scripts.append(machine_agent_script_template % machine_data)
 
-        if self._bootstrap:
+        if self._master:
             launch_scripts.append(_get_provision_agent_script(machine_data))
 
         return launch_scripts
@@ -184,7 +184,10 @@
         return _get_initialize_script(
             self.get_instance_id_command(), admin_secret)
 
-    def _on_bootstrap_launched(self, machines):
+    def _on_master_launched(self, machines):
+        # TODO this should be part of Bootstrap (and should really extend,
+        # rather than effectively replace, the result of
+        # self._provider.get_zookeeper_machines)
         instance_ids = [m.instance_id for m in machines]
         d = self._provider.save_state({"zookeeper-instances": instance_ids})
         d.addCallback(lambda _: machines)

=== modified file 'ensemble/providers/common/tests/test_base.py'
--- ensemble/providers/common/tests/test_base.py	2011-08-11 22:04:38 +0000
+++ ensemble/providers/common/tests/test_base.py	2011-08-17 12:06:25 +0000
@@ -12,8 +12,8 @@
 
 class DummyLaunchMachine(object):
 
-    def __init__(self, bootstrap=False):
-        self._bootstrap = bootstrap
+    def __init__(self, master=False):
+        self._master = master
 
     def run(self, machine_data):
         return succeed([ProviderMachine(machine_data["machine-id"])])
@@ -21,11 +21,8 @@
 
 class DummyProvider(MachineProviderBase):
 
-    def __init__(self, config=None, launch_machine_class=None):
-        if launch_machine_class is None:
-            launch_machine_class = DummyLaunchMachine
-        super(DummyProvider, self).__init__(
-            "venus", config or {}, launch_machine_class)
+    def __init__(self, config=None):
+        super(DummyProvider, self).__init__("venus", config or {})
 
 
 class MachineProviderBaseTest(TestCase):
@@ -34,7 +31,6 @@
         provider = DummyProvider({"some": "config"})
         self.assertEquals(provider.environment_name, "venus")
         self.assertEquals(provider.config, {"some": "config"})
-        self.assertEquals(provider.launch_machine_class, DummyLaunchMachine)
 
     def test_bad_config(self):
         try:
@@ -137,37 +133,3 @@
             self.assertEquals(result, machines)
         d.addCallback(verify)
         return d
-
-    def test_start_machine_error(self):
-        LaunchMachine = self.mocker.mock()
-        provider = DummyProvider(launch_machine_class=LaunchMachine)
-        LaunchMachine(provider)
-        launchmachine = self.mocker.mock()
-        self.mocker.result(launchmachine)
-        machine_data = object()
-        launchmachine.run(machine_data=machine_data)
-        self.mocker.result(fail(SomeError()))
-        self.mocker.replay()
-
-        d = provider.start_machine(machine_data)
-        self.assertFailure(d, SomeError)
-        return d
-
-    def test_start_machine_success(self):
-        LaunchMachine = self.mocker.mock()
-        provider = DummyProvider(launch_machine_class=LaunchMachine)
-        LaunchMachine(provider)
-        launchmachine = self.mocker.mock()
-        self.mocker.result(launchmachine)
-        machine_data = object()
-        launchmachine.run(machine_data=machine_data)
-        machine = object()
-        self.mocker.result(succeed([machine]))
-        self.mocker.replay()
-
-        d = provider.start_machine(machine_data)
-
-        def verify(result):
-            self.assertEquals(result, [machine])
-        d.addCallback(verify)
-        return d

=== modified file 'ensemble/providers/common/tests/test_launch.py'
--- ensemble/providers/common/tests/test_launch.py	2011-08-11 22:04:38 +0000
+++ ensemble/providers/common/tests/test_launch.py	2011-08-17 12:06:25 +0000
@@ -18,7 +18,7 @@
 class DummyLaunchMachine(LaunchMachine):
 
     def start_machine(self, variables, data):
-        if self._bootstrap:
+        if self._master:
             name = "bootstrapped-instance-id"
         else:
             name = "some-instance-id"
@@ -46,8 +46,7 @@
     class DummyProvider(MachineProviderBase):
 
         def __init__(self, config):
-            super(DummyProvider, self).__init__(
-                "venus", config, launch_class)
+            super(DummyProvider, self).__init__("venus", config)
             self._file_storage = file_storage_class()
 
         def get_zookeeper_machines(self):
@@ -61,6 +60,9 @@
         def get_file_storage(self):
             return self._file_storage
 
+        def start_machine(self, machine_data, master=False):
+            return launch_class(self, master).run(machine_data)
+
     create_config = {"authorized-keys": "abc"}
     if config is not None:
         create_config.update(config)
@@ -68,9 +70,9 @@
 
 
 def get_launch(launch_class=DummyLaunchMachine, config=None,
-               zookeeper=True, bootstrap=False):
+               zookeeper=True, master=False):
     provider = get_provider(launch_class, config, zookeeper)
-    return launch_class(provider, bootstrap=bootstrap)
+    return launch_class(provider, master=master)
 
 
 class LaunchMachineTest(TestCase):
@@ -95,7 +97,7 @@
     @inlineCallbacks
     def test_bootstrap_run(self):
         provider = get_provider(config={"admin-secret": "whatever"})
-        launch = DummyLaunchMachine(provider, bootstrap=True)
+        launch = DummyLaunchMachine(provider, master=True)
         yield launch.run({"machine-id": "machine-32767"})
         saved_state = yield provider.load_state()
         self.assertEquals(
@@ -151,8 +153,7 @@
         return d
 
     def test_get_machine_variables_bootstrap(self):
-        launch = get_launch(config={"admin-secret": "SEEKRIT"},
-                            bootstrap=True)
+        launch = get_launch(config={"admin-secret": "SEEKRIT"}, master=True)
         d = launch.get_machine_variables({"machine-id": "machine-757"})
 
         def verify_vars(vars):

=== modified file 'ensemble/providers/dummy.py'
--- ensemble/providers/dummy.py	2011-08-12 21:00:41 +0000
+++ ensemble/providers/dummy.py	2011-08-17 12:06:25 +0000
@@ -58,7 +58,7 @@
             return fail(MachinesNotFound(missing_instance_ids))
         return succeed(machines)
 
-    def start_machine(self, machine_data):
+    def start_machine(self, machine_data, master=False):
         """Start a machine in the provider."""
         if not "machine-id" in machine_data:
             return fail(ProviderError(

=== modified file 'ensemble/providers/ec2/__init__.py'
--- ensemble/providers/ec2/__init__.py	2011-08-17 09:42:00 +0000
+++ ensemble/providers/ec2/__init__.py	2011-08-17 12:06:25 +0000
@@ -23,8 +23,7 @@
 class MachineProvider(MachineProviderBase):
 
     def __init__(self, environment_name, config):
-        super(MachineProvider, self).__init__(
-            environment_name, config, EC2LaunchMachine)
+        super(MachineProvider, self).__init__(environment_name, config)
 
         if not config.get("ec2-uri"):
             ec2_uri = get_region_uri(config.get("region", "us-east-1"))
@@ -70,6 +69,17 @@
         """Retrieve an S3-backed C{FileStorage}."""
         return FileStorage(self.s3, self.config["control-bucket"])
 
+    def start_machine(self, machine_data, master=False):
+        """Start a machine in the provider.
+
+        @param machine_data: a dictionary of data to pass along to the newly
+           launched machine.
+
+        @param master: if True, machine will initialize the ensemble admin
+            and run a provisioning agent.
+        """
+        return EC2LaunchMachine(self, master).run(machine_data)
+
     @inlineCallbacks
     def get_machines(self, instance_ids=()):
         """List machines running in the provider.

=== modified file 'ensemble/providers/orchestra/__init__.py'
--- ensemble/providers/orchestra/__init__.py	2011-08-12 21:00:41 +0000
+++ ensemble/providers/orchestra/__init__.py	2011-08-17 12:06:25 +0000
@@ -10,11 +10,8 @@
 
 class MachineProvider(MachineProviderBase):
 
-    launch_machine_class = OrchestraLaunchMachine
-
     def __init__(self, environment_name, config):
-        super(MachineProvider, self).__init__(
-            environment_name, config, OrchestraLaunchMachine)
+        super(MachineProvider, self).__init__(environment_name, config)
         self.cobbler = CobblerClient(config)
 
     def get_file_storage(self):
@@ -24,6 +21,17 @@
                 "http://%(orchestra-server)s/webdav" % self.config)
         return FileStorage(self.config["storage-url"])
 
+    def start_machine(self, machine_data, master=False):
+        """Start a machine in the provider.
+
+        @param machine_data: a dictionary of data to pass along to the newly
+           launched machine.
+
+        @param master: if True, machine will initialize the ensemble admin
+            and run a provisioning agent.
+        """
+        return OrchestraLaunchMachine(self, master).run(machine_data)
+
     @inlineCallbacks
     def get_machines(self, instance_ids=()):
         """List machines running in the provider.