Merge ~mariosplivalo/charm-mongodb:bionic-fix into ~mongodb-charmers/charm-mongodb:master

Proposed by Mario Splivalo
Status: Merged
Merged at revision: 001d9b56d9597c8b20dc3b5b8cf7927a12a243b2
Proposed branch: ~mariosplivalo/charm-mongodb:bionic-fix
Merge into: ~mongodb-charmers/charm-mongodb:master
Diff against target: 67 lines (+11/-9)
2 files modified
hooks/hooks.py (+8/-7)
metadata.yaml (+3/-2)
Reviewer Review Type Date Requested Status
Tom Haddon (community) Approve
Stuart Bishop (community) Approve
Review via email: mp+340136@code.launchpad.net

Commit message

Fixes replicaset initialization for Bionic.
Also, uses leader-election to figure out the unit to initialize replicaset on.
Fixes-bug: LP: #1748214

Description of the change

Fixes replicaset initialization for Bionic.
Also, uses leader-election to figure out the unit to initialize replicaset on.

To post a comment you must log in.
Revision history for this message
Stuart Bishop (stub) wrote :

Using leadership here is correct. If I understand this correctly, when the lead unit runs its replica-set-relation-changed hook, it initializes the replica set using init_replset(). As a side effect, this also makes it the primary for the replica set so it will then add the remote unit. Non-leaders and non-primaries do nothing.

Technically, there is a race here. It possible for all units to join and setup the relation while none of them are leader. It is fairly improbable, as it involves leadership elections happening at the right time to ensure that leadership is handed to a unit after it has run the relation joined/changed hooks (requires at least two netsplits occuring at the right point in the sequence). If you want to avoid this, you need a leader-elected hook that iterates over all the existent relations and initializes them if necessary. This race would be hard to have in a reactive charm, but this isn't reactive.

The replica set initialization will happen multiple times (at least once per unit), so hopefully it is idempotent and lightweight. The existing charm does the same thing so I suspect this is fine. Similarly, join_replset will be called multiple times for each unit and also needs to be idempotent.

I personally wouldn't be committing to artful support at this stage, so unless we have a reason to be making artful deploys I'd remove that. And xenial should be first in the list of supported series, since we want that to be the default.

review: Approve
Revision history for this message
Tom Haddon (mthaddon) wrote :

Have confirmed this work in CI on bionic, as well as a charm upgrade on xenial and a deploy from scratch on xenial, fwiw, so happy to see this go live (pending other comments and those from Stuart).

Revision history for this message
Mario Splivalo (mariosplivalo) wrote :

> Using leadership here is correct. If I understand this correctly, when the
> lead unit runs its replica-set-relation-changed hook, it initializes the
> replica set using init_replset(). As a side effect, this also makes it the
> primary for the replica set so it will then add the remote unit. Non-leaders
> and non-primaries do nothing.

That is correct. Previously (before leadership) code was checking the unit id number, and
would initialize replicaset only on the lowest unit ids, among all peeered units.
That was also prone to errors, as sometimes unit/1 would come up before unit/0. Then, when
unit/0 comes up, it will realize it is the 'lowest' unit id, and it will initialize replicaset there too. So you would end up with two (or more) primaries.

With leadership check this is greatly diminished. As you said, there is a chance of a race here, but I was not able to force it.
Before checking for leader I would have failure rate cca 2/30, and now I haven't had single failure in my tests.

I have removed 'zesty' from the supported series too, and will remove artful when it goes EOD. I've tested replicaset creation on trusty, xenial, artful and bionic, with success.

Revision history for this message
Tom Haddon (mthaddon) wrote :

Looks good to me. Feel free to merge and release.

One possible nit is that if you add a trailing "," to the is_leader import that would mean future changes to things we're importing from charmhelpers.core.hookenv wouldn't need to change this line at all.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/hooks/hooks.py b/hooks/hooks.py
2index 30fc460..d3774a7 100755
3--- a/hooks/hooks.py
4+++ b/hooks/hooks.py
5@@ -58,6 +58,7 @@ from charmhelpers.core.hookenv import (
6 Hooks,
7 DEBUG,
8 WARNING,
9+ is_leader
10 )
11
12 from charmhelpers.core.hookenv import log as juju_log
13@@ -504,10 +505,11 @@ def init_replset():
14 retVal = False
15 break
16 except OperationFailure as e:
17- juju_log('init_replset: OperationFailure: %s' % e)
18+ juju_log('init_replset: OperationFailure: %s' % e, DEBUG)
19 if 'Received replSetInitiate' in str(e):
20 continue
21 else:
22+ juju_log('init_replset: Unhandled OperationFailure %s' % e)
23 raise
24 finally:
25 time.sleep(INIT_CHECK_DELAY)
26@@ -1174,6 +1176,9 @@ def am_i_primary():
27 elif 'EMPTYCONFIG' in str(e):
28 # replication not initialized yet
29 return False
30+ elif 'no replset config has been received' in str(e):
31+ # replication not initialized yet (Mongo3.4+)
32+ return False
33 elif 'not running with --replSet' in str(e):
34 # replicaset not configured
35 return False
36@@ -1200,12 +1205,8 @@ def replica_set_relation_changed():
37 juju_log('Joiner not ready yet... bailing out')
38 return
39
40- # Initialize the replicaset - we do this only on the oldest unit in replset
41- # TODO: figure a way how to avoid race conditions - when unit/1 actually
42- # comes up before unit/0 does - happens rarely, but can happen
43- # quickfix - deploy replset with only two units, use 'add-unit' to
44- # add the rest
45- if oldest_peer(peer_units('replica-set')):
46+ # Initialize the replicaset - we do this only on the leader!
47+ if is_leader():
48 juju_log('Initializing replicaset')
49 init_replset()
50
51diff --git a/metadata.yaml b/metadata.yaml
52index b5231e5..9cf90db 100644
53--- a/metadata.yaml
54+++ b/metadata.yaml
55@@ -20,9 +20,10 @@ description: |
56 tags:
57 - databases
58 series:
59- - trusty
60 - xenial
61- - zesty
62+ - artful
63+ - bionic
64+ - trusty
65 provides:
66 nrpe-external-master:
67 interface: nrpe-external-master

Subscribers

People subscribed via source and target branches

to status/vote changes: