Juju Charms Collection
cinder package

Merge lp://staging/~niedbalski/charms/trusty/cinder/remove-unused-services into lp://staging/~openstack-charmers-archive/charms/trusty/cinder/next

Proposed by Jorge Niedbalski on 2016-02-10

Status:	Rejected
Rejected by:	Edward Hope-Morley on 2016-04-25
Proposed branch:	lp://staging/~niedbalski/charms/trusty/cinder/remove-unused-services
Merge into:	lp://staging/~openstack-charmers-archive/charms/trusty/cinder/next
Diff against target:	84 lines (+70/-0) 2 files modified actions.yaml (+6/-0) actions/remove_services.py (+64/-0)
To merge this branch:	bzr merge lp://staging/~niedbalski/charms/trusty/cinder/remove-unused-services
Related bugs:	Link a bug report

Reviewer	Date Requested	Status
Edward Hope-Morley		Needs Fixing on 2016-04-25
Billy Olsen	2016-02-10	Needs Fixing on 2016-02-10
Review via email: mp+285649@code.staging.launchpad.net

Description of the change

Dear Maintainer,

This is a patch for adding a new action 'ha-remove-unused-services',
the rationale behind this is to expose a way to cleanup the services
table on the database from unused ones , those services were
created by cinder before the storage relation is joined (particularly
for stateless ones).

This is a workaround for LP: #1493931 in order to keep the
output of cinder service-list clean after deploying a HA.

Thanks.

Revision history for this message

Billy Olsen (billy-olsen) wrote on 2016-02-10:

Jorge,

Thanks for the patch! I think this is starting to be a nice work around for the bug. I've made some inline comments which I think need to be addressed.

Perhaps we should first disable the service (cinder service-disable) prior to removing it? Sure its defunct, but it might be nice to also qualify the deletion statement with a disabled qualifier.

Additionally, in >= Liberty, cinder includes the cinder-manage service remove [name], which I think should be the preferred method with this as an option for < liberty.

review: Needs Fixing

Revision history for this message

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

charm_lint_check #150 cinder-next for niedbalski mp285649
LINT OK: passed

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

Revision history for this message

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

charm_unit_test #139 cinder-next for niedbalski mp285649
UNIT OK: passed

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

Revision history for this message

Jorge Niedbalski (niedbalski) wrote on 2016-02-10:

Hey Billy,

Thanks for reviewing,

> Jorge,
>
> Thanks for the patch! I think this is starting to be a nice work around for
> the bug. I've made some inline comments which I think need to be addressed.
>
> Perhaps we should first disable the service (cinder service-disable) prior to
> removing it? Sure its defunct, but it might be nice to also qualify the
> deletion statement with a disabled qualifier.
>
> Additionally, in >= Liberty, cinder includes the cinder-manage service remove
> [name], which I think should be the preferred method with this as an option
> for < liberty.

Can you guide me exactly what you think we should add to this action? Because actions
are intended to be run on units, so probably it is up to the operator to perform those
actions previously.

Revision history for this message

Jorge Niedbalski (niedbalski) wrote on 2016-02-10:

Replied comments inline.

Revision history for this message

Billy Olsen (billy-olsen) wrote on 2016-02-10:

Replied inline

Revision history for this message

Billy Olsen (billy-olsen) wrote on 2016-02-10:

> Can you guide me exactly what you think we should add to this action? Because actions
> are intended to be run on units, so probably it is up to the operator to perform those
> actions previously.

I'm thinking somewhere along the lines of:

cinder service-disable <service-name>

if version >= liberty:
    cinder-manage service remove <service-name>
else:
    # Check to ensure any services removed are in the disabled state in this fn
    sql_approach_to_remove_service <service-name>

Revision history for this message

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

charm_amulet_test #87 cinder-next for niedbalski mp285649
AMULET OK: passed

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

lp://staging/~niedbalski/charms/trusty/cinder/remove-unused-services updated on 2016-03-03

151. By Jorge Niedbalski on 2016-03-03: Changed according to @wolsen observations.
152. By Jorge Niedbalski on 2016-03-03: Corrected schema
153. By Jorge Niedbalski on 2016-03-03: Changed session

Revision history for this message

Edward Hope-Morley (hopem) wrote on 2016-04-25:

Hi Jorge, please can you resumbit this patch to http://github.com/openstack as per the new openstack charms development workflow. Thanks.

https://github.com/openstack-charmers/openstack-community/blob/master/README.dev-charms.md

review: Needs Fixing

Unmerged revisions

153. By Jorge Niedbalski on 2016-03-03

Changed session

152. By Jorge Niedbalski on 2016-03-03

Corrected schema

151. By Jorge Niedbalski on 2016-03-03

Changed according to @wolsen observations.

150. By Jorge Niedbalski on 2016-02-10

action_set format

149. By Jorge Niedbalski on 2016-02-10

action_set format

148. By Jorge Niedbalski on 2016-02-10

action_set format

147. By Jorge Niedbalski on 2016-02-10

Added hooks path

146. By Jorge Niedbalski on 2016-02-10

Added missing symlink for action

145. By Jorge Niedbalski on 2016-02-10

- Added a new action for removing unused services
after the unit is related to a stateless backend such as the
cinder backend via cinder-ceph. Per bug #1493931.

Reference: https://bugs.launchpad.net/charms/+source/cinder-ceph/+bug/1493931