Merge ~vultaire/charm-nagios:remove-legacy-deduping into charm-nagios:master

[Paul Goins]

Removed fallback deduping methodology

On reflection, I don't believe this provided much benefit. The needed
updates to charm-nrpe were provided at nearly the same time as
charm-nagios to support the main deduping methodology. Rather than
maintaining 2 methods, we should probably maintain just one.

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

A CI job is currently in progress. A follow up comment will be added when it completes.

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

FAILED: Continuous integration, rev:038f963bb2079c33cf10369bf59f86bef5806648
https://jenkins.canonical.com/bootstack/job/lp-charm-nagios-ci/22/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/89/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/460/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-nagios-ci/22//rebuild

review: Needs Fixing (continuous-integration)

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

A CI job is currently in progress. A follow up comment will be added when it completes.

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

FAILED: Continuous integration, rev:fcba65692893be55f8115c9a252f1231f2c8f68b
https://jenkins.canonical.com/bootstack/job/lp-charm-nagios-ci/23/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/90/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/461/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-nagios-ci/23//rebuild

review: Needs Fixing (continuous-integration)

[Paul Goins (vultaire)]

Tested in my test environment where I had both "legacy" and model UUID SHA-based prefixes in play.

Upon charm upgrade, the config got rewritten, with the hosts with the "legacy" prefix dropping the prefix and instead simply using the original hostname. The hosts with the model-id present on the relation are continuing to use the prefix. In other words, this is working exactly as I expect.

The test failures here were some sort of CI timeout; I've kicked off a rebuild.

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

FAILED: Continuous integration, rev:fcba65692893be55f8115c9a252f1231f2c8f68b
https://jenkins.canonical.com/bootstack/job/lp-charm-nagios-ci/25/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/92/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/463/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-nagios-ci/25//rebuild

review: Needs Fixing (continuous-integration)

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

FAILED: Continuous integration, rev:fcba65692893be55f8115c9a252f1231f2c8f68b
https://jenkins.canonical.com/bootstack/job/lp-charm-nagios-ci/26/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/93/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/464/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-nagios-ci/26//rebuild

review: Needs Fixing (continuous-integration)

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

PASSED: Continuous integration, rev:fcba65692893be55f8115c9a252f1231f2c8f68b
https://jenkins.canonical.com/bootstack/job/lp-charm-nagios-ci/27/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/94/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/465/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-nagios-ci/27//rebuild

review: Approve (continuous-integration)

[Xav Paice (xavpaice)]

LGTM, tests OK

review: Approve

[Xav Paice (xavpaice)]

With this change, I've got two bundles in two models, and a cross model relation, see https://pastebin.canonical.com/p/vZ26FkpRK6/ for the detail.

The Nagios host is now monitoring rabbit and ubuntu on one host, machine 1 in the 'nagios' model, and because I'm using unit for the hostname type, I see two machines defined in Nagios. That's expected, I'd have to set `nagios_hostname_type=host` in nrpe to fix that.

The Nagios host also monitors two machines, separate, named `juju-ubuntu-0`. I only see one in the Nagios host definitions, which is from the model 'notnagios'. The unit `rabbitmq-server-0` covers monitoring for the machine in the `nagios` model (I'm regretting these names now).

I don't know, at this stage, if the host names should have a prefix of some description, and when I look at host `juju-ubuntu-0` I don't know which it is unless I look at it's IP address.

When running this same test from HEAD at 29dce00c69f68ac3d1e14af2e450e96ca4603021 (current master), I get a list of hosts including all and the duplicates have a relation prefix to split them out.

review: Needs Fixing

[Paul Goins (vultaire)]

The intent here is to eliminate the "fallback" deduping strategy.

Based on the model from your pastebin, the NRPE apps in use are from the present nrpe release (cs:nrpe rather than the llama-charmers-next version), so they lack the model-id field in their relation data. After this change, I would expect that only one shows up, because they would not be de-duped. This is intentional as I wish to remove this particular fallback methodology for the sake of having less code to maintain; its benefit in being left in the code is really quite minimal.

If you do a "juju upgrade-charm -m <model> nrpe --switch cs:~llama-charmers-next/nrpe" on each of the nrpe apps in your model, you'll see that the deduplication logic will work, since that version of charm-nrpe has the needed model-id in the relation data.

[Xav Paice (xavpaice)]

Discussed this change on Mattermost. The change requires changes in NRPE that are not yet released, but will be included in 21.10. Tested again with that NRPE, and this change works well.

review: Approve

[Garrett Neugent (thogarre)]

LGTM as well.

review: Approve

diff --git a/hooks/common.py b/hooks/common.py
index f4e5443..adc77b7 100644
--- a/hooks/common.py
+++ b/hooks/common.py
@@ -14,8 +14,6 @@ from charmhelpers.core.hookenv import (
     network_get,
     network_get_primary_address,
     relation_get,
-    relation_id,
-    remote_unit,
     unit_get,
 )
 
@@ -502,17 +500,12 @@ def _get_minimal_related_config_paths():
         # duplicate host names from different models.
         # Case 1: Direct match
         paths_to_remove.append(template.format(target_name))
-        # Case 2: Deduped match, main algorithm, glob on first 7 chars of sha256sum
+        # Case 2: Deduped match, glob on first 7 chars of sha256sum
         if MODEL_ID_KEY in relation_data:
             model_id = relation_data[MODEL_ID_KEY]
             sha_prefix = get_model_id_sha(model_id)[:HOST_PREFIX_MIN_LENGTH]
             glob_pattern = template.format("{}*_{}".format(sha_prefix, target_name))
             paths_to_remove.extend(glob.glob(glob_pattern))
-        # Case 3: Deduped match, fallback algorithm
-        relid_unit_prefix = get_relid_unit_prefix(relation_id(), remote_unit())
-        paths_to_remove.append(
-            template.format("{}_{}".format(relid_unit_prefix, target_name))
-        )
     return paths_to_remove
 
 
diff --git a/hooks/monitors_relation_changed.py b/hooks/monitors_relation_changed.py
index a6f66b2..2a201c6 100755
--- a/hooks/monitors_relation_changed.py
+++ b/hooks/monitors_relation_changed.py
@@ -48,7 +48,6 @@ from common import (
     get_nagios_host_config_path,
     get_pynag_host,
     get_pynag_service,
-    get_relid_unit_prefix,
     initialize_inprogress_config,
     refresh_hostgroups,
 )
@@ -92,11 +91,6 @@ def _prepare_relation_data(unit, rid):
 
             return {}
 
-    relation_data["metadata"] = {
-        "unit": unit,
-        "rid": rid,
-    }
-
     return relation_data
 
 
@@ -168,12 +162,9 @@ def main(argv, full_rewrite=False):  # noqa: C901
     for target_id, relation_settings_list in hosts_to_settings.items():
         for relation_settings in relation_settings_list:
             model_id = relation_settings.get(MODEL_ID_KEY)
-            if len(relation_settings_list) > 1:
+            if model_id and len(relation_settings_list) > 1:
                 duplicate_hostnames.add(target_id)
-                if model_id:
-                    unique_prefix = host_prefixes[model_id]
-                else:
-                    unique_prefix = compute_fallback_host_prefix(relation_settings)
+                unique_prefix = host_prefixes[model_id]
                 relation_settings[TARGET_ID_KEY] = "{}_{}".format(
                     unique_prefix, target_id
                 )
@@ -263,18 +254,6 @@ def compute_host_prefixes(model_ids):
     return result
 
 
-def compute_fallback_host_prefix(relation_settings):
-    """Compute short unique identifiers, fallback method.
-
-    This method uses the relation ID (e.g. monitors:1), in conjunction with the remote
-    unit ID as seen via the relation (e.g. app/1 or
-    remote-0123456789abcdef0123456789abcdef/1), to create a unique identifier.
-    """
-    return get_relid_unit_prefix(
-        relation_settings["metadata"]["rid"], relation_settings["metadata"]["unit"]
-    )
-
-
 def apply_relation_config(relid, units, all_hosts):  # noqa: C901
     for unit, relation_settings in units.iteritems():
         target_id = relation_settings[TARGET_ID_KEY]
diff --git a/tests/unit/test_common.py b/tests/unit/test_common.py
index 91bb4cf..c386865 100644
--- a/tests/unit/test_common.py
+++ b/tests/unit/test_common.py
@@ -57,11 +57,9 @@ class TestInitializeInprogressConfigFileCleanup:
             rget_mock.return_value = None
             self._run_test(existing_files, expected_files_to_delete)
 
-    @patch("common.remote_unit")
-    @patch("common.relation_id")
     @patch("common.get_model_id_sha")
     @patch("common.relation_get")
-    def test_with_related_unit(self, rget_mock, sha_mock, rid_mock, runit_mock, tmpdir):
+    def test_with_related_unit(self, rget_mock, sha_mock, tmpdir):
         """Related unit case.
 
         If there is a related unit, we should remove:
@@ -82,36 +80,26 @@ class TestInitializeInprogressConfigFileCleanup:
                 common.OLD_CHARM_CFG,
                 common.HOST_TEMPLATE.format("host-1"),
                 common.HOST_TEMPLATE.format("host-2"),
-                common.HOST_TEMPLATE.format("monitors:42_7_host-2"),
                 common.HOST_TEMPLATE.format("fakesha_host-2"),
                 common.HOST_TEMPLATE.format("host-3"),
-                common.HOST_TEMPLATE.format("monitors:42_8_host-3"),
                 common.HOST_TEMPLATE.format("fakesha_host-3"),
                 common.HOSTGROUP_TEMPLATE.format("host"),
-                common.HOSTGROUP_TEMPLATE.format("monitors:42_7_host"),
-                common.HOSTGROUP_TEMPLATE.format("monitors:42_8_host"),
                 common.HOSTGROUP_TEMPLATE.format("fakesha_host"),
             ]
             expected_files_to_delete = [
                 common.OLD_CHARM_CFG,
                 common.HOST_TEMPLATE.format("host-2"),
-                common.HOST_TEMPLATE.format("monitors:42_7_host-2"),
                 common.HOST_TEMPLATE.format("fakesha_host-2"),
                 common.HOSTGROUP_TEMPLATE.format("host"),
-                common.HOSTGROUP_TEMPLATE.format("monitors:42_7_host"),
                 common.HOSTGROUP_TEMPLATE.format("fakesha_host"),
             ]
             fake_sha = "fakesha"
-            fake_relation_id = "monitors:42"
-            fake_remote_unit = "foo/7"
 
             rget_mock.return_value = {
                 common.TARGET_ID_KEY: related_unit_hostname,
                 common.MODEL_ID_KEY: "fake",
             }
             sha_mock.return_value = fake_sha
-            rid_mock.return_value = fake_relation_id
-            runit_mock.return_value = fake_remote_unit
             self._run_test(existing_files, expected_files_to_delete)
 
     def _run_test(self, existing_files, expected_files_to_delete):
diff --git a/tests/unit/test_monitor_relation_changed.py b/tests/unit/test_monitor_relation_changed.py
index 09c0bc1..8578af2 100644
--- a/tests/unit/test_monitor_relation_changed.py
+++ b/tests/unit/test_monitor_relation_changed.py
@@ -72,18 +72,6 @@ class TestComputeHostPrefixes:
         assert prefixes == expected_result
 
 
-def test_compute_fallback_host_prefix():
-    prefix = monitors_relation_changed.compute_fallback_host_prefix(
-        {
-            "metadata": {
-                "rid": "monitors:1",
-                "unit": "remote-0123456789abcdef0123456789abcdef/1",
-            }
-        }
-    )
-    assert prefix == "monitors:1_1"
-
-
 class TestCleanupLeftoverHosts:
     def test_cleanup_leftover_hosts(self, tmpdir):
         mock_host_template = "{}/juju-host_{{}}.cfg".format(tmpdir)
@@ -94,11 +82,9 @@ class TestCleanupLeftoverHosts:
         ) as _2:  # noqa: F841
             existing_files = [
                 monitors_relation_changed.HOST_TEMPLATE.format("host-2"),
-                monitors_relation_changed.HOST_TEMPLATE.format("monitors:42_7_host-2"),
                 monitors_relation_changed.HOST_TEMPLATE.format("fakesha_host-2"),
             ]
             expected_files_to_delete = [
-                monitors_relation_changed.HOST_TEMPLATE.format("monitors:42_7_host-2"),
                 monitors_relation_changed.HOST_TEMPLATE.format("fakesha_host-2"),
             ]