MongoDB Charm

Merge ~ballot/charm-mongodb/+git/charm-mongodb:LP#1930552 into charm-mongodb:master

  1. Git
  2. lp:~ballot/charm-mongodb/+git/charm-mongodb
  3. LP#1930552
  4. Merge into master
Proposed by Xav Paice
Status: Merged
Approved by: James Troup
Approved revision: f1defcaa2db1099810a1aeab95dafd49ee52338a
Merged at revision: c7ffb22bef99854788f315a61dd424a2f29fc6dc
Proposed branch: ~ballot/charm-mongodb/+git/charm-mongodb:LP#1930552
Merge into: charm-mongodb:master
Prerequisite: ~ballot/charm-mongodb/+git/charm-mongodb:pytest
Diff against target: 464 lines (+230/-56)
3 files modified
files/nrpe-external-master/check_replica_sets.py (+65/-16)
tests/unit/test_check_replica_sets.py (+164/-39)
tox.ini (+1/-1)
Reviewer Review Type Date Requested Status
Linda Guo (community) Approve
Xav Paice (community) Approve
🤖 prod-jenkaas-bootstack (community) continuous-integration Approve
BootStack Reviewers Pending
Review via email: mp+415001@code.launchpad.net

This proposal supersedes a proposal from 2022-01-27.

Commit message

Add host and port in the replicaset check LP#1930552

The check considers that the replicaset name is the mongodb host to
check.

That is not a valid assumption, so let's parse the configuration file
and determine what it should be

To post a comment you must log in.
Revision history for this message
🤖 Canonical IS Merge Bot (canonical-is-mergebot) wrote : Posted in a previous version of this proposal

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

Revision history for this message
🤖 Canonical IS Merge Bot (canonical-is-mergebot) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Xav Paice (xavpaice) wrote :

Resubmitted proposal to try to get CI to pick it up.

Revision history for this message
🤖 Canonical IS Merge Bot (canonical-is-mergebot) wrote :

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

Revision history for this message
🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote :

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

Revision history for this message
🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote :

FAILED: Continuous integration, rev:74f45e3f8f24ed377b8ff3bf771fd8d3b1b8aaa3
https://jenkins.canonical.com/bootstack/job/lp-charm-mongodb-ci/44/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/458/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/901/

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

review: Needs Fixing (continuous-integration)
Revision history for this message
🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote :

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

Revision history for this message
🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote :

PASSED: Continuous integration, rev:f1defcaa2db1099810a1aeab95dafd49ee52338a
https://jenkins.canonical.com/bootstack/job/lp-charm-mongodb-ci/45/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/473/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/924/

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

review: Approve (continuous-integration)
Revision history for this message
Xav Paice (xavpaice) wrote :

Thanks for the work on this change!

LGTM, needs one more review from our team.

review: Approve
Revision history for this message
Linda Guo (lihuiguo) wrote :

LGTM

review: Approve
Revision history for this message
🤖 Canonical IS Merge Bot (canonical-is-mergebot) wrote :

Change successfully merged at revision c7ffb22bef99854788f315a61dd424a2f29fc6dc

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/files/nrpe-external-master/check_replica_sets.py b/files/nrpe-external-master/check_replica_sets.py
2index f6d8feb..17369e8 100755
3--- a/files/nrpe-external-master/check_replica_sets.py
4+++ b/files/nrpe-external-master/check_replica_sets.py
5@@ -7,6 +7,7 @@
6 # Author: David O Neill <david.o.neill@canonical.com>
7 #
8
9+import configparser
10 import json
11 import subprocess
12 import sys
13@@ -23,6 +24,9 @@ REPL_STATES_OK = ["PRIMARY", "SECONDARY", "ARBITER"]
14 # REPL_STATES_WARN = ["STARTUP", "STARTUP2", "ROLLBACK", "RECOVERING"]
15 REPL_STATES_CRIT = ["UNKNOWN", "DOWN", "REMOVED"]
16
17+DEFAULT_BIND = "localhost"
18+DEFAULT_PORT = 27017
19+
20
21 def get_cmd_result(cmd):
22 try:
23@@ -32,10 +36,53 @@ def get_cmd_result(cmd):
24 sys.exit(NAGIOS_CRIT)
25
26
27-def get_replica_set_name():
28+def get_replica_data_ini_conf(configuration: str) -> tuple:
29+ """Return the host, port and replica set name from old conf format.
30+
31+ :param configuration: MongoDB configuration file.
32+ :returns: A tuple with host, port and replicaset name.
33+ """
34+ try:
35+ conf = configparser.ConfigParser(allow_no_value=True)
36+ print(configuration)
37+ conf.read_string("[main]\n{}".format(configuration))
38+ except configparser.ParsingError:
39+ print("Wrong MongoDB configuration.")
40+ return DEFAULT_BIND, DEFAULT_PORT, False
41+ # prepend a fake section "[main]" at the top of the file to use configParser()
42+ host = conf.get("main", "bind_ip", fallback=DEFAULT_BIND)
43+ port = conf.getint("main", "port", fallback=DEFAULT_PORT)
44+ replset = conf.get("main", "replSet", fallback=False)
45+
46+ return host, port, replset
47+
48+
49+def get_replica_data_yaml_conf(
50+ configuration: dict, host=DEFAULT_BIND, port=DEFAULT_PORT
51+) -> tuple:
52+ """Return the host, port and replica set name from old conf format.
53+
54+ :param configuration: MongoDB configuration file as a dict.
55+ :returns: A tuple with host, port and replicaset name.
56+ """
57+ # Host and port can be avoided considering default value can be used
58+ if "net" in configuration:
59+ host = configuration["net"].get("bindIp", DEFAULT_BIND)
60+ port = int(configuration["net"].get("port", DEFAULT_PORT))
61+ # Fail hard if replicaset name is absent
62+ try:
63+ replset = configuration["replication"]["replSetName"]
64+ except KeyError:
65+ replset = False
66+
67+ return host, port, replset
68+
69+
70+def get_replica_data():
71 # mongo2 file format the key is 'replSet'
72 # mongo34 yaml file format the key is 'replSetName'
73 # this will match both
74+ replset = False
75
76 with open("/etc/mongodb.conf", "r") as mongo_file:
77 data = mongo_file.read()
78@@ -45,19 +92,15 @@ def get_replica_set_name():
79 config = None
80
81 if isinstance(config, dict):
82- if "replication" not in config:
83- return False
84- if "replSetName" not in config["replication"]:
85- return False
86- return config["replication"]["replSetName"]
87+ host, port, replset = get_replica_data_yaml_conf(config)
88+ else: # Old configuration format
89+ host, port, replset = get_replica_data_ini_conf(data)
90
91- for line in data.splitlines():
92- if line.strip().startswith("replSet") is False:
93- continue
94- parts = line.split("=")
95- return parts[1].strip()
96+ # Translate 0.0.0.0 to localhost for connection purpose
97+ if host == "0.0.0.0":
98+ host = "localhost"
99
100- return False
101+ return host, port, replset
102
103
104 def get_replica_status():
105@@ -74,13 +117,19 @@ def get_replica_status():
106 # ]
107 # }
108
109- name = get_replica_set_name()
110- if not name:
111- print("CRITIAL: unable to determine the replica set name")
112+ host, port, name = get_replica_data()
113+ if not name or not host or not port:
114+ print("CRITICAL: unable to determine the host, port and replica set name")
115 sys.exit(NAGIOS_CRIT)
116
117 mongo_res = get_cmd_result(
118- ["/usr/bin/mongo", name, "--quiet", "--eval", "JSON.stringify(rs.status())"]
119+ [
120+ "/usr/bin/mongo",
121+ "{}:{}".format(host, port),
122+ "--quiet",
123+ "--eval",
124+ "JSON.stringify(rs.status('{}'))".format(name),
125+ ]
126 )
127
128 try:
129diff --git a/tests/unit/test_check_replica_sets.py b/tests/unit/test_check_replica_sets.py
130index 5eb9b6e..41be9d1 100644
131--- a/tests/unit/test_check_replica_sets.py
132+++ b/tests/unit/test_check_replica_sets.py
133@@ -2,7 +2,13 @@
134 import unittest
135 import unittest.mock as mock
136
137-from check_replica_sets import get_cmd_result, get_replica_set_name, get_replica_status
138+from check_replica_sets import (
139+ get_cmd_result,
140+ get_replica_data,
141+ get_replica_data_ini_conf,
142+ get_replica_data_yaml_conf,
143+ get_replica_status,
144+)
145
146 import yaml
147
148@@ -34,7 +40,7 @@ class TestCheckReplicaSets(unittest.TestCase):
149 except Exception:
150 self.assertEqual(fake_std_out.getvalue(), result)
151
152- def test_get_replica_set_name_yaml_replsetname(self):
153+ def test_get_replica_data_yaml_replsetname(self):
154 """Test mongodb.conf file parsing replSetName success."""
155 sample = {}
156 sample["replication"] = {}
157@@ -44,11 +50,33 @@ class TestCheckReplicaSets(unittest.TestCase):
158 with mock.patch(
159 "builtins.open", mock.mock_open(read_data=sample_yaml)
160 ) as mock_file:
161- res = get_replica_set_name()
162+ host, port, res = get_replica_data()
163 mock_file.assert_called_with("/etc/mongodb.conf", "r")
164+ self.assertEqual(host, "localhost")
165+ self.assertEqual(port, 27017)
166 self.assertEqual(res, "test")
167
168- def test_get_replica_set_name_yaml_no_replsetname(self):
169+ def test_get_replica_data_yaml_bind_and_port(self):
170+ """Test mongodb.conf file parsing replSetName success."""
171+ sample = {}
172+ sample["replication"] = {}
173+ sample["replication"]["replSetName"] = "test"
174+ sample["net"] = {
175+ "bindIp": "0.0.0.0",
176+ "port": 12345,
177+ }
178+ sample_yaml = yaml.dump(sample)
179+
180+ with mock.patch(
181+ "builtins.open", mock.mock_open(read_data=sample_yaml)
182+ ) as mock_file:
183+ host, port, res = get_replica_data()
184+ mock_file.assert_called_with("/etc/mongodb.conf", "r")
185+ self.assertEqual(host, "localhost")
186+ self.assertEqual(port, 12345)
187+ self.assertEqual(res, "test")
188+
189+ def test_get_replica_data_yaml_no_replsetname(self):
190 """Test mongodb.conf file parsing fail, where there's no replSetName."""
191 sample = {}
192 sample["replication"] = {}
193@@ -57,11 +85,13 @@ class TestCheckReplicaSets(unittest.TestCase):
194 with mock.patch(
195 "builtins.open", mock.mock_open(read_data=sample_yaml)
196 ) as mock_file:
197- res = get_replica_set_name()
198+ host, port, res = get_replica_data()
199 mock_file.assert_called_with("/etc/mongodb.conf", "r")
200+ self.assertEqual(host, "localhost")
201+ self.assertEqual(port, 27017)
202 self.assertEqual(res, False)
203
204- def test_get_replica_set_name_yaml_no_replication(self):
205+ def test_get_replica_data_yaml_no_replication(self):
206 """Test mongodb.conf file parsing fail, with no replication block."""
207 sample = {}
208 sample["somethingelse"] = {}
209@@ -70,38 +100,92 @@ class TestCheckReplicaSets(unittest.TestCase):
210 with mock.patch(
211 "builtins.open", mock.mock_open(read_data=sample_yaml)
212 ) as mock_file:
213- res = get_replica_set_name()
214+ host, port, res = get_replica_data()
215 mock_file.assert_called_with("/etc/mongodb.conf", "r")
216+ self.assertEqual(host, "localhost")
217+ self.assertEqual(port, 27017)
218 self.assertEqual(res, False)
219
220- def test_get_replica_set_name_ini_true(self):
221+ def test_get_replica_data_yaml_conf(self):
222+ """Test mongodb.conf file missing replSet in yaml format."""
223+ sample = """
224+ systemLog:
225+ destination: file
226+ path: "/var/log/mongodb/mongod.log"
227+ logAppend: true
228+ storage:
229+ journal:
230+ enabled: true
231+ processManagement:
232+ fork: true
233+ net:
234+ bindIp: 0.0.0.0
235+ port: 12345
236+ setParameter:
237+ enableLocalhostAuthBypass: false
238+ replication:
239+ replSetName: test
240+ """
241+ config = yaml.safe_load(sample)
242+ host, port, repl = get_replica_data_yaml_conf(config)
243+ self.assertEqual(host, "0.0.0.0")
244+ self.assertEqual(port, 12345)
245+ self.assertEqual(repl, "test")
246+
247+ def test_get_replica_data_yaml_conf_no_replset(self):
248+ """Test mongodb.conf file missing replSet in yaml format."""
249+ sample = """
250+ systemLog:
251+ destination: file
252+ path: "/var/log/mongodb/mongod.log"
253+ logAppend: true
254+ storage:
255+ journal:
256+ enabled: true
257+ processManagement:
258+ fork: true
259+ net:
260+ bindIp: 0.0.0.0
261+ port: 12345
262+ setParameter:
263+ enableLocalhostAuthBypass: false
264+ """
265+ config = yaml.safe_load(sample)
266+ host, port, repl = get_replica_data_yaml_conf(config)
267+ self.assertEqual(host, "0.0.0.0")
268+ self.assertEqual(port, 12345)
269+ self.assertEqual(repl, False)
270+
271+ def test_get_replica_data_ini_true(self):
272 """Test mongodb.conf file parsing success in ini format."""
273 sample = """
274 # mongodb.conf
275- dbpath\t=\t/var/lib/mongodb # tab added to make it fail when parsing as YAML
276- ipv6=true
277- logpath=/var/log/mongodb/mongodb.log
278- logappend=true
279- port = 27017
280- journal=true
281+ dbpath\t = \t/var/lib/mongodb # tab added. It fail when parsing as YAML
282+ ipv6 = true
283+ logpath = /var/log/mongodb/mongodb.log
284+ logappend = true
285+ port = 12345
286+ journal = true
287 diaglog = 0
288 rest = true
289 replSet = test
290 """
291
292 with mock.patch("builtins.open", mock.mock_open(read_data=sample)) as mock_file:
293- res = get_replica_set_name()
294+ host, port, res = get_replica_data()
295 mock_file.assert_called_with("/etc/mongodb.conf", "r")
296+ self.assertEqual(host, "localhost")
297+ self.assertEqual(port, 12345)
298 self.assertEqual(res, "test")
299
300- def test_get_replica_set_name_ini_no_replset(self):
301+ def test_get_replica_data_ini_no_replset(self):
302 """Test mongodb.conf file missing replSet in ini format."""
303 sample = """
304 # mongodb.conf
305- dbpath=/var/lib/mongodb
306- ipv6=true
307- logpath=/var/log/mongodb/mongodb.log
308- logappend=true
309+ dbpath = /var/lib/mongodb
310+ ipv6 = true
311+ logpath = /var/log/mongodb/mongodb.log
312+ logappend = true
313 port = 27017
314 journal=true
315 diaglog = 0
316@@ -109,17 +193,58 @@ class TestCheckReplicaSets(unittest.TestCase):
317 """
318
319 with mock.patch("builtins.open", mock.mock_open(read_data=sample)) as mock_file:
320- res = get_replica_set_name()
321+ host, port, res = get_replica_data()
322 mock_file.assert_called_with("/etc/mongodb.conf", "r")
323+ self.assertEqual(host, "localhost")
324+ self.assertEqual(port, 27017)
325 self.assertEqual(res, False)
326
327- @mock.patch("check_replica_sets.get_replica_set_name", autospec=True)
328+ def test_get_replica_data_ini_conf_no_replset(self):
329+ """Test mongodb.conf file missing replSet in ini format."""
330+ sample = """
331+ # mongodb.conf
332+ dbpath = /var/lib/mongodb
333+ ipv6 = true
334+ logpath = /var/log/mongodb/mongodb.log
335+ logappend = true
336+ port = 12345
337+ journal = true
338+ diaglog = 0
339+ rest = true
340+ """
341+ host, port, repl = get_replica_data_ini_conf(sample)
342+ self.assertEqual(host, "localhost")
343+ self.assertEqual(port, 12345)
344+ self.assertEqual(repl, False)
345+
346+ def test_get_replica_data_ini_bind0_and_default_port(self):
347+ """Test mongodb.conf file missing replSet in ini format."""
348+ sample = """
349+ # mongodb.conf
350+ dbpath = /var/lib/mongodb
351+ ipv6 = true
352+ logpath = /var/log/mongodb/mongodb.log
353+ logappend = true
354+ bind_ip = 127.0.0.1
355+ port = 12345
356+ journal = true
357+ diaglog = 0
358+ rest = true
359+ replSet = test
360+ """
361+
362+ with mock.patch("builtins.open", mock.mock_open(read_data=sample)) as mock_file:
363+ host, port, res = get_replica_data()
364+ mock_file.assert_called_with("/etc/mongodb.conf", "r")
365+ self.assertEqual(host, "127.0.0.1")
366+ self.assertEqual(port, 12345)
367+ self.assertEqual(res, "test")
368+
369+ @mock.patch("check_replica_sets.get_replica_data", autospec=True)
370 @mock.patch("check_replica_sets.get_cmd_result", autospec=True)
371- def test_get_replica_status_ok(
372- self, mock_get_cmd_result, mock_get_replica_set_name
373- ):
374+ def test_get_replica_status_ok(self, mock_get_cmd_result, mock_get_replica_data):
375 """Test replica status in known good states gives OK."""
376- mock_get_replica_set_name.return_value = "test"
377+ mock_get_replica_data.return_value = "localhost", 27017, "test"
378 mock_get_cmd_result.return_value = """{
379 "set" : "myset",
380 "members" : [
381@@ -143,13 +268,13 @@ class TestCheckReplicaSets(unittest.TestCase):
382 self.assertEqual(fake_std_out.getvalue(), result)
383 self.assertEqual("0", str(error))
384
385- @mock.patch("check_replica_sets.get_replica_set_name", autospec=True)
386+ @mock.patch("check_replica_sets.get_replica_data", autospec=True)
387 @mock.patch("check_replica_sets.get_cmd_result", autospec=True)
388 def test_get_replica_status_recovering_warn(
389- self, mock_get_cmd_result, mock_get_replica_set_name
390+ self, mock_get_cmd_result, mock_get_replica_data
391 ):
392 """Test replica status in RECOVERING state gives WARN."""
393- mock_get_replica_set_name.return_value = "test"
394+ mock_get_replica_data.return_value = "localhost", 27017, "test"
395 mock_get_cmd_result.return_value = """{
396 "set" : "myset",
397 "members" : [
398@@ -179,13 +304,13 @@ class TestCheckReplicaSets(unittest.TestCase):
399 self.assertEqual(fake_std_out.getvalue(), result)
400 self.assertEqual("1", str(error))
401
402- @mock.patch("check_replica_sets.get_replica_set_name", autospec=True)
403+ @mock.patch("check_replica_sets.get_replica_data", autospec=True)
404 @mock.patch("check_replica_sets.get_cmd_result", autospec=True)
405 def test_get_replica_status_down_crit(
406- self, mock_get_cmd_result, mock_get_replica_set_name
407+ self, mock_get_cmd_result, mock_get_replica_data
408 ):
409 """Test replica status in DOWN state gives CRIT."""
410- mock_get_replica_set_name.return_value = "test"
411+ mock_get_replica_data.return_value = "localhost", 27017, "test"
412 mock_get_cmd_result.return_value = """{
413 "set" : "myset",
414 "members" : [
415@@ -212,15 +337,15 @@ class TestCheckReplicaSets(unittest.TestCase):
416 self.assertEqual(fake_std_out.getvalue(), result)
417 self.assertEqual("2", str(error))
418
419- @mock.patch("check_replica_sets.get_replica_set_name", autospec=True)
420+ @mock.patch("check_replica_sets.get_replica_data", autospec=True)
421 @mock.patch("check_replica_sets.get_cmd_result", autospec=True)
422 def test_get_replica_status_no_replicasetname_crit(
423- self, mock_get_cmd_result, mock_get_replica_set_name
424+ self, mock_get_cmd_result, mock_get_replica_data
425 ):
426 """Test rs.status() failure 1."""
427- mock_get_replica_set_name.return_value = False
428+ mock_get_replica_data.return_value = "localhost", 27017, False
429
430- result = "CRITIAL: unable to determine the replica set name\n"
431+ result = "CRITICAL: unable to determine the host, port and replica set name\n"
432 with mock.patch("sys.stdout", new=StringIO()) as fake_std_out:
433 try:
434 get_replica_status()
435@@ -228,13 +353,13 @@ class TestCheckReplicaSets(unittest.TestCase):
436 self.assertEqual(fake_std_out.getvalue(), result)
437 self.assertEqual("2", str(error))
438
439- @mock.patch("check_replica_sets.get_replica_set_name", autospec=True)
440+ @mock.patch("check_replica_sets.get_replica_data", autospec=True)
441 @mock.patch("check_replica_sets.get_cmd_result", autospec=True)
442 def test_get_replica_status_bad_json_crit(
443- self, mock_get_cmd_result, mock_get_replica_set_name
444+ self, mock_get_cmd_result, mock_get_replica_data
445 ):
446 """Test rs.status() failure 2."""
447- mock_get_replica_set_name.return_value = "dont care"
448+ mock_get_replica_data.return_value = "localhost", 27017, "dont care"
449 mock_get_cmd_result.return_value = "{ this json is bogus }"
450
451 result = "CRITICAL: failed to load json\n"
452diff --git a/tox.ini b/tox.ini
453index fe438fa..718bae2 100644
454--- a/tox.ini
455+++ b/tox.ini
456@@ -71,7 +71,7 @@ setenv =
457 PYTHONPATH = {toxinidir}/tests/unit:{toxinidir}/files/nrpe-external-master
458 commands =
459 pytest \
460- {posargs:-v --cov=hooks --cov-report=term-missing --cov-branch} {toxinidir}/tests/unit
461+ {posargs:-v --cov=hooks --cov-report=term-missing --cov-branch {toxinidir}/tests/unit}
462 deps = -r{toxinidir}/tests/unit/requirements.txt
463
464 [testenv:func]

Subscribers

People subscribed via source and target branches

to all changes: