juju-deployer

Merge lp://staging/~raharper/juju-deployer/populate-first into lp://staging/juju-deployer

populate-first
Merge into trunk

Proposed by Ryan Harper on 2015-02-12

Status:	Rejected
Rejected by:	Haw Loeung on 2022-10-28
Proposed branch:	lp://staging/~raharper/juju-deployer/populate-first
Merge into:	lp://staging/juju-deployer
Diff against target:	428 lines (+214/-26) (has conflicts) 11 files modified Makefile (+5/-1) deployer/action/importer.py (+109/-18) deployer/cli.py (+7/-0) deployer/deployment.py (+32/-3) deployer/env/go.py (+27/-0) deployer/env/py.py (+6/-0) deployer/service.py (+14/-1) deployer/tests/test_charm.py (+5/-0) deployer/tests/test_deployment.py (+0/-2) deployer/tests/test_guiserver.py (+8/-1) deployer/tests/test_importer.py (+1/-0) Text conflict in deployer/service.py Text conflict in deployer/tests/test_guiserver.py
To merge this branch:	bzr merge lp://staging/~raharper/juju-deployer/populate-first
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
juju-deployers		2015-02-12	Pending
Review via email: mp+249543@code.staging.launchpad.net

Description of the change

This branch introduces a new parameter, -P, --populate_first which does the following

1) reverse the order of services during action/importer.py:{deploy_services, add_units} such that importer operates on machines with unit placements first. This is to ensure that services that have placements get fulfilled first to prevent arbitrary machine placement from obtaining a machine that is targeted.

2) for maas placement, juju does not support specifying a maas machine as a --to destination in the 'deploy' command. To work around this issue we do the equivalent RPC call of this cli sequence:
   juju add-machine foo.maas
   MID=$(juju status | grep -B4 foo.maas | awk -F: '/^ "/ {print $1}')
   juju deploy service --to $MID

To enable (2), we implement a new call, add_machine. jujuclient supports add_machine, but it does not enable the Placement parameter that's available in the juju RPC. Instead, we utilize add_machines which accepts a generic MachineParams object. In deployer, we construct the correct (if empty) dictionary for maas machine placement.

3) modify the logic in deploy_services and add_units such that if we're deploying a service with placement and multiple units, we use a new method in importer which will invoke add_machine and wait until said machine is reporting status, afterwards, the machine index value is returned.

The net result is that we will attempt to invoke add_machine for all units that have placement directives first, and then subsequently deploy services or add_units passing in the correct machine id as a placement parameter respectively.

The test-case for this is:

1) maas provider
2) this yaml:
test_placement:
    series: trusty
    services:
        apache2:
            num_units: 3
            branch: lp:charms/apache2
        mysql:
            branch: lp:charms/mysql
            to:
            - maas=oil-infra-node-2.maas
        wordpress:
            num_units: 3
            branch: lp:charms/wordpress
            to:
            - maas=oil-infra-node-3.maas
            - maas=oil-infra-node-5.maas
            - maas=oil-infra-node-6.maas
    relations:
        - [wordpress, mysql]

We include one unit with no placement (apache2) and we only pass if apache2 isn't provided a unit that's allocated to any other service units with placement directives.

Sometimes you can be randomly lucky if you deploy this without supplying --placement_first; but the only way to ensure it works 100% is by allocating the targeted machines first.

lp://staging/~raharper/juju-deployer/populate-first updated on 2015-02-12

139. By Ryan Harper on 2015-02-12: Update test_charm to fix two issues. 1) if the runner of the test does not have a ~/.gitconfig with user.email set, git commit will abort. 2) The git commands run without changing to the temporary repo; address this by passing -C <path> to git commands which switches the path for the git operation.
140. By Ryan Harper on 2015-02-12: Workaround setting bzr whomi with no /home/rharper or non-writable /home/rharper
141. By Ryan Harper on 2015-02-12: invoke with bash
142. By Ryan Harper on 2015-02-12: Playing around with enviroment in schroot
143. By Ryan Harper on 2015-02-12: Once more for fun.
144. By Ryan Harper on 2015-02-12: Turns out we don't need git -C, as the _call method runs from within the repo's path, also, older git binaries don't support -C which broke building in precise chroots! Debugging output would have made this a lot easier.

Revision history for this message

Kapil Thangavelu (hazmat) wrote on 2015-02-23:

sorry missed this due to leave / email / lp issues. i'll have a look later tonight.

Revision history for this message

Kapil Thangavelu (hazmat) wrote on 2015-02-28:

first pass comments, inline below. will dig in some more, really could use some unit tests with this.

Revision history for this message

Ryan Harper (raharper) wrote on 2015-03-02:

Download full text (15.9 KiB)

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.

On Sat, Feb 28, 2015 at 7:13 AM, Kapil Thangavelu <kapilt@gmail.com> 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", "test@example.com"],
> > +            "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.
>

Revision history for this message

Ryan Harper (raharper) wrote on 2015-03-04:

Download full text (19.0 KiB)

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()
> > > ...

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.

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 <ryan.harper@canonical.com>
wrote:

> On Sat, Feb 28, 2015 at 7:13 AM, Kapil Thangavelu <kapilt@gmail.com>
> 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", "test@example.com"],
> > > +            "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.
>

Revision history for this message

Kapil Thangavelu (hazmat) wrote on 2015-03-12:

the work in the makyo's placement branch might also suffice for your use case.

Revision history for this message

Ryan Harper (raharper) wrote on 2015-03-12:

I took a quick look[1]. It'll need a little bit more work for maas, but I
should give it a try.

1. https://code.launchpad.net/~makyo/juju-deployer/machines-and-placement

On Wed, Mar 11, 2015 at 9:21 PM, Kapil Thangavelu <email address hidden> wrote:

> the work in the makyo's placement branch might also suffice for your use
> case.
> --
>
> https://code.launchpad.net/~raharper/juju-deployer/populate-first/+merge/249543
> You are the owner of lp:~raharper/juju-deployer/populate-first.
>

lp://staging/~raharper/juju-deployer/populate-first updated on 2015-03-24

145. By Ryan Harper on 2015-03-02: Remove reversing of list, that's not going to do it, instead update the services sort to move maas= items first, if present
146. By Ryan Harper on 2015-03-03: Remove some unneeded changes.
147. By Ryan Harper on 2015-03-03: Fix typo
148. By Ryan Harper on 2015-03-03: Fix typo
149. By Ryan Harper on 2015-03-04: Modify depoyment.deploy_services to bring services with placement and multiple units online to ensure subsequent services which placement can target previously deployed service units. Update get_machine to handle container placement.
150. By Ryan Harper on 2015-03-04: Allow nested placement when target uses maas= placement. Fix up debugging log message during deploy_services.

Unmerged revisions

150. By Ryan Harper on 2015-03-04: Allow nested placement when target uses maas= placement. Fix up debugging log message during deploy_services.
149. By Ryan Harper on 2015-03-04: Modify depoyment.deploy_services to bring services with placement and multiple units online to ensure subsequent services which placement can target previously deployed service units. Update get_machine to handle container placement.
148. By Ryan Harper on 2015-03-03: Fix typo
147. By Ryan Harper on 2015-03-03: Fix typo
146. By Ryan Harper on 2015-03-03: Remove some unneeded changes.
145. By Ryan Harper on 2015-03-02: Remove reversing of list, that's not going to do it, instead update the services sort to move maas= items first, if present
144. By Ryan Harper on 2015-02-12: Turns out we don't need git -C, as the _call method runs from within the repo's path, also, older git binaries don't support -C which broke building in precise chroots! Debugging output would have made this a lot easier.
143. By Ryan Harper on 2015-02-12: Once more for fun.
142. By Ryan Harper on 2015-02-12: Playing around with enviroment in schroot
141. By Ryan Harper on 2015-02-12: invoke with bash