Juju Charms Collection
keystone package

Merge lp://staging/~ajkavanagh/charms/trusty/keystone/add-service-checks-lp1524388 into lp://staging/~openstack-charmers-archive/charms/trusty/keystone/next

Proposed by Alex Kavanagh on 2016-02-10

Status:

Merged

Merged at revision:

209

Proposed branch:

lp://staging/~ajkavanagh/charms/trusty/keystone/add-service-checks-lp1524388

Merge into:

lp://staging/~openstack-charmers-archive/charms/trusty/keystone/next

Diff against target:

220 lines (+109/-5)

6 files modified

charmhelpers/contrib/network/ip.py (+15/-0)
charmhelpers/contrib/openstack/context.py (+5/-1)
charmhelpers/contrib/openstack/templates/section-keystone-authtoken (+11/-0)
charmhelpers/contrib/openstack/utils.py (+73/-2)
hooks/keystone_utils.py (+2/-1)
unit_tests/test_keystone_utils.py (+3/-1)

To merge this branch:

bzr merge lp://staging/~ajkavanagh/charms/trusty/keystone/add-service-checks-lp1524388

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
David Ames (community)		2016-02-10	Approve on 2016-02-19
Review via email: mp+285608@code.staging.launchpad.net

Description of the change

For review/comments only:

  Synced in lp:~ajkavanagh/charm-helpers/add-service-checks-lp1524388 version
  of charm-helpers to test that modification will work - this is for demo
  purposes only and will be revised prior to merge proposal.

  The modified version of charm-helpers actually checks if the service is running
  and listening on any ports specified. This change to keystone adds the service
  and port checks as part of assess_status().

  Also changed tests to to work with revision 514:
  "misc changes for haproxy template and context ..."
  which required changes to the test_haproxy_context_service_enabled test

Revision history for this message

uosci-testing-bot (uosci-testing-bot) wrote on 2016-02-12:

charm_lint_check #386 keystone-next for ajkavanagh mp285608
LINT OK: passed

Build: http://10.245.162.36:8080/job/charm_lint_check/386/

Revision history for this message

uosci-testing-bot (uosci-testing-bot) wrote on 2016-02-12:

charm_unit_test #307 keystone-next for ajkavanagh mp285608
UNIT OK: passed

Build: http://10.245.162.36:8080/job/charm_unit_test/307/

Revision history for this message

David Ames (thedac) wrote on 2016-02-12:

This looks great. And will be ready to merge after two more steps.

One, we have some text conflicts that need to be resolved.

Two, my manual run of unit tests are failing due to issues not related to your change. I'd like to see the result of the automated run of unit tests so I have moved this from WIP to needs review which should kick that off.

review: Needs Fixing

Revision history for this message

uosci-testing-bot (uosci-testing-bot) wrote on 2016-02-12:

charm_amulet_test #157 keystone-next for ajkavanagh mp285608
AMULET OK: passed

Build: http://10.245.162.36:8080/job/charm_amulet_test/157/

Revision history for this message

Alex Kavanagh (ajkavanagh) wrote on 2016-02-15:

> One, we have some text conflicts that need to be resolved.

I'm guessing this means some more changes in keystone?

I'm also guessing we need to get the charm-helpers change(s) merged first, then re-sync on keystone (on this one) + any further changes on keystone, and get that in as a merge? Also, did the 'determine_ports()' change alleviate your concerns around ports and HA?

Thanks again for the review :)

Revision history for this message

David Ames (thedac) wrote on 2016-02-16:

Alex, Sorry about this I meant my comments above to be on the Charm helpers MP.

Revision history for this message

uosci-testing-bot (uosci-testing-bot) wrote on 2016-02-18:

charm_lint_check #832 keystone-next for ajkavanagh mp285608
LINT OK: passed

Build: http://10.245.162.36:8080/job/charm_lint_check/832/

Revision history for this message

uosci-testing-bot (uosci-testing-bot) wrote on 2016-02-18:

charm_unit_test #735 keystone-next for ajkavanagh mp285608
UNIT OK: passed

Build: http://10.245.162.36:8080/job/charm_unit_test/735/

Revision history for this message

uosci-testing-bot (uosci-testing-bot) wrote on 2016-02-18:

charm_amulet_test #333 keystone-next for ajkavanagh mp285608
AMULET OK: passed

Build: http://10.245.162.36:8080/job/charm_amulet_test/333/

Revision history for this message

David Ames (thedac) wrote on 2016-02-18:

> > One, we have some text conflicts that need to be resolved.
>
> I'm guessing this means some more changes in keystone?

We still have text conflicts for this one.

bzr branch lp:~openstack-charmers/charms/trusty/keystone/next
cd keystone
bzr merge lp:~ajkavanagh/charms/trusty/keystone/add-service-checks-lp1524388
bzr st # shows text conflicts

> I'm also guessing we need to get the charm-helpers change(s) merged first,
> then re-sync on keystone (on this one) + any further changes on keystone, and
> get that in as a merge? Also, did the 'determine_ports()' change alleviate
> your concerns around ports and HA?
>
> Thanks again for the review :)

CH changes were merged. They may need to be resynced to fix part of the above text conflicts.

I like the determine_ports() solution. We might want to add the ports the keystone service is listening on as well (not haproxy) for extra thouroughness. 35347 and 4990 while still including the well known ports 35357 and 5000.

After the above is resolved, we can merge this thing.

review: Needs Fixing

lp://staging/~ajkavanagh/charms/trusty/keystone/add-service-checks-lp1524388 updated on 2016-02-19

201. By Alex Kavanagh on 2016-02-19: Resync charmhelpers to current version
202. By Alex Kavanagh on 2016-02-19: Merge keystone/next into change to get ready for final merge

Revision history for this message

Alex Kavanagh (ajkavanagh) wrote on 2016-02-19:

> > > One, we have some text conflicts that need to be resolved.
> >
> > I'm guessing this means some more changes in keystone?
>
> We still have text conflicts for this one.
>
> bzr branch lp:~openstack-charmers/charms/trusty/keystone/next
> cd keystone
> bzr merge lp:~ajkavanagh/charms/trusty/keystone/add-service-checks-lp1524388
> bzr st # shows text conflicts
>
> > I'm also guessing we need to get the charm-helpers change(s) merged first,
> > then re-sync on keystone (on this one) + any further changes on keystone,
> and
> > get that in as a merge? Also, did the 'determine_ports()' change alleviate
> > your concerns around ports and HA?
> >
> > Thanks again for the review :)
>
> CH changes were merged. They may need to be resynced to fix part of the above
> text conflicts.
>
> I like the determine_ports() solution. We might want to add the ports the
> keystone service is listening on as well (not haproxy) for extra
> thouroughness. 35347 and 4990 while still including the well known ports 35357
> and 5000.
>
> After the above is resolved, we can merge this thing.