Merge lp:~niedbalski/charms/trusty/cinder/remove-unused-services into lp:~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:~niedbalski/charms/trusty/cinder/remove-unused-services
Merge into: lp:~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:~niedbalski/charms/trusty/cinder/remove-unused-services
Reviewer Review Type 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.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.

To post a comment you must log in.
Billy Olsen (billy-olsen) wrote :

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

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

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

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

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

Jorge Niedbalski (niedbalski) wrote :

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.

Jorge Niedbalski (niedbalski) wrote :

Replied comments inline.

Billy Olsen (billy-olsen) wrote :

Replied inline

Billy Olsen (billy-olsen) wrote :

> 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>

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

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

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

Edward Hope-Morley (hopem) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'actions.yaml'
2--- actions.yaml 2015-09-15 02:21:20 +0000
3+++ actions.yaml 2016-03-03 15:43:43 +0000
4@@ -2,3 +2,9 @@
5 description: Reinstall cinder from the openstack-origin-git repositories.
6 openstack-upgrade:
7 description: Perform openstack upgrades. Config option action-managed-upgrade must be set to True.
8+remove-services:
9+ description: Remove unused services entities from the database after enabling HA with a stateless backend such as cinder-ceph.
10+ params:
11+ name:
12+ type: string
13+ description: service name to remove. (i.e: name=juju-machine-lxc-0).
14
15=== added symlink 'actions/remove-services'
16=== target is u'remove_services.py'
17=== added file 'actions/remove_services.py'
18--- actions/remove_services.py 1970-01-01 00:00:00 +0000
19+++ actions/remove_services.py 2016-03-03 15:43:43 +0000
20@@ -0,0 +1,64 @@
21+#!/usr/bin/env python
22+
23+import os
24+import sys
25+import traceback
26+
27+sys.path.append('hooks/')
28+
29+from cinder.db.sqlalchemy.api import model_query, get_session
30+from cinder.db.sqlalchemy import models
31+
32+from sqlalchemy import and_
33+from charmhelpers.core.hookenv import (
34+ action_set,
35+ action_fail,
36+ action_get,
37+)
38+
39+DEFAULT_SERVICES = (
40+ "cinder",
41+ "cinder@cinder-ceph",
42+)
43+
44+try:
45+ from cinder import flags
46+ cfg = flags.FLAGS
47+except ImportError:
48+ from cinder.common.config import CONF
49+ cfg = CONF
50+
51+
52+def load_config_file(conf):
53+ cfg(args=[], project='cinder', default_config_files=[conf])
54+
55+
56+def remove_services():
57+ load_config_file(os.path.join(os.path.sep, "etc", "cinder", "cinder.conf"))
58+
59+ name = action_get(key="name")
60+ services = model_query({}, models.Service, read_deleted="no")
61+
62+ if name:
63+ services.filter(models.Service.host == name)
64+ else:
65+ for service in DEFAULT_SERVICES:
66+ services = services.filter(models.Service.host != service)
67+
68+ removed_services = []
69+
70+ session = get_session()
71+ for service in services.all():
72+ try:
73+ service.delete(session)
74+ except:
75+ action_set({'traceback': traceback.format_exc()})
76+ action_fail("Cannot remove service: %s" % service.host)
77+ else:
78+ removed_services.append(service.host)
79+
80+ action_set({'removed': ",".join(removed_services)})
81+
82+
83+if __name__ == "__main__":
84+ remove_services()

Subscribers

People subscribed via source and target branches