Merge lp:~gz/pyjuju/os_scheduler_hints_1049858 into lp:pyjuju

[Martin Packman]

Support Openstack scheduler hints as a constraint

Adds Openstack provider specific 'os-scheduler-hints' pseudo-constraint
that instead of helping determine the flavor to use, is simply passed
through when creating the server. This depends on the SchedulerHints
extension that can then influence where to spin up the underlying vm.

As the api expects a json object of string to object pairs, the format
of the constraint is simply json too, with basic validation.

https://codereview.appspot.com/6783059/

[Martin Packman (gz)]

Reviewers: mp+131642_code.launchpad.net,

Message:
Please take a look.

Description:
Support Openstack scheduler hints as a constraint

Adds Openstack provider specific 'os-scheduler-hints' pseudo-constraint
that instead of helping determine the flavor to use, is simply passed
through when creating the server. This depends on the SchedulerHints
extension that can then influence where to spin up the underlying vm.

As the api expects a json object of string to object pairs, the format
of the constraint is simply json too, with basic validation.

https://code.launchpad.net/~gz/juju/os_scheduler_hints_1049858/+merge/131642

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/6783059/

Affected files:
   A [revision details]
   M juju/providers/openstack/client.py
   M juju/providers/openstack/launch.py
   M juju/providers/openstack/provider.py
   M juju/providers/openstack/tests/__init__.py
   M juju/providers/openstack/tests/test_client.py
   M juju/providers/openstack/tests/test_launch.py
   M juju/providers/openstack/tests/test_provider.py

[Jim Baker (jimbaker)]

+1, LGTM, with one minor

https://codereview.appspot.com/6783059/diff/1/juju/providers/openstack/launch.py
File juju/providers/openstack/launch.py (right):

https://codereview.appspot.com/6783059/diff/1/juju/providers/openstack/launch.py#newcode95
juju/providers/openstack/launch.py:95: hints =
self._constraints["os-scheduler-hints"]
Is this key guaranteed to be set? run_server does not assume it is set,
but the constraint is currently registered. However, the comments on
that registration muse that it might not be registered. Given the flow,
maybe should just be:

hints = self._constraints.get("os-scheduler-hints")

https://codereview.appspot.com/6783059/

[Martin Packman (gz)]

On 2012/10/29 11:51:37, jimbaker wrote:

> juju/providers/openstack/launch.py:95: hints =
> self._constraints["os-scheduler-hints"]
> Is this key guaranteed to be set? run_server does not assume it is
set, but the
> constraint is currently registered. However, the comments on that
registration
> muse that it might not be registered. Given the flow, maybe should
just be:

> hints = self._constraints.get("os-scheduler-hints")

This does look a little confusing, but see
machine.constraints.Constraints.__getitem__ - a KeyError is raised only
if the constraint key name does not exist, and the pre-registered
default is returned if no constraint of that name was given.

https://codereview.appspot.com/6783059/

=== modified file 'juju/providers/openstack/client.py'
--- juju/providers/openstack/client.py	2012-10-03 15:16:42 +0000
+++ juju/providers/openstack/client.py	2012-10-26 15:42:22 +0000
@@ -391,7 +391,7 @@
         return self.delete(["servers", server_id], code=204)
 
     def run_server(self, image_id, flavor_id, name, security_group_names=None,
-        user_data=None):
+            user_data=None, scheduler_hints=None):
         server = {
             'name': name,
             'flavorRef': flavor_id,
@@ -402,6 +402,8 @@
         if security_group_names is not None:
             server["security_groups"] = [{'name': n}
                 for n in security_group_names]
+        if scheduler_hints is not None:
+            server["OS-SCH-HNT:scheduler_hints"] = scheduler_hints
         return self.post(["servers"], {'server': server},
             root="server", code=202)
 

=== modified file 'juju/providers/openstack/launch.py'
--- juju/providers/openstack/launch.py	2012-08-21 16:30:17 +0000
+++ juju/providers/openstack/launch.py	2012-10-26 15:42:22 +0000
@@ -92,13 +92,17 @@
         flavors = yield self._provider.nova.list_flavor_details()
         flavor_id = _solve_flavor(self._constraints, flavor_name, flavors)
 
+        hints = self._constraints["os-scheduler-hints"]
+
         server = yield self._provider.nova.run_server(
             name="juju %s instance %s" %
                 (self._provider.environment_name, machine_id,),
             image_id=image_id,
             flavor_id=flavor_id,
             security_group_names=security_groups,
-            user_data=user_data)
+            user_data=user_data,
+            scheduler_hints=hints,
+            )
 
         if self._master:
             yield filestorage.put(id_name, StringIO(str(server['id'])))

=== modified file 'juju/providers/openstack/provider.py'
--- juju/providers/openstack/provider.py	2012-09-05 11:07:35 +0000
+++ juju/providers/openstack/provider.py	2012-10-26 15:42:22 +0000
@@ -11,6 +11,7 @@
 """
 
 import logging
+import json
 
 from twisted.internet.defer import inlineCallbacks, returnValue
 
@@ -33,6 +34,15 @@
 log = logging.getLogger("juju.openstack")
 
 
+def _convert_scheduler_hints(string):
+    """Check constraint value suitable for Nova SchedulerHints extension"""
+    obj = json.loads(string)
+    if not isinstance(obj, dict):
+        raise ValueError("Need json object of key/value strings")
+    # GZ 2012-10-26: Does nova have other restrictions on what it will accept?
+    return obj
+
+
 class MachineProvider(MachineProviderBase):
     """MachineProvider for use in an OpenStack environment"""
 
@@ -83,6 +93,13 @@
             returnValue(cs)
         cs = yield super(MachineProvider, self).get_constraint_set()
 
+        # Pseudo-constraint that does not affect flavor selected but is passed
+        # through to server creation for influencing the scheduler, for
+        # instance in the placement of the server.
+        # Perhaps only register this constraint if the deployment advertises
+        # the SchedulerHints extension as available?
+        cs.register("os-scheduler-hints", converter=_convert_scheduler_hints)
+
         # Fetch provider defined instance types (just names)
         flavors = yield self.nova.list_flavors()
         flavor_names = [f['name'] for f in flavors]

=== modified file 'juju/providers/openstack/tests/__init__.py'
--- juju/providers/openstack/tests/__init__.py	2012-08-24 19:22:13 +0000
+++ juju/providers/openstack/tests/__init__.py	2012-10-26 15:42:22 +0000
@@ -47,6 +47,8 @@
 
     def setup_constraints(self):
         self.constraint_set = ConstraintSet(self.provider_type)
+        self.constraint_set.register("os-scheduler-hints",
+            converter=json.loads)
         self.constraint_set.register_generics(
             [f['name'] for f in self.default_flavors])
 

=== modified file 'juju/providers/openstack/tests/test_client.py'
--- juju/providers/openstack/tests/test_client.py	2012-09-28 15:53:06 +0000
+++ juju/providers/openstack/tests/test_client.py	2012-10-26 15:42:22 +0000
@@ -198,6 +198,36 @@
         deferred = osc.authenticate()
         return self.assertFailure(deferred, errors.SSLVerificationError)
 
+    def is_server_with_hints(self, producer):
+        obj = json.loads(producer.content)
+        self.assertEqual({"server": {
+                "imageRef": "an-image",
+                "flavorRef": "a-flavor",
+                "name": "Test",
+                "OS-SCH-HNT:scheduler_hints": {"hint-key": "hint-val"},
+            }}, obj)
+        return True
+
+    @defer.inlineCallbacks
+    def test_run_server_passes_hints(self):
+        config, osc = self.make_client_legacy()
+        self.mock_agent.request("POST",
+            "https://testing.invalid/compute/servers", mocker.ANY,
+            mocker.MATCH(self.is_server_with_hints))
+        response = FakeResponse(202, http_headers.Headers({
+            "Content-Type": ["application/json"],
+            }),
+            json.dumps({'server': {
+            }}))
+        self.mocker.result(defer.succeed(response))
+        self.mocker.replay()
+        # Fake having already authenticated by setting token and services
+        osc.token = "tok"
+        osc.services = {"compute": "https://testing.invalid/compute"}
+        novac = client._NovaClient(osc)
+        result = yield novac.run_server("an-image", "a-flavor", "Test",
+            scheduler_hints={"hint-key": "hint-val"})
+
 
 class TestReauthentication(testing.TestCase):
 

=== modified file 'juju/providers/openstack/tests/test_launch.py'
--- juju/providers/openstack/tests/test_launch.py	2012-09-10 03:20:20 +0000
+++ juju/providers/openstack/tests/test_launch.py	2012-10-26 15:42:22 +0000
@@ -32,13 +32,15 @@
         self.nova.list_flavor_details()
         self.mocker.result(succeed(self.default_flavors))
 
-    def expect_run_server(self, machine_id, cc_match, response, flavor_id=1):
+    def expect_run_server(self, machine_id, cc_match, response, flavor_id=1,
+            hints=None):
         self.nova.run_server(
             name="juju testing instance " + machine_id,
             image_id=42,
             flavor_id=flavor_id,
             security_group_names=["juju-x", "juju-y"],
             user_data=MATCH(cc_match),
+            scheduler_hints=hints,
             )
         self.mocker.result(succeed(response))
 
@@ -128,6 +130,22 @@
         self.mocker.replay()
         return provider.launch("1", constraints=["cpu=2", "mem=3G"])
 
+    def test_start_machine_with_scheduler_hints(self):
+        provider = MockedLaunchProvider(self.mocker)
+        provider.expect_zookeeper_machines(1000, "master.invalid")
+        provider.expect_launch_setup("1")
+        provider.expect_run_server(
+            "1",
+            self.get_cc_matcher("1", provider),
+            response={
+                'id': 1001,
+                'addresses': {'public': []},
+                },
+            hints={"hint-key": "hint-value"})
+        self.mocker.replay()
+        return provider.launch("1", constraints=[
+            "os-scheduler-hints={\"hint-key\": \"hint-value\"}"])
+
     def test_start_machine(self):
         provider = MockedLaunchProvider(self.mocker)
         provider.expect_zookeeper_machines(1000, "master.invalid")

=== modified file 'juju/providers/openstack/tests/test_provider.py'
--- juju/providers/openstack/tests/test_provider.py	2012-08-28 16:11:04 +0000
+++ juju/providers/openstack/tests/test_provider.py	2012-10-26 15:42:22 +0000
@@ -192,3 +192,32 @@
         cs2 = yield provider.get_constraint_set()
         self.assertIsInstance(cs2, ConstraintSet)
         self.assertEqual(cs, cs2)
+
+    def create_constraint_set(self):
+        self.expect_nova_get("flavors",
+            response={'flavors': self.default_flavors})
+        self.mocker.replay()
+        provider = self.get_provider()
+        return provider.get_constraint_set()
+
+    @inlineCallbacks
+    def test_parse_scheduler_hints_one(self):
+        cs = yield self.create_constraint_set()
+        c = cs.parse(["os-scheduler-hints={\"hint-key\": \"hint-val\"}"])
+        self.assertEqual({"hint-key": "hint-val"}, c["os-scheduler-hints"])
+
+    @inlineCallbacks
+    def test_parse_scheduler_hints_bad_value(self):
+        cs = yield self.create_constraint_set()
+        err = self.assertRaises(errors.ConstraintError,
+            cs.parse, ["os-scheduler-hints=notjson"])
+        self.assertRegexpMatches(str(err),
+            "Bad 'os-scheduler-hints' constraint 'notjson': .*")
+
+    @inlineCallbacks
+    def test_parse_scheduler_hints_bad_array(self):
+        cs = yield self.create_constraint_set()
+        err = self.assertRaises(errors.ConstraintError,
+            cs.parse, ["os-scheduler-hints=[]"])
+        self.assertRegexpMatches(str(err),
+            "Bad 'os-scheduler-hints' constraint '\\[\\]': .*")