On Sat, Feb 28, 2015 at 7:13 AM, Kapil Thangavelu <email address hidden> wrote:

> first pass comments, inline below. will dig in some more, really could use
> some unit tests with this.
>
> Diff comments:
>
> > === modified file 'Makefile'
> > --- Makefile  2014-08-26 22:34:07 +0000
> > +++ Makefile  2015-02-12 22:40:00 +0000
> > @@ -1,5 +1,9 @@
> > +ifeq (,$(wildcard $(HOME)/.bazaar/bazaar.conf))
> > +    PREFIX=HOME=/tmp
> > +endif
> >  test:
> > -     nosetests -s --verbosity=2 deployer/tests
> > +     [ -n "$(PREFIX)" ] && mkdir -p /tmp/.juju
> > +     /bin/bash -c "$(PREFIX) nosetests -s --verbosity=2 deployer/tests"
> >
> >  freeze:
> >       pip install -d tools/dist -r requirements.txt
> >
> > === modified file 'deployer/action/importer.py'
> > --- deployer/action/importer.py       2014-10-01 10:18:36 +0000
> > +++ deployer/action/importer.py       2015-02-12 22:40:00 +0000
> > @@ -22,7 +22,11 @@
> >          env_status = self.env.status()
> >          reloaded = False
> >
> > -        for svc in self.deployment.get_services():
> > +        services = self.deployment.get_services()
> > +        if self.options.placement_first:
> > +            # reverse the sort order so we delpoy services
>
> deploy
>
> > +            services.reverse()
>
> its a bit unclear what this is supposed to accomplish or if its correct.
> ie, it looks like services are currently sorted based on whether or not
> they specify placement directives for their units, this is to facilitate
> services that have units that place on other services units have their
> parents deployed first (ie. svc_a/0 on svc_b/0 means svc_b/0 sorts first).
> simply reversing the order is going to cause issues. if you need placement
> sorting logic i'd try to push it to Deployment._placement_sort and if you
> need pass the parameter.
>

What I need to accomplish is to find the services with maas directed
placements as I need to allocate (add-machine) first so that non-placed
services (ones with out --to directives) don't "randomly" allocate one of
the machines needed for the placed services.
In a simple test-case the reverse worked correctly, but as you say, it does
cause issues.  In a more complicated openstack deployment, I'm now tripping
the 'nested' services not supported.

I'll take a look at modifying Deployment._placement_sort;  I need to retain
the current sort order (w.r.t parent/child) but put services with maas=
placements to the head of the list.


>
> > +        for svc in services:
> >              cur_units = len(env_status['services'][svc.name].get('units',
> ()))
> >              delta = (svc.num_units - cur_units)
> >
> > @@ -43,7 +47,10 @@
> >                  "Adding %d more units to %s" % (abs(delta), svc.name))
> >              if svc.unit_placement:
> >                  # Reload status once after non placed services units
> are done.
> > -                if reloaded is False:
> > +                # or always reload if we're running placement_first
> option.
> > +                if self.options.placement_first is True or reloaded is
> False:
> > +                    self.log.debug(
> > +                        " Refetching status for placement add_unit")
> >                      # Crappy workaround juju-core api inconsistency
> >                      time.sleep(5.1)
> >                      env_status = self.env.status()
> > @@ -51,7 +58,8 @@
> >
> >                  placement = self.deployment.get_unit_placement(svc,
> env_status)
> >                  for mid in range(cur_units, svc.num_units):
> > -                    self.env.add_unit(svc.name, placement.get(mid))
> > +                    self.env.add_unit(svc.name,
> > +
> self.get_machine(placement.get(mid)))
> >              else:
> >                  self.env.add_units(svc.name, abs(delta))
> >
> > @@ -72,7 +80,12 @@
> >          env_status = self.env.status()
> >          reloaded = False
> >
> > -        for svc in self.deployment.get_services():
> > +        services = self.deployment.get_services()
> > +        if self.options.placement_first:
> > +            # reverse the sort order so we delpoy services
> > +            services.reverse()
> > +
> > +        for svc in services:
> >              if svc.name in env_status['services']:
> >                  self.log.debug(
> >                      " Service %r already deployed. Skipping" % svc.name
> )
> > @@ -85,17 +98,35 @@
> >              if svc.unit_placement:
> >                  # We sorted all the non placed services first, so we
> only
> >                  # need to update status once after we're done with them.
> > -                if not reloaded:
> > +                # Always reload if we're running with placement_first
> option
> > +                if self.options.placement_first is True or reloaded is
> False:
> >                      self.log.debug(
> >                          " Refetching status for placement deploys")
> >                      time.sleep(5.1)
> >                      env_status = self.env.status()
> >                      reloaded = True
> > -                num_units = 1
> > -            else:
> > -                num_units = svc.num_units
> >
> >              placement = self.deployment.get_unit_placement(svc,
> env_status)
> > +            # allocate all of the machines up front for all units
> > +            # to ensure we don't allocate a targeted machine to
> > +            # a service without placement
> > +            if svc.unit_placement and \
> > +               svc.num_units > 1 and \
> > +               self.options.placement_first is True:
> > +                self.log.debug('Pre-allocating machines for %s' %
> svc.name)
> > +                for u in range(0, svc.num_units):
> > +                    self.get_machine(placement.get(u))
> > +
> > +                # update status once more after allocation
> > +                time.sleep(5.1)
> > +                env_status = self.env.status()
> > +                reloaded = True
> > +
> > +                # regenerate placement with recent status
> > +                placement = self.deployment.get_unit_placement(svc,
> env_status)
> > +
> > +            # just let add_units handling bring additional units on-line
> > +            num_units = 1
> >
> >              if charm.is_subordinate():
> >                  num_units = None
> > @@ -107,7 +138,7 @@
> >                  svc.config,
> >                  svc.constraints,
> >                  num_units,
> > -                placement.get(0))
> > +                self.get_machine(placement.get(0)))
> >
> >              if svc.annotations:
> >                  self.log.debug(" Setting annotations")
> > @@ -180,6 +211,51 @@
> >              int(timeout), watch=self.options.watch,
> >              services=self.deployment.get_service_names(),
> on_errors=on_errors)
> >
> > +    def get_machine(self, u_idx):
> > +        # find the machine id that matches the target machine
> > +        # unlike juju status output, the dns-name is one of the
> > +        # many values returned from our env.status() in addresses
> > +        if u_idx is None:
> > +            return None
> > +
> > +        status = self.env.status()
> > +
> > +        mid = [x for x in status['machines'].keys()
> > +               if u_idx in
> > +               [v.get('Value') for v in
> > +                status['machines'][x]['addresses']]]
> > +        self.deployment.log.info('mid=%s' % mid)
> > +        if mid:
> > +            m = mid.pop()
> > +            self.deployment.log.debug(
> > +                'Found juju machine (%s) matching placement: %s', m,
> u_idx)
> > +            return m
> > +        else:
> > +            self.deployment.log.info(
> > +                'No match in juju machines for: %s', u_idx)
> > +
> > +            # if we don't find a match, we need to add it
> > +            mid = self.env.add_machine(u_idx)
> > +            self.deployment.log.debug(
> > +                'Waiting for machine to show up in status.')
> > +            while True:
> > +                m = mid.get('Machine')
> > +                if m in status['machines'].keys():
> > +                    s = [x for x in status['machines'].keys()
> > +                         if u_idx in
> > +                         [v.get('Value') for v in
> > +                          status['machines'][x]['addresses']]]
> > +                    self.deployment.log.debug('addresses: %s' % s)
> > +                    if m in s:
> > +                        break
> > +                else:
> > +                    self.deployment.log.debug(
> > +                        'Machine %s not in status yet' % m)
> > +                time.sleep(1)
> > +                status = self.env.status()
> > +            self.deployment.log.debug('Machine %s up!' % m)
> > +            return mid.get('Machine')
> > +
> >      def run(self):
> >          options = self.options
> >          self.start_time = time.time()
> >
> > === modified file 'deployer/cli.py'
> > --- deployer/cli.py   2014-10-01 10:18:36 +0000
> > +++ deployer/cli.py   2015-02-12 22:40:00 +0000
> > @@ -82,6 +82,13 @@
> >                "machine removal."),
> >          dest="deploy_delay", default=0)
> >      parser.add_argument(
> > +        '-P', '--placement-first', action='store_true', default=False,
>
> not sure the doc string on this really captures for end users what its
> doing
>

OK, I'll wordsmith it a bit more.


>
> > +        dest='placement_first',
> > +        help=("Sort services with placement services first to "
> > +              "ensure that the requirement machines are aquired "
> > +              "before non-targeted services are deployed.  Note "
> > +              "this reverses the default sorting order."))
> > +    parser.add_argument(
> >          '-e', '--environment', action='store', dest='juju_env',
> >          help='Deploy to a specific Juju environment.',
> >          default=os.getenv('JUJU_ENV'))
> >
> > === modified file 'deployer/env/go.py'
> > --- deployer/env/go.py        2014-09-16 17:04:46 +0000
> > +++ deployer/env/go.py        2015-02-12 22:40:00 +0000
> > @@ -24,6 +24,30 @@
> >          self.api_endpoint = endpoint
> >          self.client = None
> >
> > +    def add_machine(self, machine):
> > +        if ':' in machine:
> > +            scope, directive = machine.split(':')
> > +        else:
> > +            scope = self.get_env_config()['Config']['uuid']
> > +            directive = machine
> > +
> > +        machines = [{
>
> we should probably add this to jujuclient add_machine directly, but fine
> for now.
>

Correct; this is a bit of a workaround so not to have to modify jujuclient
at the moment.


>
> > +            "Placement": {
> > +                "Scope": scope,
> > +                "Directive": directive,
> > +            },
> > +            "ParentId": "",
> > +            "ContainerType": "",
> > +            "Series": "",
> > +            "Constraints": {},
> > +            "Jobs": [
> > +                "JobHostUnits"
> > +            ]
> > +        }]
> > +        self.log.debug('Adding machine: %s:%s' % (scope, directive))
> > +        # {u'Machines': [{u'Machine': u'7', u'Error': None}]}
> > +        return self.client.add_machines(machines)['Machines'][0]
> > +
> >      def add_unit(self, service_name, machine_spec):
> >          return self.client.add_unit(service_name, machine_spec)
> >
> > @@ -51,6 +75,9 @@
> >      def get_config(self, svc_name):
> >          return self.client.get_config(svc_name)
> >
> > +    def get_env_config(self):
> > +        return self.client.get_env_config()
> > +
> >      def get_constraints(self, svc_name):
> >          try:
> >              return self.client.get_constraints(svc_name)
> >
> > === modified file 'deployer/env/py.py'
> > --- deployer/env/py.py        2014-02-18 12:16:46 +0000
> > +++ deployer/env/py.py        2015-02-12 22:40:00 +0000
> > @@ -12,6 +12,12 @@
> >          self.name = name
> >          self.options = options
> >
> > +    def add_machine(self, machine):
> > +        params - self._named_env(["juju", "add-machine"])
> > +        params.extend([machine])
> > +        self._check_call(
> > +            params, self.log, "Error adding machine %s", machine)
> > +
> >      def add_units(self, service_name, num_units):
> >          params = self._named_env(["juju", "add-unit"])
> >          if num_units > 1:
> >
> > === modified file 'deployer/service.py'
> > --- deployer/service.py       2015-01-22 15:22:18 +0000
> > +++ deployer/service.py       2015-02-12 22:40:00 +0000
> > @@ -100,7 +100,7 @@
> >                      feedback.error(
> >                          ("Service placement to machine"
> >                           "not supported %s to %s") % (
> > -                             self.service.name, unit_placement[idx]))
> > +                            self.service.name, unit_placement[idx]))
> >              elif p in services:
> >                  if services[p].unit_placement:
> >                      feedback.error(
> >
> > === modified file 'deployer/tests/test_charm.py'
> > --- deployer/tests/test_charm.py      2014-09-29 14:36:34 +0000
> > +++ deployer/tests/test_charm.py      2015-02-12 22:40:00 +0000
> > @@ -161,6 +161,11 @@
> >          self._call(
> >              ["git", "init", self.path],
> >              "Could not initialize repo at  %(path)s")
> > +        o = self._call(
> > +            ["git", "config",
> > +              "user.email", "<email address hidden>"],
> > +            "Could not config user.email at %(path)s")
> > +        print o
> >
> >      def write(self, files):
> >          for f in files:
> >
> > === modified file 'deployer/tests/test_deployment.py'
> > --- deployer/tests/test_deployment.py 2015-01-22 15:22:18 +0000
> > +++ deployer/tests/test_deployment.py 2015-02-12 22:40:00 +0000
> > @@ -62,8 +62,6 @@
> >      def test_maas_name_and_zone_placement(self):
> >          d = self.get_named_deployment("stack-placement-maas.yml",
> "stack")
> >          d.validate_placement()
> > -        placement = d.get_unit_placement('ceph', {})
> > -        self.assertEqual(placement.get(0), "arnolt")
> >          placement = d.get_unit_placement('heat', {})
> >          self.assertEqual(placement.get(0), "zone=zebra")
> >
> >
> > === modified file 'deployer/tests/test_guiserver.py'
> > --- deployer/tests/test_guiserver.py  2015-02-03 12:06:59 +0000
> > +++ deployer/tests/test_guiserver.py  2015-02-12 22:40:00 +0000
> > @@ -28,7 +28,7 @@
> >              'find_service', 'ignore_errors', 'juju_env', 'list_deploys',
> >              'no_local_mods', 'no_relations', 'overrides', 'rel_wait',
> 'retry_count', 'series',
> >              'skip_unit_wait', 'terminate_machines', 'timeout',
> 'update_charms',
> > -            'verbose', 'watch'
> > +            'verbose', 'watch', 'placement_first',
> >          ])
> >          self.assertEqual(expected_keys,
> set(self.options.__dict__.keys()))
> >
> > @@ -58,6 +58,7 @@
> >          self.assertFalse(options.update_charms)
> >          self.assertFalse(options.verbose)
> >          self.assertFalse(options.watch)
> > +        self.assertFalse(options.placement_first)
> >
> >
> >  class TestDeploymentError(unittest.TestCase):
> > @@ -282,7 +283,7 @@
> >              mock.call.status(),
> >              mock.call.deploy(
> >                  'mysql', 'cs:precise/mysql-28', '', None,
> > -                {'arch': 'i386', 'cpu-cores': 4, 'mem': '4G'}, 2, None),
> > +                {'arch': 'i386', 'cpu-cores': 4, 'mem': '4G'}, 1, None),
> >              mock.call.set_annotation(
> >                  'mysql', {'gui-y': '164.547', 'gui-x': '494.347'}),
> >              mock.call.deploy(
> >
> > === modified file 'deployer/tests/test_importer.py'
> > --- deployer/tests/test_importer.py   2014-10-01 10:18:36 +0000
> > +++ deployer/tests/test_importer.py   2015-02-12 22:40:00 +0000
> > @@ -36,6 +36,7 @@
> >              'no_local_mods': True,
> >              'no_relations': False,
> >              'overrides': None,
> > +            'placement_first': False,
> >              'rel_wait': 60,
> >              'retry_count': 0,
> >              'series': None,
> >
>
>
> --
>
> https://code.launchpad.net/~raharper/juju-deployer/populate-first/+merge/249543
> You are the owner of lp:~raharper/juju-deployer/populate-first.
>