Merge ~lcvcode/charm-nagios:LP1939068-duplicates into charm-nagios:master

[Connor Chamberlain]

Force uniqueness in cases of host_name duplication

This change ensures that overlapping host names in nagios are made unique so that parent-child relationships are never ambiguous.

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

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

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

Unable to determine commit message from repository - please click "Set commit message" and enter the commit message manually.

[🤖 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:b4f0578daeeb92bdbcd0d51cc21262822a023cfc

No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want jenkins to rebuild you need to trigger it yourself):
https://code.launchpad.net/~lcvcode/charm-nagios/+git/charm-nagios/+merge/406873/+edit-commit-message

https://jenkins.canonical.com/bootstack/job/lp-charm-nagios-ci/6/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/42/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/407/

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

review: Needs Fixing (continuous-integration)

[Drew Freiberger (afreiberger)]

Please see comments inline.

review: Needs Fixing

[Zachary Zehring (zzehring)]

Added some comments.

review: Needs Fixing

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

PASSED: Continuous integration, rev:b4f0578daeeb92bdbcd0d51cc21262822a023cfc
https://jenkins.canonical.com/bootstack/job/lp-charm-nagios-ci/7/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/46/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/414/

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

review: Approve (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:f85717778d5c6da2fd2e846907e2014e71de321a
https://jenkins.canonical.com/bootstack/job/lp-charm-nagios-ci/8/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/48/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/416/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-nagios-ci/8//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)]

PASSED: Continuous integration, rev:8c6d53f56e417db4739bb4ce3d77c9b596d4d0e5
https://jenkins.canonical.com/bootstack/job/lp-charm-nagios-ci/10/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/50/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/418/

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

review: Approve (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)]

PASSED: Continuous integration, rev:137f7a67d44e28e6bac9a0ff70244e2e480a701b
https://jenkins.canonical.com/bootstack/job/lp-charm-nagios-ci/11/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/52/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/420/

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

review: Approve (continuous-integration)

[Paul Goins (vultaire)]

I'm +1 for the change as it fills a gap in the case that nagios_host_context is specified with the same value on multiple models.

However, I believe it is superior to use distinct nagios_host_contexts for each model, and to include something in the host context to identify the model from which the record came.

For example, if you have keystone deployed to both an OpenStack and K8s model; and if hostnames are autogenerated as e.g. keystone-0, keystone-1, etc.; then including the model name in the host context would generate hostnames like e.g. <prefix>-openstack-keystone-0, <prefix>-kubernetes-keystone-0. That'd give more semantic meaning to the hostnames and would have less problems re: event history in cases things get added/removed.

review: Approve

[Paul Goins (vultaire)]

During a 1:1, I remembered: I had concerns over how the status was being set here, and whether it might be being clobbered elsewhere in the charm. I very much support this MR as a stopgap to avoid having services clobber the wrong host in Nagios, especially if we have a message indicating that duplicate hostnames have been found, as that may guide toward reconfiguring the nagios_host_context to something more appropriate to avoid the collisions.

Temporarily marking as abstain; intending to check this tomorrow.

review: Abstain

[Paul Goins (vultaire)]

status_set is only really called in 2 places - one on charm upgrade, the other in the main() used by basically all other hooks. With that in mind, this is probably good enough in my opinion. I'm +1.

review: Approve

[Paul Goins (vultaire)]

Just FYI: added a warning log message as part of a new MR which builds upon this one as a prerequisite: https://code.launchpad.net/~vultaire/charm-nagios/+git/charm-nagios/+merge/407277

To be clear: this MR still needs one more +1 before merge.

[Paul Goins (vultaire)]

Sorry for the thrash here.

I've tried to deploy this on serverstack, with multiple related models and nrpe related to apps on top-level nodes and to LXDs, and I'm seeing these issues:

* For a hostname collision at the parent level, I do see that I get records for both hosts; one has a suffix appended. However, the host group names are not consistent. For example, host juju-host-0 is associated with host group juju-host, while host juju-host-0-[1] is associated with host group juju-host-0. This is potentially confusing, especially since the latter host group matches the former host which isn't even part of that host group.

* If I have an app of the same name installed onto containers in two different models, they collide and I only see one in nagios.

This is with the following deployment: https://pastebin.ubuntu.com/p/26hb2ptYHt/
(Summary: nagios on one model, plus 2 other models, each with cs:ubuntu deployed as host and LXD, with nrpe related to each.)

This was done on serverstack (i.e. OpenStack), not a MAAS or LXD based controller.

I'm not sure what exactly in the code needs fixing yet, but I'm -1'ing for now.

review: Needs Fixing

diff --git a/hooks/monitors_relation_changed.py b/hooks/monitors_relation_changed.py
index 9297cdf..441fe4e 100755
--- a/hooks/monitors_relation_changed.py
+++ b/hooks/monitors_relation_changed.py
@@ -28,6 +28,7 @@ from charmhelpers.core.hookenv import (
     related_units,
     relation_get,
     relation_ids,
+    status_set,
 )
 
 from common import (
@@ -41,8 +42,10 @@ from common import (
 
 import yaml
 
-REQUIRED_REL_DATA_KEYS = ["target-address", "monitors", "target-id"]
+MACHINE_ID_KEY = "machine_id"
 MODEL_ID_KEY = "model_id"
+TARGET_ID_KEY = "target-id"
+REQUIRED_REL_DATA_KEYS = ["target-address", "monitors", TARGET_ID_KEY]
 
 
 def _prepare_relation_data(unit, rid):
@@ -58,7 +61,7 @@ def _prepare_relation_data(unit, rid):
 
     if rid.split(":")[0] == "nagios":
         # Fake it for the more generic 'nagios' relation
-        relation_data["target-id"] = unit.replace("/", "-")
+        relation_data[TARGET_ID_KEY] = unit.replace("/", "-")
         relation_data["monitors"] = {"monitors": {"remote": {}}}
 
     if not relation_data.get("target-address"):
@@ -101,7 +104,7 @@ def main(argv):  # noqa: C901
     #
 
     if len(argv) > 1:
-        relation_settings = {"monitors": open(argv[1]).read(), "target-id": argv[2]}
+        relation_settings = {"monitors": open(argv[1]).read(), TARGET_ID_KEY: argv[2]}
 
         if len(argv) > 3:
             relation_settings["target-address"] = argv[3]
@@ -114,13 +117,13 @@ def main(argv):  # noqa: C901
 
     for relid, units in all_relations.iteritems():
         for unit, relation_settings in units.items():
-            if "target-id" in relation_settings:
-                targets_with_addresses.add(relation_settings["target-id"])
+            if TARGET_ID_KEY in relation_settings:
+                targets_with_addresses.add(relation_settings.get(TARGET_ID_KEY))
     new_all_relations = {}
 
     for relid, units in all_relations.iteritems():
         for unit, relation_settings in units.items():
-            if relation_settings["target-id"] in targets_with_addresses:
+            if relation_settings.get(TARGET_ID_KEY) in targets_with_addresses:
                 if relid not in new_all_relations:
                     new_all_relations[relid] = {}
                 new_all_relations[relid][unit] = relation_settings
@@ -129,23 +132,55 @@ def main(argv):  # noqa: C901
 
     initialize_inprogress_config()
 
+    uniq_hostnames = set()
+    duplicate_hostnames = defaultdict(int)
+
+    def record_hostname(hostname):
+        if hostname not in uniq_hostnames:
+            uniq_hostnames.add(hostname)
+        else:
+            duplicate_hostnames[hostname] += 1
+
     # make a dict of machine ids to target-id hostnames
     all_hosts = {}
     for relid, units in all_relations.items():
         for unit, relation_settings in units.iteritems():
-            machine_id = relation_settings.get("machine_id", None)
-            model_id = relation_settings.get(MODEL_ID_KEY, None)
+            machine_id = relation_settings.get(MACHINE_ID_KEY)
+            model_id = relation_settings.get(MODEL_ID_KEY)
+            target_id = relation_settings.get(TARGET_ID_KEY)
 
-            if not machine_id:
+            if not machine_id or not target_id:
                 continue
 
+            # Check for duplicate hostnames and amend them if needed
+            record_hostname(target_id)
+            if target_id in duplicate_hostnames:
+                # Duplicate hostname has been found
+                # Append "-[<number of duplicates>]" hostname
+                # Example:
+                #   1st hostname of "juju-ubuntu-0" is unchanged
+                #   2nd hostname of "juju-ubuntu-0" is changed to "juju-ubuntu-0-[1]"
+                #   3rd hostname of "juju-ubuntu-0" is changed to "juju-ubuntu-0-[2]"
+                target_id += "-[{}]".format(duplicate_hostnames[target_id])
+                relation_settings[TARGET_ID_KEY] = target_id
+
             # Backwards compatible hostname from machine id
             if not model_id:
-                all_hosts[machine_id] = relation_settings["target-id"]
+                all_hosts[machine_id] = target_id
             # New hostname from machine id using model id
             else:
                 all_hosts.setdefault(model_id, {})
-                all_hosts[model_id][machine_id] = relation_settings["target-id"]
+                all_hosts[model_id][machine_id] = target_id
+
+    if duplicate_hostnames:
+        status_set(
+            "active",
+            "Duplicate host names detected: {}".format(
+                ", ".join(duplicate_hostnames.keys())
+            ),
+        )
+    else:
+        status_set("active", "ready")
 
     for relid, units in all_relations.items():
         apply_relation_config(relid, units, all_hosts)
@@ -158,11 +193,11 @@ def main(argv):  # noqa: C901
 def apply_relation_config(relid, units, all_hosts):  # noqa: C901
     for unit, relation_settings in units.iteritems():
         monitors = relation_settings["monitors"]
-        target_id = relation_settings["target-id"]
-        machine_id = relation_settings.get("machine_id", None)
+        target_id = relation_settings[TARGET_ID_KEY]
+        machine_id = relation_settings.get(MACHINE_ID_KEY)
         parent_host = None
 
-        model_id = relation_settings.get(MODEL_ID_KEY, None)
+        model_id = relation_settings.get(MODEL_ID_KEY)
 
         if machine_id:
 
@@ -173,7 +208,7 @@ def apply_relation_config(relid, units, all_hosts):  # noqa: C901
                 # Get hostname using model id
                 if model_id:
                     model_hosts = all_hosts.get(model_id, {})
-                    parent_host = model_hosts.get(parent_machine, None)
+                    parent_host = model_hosts.get(parent_machine)
 
                 # Get hostname without model id
                 # this conserves backwards compatibility with older
@@ -184,7 +219,7 @@ def apply_relation_config(relid, units, all_hosts):  # noqa: C901
         # If not set, we don't mess with it, as multiple services may feed
         # monitors in for a particular address. Generally a primary will set
         # this to its own private-address
-        target_address = relation_settings.get("target-address", None)
+        target_address = relation_settings.get("target-address")
 
         if type(monitors) != dict:
             monitors = yaml.safe_load(monitors)