This MP will need significant changes, I've got something working, but I'd
like to restructure how it's done.  Here's the gist:

1.  Update _placement_sort() such that when we have two servics with
unit_placements, we sort them in a new function.
     The new sort function prefers maas= units first.  This results in
ensuring we get all of the maas= placements first since
      they are critical for a successful deployment with placement.

2.  The next challenge that arose after implementing (1) was that the
method for allocating machines wasn't handling lxc placement targets
      that was easily fixed since we just return the placement if it's not
something we need to allocate a machine.

3.  After fixing (2) as described, then the bigger issue of supporting -to
container:service=unit which requires that service *has* the target unit.
one of the changes needed to support maas= with placement first was to
bring up the unit=0 machine, and then wait until add_units() to bring
the remaning units online.  This breaks when you have a primary service
targeting a service:unit=X where X is > 0.  In deployer, multi-unit services
are deployed with num_units=svc.units;  but that doesn't work as-is with
placement.

So, if we run with placement first, we deploy the base service, and then
immediately add the additional placed units.  This ensures that when
ceilometer wants to run on lxc:ceph=2
that we actually have a ceph unit=2 available for placement directives.

Now with that said, what I think makes more sense is to keep (1), but
modify the env.base deploy to accept the placement directives and when
doing a deploy a service with multiple units
it also takes the unit placement list, then inside deploy, it handles
bringing the machines online via the add_machine() call we've added.

This should leave the the deploy_services() and add_units() methods mostly
untouched.  If/when juju deploy starts accepting -n 3 -to
maas=host1,maas=host2,maas=host3 as input, juju-deployer can drop
calling env.add_machine() and instead just pass the params through to juju
itself.


On Mon, Mar 2, 2015 at 11:03 AM, Ryan Harper <email address hidden>
wrote:

> 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.
> >
>
> --
>
> https://code.launchpad.net/~raharper/juju-deployer/populate-first/+merge/249543
> You are the owner of lp:~raharper/juju-deployer/populate-first.
>