Merge lp:~hazmat/pyjuju/cli-placement-restore into lp:pyjuju

[Kapil Thangavelu]

Restore command line placement.

There are several folks who would like to see its restoration. Both for
flexibility down the road and more immediately as mechanism to conserve
machine resources. The alternative of modifying environment config between
deploys is suboptimal. Its being used for example in reducing the number of
machines required for openstack. Although future use of ideal policies (limited
number of machines) is suboptimal, there are other placement policies taking
into account machine resources or cross availability zone for which this option
would be ideal.

[Gustavo Niemeyer (niemeyer)]

Why is the placement option in the environment suboptimal?

I'm not trying to make things difficult gratuitously, but I feel quite unhappy about the mechanism we're introducing here. This was introduced as a hack, and I'm now hearing people depending on the hack.. soon we'll see blog posts around the hack and people believing that this is the way Ensemble is supposed to work.

I understand folks want to have better control of placement. I surely want it too.. this is just not the way to do it in Ensemble, for several reasons that I'll be happy to discuss in a better forum.

If the placement option in the environment enable the Adam and Clint to do what they have to do, I'd prefer to keep it that way.

review: Needs Information

[Adam Gandelman (gandelman-a)]

Hi--

Moving "--placement=local" from argv to "placement: local" in environments.yaml seems trivial enough, however, my specific use case often has me dealing with many different formulas, some of which are destroyed and re-deployed many times in one sitting. Having to to parse and modify environments.yaml many times over between 'juju deploy' to switch to and from 'local' placement policy seems needlessly tedious. Having to put together my own wrappers around 'juju deploy' and environments.yaml to do this for me and emulate what '--placement=local' once provided seems even more suboptimal.

We understand its a hack but we're using it as a workaround to Bug #806241. Without it, Juju goes from something that might be useful "sometime in the future" given our hardware constraints to something that is extremely useful *now* and *central* to our QA and integration testing of specific software stacks. IMHO, making it more difficult to use due to user facing changes like this (especially this late in the cycle) would be unfortunate.

[Clint Byrum (clint-fewbar)]

I share your concern that --placement local will be abused if it is available. However, without this, testing scripts for hardware deployment or against limited size clouds become incredibly complex. I'd be comfortable with a WARNING: --placement on the command line is deprecated type of message once there is an alternative that allows making use of limited resources, but until then, I think we have to have at least *some* story other than "go buy more hardware".

[Gustavo Niemeyer (niemeyer)]

Alright, so here is a review to try to get to some agreement:

[1]

Let's have the renamed the option to a boolean called

    --temporary-local-hack

With the following help message:

    help="Temporary placement hack for testing juju."

[2]

Let's not change the provider interface, or introduce pick_policy,
etc. The provider interface we have today looks nice and is going
into the direction we want.

Instead, we do something as simple as this:

    placement = provider.get_placement_policy()
    if options.temporary_local_hack:
        placement = "local"

[3]

Once we sort out making our system stable for 12.04, let's come back
and do placement correctly. It's definitely something that should be
high in our priority list.

review: Needs Fixing

[Kapil Thangavelu (hazmat)]

Excerpts from Gustavo Niemeyer's message of Thu Sep 29 20:48:30 UTC 2011:
> Review: Needs Fixing
>
> Alright, so here is a review to try to get to some agreement:
>
> [1]
>
> Let's have the renamed the option to a boolean called
>
> --temporary-local-hack
>
> With the following help message:
>
> help="Temporary placement hack for testing juju."

We can even hide it from the default cli help.

>
> [2]
>
> Let's not change the provider interface, or introduce pick_policy,
> etc. The provider interface we have today looks nice and is going
> into the direction we want.
>
> Instead, we do something as simple as this:
>
> placement = provider.get_placement_policy()
> if options.temporary_local_hack:
> placement = "local"
>
>
> [3]
>
> Once we sort out making our system stable for 12.04, let's come back
> and do placement correctly. It's definitely something that should be
> high in our priority list.

sounds reasonable.

[Kapil Thangavelu (hazmat)]

i thought about this some more, and i think i'd rather just drop this and adopt clint's approach of giving users direct machine placement options.

[Benjamin Saller (bcsaller)]

I think thats just another policy, --placement machine:1

Those were always supposed to be strategy name or plans (implied by
the above) in my mind.

On Wed, Oct 5, 2011 at 7:46 AM, Kapil Thangavelu
<email address hidden> wrote:
> i thought about this some more, and i think i'd rather just drop this and adopt clint's approach of giving users direct machine placement options.
> --
> https://code.launchpad.net/~hazmat/juju/cli-placement-restore/+merge/77245
> You are subscribed to branch lp:juju.
>

[Kapil Thangavelu (hazmat)]

interesting, thanks.

Excerpts from Benjamin Saller's message of Wed Oct 05 15:38:39 UTC 2011:
> I think thats just another policy, --placement machine:1
>
> Those were always supposed to be strategy name or plans (implied by
> the above) in my mind.
>
>
> On Wed, Oct 5, 2011 at 7:46 AM, Kapil Thangavelu
> <email address hidden> wrote:
> > i thought about this some more, and i think i'd rather just drop this and adopt clint's approach of giving users direct machine placement options.
> > --
> > https://code.launchpad.net/~hazmat/juju/cli-placement-restore/+merge/77245
> > You are subscribed to branch lp:juju.
> >
>

Unmerged revisions

397. By Kapil Thangavelu on 2011-09-27

restore placement cli

=== modified file 'juju/control/add_unit.py'
--- juju/control/add_unit.py	2011-09-27 02:14:56 +0000
+++ juju/control/add_unit.py	2011-09-27 22:31:25 +0000
@@ -3,7 +3,7 @@
 from twisted.internet.defer import inlineCallbacks
 
 from juju.control.utils import get_environment
-from juju.state.placement import place_unit
+from juju.state.placement import place_unit, pick_policy
 from juju.state.service import ServiceStateManager
 
 
@@ -16,6 +16,9 @@
     sub_parser.add_argument(
         "service_name",
         help="Name of the service a unit should be added for")
+    sub_parser.add_argument(
+        "--placement",
+        help="Placement policy used to place units of this service")
 
     return sub_parser
 
@@ -23,20 +26,26 @@
 def command(options):
     """Add a new service unit."""
     environment = get_environment(options)
+
+    if not options.placement:
+        options.placement = environment.placement
+
     return add_unit(
         options.environments,
         environment,
         options.verbose,
         options.log,
-        options.service_name)
+        options.service_name,
+        options.placement)
 
 
 @inlineCallbacks
-def add_unit(config, environment, verbose, log, service_name):
+def add_unit(
+    config, environment, verbose, log, service_name, placement_policy):
     """Add a unit of a service to the environment.
     """
     provider = environment.get_machine_provider()
-    placement_policy = provider.get_placement_policy()
+    placement_policy = pick_policy(placement_policy, provider)
     client = yield provider.connect()
 
     try:

=== modified file 'juju/control/deploy.py'
--- juju/control/deploy.py	2011-09-27 02:14:56 +0000
+++ juju/control/deploy.py	2011-09-27 22:31:25 +0000
@@ -12,7 +12,7 @@
 
 from juju.state.endpoint import RelationEndpoint
 from juju.state.environment import EnvironmentStateManager
-from juju.state.placement import place_unit
+from juju.state.placement import place_unit, pick_policy
 from juju.state.relation import RelationStateManager
 from juju.state.service import ServiceStateManager
 
@@ -41,6 +41,10 @@
         "--config",
         help="YAML file containing service options")
 
+    sub_parser.add_argument(
+        "--placement",
+        help="Placement policy used to place units of this service")
+
     return sub_parser
 
 
@@ -56,7 +60,8 @@
         options.charm,
         options.service_name,
         options.log,
-        options.config)
+        options.config,
+        options.placement)
 
 
 def parse_config_options(config_file, service_name):
@@ -77,7 +82,7 @@
 
 @inlineCallbacks
 def deploy(env_config, environment, repository_path, charm_name,
-           service_name, log, config_file=None):
+           service_name, log, config_file=None, placement_policy=None):
     """Deploy a charm within an environment.
 
 
@@ -96,7 +101,7 @@
     charm = repository.find(charm_name)
 
     provider = environment.get_machine_provider()
-    placement_policy = provider.get_placement_policy()
+    placement_policy = pick_policy(placement_policy, provider)
     client = yield provider.connect()
 
     try:
@@ -133,6 +138,7 @@
 
         # Create a service unit
         unit_state = yield service_state.add_unit_state()
+
         yield place_unit(client, placement_policy, unit_state)
 
         # Check if we have any peer relations to establish

=== modified file 'juju/control/tests/test_add_unit.py'
--- juju/control/tests/test_add_unit.py	2011-09-27 02:14:56 +0000
+++ juju/control/tests/test_add_unit.py	2011-09-27 22:31:25 +0000
@@ -87,6 +87,42 @@
         yield self.assert_machine_assignments("mysql", [1, 2])
 
     @inlineCallbacks
+    def test_add_unit_with_local_policy(self):
+        """
+        'juju add-unit <service_name>' will add a new service
+        unit of the given service.
+        """
+        # rework setUp so that service_unit1 is on machine 0
+        # to simulate local deployment
+        ms0 = yield self.machine_state_manager.get_machine_state(0)
+        yield self.service_unit1.unassign_from_machine()
+        yield self.service_unit1.assign_to_machine(ms0)
+
+        unit_names = yield self.service_state1.get_unit_names()
+        self.assertEqual(len(unit_names), 1)
+
+        finished = self.setup_cli_reactor()
+        self.setup_exit(0)
+        self.mocker.replay()
+        main(["add-unit", "mysql", "--placement", "local"])
+        yield finished
+
+        # verify the unit and its machine assignment.
+        unit_names = yield self.service_state1.get_unit_names()
+        self.assertEqual(len(unit_names), 2)
+
+        topology = yield self.get_topology()
+        unit = yield self.service_state1.get_unit_state("mysql/1")
+        machine_id = topology.get_service_unit_machine(
+            self.service_state1.internal_id, unit.internal_id)
+        self.assertNotEqual(machine_id, None)
+        self.assertIn(
+            "Unit 'mysql/1' added to service 'mysql'",
+            self.output.getvalue())
+        # adding a second unit still assigns to machine 0 with local policy
+        yield self.assert_machine_assignments("mysql", [0, 0])
+
+    @inlineCallbacks
     def test_policy_from_environment(self):
         config = {
             "environments": {"firstenv": {

=== modified file 'juju/control/tests/test_deploy.py'
--- juju/control/tests/test_deploy.py	2011-09-27 02:14:56 +0000
+++ juju/control/tests/test_deploy.py	2011-09-27 22:31:25 +0000
@@ -150,7 +150,7 @@
         command(MATCH(match_config), MATCH(match_environment),
                 self.unbundled_repo_path, "sample", None,
                 MATCH(lambda x: isinstance(x, logging.Logger)),
-                None)
+                None, None)
         self.mocker.replay()
         self.mocker.result(succeed(True))
 
@@ -301,6 +301,38 @@
         self.assertEqual(relations[0].relation_name, "ring")
 
     @inlineCallbacks
+    def test_deploy_with_local_policy(self):
+        environment = self.config.get("firstenv")
+        yield deploy.deploy(
+            self.config, environment, self.unbundled_repo_path,
+            "sample", "myblog", logging.getLogger("deploy"),
+            placement_policy="local")
+
+        topology = yield self.get_topology()
+        service_id = topology.find_service_with_name("myblog")
+        self.assertEqual(service_id, "service-%010d" % 0)
+        exists = yield self.client.exists("/services/%s" % service_id)
+        self.assertTrue(exists)
+
+        machine_ids = topology.get_machines()
+
+        # There should just be the one machine with local deployment
+        self.assertEqual(machine_ids, ["machine-%010d" % 0])
+        exists = yield self.client.exists("/machines/%s" % machine_ids[0])
+        self.assertTrue(exists)
+
+        # Verify the process still completes as expected
+        unit_ids = topology.get_service_units(service_id)
+        self.assertEqual(unit_ids, ["unit-%010d" % 0])
+        exists = yield self.client.exists("/units/%s" % unit_ids[0])
+        self.assertTrue(exists)
+
+        service = yield self.service_state_manager.get_service_state("myblog")
+        unit_state = yield service.get_unit_state("myblog/0")
+        machine_id = yield unit_state.get_assigned_machine_id()
+        self.assertEqual(machine_id, 0)
+
+    @inlineCallbacks
     def test_deploy_policy_from_environment(self):
         config = {
             "environments": {"firstenv": {

=== modified file 'juju/providers/common/base.py'
--- juju/providers/common/base.py	2011-09-27 02:14:56 +0000
+++ juju/providers/common/base.py	2011-09-27 22:31:25 +0000
@@ -4,7 +4,7 @@
 from twisted.internet.defer import inlineCallbacks, returnValue
 
 from juju.environment.errors import EnvironmentsConfigError
-from juju.state.placement import UNASSIGNED_POLICY
+from juju.state.placement import list_policies
 
 
 from .bootstrap import Bootstrap
@@ -48,12 +48,10 @@
         self.environment_name = environment_name
         self.config = config
 
-    def get_placement_policy(self):
-        """Get the unit placement policy for the provider.
-
-        :param preference: A user specified plcaement policy preference
+    def get_placement_policies(self):
+        """Get the available unit placement policies for the provider.
         """
-        return self.config.get("placement", UNASSIGNED_POLICY)
+        return list_policies()
 
     def get_serialization_data(self):
         """Get provider configuration suitable for serialization."""

=== modified file 'juju/providers/common/tests/test_base.py'
--- juju/providers/common/tests/test_base.py	2011-09-27 02:14:56 +0000
+++ juju/providers/common/tests/test_base.py	2011-09-27 22:31:25 +0000
@@ -4,7 +4,7 @@
 from juju.lib.testing import TestCase
 from juju.machine import ProviderMachine
 from juju.providers.common.base import MachineProviderBase
-from juju.state.placement import UNASSIGNED_POLICY
+from juju.state.placement import list_policies
 
 
 class SomeError(Exception):
@@ -58,10 +58,7 @@
     def test_get_provider_placement(self):
         provider = DummyProvider()
         self.assertEqual(
-            provider.get_placement_policy(), UNASSIGNED_POLICY)
-        provider = DummyProvider({"placement": "local"})
-        self.assertEqual(
-            provider.get_placement_policy(), "local")
+            provider.get_placement_policies(), list_policies())
 
     def test_get_machine_error(self):
         provider = DummyProvider()

=== modified file 'juju/providers/dummy.py'
--- juju/providers/dummy.py	2011-09-27 02:14:56 +0000
+++ juju/providers/dummy.py	2011-09-27 22:31:25 +0000
@@ -11,7 +11,7 @@
 
 
 from juju.machine import ProviderMachine
-from juju.state.placement import UNASSIGNED_POLICY
+from juju.state.placement import list_policies
 from juju.providers.common.files import FileStorage
 
 log = logging.getLogger("juju.providers")
@@ -34,12 +34,10 @@
         self._state = None
         self._storage = None
 
-    def get_placement_policy(self):
-        """Get the unit placement policy for the provider.
-
-        :param preference: A user specified plcaement policy preference
+    def get_placement_policies(self):
+        """Get the available unit placement policies for the provider.
         """
-        return self.config.get("placement", UNASSIGNED_POLICY)
+        return list_policies()
 
     @property
     def provider_type(self):

=== modified file 'juju/providers/lxc/__init__.py'
--- juju/providers/lxc/__init__.py	2011-09-27 02:51:59 +0000
+++ juju/providers/lxc/__init__.py	2011-09-27 22:31:25 +0000
@@ -39,12 +39,9 @@
         self._directory = os.path.join(
             self.config["data-dir"], self._qualified_name)
 
-    def get_placement_policy(self):
+    def get_placement_policies(self):
         """Local dev supports only one unit placement policy."""
-        if self.config.get("placement", LOCAL_POLICY) != LOCAL_POLICY:
-            raise ProviderError(
-                "Unsupported placement policy for local provider")
-        return LOCAL_POLICY
+        return [LOCAL_POLICY]
 
     @property
     def provider_type(self):

=== modified file 'juju/providers/lxc/tests/test_provider.py'
--- juju/providers/lxc/tests/test_provider.py	2011-09-27 02:51:59 +0000
+++ juju/providers/lxc/tests/test_provider.py	2011-09-27 22:31:25 +0000
@@ -41,15 +41,10 @@
         user_name = pwd.getpwuid(os.getuid()).pw_name
         return "%s-%s" % (user_name, self.provider.environment_name)
 
-    def test_get_placement_policy(self):
+    def test_get_placement_policies(self):
         """Lxc provider only supports local placement."""
         self.assertEqual(
-            self.provider.get_placement_policy(), "local")
-        provider = MachineProvider(
-            "test", {"placement": "unassigned",
-                     "data-dir": self.makeDir()})
-        self.assertRaises(
-            ProviderError, provider.get_placement_policy)
+            self.provider.get_placement_policies(), ["local"])
 
     def assertDir(self, *path_parts):
         path = os.path.join(*path_parts)

=== modified file 'juju/providers/tests/test_dummy.py'
--- juju/providers/tests/test_dummy.py	2011-09-27 02:14:56 +0000
+++ juju/providers/tests/test_dummy.py	2011-09-27 22:31:25 +0000
@@ -7,7 +7,7 @@
 from juju.errors import ProviderError
 from juju.machine import ProviderMachine
 from juju.providers.dummy import MachineProvider, DummyMachine
-from juju.state.placement import UNASSIGNED_POLICY
+from juju.state.placement import list_policies
 from juju.lib.testing import TestCase
 
 
@@ -24,12 +24,9 @@
     def test_provider_type(self):
         self.assertEqual(self.provider.provider_type, "dummy")
 
-    def test_get_placement_policy(self):
-        self.assertEqual(
-            self.provider.get_placement_policy(), UNASSIGNED_POLICY)
-        self.provider = MachineProvider("foo", {"placement": "local"})
-        self.assertEqual(
-            self.provider.get_placement_policy(), "local")
+    def test_get_placement_policies(self):
+        self.assertEqual(self.provider.get_placement_policies(),
+                         list_policies())
 
     @inlineCallbacks
     def test_bootstrap(self):

=== modified file 'juju/state/placement.py'
--- juju/state/placement.py	2011-09-27 02:02:42 +0000
+++ juju/state/placement.py	2011-09-27 22:31:25 +0000
@@ -55,10 +55,29 @@
     }
 
 
+def list_policies():
+    """List all policies
+    """
+    # Ordering is important, and permissive providers use this
+    # function to get the policy enumeratoin, the first item is the
+    # default used in the absence of a user preference.
+    return [UNASSIGNED_POLICY, LOCAL_POLICY]
+
+
 def pick_policy(preference, provider):
+    """Pick a provider appropriate placement policy.
+
+    :param preference: A user expressed placement policy preference.
+    :param provider: A machine provider.
+    """
     policies = provider.get_placement_policies()
+    # If no user policy expressed, check the environment config
+    if not preference:
+        preference = provider.config.get("placement")
+    # If there's still no preference, chooce the provider's first policy.
     if not preference:
         return policies[0]
+    # Validate the user policy preference.
     if preference not in policies:
         raise InvalidPlacementPolicy(
             preference, provider.provider_type, policies)

=== modified file 'juju/state/tests/test_placement.py'
--- juju/state/tests/test_placement.py	2011-09-27 02:02:42 +0000
+++ juju/state/tests/test_placement.py	2011-09-27 22:31:25 +0000
@@ -4,6 +4,7 @@
 from juju.errors import InvalidPlacementPolicy
 from juju.state.placement import place_unit, pick_policy
 from juju.state.tests.test_service import ServiceStateManagerTestBase
+from juju.providers.dummy import MachineProvider
 
 
 class TestPlacement(ServiceStateManagerTestBase):
@@ -16,26 +17,28 @@
         self.unit_state = yield self.service.add_unit_state()
 
     def test_pick_policy(self):
-        mock_provider = self.mocker.mock()
-        mock_provider.get_placement_policies()
-        self.mocker.result(["unassigned", "local", "new"])
-        self.mocker.count(3)
-        mock_provider.provider_type
-        self.mocker.result("dummy")
-        self.mocker.replay()
-
+        provider = MachineProvider("foobar", {})
         # No selection gets first listed provider policy
         self.assertEqual(
-            pick_policy(None, mock_provider), "unassigned")
+            pick_policy(None, provider), "unassigned")
 
         # If the user selection doesn't match we get an error
         self.assertRaises(
             InvalidPlacementPolicy,
-            pick_policy, "smart", mock_provider)
+            pick_policy, "smart", provider)
 
         # The user choice is respected if its available
         self.assertEqual(
-            pick_policy("new", mock_provider), "new")
+            pick_policy("local", provider), "local")
+
+        # Invalid environment placement setting is an error
+        provider = MachineProvider("foobar", {"placement": "smart"})
+        self.assertRaises(
+            InvalidPlacementPolicy, pick_policy, None, provider)
+
+        # User selection takes precedence over environment
+        provider = MachineProvider("foobar", {"placement": "unassigned"})
+        self.assertEqual(pick_policy("local", provider), "local")
 
     @inlineCallbacks
     def test_unassign_placement(self):