Merge ~vultaire/charm-graylog:protected-upgrades into charm-graylog:master

[Paul Goins]

Implemented "protected upgrades" support

The intent is to provide "checkpoints" around major version upgrades
of the Graylog snap:

* Major upgrade (e.g. track increased from 2 to 3): block and guide
  user on next steps.

* Major downgrade (e.g. track decreased from 3 to 2): disallow; ask
  user to revert channel change in charm config.

* Special meaning tracks (i.e. latest): disallow; ask user to revert.

* No track change (e.g. 3/stable to 3/candidate): allow.

[Peter Sabaini (peter-sabaini)]

Hey, some comments inline. Think we should also discuss some of the behaviour details to get to a common view where want to go

cheers,
peter.

review: Needs Information

[Joe Guo (guoqiao)]

A few inline comments, otherwise looks good to me.

review: Needs Fixing

[Drew Freiberger (afreiberger)]

mainly echoing joe's comments inline, Love the constants, feel they should be named in imports rather than wildcarded so it passes lint.

[Paul Goins (vultaire)]

Replied to comments.

[Paul Goins (vultaire)]

OK, I think I've addressed most of the comments here.

I've intentionally skipped running black on the changed sections as that is extra work with very little gain right now, as we're planning to run black over the entire charm afterwards anyway.

Ready for re-review.

[Drew Freiberger (afreiberger)]

Thanks for the cleanup, looks good to me.

review: Approve

[Alvaro Uria (aluria)]

I've found a few issues that could be fixed as follows:
https://pastebin.ubuntu.com/p/pzhghDxGJ7/

In the Makefile, /tmp/graylog-built should be used or this error will happen:
"""
2020-08-03 12:19:46 [INFO] Deploying bundle './tests/bundles/bionic-graylog2.yaml' on to 'zaza-d99cf0eb30e2' model
2020-08-03 12:19:46 [INFO] Rendered template '<Template 'local-charm-overlay.yaml.j2'>' to file '/tmp/tmp77_zr8xr/local-charm-overlay.yaml'
2020-08-03 12:19:46 [INFO] Deploying overlay '/tmp/tmp77_zr8xr/local-charm-overlay.yaml' on to 'zaza-d99cf0eb30e2' model
2020-08-03 12:19:46 [INFO] ERROR cannot deploy bundle: the provided bundle has the following errors:
2020-08-03 12:19:46 [INFO] charm path in application "graylog" does not exist: /tmp/graylog-built/builds/graylog
"""

I added tenacity to retry the search for results, as a slow test env may return 0 findings. I took the following as an example:
https://github.com/openstack-charmers/zaza-openstack-tests/blob/3f7d82c8509749e72ae3818079e942f88688e24f/zaza/openstack/charm_tests/neutron_arista/tests.py#L41

the hooks.hook decorator needs to be called, since the decorated function is passed in a wrapped function (because hook method can take arguments). Since the hook name is taken from the function name in actions/*.py, the func name needs to be renamed.

On line 153, the do-upgrade action will stop the graylog service, and then remove charm states. It seems the unit will wait until update-status (or any other hook) is triggered to reconfigure graylog and start the service again. Or will removing a state trigger the reconfiguration immediately?

Linting failed because "sysdeps" calls "sudo". Since the Makefiles will be changed across all the LMA stack, I only ran "make functional" which seems to work as expected (tests are still running, but the first bundle passed fine).

review: Needs Fixing

[Paul Goins (vultaire)]

Replied to some of the comments. Will think this over.

[Alvaro Uria (aluria)]

Func tests fail because "charm build" happens in ../graylog-built when it should happen in /tmp/graylog-built (otherwise, overlay.yaml file won't find the graylog code).

review: Needs Fixing

[Paul Goins (vultaire)]

Just ran a full set of tests (and got lucky that elasticsearch didn't have any of its random failures). Worked fine.

Am preparing to run another set of tests with CHARM_*_DIR unset in the environment, so as to exercise the /tmp/graylog-built path. I also cleared out the charm build directory just in case, and ensured that neither /tmp/graylog-built nor ../graylog-built exist.

Note: I am using global-level proxy settings in my MODEL_SETTINGS in order for this to run okay as the current cs:elasticsearch requires this.

[Paul Goins (vultaire)]

Re-ran functional tests successfully. Had one run which timed out, admittedly, and with the current issues of cs:elasticsearch I can't say things are reliable, but I believe things are good enough to let us wrap this MR up, if it passes review and if someone else can verify the tests passing.

[Xav Paice (xavpaice)]

`make lint` fails with several errors, but I think that should be left for the lint/black process. The current iteration of the makefile refuses to run unit tests because of this, but tox -e unit results in good tests.

The functional tests are failing:

2020-08-10 11:17:40 [INFO] FAIL
2020-08-10 11:17:40 [INFO] ======================================================================
2020-08-10 11:17:40 [INFO] ERROR: test_02_channel_changed_to_newer_and_finalized (tests.test_graylog_upgrade.UpgradeTest)
2020-08-10 11:17:40 [INFO] ----------------------------------------------------------------------
2020-08-10 11:17:40 [INFO] Traceback (most recent call last):
2020-08-10 11:17:40 [INFO] File "./tests/test_graylog_upgrade.py", line 58, in test_02_channel_changed_to_newer_and_finalized
2020-08-10 11:17:40 [INFO] self._block_until(
2020-08-10 11:17:40 [INFO] File "./tests/test_graylog_upgrade.py", line 155, in _block_until
2020-08-10 11:17:40 [INFO] model.block_until_unit_wl_status(unit, expected_workload_status, timeout=300)
2020-08-10 11:17:40 [INFO] File "/home/xav/charms/charm-graylog/.tox/func/lib/python3.8/site-packages/zaza/__init__.py", line 48, in _wrapper
2020-08-10 11:17:40 [INFO] return run(_run_it())
2020-08-10 11:17:40 [INFO] File "/home/xav/charms/charm-graylog/.tox/func/lib/python3.8/site-packages/zaza/__init__.py", line 36, in run
2020-08-10 11:17:40 [INFO] return task.result()
2020-08-10 11:17:40 [INFO] File "/home/xav/charms/charm-graylog/.tox/func/lib/python3.8/site-packages/zaza/__init__.py", line 47, in _run_it
2020-08-10 11:17:40 [INFO] return await f(*args, **kwargs)
2020-08-10 11:17:40 [INFO] File "/home/xav/charms/charm-graylog/.tox/func/lib/python3.8/site-packages/zaza/model.py", line 1558, in async_block_until_unit_wl_status
2020-08-10 11:17:40 [INFO] await async_block_until(_unit_status, timeout=timeout)
2020-08-10 11:17:40 [INFO] File "/home/xav/charms/charm-graylog/.tox/func/lib/python3.8/site-packages/zaza/model.py", line 1240, in async_block_until
2020-08-10 11:17:40 [INFO] await asyncio.wait_for(_block(), timeout, loop=loop)
2020-08-10 11:17:40 [INFO] File "/usr/lib/python3.8/asyncio/tasks.py", line 490, in wait_for
2020-08-10 11:17:40 [INFO] raise exceptions.TimeoutError()
2020-08-10 11:17:40 [INFO] asyncio.exceptions.TimeoutError
2020-08-10 11:17:40 [INFO] ======================================================================
2020-08-10 11:17:40 [INFO] ERROR: test_03_channel_changed_to_older_and_back (tests.test_graylog_upgrade.UpgradeTest)
2020-08-10 11:17:40 [INFO] ----------------------------------------------------------------------
2020-08-10 11:17:40 [INFO] Traceback (most recent call last):
2020-08-10 11:17:40 [INFO] File "./tests/test_graylog_upgrade.py", line 72, in test_03_channel_changed_to_older_and_back
2020-08-10 11:17:40 [INFO] self._block_until(
2020-08-10 11:17:40 [INFO] File "./tests/test_graylog_upgrade.py", line 155, in _block_until
2020-08-10 11:17:40 [INFO] model.block_until_unit_wl_status(unit, expected_workload_status, timeout=300)
2020-08-10 11:17:40 [INFO] File "/home/xav/charms/charm-graylog/.tox/func/lib/python3.8/site-pac...

review: Needs Fixing

[Xav Paice (xavpaice)]

Re-ran the functests, and this time they all ran through fine (including the previously failed tests). I guess it could be environmental on my test platform - but will ask for another review also.

review: Approve

[Alvaro Uria (aluria)]

As mentioned in previous weeks, Lint and unit tests fail but will leave that after the Makefile and tox.ini files are updated. Unit tests failed because they require sudo privileges, which shouldn't be needed (required packages should live in a tox env).

WRT functests, they worked until the last one, focal-graylog3, which failed on test_10. Juju status looked fine so I guess retries should happen every 10s (as it is configured) but 6 times (instead of the current 3 times). When I re-ran the focal-graylog3 func test, it succeeded without issues.

I'll give this +1 so the other reworks can be started (makefile, blacken, extra lint).

review: Approve

diff --git a/Makefile b/Makefile
index ca1b305..d9b5017 100644
--- a/Makefile
+++ b/Makefile
@@ -1,8 +1,8 @@
 # Set charm build directory via CHARM_BUILD_DIR.
 # JUJU_REPOSITORY is here purely for backwards compatibility.
-# If not specified, CHARM_BUILD_DIR will default to ../graylog-built/builds
+# If not specified, CHARM_BUILD_DIR will default to /tmp/graylog-built/builds
 
-JUJU_REPOSITORY:=$(if $(JUJU_REPOSITORY),$(JUJU_REPOSITORY),../graylog-built)
+JUJU_REPOSITORY:=$(if $(JUJU_REPOSITORY),$(JUJU_REPOSITORY),/tmp/graylog-built)
 CHARM_BUILD_DIR:=$(if $(CHARM_BUILD_DIR),$(CHARM_BUILD_DIR),$(JUJU_REPOSITORY)/builds)
 BUILTCHARMDIR:=$(CHARM_BUILD_DIR)/graylog
 
diff --git a/actions.yaml b/actions.yaml
index c8d6f78..90d3b74 100644
--- a/actions.yaml
+++ b/actions.yaml
@@ -11,5 +11,14 @@ set-admin-password:
     - password
 ignore-indexer-failures:
   description: |
-    From NRPE check viewpoint, ignore all indexer failures that have occurred before the action is run.
-    Future indexer failure occurrences will still be checked.
+    From NRPE check viewpoint, ignore all indexer failures that have occurred before
+    the action is run. Future indexer failure occurrences will still be checked.
+do-upgrade:
+  description: |
+    Perform a snap upgrade. This should be run after having already changed the
+    configured channel and having followed the manual upgrade instructions documented
+    in the charm's README.md  This action will perform the final automatic steps
+    to complete the upgrade.
+
+    If the channel config option has not been changed appropriately, this action will
+    not proceed.
diff --git a/actions/actions.py b/actions/actions.py
index 5b9808d..3564741 100755
--- a/actions/actions.py
+++ b/actions/actions.py
@@ -18,6 +18,14 @@ from charms import reactive  # noqa E402
 from charmhelpers.core import hookenv, host, unitdata  # noqa E402
 
 from charms.layer.graylog.api import get_ignore_indexer_failures_file   # noqa E402
+from charms.layer.graylog.snap_change import (
+    ChannelChangeStatus,
+    get_channel_change_status,
+    perform_snap_channel_change,
+)
+
+
+hooks = hookenv.Hooks()
 
 
 def reactive_remove_state(state):
@@ -25,7 +33,8 @@ def reactive_remove_state(state):
     reactive.main()
 
 
-def get_admin_passwd():
[e-mail]('show-admin-password')
+def show_admin_password():
     db = unitdata.kv()
     admin_password = db.get('admin_password')
     if not admin_password:
@@ -35,7 +44,9 @@ def get_admin_passwd():
     hookenv.action_set({'admin-password': admin_password})
 
 
-def set_admin_passwd(admin_password=None):
[e-mail]('set-admin-password')
+def set_admin_password():
+    admin_password = hookenv.action_get()['password']
     db = unitdata.kv()
     stored_password = db.get('admin_password')
     if admin_password == stored_password:
@@ -52,28 +63,34 @@ def set_admin_passwd(admin_password=None):
     reactive_remove_state('graylog.configured')
 
 
[e-mail]('ignore-indexer-failures')
 def ignore_indexer_failures():
     with open(get_ignore_indexer_failures_file(), 'w') as f:
         f.write(datetime.datetime.utcnow().isoformat())
 
 
[e-mail]('do-upgrade')
+def do_upgrade():
+    status = get_channel_change_status()
+    if status == ChannelChangeStatus.UPGRADE_REQUESTED:
+        new_channel = hookenv.config("channel")
+        perform_snap_channel_change(new_channel)
+        # Let reactive hooks react to the above.
+        reactive.main()
+    else:
+        hookenv.action_fail(
+            "No action taken; app is not blocked by a pending channel upgrade"
+        )
+
+
 def main(argv):
-    action = os.path.basename(argv[0])
-    params = hookenv.action_get()
     try:
-        if action == 'show-admin-password':
-            get_admin_passwd()
-        elif action == 'set-admin-password':
-            set_admin_passwd(params['password'])
-        elif action == 'ignore-indexer-failures':
-            ignore_indexer_failures()
-        else:
-            hookenv.action_fail('Action {} not implemented'.format(action))
+        hooks.execute(argv)
     except Exception:
         hookenv.action_fail('Unhandled exception')
         tb = traceback.format_exc()
         hookenv.action_set(dict(traceback=tb))
-        hookenv.log('Unhandled exception in action {}'.format(action))
+        hookenv.log('Unhandled exception while running: {!r}'.format(argv))
         print(tb)
 
 
diff --git a/actions/do-upgrade b/actions/do-upgrade
new file mode 120000
index 0000000..405a394
--- /dev/null
+++ b/actions/do-upgrade
@@ -0,0 +1 @@
+actions.py
\ No newline at end of file
diff --git a/lib/charms/layer/graylog/constants.py b/lib/charms/layer/graylog/constants.py
new file mode 100644
index 0000000..2ebea47
--- /dev/null
+++ b/lib/charms/layer/graylog/constants.py
@@ -0,0 +1,11 @@
+SNAP_NAME = "graylog"
+CONF_FILE = '/var/snap/graylog/common/server.conf'
+SNAP_CONF_FILE = '/snap/graylog/current/etc/graylog/server/server.conf'
+# /snap/graylog is a read-only squashfs so we use the initial settings
+# and write out an override to SERVER_DEFAULT_CONF_FILE
+SHIPPED_SNAP_SERVER_DEFAULT_CONF_FILE = '/snap/graylog/current/etc/default/graylog-server'
+SERVER_DEFAULT_CONF_FILE = '/var/snap/graylog/current/default-graylog-server'
+ELASTICSEARCH_DISCOVERY_PORT = '9300'
+SERVICE_NAME = 'snap.graylog.graylog'
+DEFAULT_REST_API_TIMEOUT = 120
+NAGIOS_USERNAME = 'nagios'
diff --git a/lib/charms/layer/graylog/snap_change.py b/lib/charms/layer/graylog/snap_change.py
new file mode 100644
index 0000000..e8ed216
--- /dev/null
+++ b/lib/charms/layer/graylog/snap_change.py
@@ -0,0 +1,95 @@
+import enum
+import os
+import shutil
+
+from charms.layer.graylog.constants import (
+    SNAP_NAME,
+    CONF_FILE,
+    SNAP_CONF_FILE,
+    SERVICE_NAME,
+)
+
+from charmhelpers.core import host, hookenv, unitdata
+from charms.reactive import remove_state
+from charms.reactive.helpers import data_changed
+
+# Allow mocking of other layers during unit tests
+try:
+    from charms.layer import snap
+except ImportError:
+    snap = None
+
+
+class ChannelChangeStatus(enum.Enum):
+    NO_CHANGE = 1
+    INVALID = 2
+    UPGRADE_REQUESTED = 3
+    DOWNGRADE_REQUESTED = 4
+    LATEST_REQUESTED = 5
+    MINOR_CHANGE = 6
+
+
+def get_channel_change_status():
+    """Given the current and requested channel, computes the channel change status.
+
+    Specifically, this refers to whether there is any meaningful delta between the
+    installed snap's channel and the requested channel - specifically, the track/risk,
+    excluding the optional branch.
+
+    Returns a value from the ChannelChangeStatus enum.
+
+    """
+    new_channel = hookenv.config("channel")
+    cur_channel = snap.get_installed_channel(SNAP_NAME)
+    if (not new_channel) or new_channel == cur_channel:
+        return ChannelChangeStatus.NO_CHANGE
+    elif not _is_channel_valid(new_channel):
+        return ChannelChangeStatus.INVALID
+    else:
+        cur_track = cur_channel.split("/")[0]
+        new_track = new_channel.split("/")[0]
+        if new_track == "latest":
+            return ChannelChangeStatus.LATEST_REQUESTED
+        else:
+            cur_track = int(cur_track)
+            new_track = int(new_track)
+            if new_track > cur_track:
+                return ChannelChangeStatus.UPGRADE_REQUESTED
+            elif new_track < cur_track:
+                return ChannelChangeStatus.DOWNGRADE_REQUESTED
+            else:
+                return ChannelChangeStatus.MINOR_CHANGE
+
+
+def _is_channel_valid(channel):
+    try:
+        track, risk = channel.split('/')[:2]
+    except ValueError:
+        return False
+    # NOTE: While we won't allow changing to the "latest" channel, it is a valid
+    # channel for the snap.  We'll reject it elsewhere.
+    snap_tracks = ('2', '3', 'latest')
+    snap_risks = ('stable', 'candidate', 'beta', 'edge')
+    return track in snap_tracks and risk in snap_risks
+
+
+def perform_snap_channel_change(new_channel):
+    host.service_stop(SERVICE_NAME)
+    # backup config file using the current version as a suffix
+    cur_snap = snap.get_installed_version(SNAP_NAME)
+    if os.path.exists(CONF_FILE):
+        shutil.copy2(CONF_FILE, "{path}.{ext}".format(path=CONF_FILE, ext=cur_snap))
+    hookenv.status_set("maintenance", "Refreshing graylog snap {}".format(new_channel))
+    snap.refresh(SNAP_NAME, channel=new_channel)
+    new_snap = snap.get_installed_version(SNAP_NAME)
+
+    # Clear data related to the upgrade block
+    unitdata.kv().unset("channel_block_type")
+
+    # When the snap version changes, use the config file from the snap and
+    # adjust states to ensure appropriate re-config happens.
+    if cur_snap != new_snap:
+        shutil.copy2(SNAP_CONF_FILE, CONF_FILE)
+        data_changed("elasticsearch.relation", None)
+        data_changed("mongodb.uri", None)
+        remove_state("graylog.configured")
diff --git a/lib/charms/layer/graylog/utils.py b/lib/charms/layer/graylog/utils.py
index 695b696..848e07d 100644
--- a/lib/charms/layer/graylog/utils.py
+++ b/lib/charms/layer/graylog/utils.py
@@ -3,9 +3,13 @@ from urllib.parse import urlparse
 import netifaces
 
 from charmhelpers.core import hookenv
-from charms.layer import snap
+from charms.layer.graylog.constants import SNAP_NAME
 
-SNAP_NAME = "graylog"
+# Allow mocking of other layers during unit tests
+try:
+    from charms.layer import snap
+except ImportError:
+    snap = None
 
 
 def get_api_port():
diff --git a/reactive/graylog.py b/reactive/graylog.py
index 3db920c..716ebd9 100644
--- a/reactive/graylog.py
+++ b/reactive/graylog.py
@@ -1,7 +1,6 @@
 import hashlib
 import os
 import re
-import shutil
 import time
 import yaml
 from urllib.parse import urlparse
@@ -16,24 +15,26 @@ from charms.layer import snap
 from charms.layer.graylog import (
     GraylogApi,
     LogExtractPipeline,
-    SNAP_NAME,
     get_api_port,
     get_api_url,
     is_v2,
     validate_api_uri
 )
-
-
-CONF_FILE = '/var/snap/graylog/common/server.conf'
-SNAP_CONF_FILE = '/snap/graylog/current/etc/graylog/server/server.conf'
-# /snap/graylog is a read-only squashfs so we use the initial settings
-# and write out an override to SERVER_DEFAULT_CONF_FILE
-SHIPPED_SNAP_SERVER_DEFAULT_CONF_FILE = '/snap/graylog/current/etc/default/graylog-server'
-SERVER_DEFAULT_CONF_FILE = '/var/snap/graylog/current/default-graylog-server'
-ELASTICSEARCH_DISCOVERY_PORT = '9300'
-SERVICE_NAME = 'snap.graylog.graylog'
-DEFAULT_REST_API_TIMEOUT = 120
-NAGIOS_USERNAME = 'nagios'
+from charms.layer.graylog.snap_change import (
+    ChannelChangeStatus,
+    get_channel_change_status,
+    perform_snap_channel_change,
+)
+from charms.layer.graylog.constants import (
+    SNAP_NAME,
+    CONF_FILE,
+    SHIPPED_SNAP_SERVER_DEFAULT_CONF_FILE,
+    SERVER_DEFAULT_CONF_FILE,
+    ELASTICSEARCH_DISCOVERY_PORT,
+    SERVICE_NAME,
+    DEFAULT_REST_API_TIMEOUT,
+    NAGIOS_USERNAME,
+)
 
 
 class ApiTimeout(Exception):
@@ -89,37 +90,20 @@ def install_graylog():
     snap.install(SNAP_NAME, channel=channel)
 
 
-@when('snap.installed.graylog')
-@when('config.changed.channel')
+@when("snap.installed.graylog")
+@when("config.changed.channel")
 def refresh_graylog():
     """Refresh the graylog snap.
 
     A change to the 'channel' config option may warrant a snap refresh. If so,
     stop the service, backup the current config, and refresh.
     """
-    channel = hookenv.config('channel')
-    cur_snap = new_snap = snap.get_installed_version(SNAP_NAME)
-
-    if channel:
-        host.service_stop(SERVICE_NAME)
-        # backup config file using the current version as a suffix
-        if os.path.exists(CONF_FILE):
-            shutil.copy2(CONF_FILE, '{path}.{ext}'.format(path=CONF_FILE, ext=cur_snap))
-
-        hookenv.status_set('maintenance', 'Refreshing graylog snap {}'.format(channel))
-        snap.refresh(SNAP_NAME, channel=channel)
-        new_snap = snap.get_installed_version(SNAP_NAME)
+    new_channel = hookenv.config("channel")
+    if new_channel:
+        if get_channel_change_status() == ChannelChangeStatus.MINOR_CHANGE:
+            perform_snap_channel_change(new_channel)
     else:
         hookenv.log("Cannot refresh snap when the 'channel' config option is missing")
-
-    # When the snap version changes, use the config file from the snap and
-    # adjust states to ensure appropriate re-config happens.
-    if cur_snap != new_snap:
-        shutil.copy2(SNAP_CONF_FILE, CONF_FILE)
-        data_changed('elasticsearch.relation', None)
-        data_changed('mongodb.uri', None)
-        remove_state('graylog.configured')
-
     report_status()
 
 
@@ -272,11 +256,30 @@ def report_status():
     # Check for missing required applications
     missing_apps = []
     if not es_connected:
-        missing_apps.append('elasticsearch')
+        missing_apps.append("elasticsearch")
     if not mongodb_connected:
-        missing_apps.append('mongodb')
-
-    if missing_apps:
+        missing_apps.append("mongodb")
+    channel_change_status = get_channel_change_status()
+    if channel_change_status not in (ChannelChangeStatus.NO_CHANGE,
+                                     ChannelChangeStatus.MINOR_CHANGE):
+        if channel_change_status == ChannelChangeStatus.LATEST_REQUESTED:
+            msg = ('The "latest" track is not supported; revert the channel config '
+                   'change to unblock')
+        elif channel_change_status == ChannelChangeStatus.INVALID:
+            msg = 'Invalid snap channel: {}'.format(
+                hookenv.config("channel"))
+        elif channel_change_status == ChannelChangeStatus.UPGRADE_REQUESTED:
+            msg = ("PACKAGE UPGRADE REQUIRES MANUAL INTERVENTION: see the charm's "
+                   "README.md for more info")
+        elif channel_change_status == ChannelChangeStatus.DOWNGRADE_REQUESTED:
+            msg = (
+            "PACKAGE DOWNGRADES ARE NOT SUPPORTED BY THIS CHARM - to clear this "
+            "message, set config 'snap_channel' to '{}'".format(
+                snap.get_installed_channel(SNAP_NAME)))
+        else:
+            raise ValueError("Unexpected channel_block_type value: {!r}".format(channel_change_status))
+        hookenv.status_set("blocked", msg)
+    elif missing_apps:
         # Report blocked if we have missing apps
         msg = "Missing relation to: {}".format(', '.join(missing_apps))
         hookenv.status_set('blocked', msg)
@@ -953,12 +956,15 @@ def get_default_graylog_client():
 
 
 def _verify_rest_api_is_alive(timeout=DEFAULT_REST_API_TIMEOUT):
+    hookenv.log('Verifying REST API is alive...')
     g = get_default_graylog_client()
     url = ''  # Will query using the base URL of the client, i.e. /api/
     resp = g.request(url)
     start_ts = time.time()
     while resp is None:
         time.sleep(5)
+        hookenv.log('Retrying REST API check...')
         resp = g.request(url)
         if time.time() - start_ts > timeout:
             raise ApiTimeout()
+    hookenv.log('REST API is up')
diff --git a/tests/requirements.txt b/tests/requirements.txt
index b7c9112..7345526 100644
--- a/tests/requirements.txt
+++ b/tests/requirements.txt
@@ -1 +1,2 @@
+tenacity
 git+https://github.com/openstack-charmers/zaza.git#egg=zaza
diff --git a/tests/test_graylog_charm.py b/tests/test_graylog_charm.py
index 2f61b13..3c524a2 100644
--- a/tests/test_graylog_charm.py
+++ b/tests/test_graylog_charm.py
@@ -4,6 +4,8 @@ import logging
 import time
 import unittest
 
+import tenacity
+
 import zaza.model as model
 
 from api import GraylogApi
@@ -126,6 +128,10 @@ class CharmOperationTest(BaseGraylogTest):
             "query": '{}: "/var/log/cloud-init-output.log" AND ci-info'.format(self.file_field),
             "keyword": "4 hours ago",
         }
-
-        resp = self.api.request("/search/universal/keyword", params=params)
-        self.assertTrue(int(resp["total_results"]) > 0)
+        for attempt in tenacity.Retrying(
+                wait=tenacity.wait_fixed(10),  # seconds
+                stop=tenacity.stop_after_attempt(3),
+                reraise=True):
+            with attempt:
+                resp = self.api.request("/search/universal/keyword", params=params)
+                self.assertTrue(int(resp["total_results"]) > 0)
diff --git a/tests/test_graylog_upgrade.py b/tests/test_graylog_upgrade.py
new file mode 100644
index 0000000..d2c09eb
--- /dev/null
+++ b/tests/test_graylog_upgrade.py
@@ -0,0 +1,159 @@
+import time
+import unittest
+
+from zaza import model
+
+
+class UpgradeTest(unittest.TestCase):
+    def setUp(self):
+        self.unit_names = [unit.name for unit in model.get_units('graylog')]
+
+    def test_01_channel_changed_to_newer_and_back(self):
+        # Assumption: graylog channel is 2/stable before reaching this test
+        self.assertEqual(
+            model.get_application_config("graylog")["channel"].get("value"), "2/stable"
+        )
+
+        # Upon attempting an upgrade, we should go to a blocked state, with a reference to
+        # instructions on how to complete the upgrade.
+        model.set_application_config("graylog", {"channel": "3/stable"})
+        for unit_name in self.unit_names:
+            self._block_until(
+                unit_name,
+                "blocked",
+                "PACKAGE UPGRADE REQUIRES MANUAL INTERVENTION: see the charm's README.md "
+                "for more info"
+            )
+
+        # Let's roll back to 2/stable; at this point it's still supported
+        model.set_application_config("graylog", {"channel": "2/stable"})
+        for unit_name in self.unit_names:
+            self._block_until(
+                unit_name, "active", "Ready with: filebeat, elasticsearch, mongodb"
+            )
+
+    def test_02_channel_changed_to_newer_and_finalized(self):
+        # Assumption: graylog channel is 2/stable before reaching this test
+        self.assertEqual(
+            model.get_application_config("graylog")["channel"].get("value"), "2/stable"
+        )
+
+        # Upon attempting an upgrade, we should go to a blocked state, with a reference to
+        # instructions on how to complete the upgrade.
+        model.set_application_config("graylog", {"channel": "3/stable"})
+        for unit_name in self.unit_names:
+            self._block_until(
+                unit_name,
+                "blocked",
+                "PACKAGE UPGRADE REQUIRES MANUAL INTERVENTION: see the charm's README.md "
+                "for more info"
+            )
+
+        # Perform the "manual" upgrade
+        for unit_name in self.unit_names:
+            model.run_action(unit_name, "do-upgrade")
+
+        # Verify that we've come back out the other side in one piece.
+        for unit_name in self.unit_names:
+            self._block_until(
+                unit_name, "active", "Ready with: filebeat, elasticsearch, mongodb"
+            )
+
+    def test_03_channel_changed_to_older_and_back(self):
+        # Assumption: graylog channel is 3/stable before reaching this test
+        self.assertEqual(
+            model.get_application_config("graylog")["channel"].get("value"), "3/stable"
+        )
+
+        # Upon attempting a downgrade, we should go to a blocked state, explaining that downgrades are not supported.
+        # (Reverting the config setting back will unblock the app.)
+        model.set_application_config("graylog", {"channel": "2/stable"})
+        for unit_name in self.unit_names:
+            self._block_until(
+                unit_name,
+                "blocked",
+                "PACKAGE DOWNGRADES ARE NOT SUPPORTED BY THIS CHARM - to clear "
+                "this message, set config 'snap_channel' to '3/stable'",
+            )
+
+        # Let's roll back to 2/stable; at this point it's still supported
+        model.set_application_config("graylog", {"channel": "3/stable"})
+        for unit_name in self.unit_names:
+            self._block_until(
+                unit_name, "active", "Ready with: filebeat, elasticsearch, mongodb"
+            )
+
+    def test_04_channel_changed_to_latest_and_back(self):
+        # Assumption: graylog channel is 3/stable before reaching this test
+        self.assertEqual(
+            model.get_application_config("graylog")["channel"].get("value"), "3/stable"
+        )
+
+        # We shouldn't allow upgrading to anything in the "latest" track; this may be newer *or* older than the current channel.
+        # Require an explicit track, i.e. 2/* or 3/*..
+        model.set_application_config("graylog", {"channel": "latest/stable"})
+        for unit_name in self.unit_names:
+            self._block_until(
+                unit_name,
+                "blocked",
+                'The "latest" track is not supported; revert the channel config change to '
+                "unblock",
+            )
+
+        # Verify that we've come back out the other side in one piece.
+        model.set_application_config("graylog", {"channel": "3/stable"})
+        for unit_name in self.unit_names:
+            self._block_until(
+                unit_name, "active", "Ready with: filebeat, elasticsearch, mongodb"
+            )
+
+    def test_05_channel_changed_to_invalid_value(self):
+        # Assumption: graylog channel is 3/stable before reaching this test
+        self.assertEqual(
+            model.get_application_config("graylog")["channel"].get("value"), "3/stable"
+        )
+
+        # The channel can be set to an arbitrary value; we should catch if the
+        # channel is bogus.
+        model.set_application_config("graylog", {"channel": "fake"})
+        for unit_name in self.unit_names:
+            self._block_until(
+                unit_name,
+                "blocked",
+                'Invalid snap channel: fake',
+            )
+
+        # Roll back
+        model.set_application_config("graylog", {"channel": "3/stable"})
+        for unit_name in self.unit_names:
+            self._block_until(
+                unit_name, "active", "Ready with: filebeat, elasticsearch, mongodb"
+            )
+
+    def test_06_channel_changed_within_track(self):
+        # Assumption: graylog channel is 3/stable before reaching this test
+        self.assertEqual(
+            model.get_application_config("graylog")["channel"].get("value"), "3/stable"
+        )
+
+        # Snap refreshes within the same major series should be allowed.
+        model.set_application_config("graylog", {"channel": "3/edge"})
+        # config changed may not run right away; wait a bit, else we might see the pre-update "active" state and think we're done.
+        time.sleep(10)
+        for unit_name in self.unit_names:
+            self._block_until(
+                unit_name, "active", "Ready with: filebeat, elasticsearch, mongodb"
+            )
+
+    def _block_until(
+        self, unit, expected_workload_status, expected_workload_status_message
+    ):
+        app = unit.split("/")[0]
+        # Zaza doesn't presently seem to provide a way to check these 3 things
+        # at the same time.  I could write a helper to do this, but this may be "good
+        # enough" for the time being.
+        model.block_until_unit_wl_status(unit, expected_workload_status, timeout=300)
+        model.block_until_wl_status_info_starts_with(
+            app, expected_workload_status_message, timeout=300
+        )
+        model.wait_for_unit_idle(unit, timeout=300)
diff --git a/tests/tests.yaml b/tests/tests.yaml
index bd3f9ba..1b6afb6 100644
--- a/tests/tests.yaml
+++ b/tests/tests.yaml
@@ -1,18 +1,23 @@
 charm_name: graylog
 gate_bundles:
-  - bionic-graylog2
-  - bionic-graylog3
-  - focal-graylog2
-  - focal-graylog3
-  - bionic-graylog2-ha
-  - bionic-graylog3-ha
+  - gl2: bionic-graylog2
+  - gl3: bionic-graylog3
+  - gl2: focal-graylog2
+  - gl3: focal-graylog3
+#  - gl2: bionic-graylog2-ha
+#  - gl3: bionic-graylog3-ha
 smoke_bundles:
-  - bionic-graylog3
+  - gl3: bionic-graylog3
 dev_bundles:
-  - bionic-graylog3
+  - gl3: bionic-graylog3
 tests:
-  - tests.test_legacy.LegacyTests
-  - tests.test_graylog_charm.CharmOperationTest
+  - gl2:
+    - tests.test_legacy.LegacyTests
+    - tests.test_graylog_charm.CharmOperationTest
+    - tests.test_graylog_upgrade.UpgradeTest
+  - gl3:
+    - tests.test_legacy.LegacyTests
+    - tests.test_graylog_charm.CharmOperationTest
 target_deploy_status:
   filebeat:
     workload-status: active
diff --git a/unit_tests/test_graylog.py b/unit_tests/test_graylog.py
index 4c1f8dd..38a8e35 100644
--- a/unit_tests/test_graylog.py
+++ b/unit_tests/test_graylog.py
@@ -4,6 +4,10 @@ import tempfile
 import unittest
 from unittest import mock
 from charms.reactive import set_state, is_state
+from charms.layer.graylog.snap_change import (
+    _is_channel_valid,
+    ChannelChangeStatus,
+)
 
 # some dep layers only exists in the built charm; mock them out before
 # the graylog imports since those depend on post-build charms.* layers
@@ -54,7 +58,7 @@ class TestInstallUpdateUpgrade(unittest.TestCase):
 class TestRefresh(unittest.TestCase):
     """Test the snap refresh handler."""
 
-    @mock.patch('reactive.graylog.snap')
+    @mock.patch('charms.layer.graylog.snap_change.snap')
     @mock.patch('reactive.graylog.hookenv.config')
     def test_no_channel(self, mock_config, mock_snap):
         """Ensure a no-op if there is no 'channel' config."""
@@ -65,38 +69,51 @@ class TestRefresh(unittest.TestCase):
         self.assertFalse(mock_snap.refresh.called)
         self.assertTrue(is_state('graylog.configured'))
 
-    @mock.patch('reactive.graylog.shutil')
-    @mock.patch('reactive.graylog.os')
-    @mock.patch('reactive.graylog.host')
+    # test_change and test_no_change are getting a bit cumbersome due to
+    # reworks/refactorings; we may want to consider dropping these 2 and instead
+    # relying on functional coverage.
+
+    @mock.patch('charms.layer.graylog.snap_change.unitdata')  # Avoid unintended local state changes
+    @mock.patch('reactive.graylog.unitdata')                  # Same as above
+    @mock.patch('charms.layer.graylog.snap_change.os')
+    @mock.patch('charms.layer.graylog.snap_change.snap')
     @mock.patch('reactive.graylog.snap')
     @mock.patch('reactive.graylog.hookenv.config')
-    def test_no_change(self, mock_config, mock_snap, mock_host, mock_os, mock_shutil):
+    def test_no_change(self, mock_config, mock_snap, mock_snap2, mock_os, mock_ud, mock_ud2):
         """Ensure no reconfiguration if the snap version does not change."""
-        mock_config.return_value = 'foo'
-        mock_snap.get_installed_version.side_effect = ['2.foo', '2.foo']
+        mock_config.return_value = '2/stable'
+        mock_snap.get_installed_version.side_effect = mock_snap2.get_installed_version.side_effect = ['2.1', '2.1']
+        mock_snap.get_installed_channel.return_value = mock_snap2.get_installed_channel.return_value = '2/stable'
         mock_os.path.exists.return_value = True
-        mock_shutil.copy2.return_value = 'foo'
+        mock_ud.kv.return_value = mock_kv = mock.MagicMock()
+        mock_kv.get.return_value = None  # For update_status, mock that no channel_block_type has been set
         set_state('graylog.configured')
         refresh_graylog()
-        self.assertTrue(mock_snap.refresh.called)
+        self.assertFalse(mock_snap.refresh.called)
         self.assertTrue(is_state('graylog.configured'))
 
-    @mock.patch('reactive.graylog.data_changed')
-    @mock.patch('reactive.graylog.shutil.copy2')
-    @mock.patch('reactive.graylog.os')
-    @mock.patch('reactive.graylog.host')
+    @mock.patch('charms.layer.graylog.snap_change.unitdata')  # Avoid unintended local state changes
+    @mock.patch('reactive.graylog.unitdata')                  # Same as above
+    @mock.patch('charms.layer.graylog.snap_change.data_changed')
+    @mock.patch('charms.layer.graylog.snap_change.shutil.copy2')
+    @mock.patch('charms.layer.graylog.snap_change.os')
+    @mock.patch('charms.layer.graylog.snap_change.host')
+    @mock.patch('charms.layer.graylog.snap_change.snap')
     @mock.patch('reactive.graylog.snap')
     @mock.patch('reactive.graylog.hookenv.config')
-    def test_change(self, mock_config, mock_snap, mock_host, mock_os, mock_shutil, mock_data_changed):
+    def test_change(self, mock_config, mock_snap, mock_snap2, mock_host, mock_os, mock_shutil, mock_data_changed, mock_ud, mock_ud2):
         """Ensure calls and states when the snap has changed."""
-        mock_config.return_value = 'foo'
-        mock_snap.get_installed_version.side_effect = ['2.foo', '3.foo']
+        mock_config.return_value = '2/stable'
+        mock_snap.get_installed_version.side_effect = mock_snap2.get_installed_version.side_effect = ['2.1', '2.2']
+        mock_snap.get_installed_channel.return_value = mock_snap2.get_installed_channel.return_value = '2/candidate'
         mock_os.path.exists.return_value = True
         mock_shutil.return_value = 'foo'
+        mock_ud.kv.return_value = mock_kv = mock.MagicMock()
+        mock_kv.get.return_value = None  # For update_status, mock that no channel_block_type has been set
         set_state('graylog.configured')
         refresh_graylog()
         self.assertTrue(mock_host.service_stop.called)
-        self.assertTrue(mock_snap.refresh.called)
+        self.assertTrue(mock_snap2.refresh.called)
         self.assertEqual(mock_shutil.call_count, 2)
         self.assertEqual(mock_data_changed.call_count, 2)
         self.assertFalse(is_state('graylog.configured'))
@@ -114,16 +131,19 @@ class TestConfig(unittest.TestCase):
         configure_graylog()
         self.assertTrue(mock_hookenv.status_set.called)
 
+    @mock.patch('reactive.graylog.get_channel_change_status')
     @mock.patch('reactive.graylog.is_master')
     @mock.patch('reactive.graylog.set_jvm_heap_size')
     @mock.patch('reactive.graylog.validate_api_uri')
+    @mock.patch('charms.layer.graylog.utils.is_v2')
     @mock.patch('reactive.graylog.is_v2')
     @mock.patch('reactive.graylog.set_conf')
     @mock.patch('reactive.graylog.hookenv')
     @mock.patch('reactive.graylog.os')
     @mock.patch('reactive.graylog.unitdata')
     def test_v2_config(self, mock_ud, mock_os, mock_hookenv, mock_set_conf, mock_v2,
-                       mock_validate_uri, mock_jvm_heap, mock_is_master):
+                       mock_v2_2, mock_validate_uri, mock_jvm_heap, mock_is_master,
+                       mock_gccs):
         """Ensure calls and states with various config values for v2."""
         class Config(dict):
             def __init__(self, *args, **kw):
@@ -136,8 +156,9 @@ class TestConfig(unittest.TestCase):
                                                    'web_listen_uri': None,
                                                    'rest_transport_uri': 'foo/api',
                                                    'web_endpoint_uri': 'bar/api'})
-        mock_v2.return_value = True
+        mock_v2.return_value = mock_v2_2.return_value = True
         mock_is_master.return_value = True
+        mock_gccs.return_value = ChannelChangeStatus.NO_CHANGE
         configure_graylog()
         self.assertEqual(mock_validate_uri.call_count, 2)
         self.assertTrue(mock_jvm_heap.called)
@@ -145,16 +166,19 @@ class TestConfig(unittest.TestCase):
         self.assertTrue(mock_hookenv.open_port.called)
         self.assertTrue(mock_is_master.called)
 
+    @mock.patch('reactive.graylog.get_channel_change_status')
     @mock.patch('reactive.graylog.is_master')
     @mock.patch('reactive.graylog.set_jvm_heap_size')
     @mock.patch('reactive.graylog.validate_api_uri')
+    @mock.patch('charms.layer.graylog.utils.is_v2')
     @mock.patch('reactive.graylog.is_v2')
     @mock.patch('reactive.graylog.set_conf')
     @mock.patch('reactive.graylog.hookenv')
     @mock.patch('reactive.graylog.os')
     @mock.patch('reactive.graylog.unitdata')
     def test_v3_config(self, mock_ud, mock_os, mock_hookenv, mock_set_conf, mock_v2,
-                       mock_validate_uri, mock_jvm_heap, mock_is_master):
+                       mock_v2_2, mock_validate_uri, mock_jvm_heap, mock_is_master,
+                       mock_gccs):
         """Ensure calls and states with various config values for v3."""
         class Config(dict):
             def __init__(self, *args, **kw):
@@ -167,8 +191,9 @@ class TestConfig(unittest.TestCase):
                                                    'web_listen_uri': None,
                                                    'rest_transport_uri': 'foo/api',
                                                    'web_endpoint_uri': 'bar/api'})
-        mock_v2.return_value = False
+        mock_v2.return_value = mock_v2_2.return_value = False
         mock_is_master.return_value = True
+        mock_gccs.return_value = ChannelChangeStatus.NO_CHANGE
         configure_graylog()
         self.assertEqual(mock_validate_uri.call_count, 2)
         self.assertTrue(mock_jvm_heap.called)
@@ -450,3 +475,19 @@ class TestSetJVMHeapSize(unittest.TestCase):
         set_jvm_heap_size('6g', self.conf_file)
         set_jvm_heap_size('8g', self.conf_file)
         self.assertTrue(self.conf_equals(etc_default_conf.format(heap_twice)))
+
+
+class TestIsChannelValid(unittest.TestCase):
+
+    def test_happy_path(self):
+        for track in ('2', '3'):
+            for risk in ('stable', 'candidate', 'beta', 'edge'):
+                channel = '{}/{}'.format(track, risk)
+                self.assertTrue(_is_channel_valid(channel))
+
+    def test_sad_path(self):
+        for channel in (
+            'fake/stable',  # invalid track
+            '2',            # track w/o risk
+        ):
+            self.assertFalse(_is_channel_valid(channel))
diff --git a/unit_tests/test_lib.py b/unit_tests/test_lib.py
index 565ffc1..b644280 100644
--- a/unit_tests/test_lib.py
+++ b/unit_tests/test_lib.py
@@ -11,26 +11,26 @@ from charms.layer.graylog import (
 class TestLibraryUtils(unittest.TestCase):
     """Test lib.charms.layer.graylog utils."""
 
-    @mock.patch('charms.layer.graylog.snap.get_installed_version')
-    def test_api(self, mock_snap):
+    @mock.patch('charms.layer.graylog.utils.is_v2')
+    def test_api(self, mock_v2):
         """Validate the api port/url match what we expect for v2 and v3."""
-        mock_snap.return_value = '2.foo'
+        mock_v2.return_value = True
         self.assertEqual(get_api_port(), '9001')
         self.assertEqual(get_api_url(), 'http://127.0.0.1:9001/api/')
 
-        mock_snap.return_value = '3.bar'
+        mock_v2.return_value = False
         self.assertEqual(get_api_port(), '9000')
         self.assertEqual(get_api_url(), 'http://127.0.0.1:9000/api/')
 
-    @mock.patch('charms.layer.graylog.snap.get_installed_version')
-    def test_validate_api_url(self, mock_snap):
+    @mock.patch('charms.layer.graylog.utils.is_v2')
+    def test_validate_api_url(self, mock_v2):
         """Validate the URI suffix is valid for v2 and v3."""
-        mock_snap.return_value = '2.foo'
+        mock_v2.return_value = True
         self.assertEqual(validate_api_uri('foo'), 'foo/api/')
         self.assertEqual(validate_api_uri('foo/'), 'foo/api/')
         self.assertEqual(validate_api_uri('foo/api'), 'foo/api/')
 
-        mock_snap.return_value = '3.bar'
+        mock_v2.return_value = False
         self.assertEqual(validate_api_uri('foo'), 'foo/')
         self.assertEqual(validate_api_uri('foo/'), 'foo/')
         self.assertEqual(validate_api_uri('foo/api/'), 'foo/')
diff --git a/unit_tests/test_logextract.py b/unit_tests/test_logextract.py
index 5bae816..8bfd61b 100644
--- a/unit_tests/test_logextract.py
+++ b/unit_tests/test_logextract.py
@@ -44,16 +44,20 @@ testdata_stream_connect = {
 
 class TestGraylogPipelines(unittest.TestCase):
 
+    @mock.patch('charms.layer.graylog.logextract.is_v2')
     @mock.patch('charms.layer.graylog.api.GraylogApi')
-    def test_get_pipeline_for(self, mock_api):
+    def test_get_pipeline_for(self, mock_api, mock_v2):
         mock_api.request.return_value = testdata_simple_pipeline
+        mock_v2.return_value = False
         gpipes = GraylogPipelines(mock_api)
         test = gpipes.get_for_title('juju-logextract-pipeline')
         self.assertEqual(test['title'], 'juju-logextract-pipeline')
 
+    @mock.patch('charms.layer.graylog.logextract.is_v2')
     @mock.patch('charms.layer.graylog.api.GraylogApi')
-    def test_set_pipeline(self, mock_api):
+    def test_set_pipeline(self, mock_api, mock_v2):
         mock_api.request.return_value = testdata_simple_pipeline
+        mock_v2.return_value = False
         gpipes = GraylogPipelines(mock_api)
         gpipes.set('juju-logextract-pipeline', testdata_simple_pipeline_source)
         self.assertEqual(mock_api.request.call_args_list[1][1]['data']['title'], 'juju-logextract-pipeline')
@@ -61,9 +65,11 @@ class TestGraylogPipelines(unittest.TestCase):
 
 class TestGraylogRules(unittest.TestCase):
 
+    @mock.patch('charms.layer.graylog.logextract.is_v2')
     @mock.patch('charms.layer.graylog.api.GraylogApi')
-    def test_get_rules(self, mock_api):
+    def test_get_rules(self, mock_api, mock_v2):
         mock_api.request.return_value = testdata_rules
+        mock_v2.return_value = False
         grules = GraylogRules(mock_api)
         rules = grules.get()
         self.assertEqual(rules[0]['title'], "from ceph mon")
@@ -92,8 +98,10 @@ class TestLogExtractPipeline(unittest.TestCase):
         t = logext.parse_rule_title(testdata_rules[0]['source'])
         self.assertEqual(t, 'from ceph mon')
 
+    @mock.patch('charms.layer.graylog.logextract.is_v2')
     @mock.patch('charms.layer.graylog.api.GraylogApi')
-    def test_set_rules(self, mock_api):
+    def test_set_rules(self, mock_api, mock_v2):
+        mock_v2.return_value = True
         logext = LogExtractPipeline(mock_api)
         logext.set_rules([testdata_rules[0]['source']])
         exp = [mock.call('/plugins/org.graylog.plugins.pipelineprocessor/system/pipelines/pipeline')]