Code review comment for ~mariosplivalo/charm-mongodb:bionic-fix

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

« Back to merge proposal