openstack
/
charm-openstack-dashboard
Code Issues Proposed changes

Fix issue with plugins breaking packages 

The core issue is that the plugin had no way to signal to the
principal charm what packages needed to be installed, and crucially,
which conflicted with the packages that the plugin needs to operate.
The referenced bug exhibits this issue in that, on install, a package
is removed by the plugin, but the principal charm "doesn't know".  Then
on upgrade, the principal charm re-installs the package, and breaks
the plugin.

This patch allows the plugin to signal which packages it requires to
operate via the dashboard-plugin interface.  This ensures that when
the openstack-dashboard charm upgrades it already "knows" what a
plugin needs and acts accordingly.  Equally, plugins can change their
requirements and this patch allows them to update/remove/install
packages as needed.

The local_settings.py is already controlled by the principal, and this
just shifts absolute control over packaging to the principal as well.
The plugin charm's purpose is to indicate packages and config to the
principal.

Note.  There should be no backwards compatibility issues with this
change. If a plugin doesn't notify the principal of any packages then it
won't take any action.  This does mean that the openstack-charm should
be upgrade prior to any plugins that gain this feature.

Also disable disco test as disco is EOL.

Change-Id: Ib3fc0b0525dabf70f45fd050af2ed05ba31129b9
Related-Bug: #1853851
This commit is contained in:
Alex Kavanagh 2020-01-28 14:02:25 +00:00
parent de4e9ec338
commit 1e2515e13f
5 changed files with 430 additions and 27 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

108
hooks/horizon_hooks.py
View File

@ -33,22 +33,29 @@ def _add_path(path):
_add_path(_root)
from charmhelpers.core.hookenv import (
Hooks, UnregisteredHookError,
log,
open_port,
config,
relation_set,
relation_get,
relation_ids,
related_units,
unit_get,
status_set,
Hooks,
is_leader,
local_unit,
log,
open_port,
related_units,
relation_get,
relation_id as juju_relation_id,
relation_ids,
relation_set,
remote_unit as juju_remote_unit,
status_set,
unit_get,
UnregisteredHookError,
)
from charmhelpers.fetch import (
apt_update, apt_install,
apt_autoremove,
apt_install,
apt_purge,
apt_update,
filter_installed_packages,
filter_missing_packages,
)
from charmhelpers.core.host import (
lsb_release,
@ -86,22 +93,24 @@ from charmhelpers.contrib.charmsupport import nrpe
from charmhelpers.contrib.hardening.harden import harden
from hooks.horizon_utils import (
determine_packages,
register_configs,
restart_map,
services,
LOCAL_SETTINGS, HAPROXY_CONF,
enable_ssl,
do_openstack_upgrade,
setup_ipv6,
INSTALL_DIR,
restart_on_change,
assess_status,
db_migration,
check_custom_theme,
db_migration,
determine_packages,
do_openstack_upgrade,
enable_ssl,
get_plugin_packages_from_kv,
INSTALL_DIR,
LOCAL_SETTINGS, HAPROXY_CONF,
pause_unit_helper,
resume_unit_helper,
register_configs,
remove_old_packages,
restart_map,
restart_on_change,
resume_unit_helper,
services,
setup_ipv6,
update_plugin_packages_in_kv,
)
@ -298,6 +307,61 @@ def plugin_relation_joined(rel_id=None):
@restart_on_change(restart_map(), stopstart=True, sleep=3)
def update_plugin_config():
resolve_CONFIGS()
# NOTE(ajkavanagh) - plugins can indicate that they have packages to
# install and purge. Grab them from the relation and install/update as
# needed.
rid = juju_relation_id()
runit = juju_remote_unit()
(add_packages, remove_packages) = update_plugin_packages_in_kv(rid, runit)
remove_packages = filter_missing_packages(remove_packages)
if remove_packages:
status_set('maintenance', 'Removing packages')
apt_purge(remove_packages, fatal=True)
apt_autoremove(purge=True, fatal=True)
add_packages = filter_installed_packages(add_packages)
if add_packages:
status_set('maintenance', 'Installing packages')
apt_install(add_packages, fatal=True)
if remove_packages or add_packages:
log("Package installation/purge detected, restarting services", "INFO")
for s in services():
service_restart(s)
CONFIGS.write(LOCAL_SETTINGS)
@hooks.hook('dashboard-plugin-relation-departed')
@restart_on_change(restart_map(), stopstart=True, sleep=3)
def remove_plugin_config():
"""Called when a dashboard plugin is leaving.
This is necessary so that the packages that the plugin asked to install are
removed and any conflicting packages are restored and the config updated.
This ensures that when changing plugins the system isn't left in a broken
state.
"""
resolve_CONFIGS()
rid = juju_relation_id()
runit = juju_remote_unit()
pkg_data = get_plugin_packages_from_kv(rid, runit)
changed = False
if pkg_data['install_packages']:
remove_packages = filter_missing_packages(pkg_data['install_packages'])
if remove_packages:
status_set('maintenance', 'Removing packages')
apt_purge(remove_packages, fatal=True)
apt_autoremove(purge=True, fatal=True)
changed = True
if pkg_data['conflicting_packages']:
install_packages = filter_installed_packages(
pkg_data['conflicting_packages'])
if install_packages:
status_set('maintenance', 'Installing packages')
apt_install(install_packages, fatal=True)
changed = True
if changed:
log("Package installation/purge detected, restarting services", "INFO")
for s in services():
service_restart(s)
CONFIGS.write(LOCAL_SETTINGS)

165
hooks/horizon_utils.py
View File

@ -16,6 +16,7 @@
from collections import OrderedDict
from copy import deepcopy
import json
import os
import shutil
import subprocess
@ -46,6 +47,9 @@ from charmhelpers.core.hookenv import (
hook_name,
INFO,
log,
related_units,
relation_get,
relation_ids,
resource_get,
)
from charmhelpers.core.host import (
@ -66,6 +70,7 @@ from charmhelpers.fetch import (
apt_autoremove,
filter_missing_packages,
)
import charmhelpers.core.unitdata as unitdata
import hooks.horizon_contexts as horizon_contexts
@ -501,8 +506,42 @@ def determine_packages():
# NOTE(jamespage): Django in Ubuntu disco or later uses
# mysqldb rather than pymysql.
packages.append('python3-mysqldb')
packages = set(packages)
if release >= 'train':
packages.remove('python3-neutron-lbaas-dashboard')
# NOTE(ajkavanagh) - don't reinstall packages (e.g. on upgrade) that
# plugins have already indicated should not be installed as they clash with
# the plugin. Do add in any packages that the plugins want. Note that
# these will be [] during install, and thus only matter during upgrades.
skip_packages = determine_purge_packages_dashboard_plugin()
add_in_packages = determine_packages_dashboard_plugin()
packages = (packages - set(skip_packages)) | set(add_in_packages)
return list(packages)
def determine_packages_dashboard_plugin():
"""Determine the packages to install from the 'dashboard-plugin' relation.
The relation defines two keys 'conflicting-packages' and 'install-packages'
that are used by the plugin to signal to this charm which packages should
be installed and which are conflicting.
:returns: List of packages to install from dashboard plugins
:rtype: List[str]
"""
packages = []
for rid in relation_ids("dashboard-plugin"):
for unit in related_units(rid):
rdata = relation_get(unit=unit, rid=rid)
install_packages_json = rdata.get("install-packages", "[]")
try:
packages.extend(json.loads(install_packages_json))
except json.JSONDecodeError as e:
log("Error decoding json from {}/{}: on dashboard-plugin "
" relation - ignoring '{}' - error is:{}"
.format(rid, unit, install_packages_json, str(e)),
level=ERROR)
return list(set(packages))
@ -527,7 +566,34 @@ def determine_purge_packages():
])
if release >= 'train':
pkgs.append('python3-neutron-lbaas-dashboard')
return pkgs
# NOTE(ajkavanagh) also ensure that associated plugins can purge on upgrade
return list(set(pkgs)
.union(set(determine_purge_packages_dashboard_plugin())))
def determine_purge_packages_dashboard_plugin():
"""Determine the packages to purge from the 'dashboard-plugin' relation.
The relation defines two keys 'conflicting-packages' and 'install-packages'
that are used by the plugin to signal to this charm which packages should
be installed and which are conflicting.
:returns: List of packages to purge from dashboard plugins
:rtype: List[str]
"""
conflict_packages = []
for rid in relation_ids("dashboard-plugin"):
for unit in related_units(rid):
rdata = relation_get(unit=unit, rid=rid)
conflicting_packages_json = rdata.get("conflicting-packages", "[]")
try:
conflict_packages.extend(json.loads(conflicting_packages_json))
except json.JSONDecodeError as e:
log("Error decoding json from {}/{}: on dashboard-plugin "
" relation - ignoring '{}' - error is:{}"
.format(rid, unit, conflicting_packages_json, str(e)),
level=ERROR)
return list(set(conflict_packages))
def remove_old_packages():
@ -542,6 +608,103 @@ def remove_old_packages():
return bool(installed_packages)
PLUGIN_PACKAGES_KV_KEY = "dashboard-plugin:{}:{}"
def make_dashboard_plugin_packages_kv_key(rid, runit):
"""Construct a key for the kv store for the packages from a unit.
:param rid: The relation_id of the unit
:type rid: str
:param runit: The unit name of the unit
:type runit: str
:returns: String to use as a key to store the packages.
:rtype: str
"""
return PLUGIN_PACKAGES_KV_KEY.format(rid, runit)
def update_plugin_packages_in_kv(rid, runit):
"""Update the plugin packages for this unit in the kv store.
It returns a tuple of 'install_packages' and 'purge_packages' that are
different from that which was previously stored.
:param rid: The relation_id of the unit
:type rid: str
:param runit: The unit name of the unit
:type runit: str
:returns: tuple of (added, removed) packages.
:rtype: Tuple[List[Str],List[str]]
"""
current = get_plugin_packages_from_kv(rid, runit)
rdata = relation_get(unit=runit, rid=rid)
install_packages_json = rdata.get("install-packages", "[]")
install_packages = json.loads(install_packages_json)
conflicting_packages_json = rdata.get("conflicting-packages", "[]")
conflicting_packages = json.loads(conflicting_packages_json)
removed = list(
(set(current['install_packages']) - set(install_packages)) |
(set(conflicting_packages) - set(current['conflicting_packages'])))
added = list(
(set(install_packages) - set(current['install_packages'])) |
(set(current['conflicting_packages']) - set(conflicting_packages)))
store_plugin_packages_in_kv(
rid, runit, conflicting_packages, install_packages)
return (added, removed)
def store_plugin_packages_in_kv(
rid, runit, conflicting_packages, install_packages):
"""Store information from the dashboard plugin for packages
Essentially, the charm needs to know what the charm wants installed and
what packages conflict as if/when the package is removed, the charm has
to be able to restore the original situation (prior to the plugin) and that
means recording what the plugin installed so that it can be removed.
:param rid: The relation_id of the unit
:type rid: str
:param runit: The unit name of the unit
:type runit: str
:param conflicting_packages: the packages the plugin says conflicts with
the ones it wants to have installed.
:type conflicting_packages: List[str]
:param install_packages: the packages the plugin requires to operate.
:type install_packages: List[str]
"""
kv = unitdata.kv()
kv.set(make_dashboard_plugin_packages_kv_key(rid, runit),
{"conflicting_packages": conflicting_packages,
"install_packages": install_packages})
kv.flush()
def get_plugin_packages_from_kv(rid, runit):
"""Get package information concerning a dashboard plugin.
Essentially, the charm needs to know what the charm wants installed and
what packages conflict as if/when the package is removed, the charm has
to be able to restore the original situation (prior to the plugin) and that
means recording what the plugin installed so that it can be removed.
:param rid: The relation_id of the unit
:type rid: str
:param runit: The unit name of the unit
:type runit: str
:returns: Dictionary of 'conflicting_packages' and 'install_packages' from
the plugin.
:rtype: Dict[str, List[str]]
"""
kv = unitdata.kv()
data = kv.get(make_dashboard_plugin_packages_kv_key(rid, runit),
default=None)
if data is None:
data = {}
return {"conflicting_packages": data.get("conflicting_packages", []),
"install_packages": data.get("install_packages", [])}
def do_openstack_upgrade(configs):
"""
Perform an upgrade. Takes care of upgrading packages, rewriting

7
tests/bundles/disco-stein.yaml → tests/bundles/eoan-train.yaml
View File

@ -1,4 +1,4 @@
series: disco
series: eoan
comment:
- 'machines section to decide order of deployment. database sooner = faster'
@ -17,7 +17,6 @@ relations:
- ["openstack-dashboard:shared-db", "mysql:shared-db"]
- ["openstack-dashboard:identity-service", "keystone:identity-service"]
applications:
mysql:
charm: cs:~openstack-charmers-next/percona-cluster
@ -27,10 +26,14 @@ applications:
keystone:
charm: cs:~openstack-charmers-next/keystone
num_units: 1
options:
openstack-origin: cloud:eoan-train
to:
- '1'
openstack-dashboard:
charm: ../../../openstack-dashboard
num_units: 1
options:
openstack-origin: cloud:eoan-train
to:
- '2'

2
tests/tests.yaml
View File

@ -15,6 +15,8 @@ gate_bundles:
- bionic-rocky
- bionic-stein
- bionic-train
dev_bundles:
- eoan-train
configure:
- zaza.openstack.charm_tests.keystone.setup.add_demo_user

175
unit_tests/test_horizon_utils.py
View File

@ -36,6 +36,9 @@ TO_PATCH = [
'os_release',
'os_application_version_set',
'reset_os_release',
'relation_ids',
'related_units',
'relation_get',
'HorizonOSConfigRenderer',
]
@ -86,13 +89,14 @@ class TestHorizonUtils(CharmTestCase):
def test_determine_purge_packages(self):
'Ensure no packages are identified for purge prior to rocky'
horizon_utils.os_release.return_value = 'queens'
horizon_utils.relation_ids.return_value = []
self.assertEqual(horizon_utils.determine_purge_packages(), [])
def test_determine_purge_packages_rocky(self):
'Ensure python packages are identified for purge at rocky'
horizon_utils.relation_ids.return_value = []
horizon_utils.os_release.return_value = 'rocky'
self.assertEqual(
horizon_utils.determine_purge_packages(),
verify_pkgs = (
[p for p in horizon_utils.BASE_PACKAGES
if p.startswith('python-')] +
['python-django-horizon',
@ -101,6 +105,173 @@ class TestHorizonUtils(CharmTestCase):
'python-neutron-lbaas-dashboard',
'python-designate-dashboard',
'python-heat-dashboard'])
self.assertEqual(
sorted(horizon_utils.determine_purge_packages()),
sorted(verify_pkgs))
def _patch_for_dashboard_plugin_packages(self):
def relation_ids_side_effect(rname):
return {
'dashboard-plugin': [
'dashboard-plugin:0',
'dashboard-plugin:1',
],
}[rname]
horizon_utils.relation_ids.side_effect = relation_ids_side_effect
def related_units_side_effect(rid):
return {
'dashboard-plugin:0': ['r0', 'r1'],
'dashboard-plugin:1': ['r2'],
}[rid]
horizon_utils.related_units.side_effect = related_units_side_effect
def relation_get_side_effect(unit=None, rid=None):
return {
"dashboard-plugin:0/r0": {
'install-packages': '["p1", "p3", "p2"]',
'conflicting-packages': '["n2"]',
},
"dashboard-plugin:0/r1": {
'install-packages': '["p5", "p6"]',
'conflicting-packages': "",
},
"dashboard-plugin:1/r2": {
'install-packages': '["p4", "p6"]',
'conflicting-packages': '["n1"]',
},
}["{}/{}".format(rid, unit)]
horizon_utils.relation_get.side_effect = relation_get_side_effect
def test_determine_packages_dashboard_plugin(self):
'Ensure that plugins can provide their packages.'
self._patch_for_dashboard_plugin_packages()
self.assertEqual(
sorted(horizon_utils.determine_packages_dashboard_plugin()),
sorted(['p1', 'p2', 'p3', 'p4', 'p5', 'p6']))
def test_determine_packages_with_dashboard_plugin_rocky(self):
'Ensure that plugin packages are part of determine_packages()'
horizon_utils.os_release.return_value = 'rocky'
pkgs = ([p for p in horizon_utils.BASE_PACKAGES
if not p.startswith('python-')] +
horizon_utils.PY3_PACKAGES +
['p1', 'p2', 'p3', 'p4', 'p5', 'p6'])
self._patch_for_dashboard_plugin_packages()
self.assertEqual(
sorted(horizon_utils.determine_packages()),
sorted(pkgs)
)
def test_determine_purge_packages_dashboard_plugin(self):
'Ensure that plugins can provide their conflicting packages'
self._patch_for_dashboard_plugin_packages()
self.assertEqual(
sorted(horizon_utils.determine_purge_packages_dashboard_plugin()),
sorted(['n1', 'n2']))
def test_determine_purge_packages_dashboard_plugin_rocky(self):
'Ensure python packages are identified for purge at rocky'
self._patch_for_dashboard_plugin_packages()
horizon_utils.os_release.return_value = 'rocky'
verify_pkgs = (
[p for p in horizon_utils.BASE_PACKAGES
if p.startswith('python-')] +
['python-django-horizon',
'python-django-openstack-auth',
'python-pymysql',
'python-neutron-lbaas-dashboard',
'python-designate-dashboard',
'python-heat-dashboard'])
self.assertEqual(
sorted(horizon_utils.determine_purge_packages()),
sorted(verify_pkgs + ["n1", "n2"]))
@patch('charmhelpers.core.unitdata.kv')
def test_store_plugin_packages_in_kv(self, mock_kv):
'Ensure that plugin packages state can be stored.'
_key = None
_value = None
_flush = None
class MockKV():
def set(key, value):
nonlocal _key, _value
_key = key
_value = value
def flush():
nonlocal _flush
_flush = True
mock_kv.return_value = MockKV
horizon_utils.store_plugin_packages_in_kv('r1', 'u1',
['n1'],
['p1', 'p2', 'p3'])
self.assertEqual(_value,
{"conflicting_packages": ['n1'],
"install_packages": ['p1', 'p2', 'p3']})
self.assertEqual(
_key,
horizon_utils.make_dashboard_plugin_packages_kv_key('r1', 'u1'))
self.assertTrue(_flush)
@patch('charmhelpers.core.unitdata.kv')
def test_get_plugin_packages_from_kv(self, mock_kv):
'Ensure that plugin packages state can be recovered.'
# no data stored yet; ensure that it handles it gracefully
_key = None
_value = None
class MockKV():
def get(key, default=None):
nonlocal _key
_key = key
return _value
mock_kv.return_value = MockKV
self.assertEqual(horizon_utils.get_plugin_packages_from_kv('r1', 'u1'),
{"conflicting_packages": [],
"install_packages": []})
self.assertEqual(
_key,
horizon_utils.make_dashboard_plugin_packages_kv_key('r1', 'u1'))
# just packages are configured.
_key = None
_value = {"install_packages": ['p1', 'p2']}
self.assertEqual(horizon_utils.get_plugin_packages_from_kv('r1', 'u1'),
{"conflicting_packages": [],
"install_packages": ['p1', 'p2']})
# just conflicting packages are configured.
_key = None
_value = {"conflicting_packages": ['n1']}
self.assertEqual(horizon_utils.get_plugin_packages_from_kv('r1', 'u1'),
{"conflicting_packages": ['n1'],
"install_packages": []})
# configure both being stored.
_key = None
_value = {"install_packages": ['p1', 'p2'],
"conflicting_packages": ['n1']}
self.assertEqual(horizon_utils.get_plugin_packages_from_kv('r1', 'u1'),
{"conflicting_packages": ['n1'],
"install_packages": ['p1', 'p2']})
@patch.object(horizon_utils, 'get_plugin_packages_from_kv')
@patch.object(horizon_utils, 'store_plugin_packages_in_kv')
def test_update_plugin_packages_in_kv(self,
mock_store_plugin_packages_in_kv,
mock_get_plugin_packages_from_kv):
'Ensure that plugin packages state can be faithfully held'
self._patch_for_dashboard_plugin_packages()
mock_get_plugin_packages_from_kv.return_value = {
"install_packages": ["p1", "q1"],
"conflicting_packages": ["m1"],
}
(added, removed) = horizon_utils.update_plugin_packages_in_kv(
"dashboard-plugin:0", "r0")
self.assertEqual(sorted(added), ["m1", "p2", "p3"])
self.assertEqual(sorted(removed), ["n2", "q1"])
mock_store_plugin_packages_in_kv.assert_called_once_with(
"dashboard-plugin:0", "r0", ["n2"], ["p1", "p3", "p2"])
@patch('subprocess.call')
def test_enable_ssl(self, _call):