From edfd6a55afd6596d793e13dc8f8886b70e527f0c Mon Sep 17 00:00:00 2001 From: Alex Kavanagh Date: Thu, 3 May 2018 15:30:52 +0100 Subject: [PATCH] Tighten up the leader_set(..) usage to handle any errors This stops leader_set from throwing backtraces, and instead logs the error and continues as though the charm is not the leader (which is the case when leader_set(...) fails). Changed the py35 tox job to invoke py3 (to allow it to also test under py3.6 on artful+). Note that the pep8 check is still py27, so the additional # NOQA is to handle Py2 not having FileNotFoundError. Change-Id: Ic25d29983db9a0738d83e66de4673bb50594b599 --- hooks/ceph_hooks.py | 49 ++++++++++++++++++++++++++++------------ tox.ini | 13 ++++------- unit_tests/test_utils.py | 21 +---------------- 3 files changed, 41 insertions(+), 42 deletions(-) diff --git a/hooks/ceph_hooks.py b/hooks/ceph_hooks.py index 7f8876cd..b7013633 100755 --- a/hooks/ceph_hooks.py +++ b/hooks/ceph_hooks.py @@ -205,25 +205,33 @@ def config_changed(): mon_secret = config('monitor-secret') else: mon_secret = "{}".format(ceph.generate_monitor_secret()) - status_set('maintenance', 'Creating FSID and Monitor Secret') opts = { 'fsid': fsid, 'monitor-secret': mon_secret, } - log("Settings for the cluster are: {}".format(opts)) - leader_set(opts) - elif cfg.changed('no-bootstrap') and \ - is_relation_made('bootstrap-source'): + try: + leader_set(opts) + status_set('maintenance', + 'Created FSID and Monitor Secret') + log("Settings for the cluster are: {}".format(opts)) + except Exception as e: + # we're probably not the leader an exception occured + # let's log it anyway. + log("leader_set failed: {}".format(str(e))) + elif (cfg.changed('no-bootstrap') and + is_relation_made('bootstrap-source')): # User changed the no-bootstrap config option, we're the leader, # and the bootstrap-source relation has been made. The charm should # be in a blocked state indicating that the no-bootstrap option # must be set. This block is invoked when the user is trying to # get out of that scenario by enabling no-bootstrap. bootstrap_source_relation_changed() - elif leader_get('fsid') is None or leader_get('monitor-secret') is None: + # unconditionally verify that the fsid and monitor-secret are set now + # otherwise we exit until a leader does this. + if leader_get('fsid') is None or leader_get('monitor-secret') is None: log('still waiting for leader to setup keys') status_set('waiting', 'Waiting for leader to setup keys') - sys.exit(0) + return emit_cephconf() @@ -231,7 +239,12 @@ def config_changed(): if (not ceph.is_bootstrapped() and int(config('monitor-count')) == 1 and is_leader()): status_set('maintenance', 'Bootstrapping single Ceph MON') - ceph.bootstrap_monitor_cluster(leader_get('monitor-secret')) + # the following call raises an exception if it can't add the keyring + try: + ceph.bootstrap_monitor_cluster(leader_get('monitor-secret')) + except FileNotFoundError as e: # NOQA -- PEP8 is still PY2 + log("Couldn't bootstrap the monitor yet: {}".format(str(e))) + return ceph.wait_for_bootstrap() if cmp_pkgrevno('ceph', '12.0.0') >= 0: status_set('maintenance', 'Bootstrapping single Ceph MGR') @@ -323,15 +336,18 @@ def bootstrap_source_relation_changed(): assert curr_secret == mon_secret, \ "bootstrap secret '{}' != current secret '{}'".format( mon_secret, curr_secret) - opts = { 'fsid': fsid, 'monitor-secret': mon_secret, } - - log('Updating leader settings for fsid and monitor-secret ' - 'from remote relation data: {}'.format(opts)) - leader_set(opts) + try: + leader_set(opts) + log('Updating leader settings for fsid and monitor-secret ' + 'from remote relation data: {}'.format(opts)) + except Exception as e: + # we're probably not the leader an exception occured + # let's log it anyway. + log("leader_set failed: {}".format(str(e))) # The leader unit needs to bootstrap itself as it won't receive the # leader-settings-changed hook elsewhere. @@ -353,7 +369,12 @@ def mon_relation(): moncount = int(config('monitor-count')) if len(get_mon_hosts()) >= moncount: status_set('maintenance', 'Bootstrapping MON cluster') - ceph.bootstrap_monitor_cluster(leader_get('monitor-secret')) + # the following call raises an exception if it can't add the keyring + try: + ceph.bootstrap_monitor_cluster(leader_get('monitor-secret')) + except FileNotFoundError as e: # NOQA -- PEP8 is still PY2 + log("Couldn't bootstrap the monitor yet: {}".format(str(e))) + exit(0) ceph.wait_for_bootstrap() ceph.wait_for_quorum() if cmp_pkgrevno('ceph', '12.0.0') >= 0: diff --git a/tox.ini b/tox.ini index e5d01d8a..2bbbc97d 100644 --- a/tox.ini +++ b/tox.ini @@ -2,9 +2,10 @@ # This file is managed centrally by release-tools and should not be modified # within individual charm repos. [tox] -envlist = pep8,py27,py35,py36 +;envlist = pep8,py27,py35,py36 +envlist = pep8,py27,py35 skipsdist = True -skip_missing_interpreters = True +;skip_missing_interpreters = True [testenv] setenv = VIRTUAL_ENV={envdir} @@ -24,13 +25,9 @@ deps = -r{toxinidir}/requirements.txt # temporarily disable py27 commands = /bin/true +; keep zuul happy until we change the py35 job [testenv:py35] -basepython = python3.5 -deps = -r{toxinidir}/requirements.txt - -r{toxinidir}/test-requirements.txt - -[testenv:py36] -basepython = python3.6 +basepython = python3 deps = -r{toxinidir}/requirements.txt -r{toxinidir}/test-requirements.txt diff --git a/unit_tests/test_utils.py b/unit_tests/test_utils.py index bc33c4ff..8539d8ec 100644 --- a/unit_tests/test_utils.py +++ b/unit_tests/test_utils.py @@ -17,8 +17,7 @@ import unittest import os import yaml -from contextlib import contextmanager -from mock import patch, MagicMock +from mock import patch def load_config(): @@ -145,21 +144,3 @@ class TestLeaderSettings(object): elif attr in self.settings: return self.settings[attr] return None - - -@contextmanager -def patch_open(): - '''Patch open() to allow mocking both open() itself and the file that is - yielded. - - Yields the mock for "open" and "file", respectively.''' - mock_open = MagicMock(spec=open) - mock_file = MagicMock(spec=file) - - @contextmanager - def stub_open(*args, **kwargs): - mock_open(*args, **kwargs) - yield mock_file - - with patch('__builtin__.open', stub_open): - yield mock_open, mock_file