Merge lp:~hazmat/pyjuju/cli-placement-restore into lp:pyjuju
- cli-placement-restore
- Merge into trunk
Status: | Work in progress | ||||
---|---|---|---|---|---|
Proposed branch: | lp:~hazmat/pyjuju/cli-placement-restore | ||||
Merge into: | lp:pyjuju | ||||
Diff against target: |
450 lines (+143/-56) 12 files modified
juju/control/add_unit.py (+13/-4) juju/control/deploy.py (+10/-4) juju/control/tests/test_add_unit.py (+36/-0) juju/control/tests/test_deploy.py (+33/-1) juju/providers/common/base.py (+4/-6) juju/providers/common/tests/test_base.py (+2/-5) juju/providers/dummy.py (+4/-6) juju/providers/lxc/__init__.py (+2/-5) juju/providers/lxc/tests/test_provider.py (+2/-7) juju/providers/tests/test_dummy.py (+4/-7) juju/state/placement.py (+19/-0) juju/state/tests/test_placement.py (+14/-11) |
||||
To merge this branch: | bzr merge lp:~hazmat/pyjuju/cli-placement-restore | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Gustavo Niemeyer | Needs Fixing | ||
Review via email: mp+77245@code.launchpad.net |
Commit message
Description of the change
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.
Adam Gandelman (gandelman-a) wrote : | # |
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) wrote : | # |
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) wrote : | # |
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
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.
if options.
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.
Kapil Thangavelu (hazmat) wrote : | # |
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-
>
> 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.
> if options.
> 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) 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.
Benjamin Saller (bcsaller) wrote : | # |
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:/
> You are subscribed to branch lp:juju.
>
Kapil Thangavelu (hazmat) wrote : | # |
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:/
> > You are subscribed to branch lp:juju.
> >
>
Unmerged revisions
- 397. By Kapil Thangavelu
-
restore placement cli
Preview Diff
1 | === modified file 'juju/control/add_unit.py' | |||
2 | --- juju/control/add_unit.py 2011-09-27 02:14:56 +0000 | |||
3 | +++ juju/control/add_unit.py 2011-09-27 22:31:25 +0000 | |||
4 | @@ -3,7 +3,7 @@ | |||
5 | 3 | from twisted.internet.defer import inlineCallbacks | 3 | from twisted.internet.defer import inlineCallbacks |
6 | 4 | 4 | ||
7 | 5 | from juju.control.utils import get_environment | 5 | from juju.control.utils import get_environment |
9 | 6 | from juju.state.placement import place_unit | 6 | from juju.state.placement import place_unit, pick_policy |
10 | 7 | from juju.state.service import ServiceStateManager | 7 | from juju.state.service import ServiceStateManager |
11 | 8 | 8 | ||
12 | 9 | 9 | ||
13 | @@ -16,6 +16,9 @@ | |||
14 | 16 | sub_parser.add_argument( | 16 | sub_parser.add_argument( |
15 | 17 | "service_name", | 17 | "service_name", |
16 | 18 | help="Name of the service a unit should be added for") | 18 | help="Name of the service a unit should be added for") |
17 | 19 | sub_parser.add_argument( | ||
18 | 20 | "--placement", | ||
19 | 21 | help="Placement policy used to place units of this service") | ||
20 | 19 | 22 | ||
21 | 20 | return sub_parser | 23 | return sub_parser |
22 | 21 | 24 | ||
23 | @@ -23,20 +26,26 @@ | |||
24 | 23 | def command(options): | 26 | def command(options): |
25 | 24 | """Add a new service unit.""" | 27 | """Add a new service unit.""" |
26 | 25 | environment = get_environment(options) | 28 | environment = get_environment(options) |
27 | 29 | |||
28 | 30 | if not options.placement: | ||
29 | 31 | options.placement = environment.placement | ||
30 | 32 | |||
31 | 26 | return add_unit( | 33 | return add_unit( |
32 | 27 | options.environments, | 34 | options.environments, |
33 | 28 | environment, | 35 | environment, |
34 | 29 | options.verbose, | 36 | options.verbose, |
35 | 30 | options.log, | 37 | options.log, |
37 | 31 | options.service_name) | 38 | options.service_name, |
38 | 39 | options.placement) | ||
39 | 32 | 40 | ||
40 | 33 | 41 | ||
41 | 34 | @inlineCallbacks | 42 | @inlineCallbacks |
43 | 35 | def add_unit(config, environment, verbose, log, service_name): | 43 | def add_unit( |
44 | 44 | config, environment, verbose, log, service_name, placement_policy): | ||
45 | 36 | """Add a unit of a service to the environment. | 45 | """Add a unit of a service to the environment. |
46 | 37 | """ | 46 | """ |
47 | 38 | provider = environment.get_machine_provider() | 47 | provider = environment.get_machine_provider() |
49 | 39 | placement_policy = provider.get_placement_policy() | 48 | placement_policy = pick_policy(placement_policy, provider) |
50 | 40 | client = yield provider.connect() | 49 | client = yield provider.connect() |
51 | 41 | 50 | ||
52 | 42 | try: | 51 | try: |
53 | 43 | 52 | ||
54 | === modified file 'juju/control/deploy.py' | |||
55 | --- juju/control/deploy.py 2011-09-27 02:14:56 +0000 | |||
56 | +++ juju/control/deploy.py 2011-09-27 22:31:25 +0000 | |||
57 | @@ -12,7 +12,7 @@ | |||
58 | 12 | 12 | ||
59 | 13 | from juju.state.endpoint import RelationEndpoint | 13 | from juju.state.endpoint import RelationEndpoint |
60 | 14 | from juju.state.environment import EnvironmentStateManager | 14 | from juju.state.environment import EnvironmentStateManager |
62 | 15 | from juju.state.placement import place_unit | 15 | from juju.state.placement import place_unit, pick_policy |
63 | 16 | from juju.state.relation import RelationStateManager | 16 | from juju.state.relation import RelationStateManager |
64 | 17 | from juju.state.service import ServiceStateManager | 17 | from juju.state.service import ServiceStateManager |
65 | 18 | 18 | ||
66 | @@ -41,6 +41,10 @@ | |||
67 | 41 | "--config", | 41 | "--config", |
68 | 42 | help="YAML file containing service options") | 42 | help="YAML file containing service options") |
69 | 43 | 43 | ||
70 | 44 | sub_parser.add_argument( | ||
71 | 45 | "--placement", | ||
72 | 46 | help="Placement policy used to place units of this service") | ||
73 | 47 | |||
74 | 44 | return sub_parser | 48 | return sub_parser |
75 | 45 | 49 | ||
76 | 46 | 50 | ||
77 | @@ -56,7 +60,8 @@ | |||
78 | 56 | options.charm, | 60 | options.charm, |
79 | 57 | options.service_name, | 61 | options.service_name, |
80 | 58 | options.log, | 62 | options.log, |
82 | 59 | options.config) | 63 | options.config, |
83 | 64 | options.placement) | ||
84 | 60 | 65 | ||
85 | 61 | 66 | ||
86 | 62 | def parse_config_options(config_file, service_name): | 67 | def parse_config_options(config_file, service_name): |
87 | @@ -77,7 +82,7 @@ | |||
88 | 77 | 82 | ||
89 | 78 | @inlineCallbacks | 83 | @inlineCallbacks |
90 | 79 | def deploy(env_config, environment, repository_path, charm_name, | 84 | def deploy(env_config, environment, repository_path, charm_name, |
92 | 80 | service_name, log, config_file=None): | 85 | service_name, log, config_file=None, placement_policy=None): |
93 | 81 | """Deploy a charm within an environment. | 86 | """Deploy a charm within an environment. |
94 | 82 | 87 | ||
95 | 83 | 88 | ||
96 | @@ -96,7 +101,7 @@ | |||
97 | 96 | charm = repository.find(charm_name) | 101 | charm = repository.find(charm_name) |
98 | 97 | 102 | ||
99 | 98 | provider = environment.get_machine_provider() | 103 | provider = environment.get_machine_provider() |
101 | 99 | placement_policy = provider.get_placement_policy() | 104 | placement_policy = pick_policy(placement_policy, provider) |
102 | 100 | client = yield provider.connect() | 105 | client = yield provider.connect() |
103 | 101 | 106 | ||
104 | 102 | try: | 107 | try: |
105 | @@ -133,6 +138,7 @@ | |||
106 | 133 | 138 | ||
107 | 134 | # Create a service unit | 139 | # Create a service unit |
108 | 135 | unit_state = yield service_state.add_unit_state() | 140 | unit_state = yield service_state.add_unit_state() |
109 | 141 | |||
110 | 136 | yield place_unit(client, placement_policy, unit_state) | 142 | yield place_unit(client, placement_policy, unit_state) |
111 | 137 | 143 | ||
112 | 138 | # Check if we have any peer relations to establish | 144 | # Check if we have any peer relations to establish |
113 | 139 | 145 | ||
114 | === modified file 'juju/control/tests/test_add_unit.py' | |||
115 | --- juju/control/tests/test_add_unit.py 2011-09-27 02:14:56 +0000 | |||
116 | +++ juju/control/tests/test_add_unit.py 2011-09-27 22:31:25 +0000 | |||
117 | @@ -87,6 +87,42 @@ | |||
118 | 87 | yield self.assert_machine_assignments("mysql", [1, 2]) | 87 | yield self.assert_machine_assignments("mysql", [1, 2]) |
119 | 88 | 88 | ||
120 | 89 | @inlineCallbacks | 89 | @inlineCallbacks |
121 | 90 | def test_add_unit_with_local_policy(self): | ||
122 | 91 | """ | ||
123 | 92 | 'juju add-unit <service_name>' will add a new service | ||
124 | 93 | unit of the given service. | ||
125 | 94 | """ | ||
126 | 95 | # rework setUp so that service_unit1 is on machine 0 | ||
127 | 96 | # to simulate local deployment | ||
128 | 97 | ms0 = yield self.machine_state_manager.get_machine_state(0) | ||
129 | 98 | yield self.service_unit1.unassign_from_machine() | ||
130 | 99 | yield self.service_unit1.assign_to_machine(ms0) | ||
131 | 100 | |||
132 | 101 | unit_names = yield self.service_state1.get_unit_names() | ||
133 | 102 | self.assertEqual(len(unit_names), 1) | ||
134 | 103 | |||
135 | 104 | finished = self.setup_cli_reactor() | ||
136 | 105 | self.setup_exit(0) | ||
137 | 106 | self.mocker.replay() | ||
138 | 107 | main(["add-unit", "mysql", "--placement", "local"]) | ||
139 | 108 | yield finished | ||
140 | 109 | |||
141 | 110 | # verify the unit and its machine assignment. | ||
142 | 111 | unit_names = yield self.service_state1.get_unit_names() | ||
143 | 112 | self.assertEqual(len(unit_names), 2) | ||
144 | 113 | |||
145 | 114 | topology = yield self.get_topology() | ||
146 | 115 | unit = yield self.service_state1.get_unit_state("mysql/1") | ||
147 | 116 | machine_id = topology.get_service_unit_machine( | ||
148 | 117 | self.service_state1.internal_id, unit.internal_id) | ||
149 | 118 | self.assertNotEqual(machine_id, None) | ||
150 | 119 | self.assertIn( | ||
151 | 120 | "Unit 'mysql/1' added to service 'mysql'", | ||
152 | 121 | self.output.getvalue()) | ||
153 | 122 | # adding a second unit still assigns to machine 0 with local policy | ||
154 | 123 | yield self.assert_machine_assignments("mysql", [0, 0]) | ||
155 | 124 | |||
156 | 125 | @inlineCallbacks | ||
157 | 90 | def test_policy_from_environment(self): | 126 | def test_policy_from_environment(self): |
158 | 91 | config = { | 127 | config = { |
159 | 92 | "environments": {"firstenv": { | 128 | "environments": {"firstenv": { |
160 | 93 | 129 | ||
161 | === modified file 'juju/control/tests/test_deploy.py' | |||
162 | --- juju/control/tests/test_deploy.py 2011-09-27 02:14:56 +0000 | |||
163 | +++ juju/control/tests/test_deploy.py 2011-09-27 22:31:25 +0000 | |||
164 | @@ -150,7 +150,7 @@ | |||
165 | 150 | command(MATCH(match_config), MATCH(match_environment), | 150 | command(MATCH(match_config), MATCH(match_environment), |
166 | 151 | self.unbundled_repo_path, "sample", None, | 151 | self.unbundled_repo_path, "sample", None, |
167 | 152 | MATCH(lambda x: isinstance(x, logging.Logger)), | 152 | MATCH(lambda x: isinstance(x, logging.Logger)), |
169 | 153 | None) | 153 | None, None) |
170 | 154 | self.mocker.replay() | 154 | self.mocker.replay() |
171 | 155 | self.mocker.result(succeed(True)) | 155 | self.mocker.result(succeed(True)) |
172 | 156 | 156 | ||
173 | @@ -301,6 +301,38 @@ | |||
174 | 301 | self.assertEqual(relations[0].relation_name, "ring") | 301 | self.assertEqual(relations[0].relation_name, "ring") |
175 | 302 | 302 | ||
176 | 303 | @inlineCallbacks | 303 | @inlineCallbacks |
177 | 304 | def test_deploy_with_local_policy(self): | ||
178 | 305 | environment = self.config.get("firstenv") | ||
179 | 306 | yield deploy.deploy( | ||
180 | 307 | self.config, environment, self.unbundled_repo_path, | ||
181 | 308 | "sample", "myblog", logging.getLogger("deploy"), | ||
182 | 309 | placement_policy="local") | ||
183 | 310 | |||
184 | 311 | topology = yield self.get_topology() | ||
185 | 312 | service_id = topology.find_service_with_name("myblog") | ||
186 | 313 | self.assertEqual(service_id, "service-%010d" % 0) | ||
187 | 314 | exists = yield self.client.exists("/services/%s" % service_id) | ||
188 | 315 | self.assertTrue(exists) | ||
189 | 316 | |||
190 | 317 | machine_ids = topology.get_machines() | ||
191 | 318 | |||
192 | 319 | # There should just be the one machine with local deployment | ||
193 | 320 | self.assertEqual(machine_ids, ["machine-%010d" % 0]) | ||
194 | 321 | exists = yield self.client.exists("/machines/%s" % machine_ids[0]) | ||
195 | 322 | self.assertTrue(exists) | ||
196 | 323 | |||
197 | 324 | # Verify the process still completes as expected | ||
198 | 325 | unit_ids = topology.get_service_units(service_id) | ||
199 | 326 | self.assertEqual(unit_ids, ["unit-%010d" % 0]) | ||
200 | 327 | exists = yield self.client.exists("/units/%s" % unit_ids[0]) | ||
201 | 328 | self.assertTrue(exists) | ||
202 | 329 | |||
203 | 330 | service = yield self.service_state_manager.get_service_state("myblog") | ||
204 | 331 | unit_state = yield service.get_unit_state("myblog/0") | ||
205 | 332 | machine_id = yield unit_state.get_assigned_machine_id() | ||
206 | 333 | self.assertEqual(machine_id, 0) | ||
207 | 334 | |||
208 | 335 | @inlineCallbacks | ||
209 | 304 | def test_deploy_policy_from_environment(self): | 336 | def test_deploy_policy_from_environment(self): |
210 | 305 | config = { | 337 | config = { |
211 | 306 | "environments": {"firstenv": { | 338 | "environments": {"firstenv": { |
212 | 307 | 339 | ||
213 | === modified file 'juju/providers/common/base.py' | |||
214 | --- juju/providers/common/base.py 2011-09-27 02:14:56 +0000 | |||
215 | +++ juju/providers/common/base.py 2011-09-27 22:31:25 +0000 | |||
216 | @@ -4,7 +4,7 @@ | |||
217 | 4 | from twisted.internet.defer import inlineCallbacks, returnValue | 4 | from twisted.internet.defer import inlineCallbacks, returnValue |
218 | 5 | 5 | ||
219 | 6 | from juju.environment.errors import EnvironmentsConfigError | 6 | from juju.environment.errors import EnvironmentsConfigError |
221 | 7 | from juju.state.placement import UNASSIGNED_POLICY | 7 | from juju.state.placement import list_policies |
222 | 8 | 8 | ||
223 | 9 | 9 | ||
224 | 10 | from .bootstrap import Bootstrap | 10 | from .bootstrap import Bootstrap |
225 | @@ -48,12 +48,10 @@ | |||
226 | 48 | self.environment_name = environment_name | 48 | self.environment_name = environment_name |
227 | 49 | self.config = config | 49 | self.config = config |
228 | 50 | 50 | ||
233 | 51 | def get_placement_policy(self): | 51 | def get_placement_policies(self): |
234 | 52 | """Get the unit placement policy for the provider. | 52 | """Get the available unit placement policies for the provider. |
231 | 53 | |||
232 | 54 | :param preference: A user specified plcaement policy preference | ||
235 | 55 | """ | 53 | """ |
237 | 56 | return self.config.get("placement", UNASSIGNED_POLICY) | 54 | return list_policies() |
238 | 57 | 55 | ||
239 | 58 | def get_serialization_data(self): | 56 | def get_serialization_data(self): |
240 | 59 | """Get provider configuration suitable for serialization.""" | 57 | """Get provider configuration suitable for serialization.""" |
241 | 60 | 58 | ||
242 | === modified file 'juju/providers/common/tests/test_base.py' | |||
243 | --- juju/providers/common/tests/test_base.py 2011-09-27 02:14:56 +0000 | |||
244 | +++ juju/providers/common/tests/test_base.py 2011-09-27 22:31:25 +0000 | |||
245 | @@ -4,7 +4,7 @@ | |||
246 | 4 | from juju.lib.testing import TestCase | 4 | from juju.lib.testing import TestCase |
247 | 5 | from juju.machine import ProviderMachine | 5 | from juju.machine import ProviderMachine |
248 | 6 | from juju.providers.common.base import MachineProviderBase | 6 | from juju.providers.common.base import MachineProviderBase |
250 | 7 | from juju.state.placement import UNASSIGNED_POLICY | 7 | from juju.state.placement import list_policies |
251 | 8 | 8 | ||
252 | 9 | 9 | ||
253 | 10 | class SomeError(Exception): | 10 | class SomeError(Exception): |
254 | @@ -58,10 +58,7 @@ | |||
255 | 58 | def test_get_provider_placement(self): | 58 | def test_get_provider_placement(self): |
256 | 59 | provider = DummyProvider() | 59 | provider = DummyProvider() |
257 | 60 | self.assertEqual( | 60 | self.assertEqual( |
262 | 61 | provider.get_placement_policy(), UNASSIGNED_POLICY) | 61 | provider.get_placement_policies(), list_policies()) |
259 | 62 | provider = DummyProvider({"placement": "local"}) | ||
260 | 63 | self.assertEqual( | ||
261 | 64 | provider.get_placement_policy(), "local") | ||
263 | 65 | 62 | ||
264 | 66 | def test_get_machine_error(self): | 63 | def test_get_machine_error(self): |
265 | 67 | provider = DummyProvider() | 64 | provider = DummyProvider() |
266 | 68 | 65 | ||
267 | === modified file 'juju/providers/dummy.py' | |||
268 | --- juju/providers/dummy.py 2011-09-27 02:14:56 +0000 | |||
269 | +++ juju/providers/dummy.py 2011-09-27 22:31:25 +0000 | |||
270 | @@ -11,7 +11,7 @@ | |||
271 | 11 | 11 | ||
272 | 12 | 12 | ||
273 | 13 | from juju.machine import ProviderMachine | 13 | from juju.machine import ProviderMachine |
275 | 14 | from juju.state.placement import UNASSIGNED_POLICY | 14 | from juju.state.placement import list_policies |
276 | 15 | from juju.providers.common.files import FileStorage | 15 | from juju.providers.common.files import FileStorage |
277 | 16 | 16 | ||
278 | 17 | log = logging.getLogger("juju.providers") | 17 | log = logging.getLogger("juju.providers") |
279 | @@ -34,12 +34,10 @@ | |||
280 | 34 | self._state = None | 34 | self._state = None |
281 | 35 | self._storage = None | 35 | self._storage = None |
282 | 36 | 36 | ||
287 | 37 | def get_placement_policy(self): | 37 | def get_placement_policies(self): |
288 | 38 | """Get the unit placement policy for the provider. | 38 | """Get the available unit placement policies for the provider. |
285 | 39 | |||
286 | 40 | :param preference: A user specified plcaement policy preference | ||
289 | 41 | """ | 39 | """ |
291 | 42 | return self.config.get("placement", UNASSIGNED_POLICY) | 40 | return list_policies() |
292 | 43 | 41 | ||
293 | 44 | @property | 42 | @property |
294 | 45 | def provider_type(self): | 43 | def provider_type(self): |
295 | 46 | 44 | ||
296 | === modified file 'juju/providers/lxc/__init__.py' | |||
297 | --- juju/providers/lxc/__init__.py 2011-09-27 02:51:59 +0000 | |||
298 | +++ juju/providers/lxc/__init__.py 2011-09-27 22:31:25 +0000 | |||
299 | @@ -39,12 +39,9 @@ | |||
300 | 39 | self._directory = os.path.join( | 39 | self._directory = os.path.join( |
301 | 40 | self.config["data-dir"], self._qualified_name) | 40 | self.config["data-dir"], self._qualified_name) |
302 | 41 | 41 | ||
304 | 42 | def get_placement_policy(self): | 42 | def get_placement_policies(self): |
305 | 43 | """Local dev supports only one unit placement policy.""" | 43 | """Local dev supports only one unit placement policy.""" |
310 | 44 | if self.config.get("placement", LOCAL_POLICY) != LOCAL_POLICY: | 44 | return [LOCAL_POLICY] |
307 | 45 | raise ProviderError( | ||
308 | 46 | "Unsupported placement policy for local provider") | ||
309 | 47 | return LOCAL_POLICY | ||
311 | 48 | 45 | ||
312 | 49 | @property | 46 | @property |
313 | 50 | def provider_type(self): | 47 | def provider_type(self): |
314 | 51 | 48 | ||
315 | === modified file 'juju/providers/lxc/tests/test_provider.py' | |||
316 | --- juju/providers/lxc/tests/test_provider.py 2011-09-27 02:51:59 +0000 | |||
317 | +++ juju/providers/lxc/tests/test_provider.py 2011-09-27 22:31:25 +0000 | |||
318 | @@ -41,15 +41,10 @@ | |||
319 | 41 | user_name = pwd.getpwuid(os.getuid()).pw_name | 41 | user_name = pwd.getpwuid(os.getuid()).pw_name |
320 | 42 | return "%s-%s" % (user_name, self.provider.environment_name) | 42 | return "%s-%s" % (user_name, self.provider.environment_name) |
321 | 43 | 43 | ||
323 | 44 | def test_get_placement_policy(self): | 44 | def test_get_placement_policies(self): |
324 | 45 | """Lxc provider only supports local placement.""" | 45 | """Lxc provider only supports local placement.""" |
325 | 46 | self.assertEqual( | 46 | self.assertEqual( |
332 | 47 | self.provider.get_placement_policy(), "local") | 47 | self.provider.get_placement_policies(), ["local"]) |
327 | 48 | provider = MachineProvider( | ||
328 | 49 | "test", {"placement": "unassigned", | ||
329 | 50 | "data-dir": self.makeDir()}) | ||
330 | 51 | self.assertRaises( | ||
331 | 52 | ProviderError, provider.get_placement_policy) | ||
333 | 53 | 48 | ||
334 | 54 | def assertDir(self, *path_parts): | 49 | def assertDir(self, *path_parts): |
335 | 55 | path = os.path.join(*path_parts) | 50 | path = os.path.join(*path_parts) |
336 | 56 | 51 | ||
337 | === modified file 'juju/providers/tests/test_dummy.py' | |||
338 | --- juju/providers/tests/test_dummy.py 2011-09-27 02:14:56 +0000 | |||
339 | +++ juju/providers/tests/test_dummy.py 2011-09-27 22:31:25 +0000 | |||
340 | @@ -7,7 +7,7 @@ | |||
341 | 7 | from juju.errors import ProviderError | 7 | from juju.errors import ProviderError |
342 | 8 | from juju.machine import ProviderMachine | 8 | from juju.machine import ProviderMachine |
343 | 9 | from juju.providers.dummy import MachineProvider, DummyMachine | 9 | from juju.providers.dummy import MachineProvider, DummyMachine |
345 | 10 | from juju.state.placement import UNASSIGNED_POLICY | 10 | from juju.state.placement import list_policies |
346 | 11 | from juju.lib.testing import TestCase | 11 | from juju.lib.testing import TestCase |
347 | 12 | 12 | ||
348 | 13 | 13 | ||
349 | @@ -24,12 +24,9 @@ | |||
350 | 24 | def test_provider_type(self): | 24 | def test_provider_type(self): |
351 | 25 | self.assertEqual(self.provider.provider_type, "dummy") | 25 | self.assertEqual(self.provider.provider_type, "dummy") |
352 | 26 | 26 | ||
359 | 27 | def test_get_placement_policy(self): | 27 | def test_get_placement_policies(self): |
360 | 28 | self.assertEqual( | 28 | self.assertEqual(self.provider.get_placement_policies(), |
361 | 29 | self.provider.get_placement_policy(), UNASSIGNED_POLICY) | 29 | list_policies()) |
356 | 30 | self.provider = MachineProvider("foo", {"placement": "local"}) | ||
357 | 31 | self.assertEqual( | ||
358 | 32 | self.provider.get_placement_policy(), "local") | ||
362 | 33 | 30 | ||
363 | 34 | @inlineCallbacks | 31 | @inlineCallbacks |
364 | 35 | def test_bootstrap(self): | 32 | def test_bootstrap(self): |
365 | 36 | 33 | ||
366 | === modified file 'juju/state/placement.py' | |||
367 | --- juju/state/placement.py 2011-09-27 02:02:42 +0000 | |||
368 | +++ juju/state/placement.py 2011-09-27 22:31:25 +0000 | |||
369 | @@ -55,10 +55,29 @@ | |||
370 | 55 | } | 55 | } |
371 | 56 | 56 | ||
372 | 57 | 57 | ||
373 | 58 | def list_policies(): | ||
374 | 59 | """List all policies | ||
375 | 60 | """ | ||
376 | 61 | # Ordering is important, and permissive providers use this | ||
377 | 62 | # function to get the policy enumeratoin, the first item is the | ||
378 | 63 | # default used in the absence of a user preference. | ||
379 | 64 | return [UNASSIGNED_POLICY, LOCAL_POLICY] | ||
380 | 65 | |||
381 | 66 | |||
382 | 58 | def pick_policy(preference, provider): | 67 | def pick_policy(preference, provider): |
383 | 68 | """Pick a provider appropriate placement policy. | ||
384 | 69 | |||
385 | 70 | :param preference: A user expressed placement policy preference. | ||
386 | 71 | :param provider: A machine provider. | ||
387 | 72 | """ | ||
388 | 59 | policies = provider.get_placement_policies() | 73 | policies = provider.get_placement_policies() |
389 | 74 | # If no user policy expressed, check the environment config | ||
390 | 75 | if not preference: | ||
391 | 76 | preference = provider.config.get("placement") | ||
392 | 77 | # If there's still no preference, chooce the provider's first policy. | ||
393 | 60 | if not preference: | 78 | if not preference: |
394 | 61 | return policies[0] | 79 | return policies[0] |
395 | 80 | # Validate the user policy preference. | ||
396 | 62 | if preference not in policies: | 81 | if preference not in policies: |
397 | 63 | raise InvalidPlacementPolicy( | 82 | raise InvalidPlacementPolicy( |
398 | 64 | preference, provider.provider_type, policies) | 83 | preference, provider.provider_type, policies) |
399 | 65 | 84 | ||
400 | === modified file 'juju/state/tests/test_placement.py' | |||
401 | --- juju/state/tests/test_placement.py 2011-09-27 02:02:42 +0000 | |||
402 | +++ juju/state/tests/test_placement.py 2011-09-27 22:31:25 +0000 | |||
403 | @@ -4,6 +4,7 @@ | |||
404 | 4 | from juju.errors import InvalidPlacementPolicy | 4 | from juju.errors import InvalidPlacementPolicy |
405 | 5 | from juju.state.placement import place_unit, pick_policy | 5 | from juju.state.placement import place_unit, pick_policy |
406 | 6 | from juju.state.tests.test_service import ServiceStateManagerTestBase | 6 | from juju.state.tests.test_service import ServiceStateManagerTestBase |
407 | 7 | from juju.providers.dummy import MachineProvider | ||
408 | 7 | 8 | ||
409 | 8 | 9 | ||
410 | 9 | class TestPlacement(ServiceStateManagerTestBase): | 10 | class TestPlacement(ServiceStateManagerTestBase): |
411 | @@ -16,26 +17,28 @@ | |||
412 | 16 | self.unit_state = yield self.service.add_unit_state() | 17 | self.unit_state = yield self.service.add_unit_state() |
413 | 17 | 18 | ||
414 | 18 | def test_pick_policy(self): | 19 | def test_pick_policy(self): |
423 | 19 | mock_provider = self.mocker.mock() | 20 | provider = MachineProvider("foobar", {}) |
416 | 20 | mock_provider.get_placement_policies() | ||
417 | 21 | self.mocker.result(["unassigned", "local", "new"]) | ||
418 | 22 | self.mocker.count(3) | ||
419 | 23 | mock_provider.provider_type | ||
420 | 24 | self.mocker.result("dummy") | ||
421 | 25 | self.mocker.replay() | ||
422 | 26 | |||
424 | 27 | # No selection gets first listed provider policy | 21 | # No selection gets first listed provider policy |
425 | 28 | self.assertEqual( | 22 | self.assertEqual( |
427 | 29 | pick_policy(None, mock_provider), "unassigned") | 23 | pick_policy(None, provider), "unassigned") |
428 | 30 | 24 | ||
429 | 31 | # If the user selection doesn't match we get an error | 25 | # If the user selection doesn't match we get an error |
430 | 32 | self.assertRaises( | 26 | self.assertRaises( |
431 | 33 | InvalidPlacementPolicy, | 27 | InvalidPlacementPolicy, |
433 | 34 | pick_policy, "smart", mock_provider) | 28 | pick_policy, "smart", provider) |
434 | 35 | 29 | ||
435 | 36 | # The user choice is respected if its available | 30 | # The user choice is respected if its available |
436 | 37 | self.assertEqual( | 31 | self.assertEqual( |
438 | 38 | pick_policy("new", mock_provider), "new") | 32 | pick_policy("local", provider), "local") |
439 | 33 | |||
440 | 34 | # Invalid environment placement setting is an error | ||
441 | 35 | provider = MachineProvider("foobar", {"placement": "smart"}) | ||
442 | 36 | self.assertRaises( | ||
443 | 37 | InvalidPlacementPolicy, pick_policy, None, provider) | ||
444 | 38 | |||
445 | 39 | # User selection takes precedence over environment | ||
446 | 40 | provider = MachineProvider("foobar", {"placement": "unassigned"}) | ||
447 | 41 | self.assertEqual(pick_policy("local", provider), "local") | ||
448 | 39 | 42 | ||
449 | 40 | @inlineCallbacks | 43 | @inlineCallbacks |
450 | 41 | def test_unassign_placement(self): | 44 | def test_unassign_placement(self): |
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.