Merge lp://qastaging/~free.ekanayaka/landscape-charm/dont-restart-when-leader-is-deposed into lp://qastaging/~landscape/landscape-charm/trunk

Proposed by Free Ekanayaka
Status: Merged
Approved by: Free Ekanayaka
Approved revision: 370
Merged at revision: 368
Proposed branch: lp://qastaging/~free.ekanayaka/landscape-charm/dont-restart-when-leader-is-deposed
Merge into: lp://qastaging/~landscape/landscape-charm/trunk
Diff against target: 125 lines (+79/-3)
2 files modified
lib/callbacks/scripts.py (+47/-1)
lib/callbacks/tests/test_scripts.py (+32/-2)
To merge this branch: bzr merge lp://qastaging/~free.ekanayaka/landscape-charm/dont-restart-when-leader-is-deposed
Reviewer Review Type Date Requested Status
Adam Collard (community) Approve
🤖 Landscape Builder test results Approve
Данило Шеган (community) Approve
Review via email: mp+305926@code.qastaging.launchpad.net

Commit message

This branch fixes hook errors happening when removing the
leader unit. The errors happen in the "leader-settings-changed"
hook, which is fired before any other hook and that fails
to restart services because in the meantime the postgresql
charm has already changed its configuration to not accept
connections from the leader unit.

Description of the change

This branch fixes hook errors happening when removing the
leader unit. The errors happen in the "leader-settings-changed"
hook, which is fired before any other hook and that fails
to restart services because in the meantime the postgresql
charm has already changed its configuration to not accept
connections from the leader unit.

The fix is somehow a workaround, as explained in the code. The
best approach would be to not have singletons services at all
(or in case we really need to, use the hacluster charm).

Testing instructions:

I created a 2-unit deployment and removed the leader unit, it
all worked, contrary to trunk.

To post a comment you must log in.
Revision history for this message
🤖 Landscape Builder (landscape-builder) :
review: Abstain (executing tests)
Revision history for this message
🤖 Landscape Builder (landscape-builder) wrote :
review: Approve (test results)
Revision history for this message
Данило Шеган (danilo) wrote :

Looks good: it is a bit nasty, and I believe the special-case can be simplified a bit, but otherwise good to go. +1

review: Approve
Revision history for this message
Adam Collard (adam-collard) wrote :

Having the assumption that the trigger for leader changing is because the old leader is dying feels too broad.

This needs a bug link too.

Further question/nits inline

review: Needs Information
Revision history for this message
Free Ekanayaka (free.ekanayaka) :
369. By Free Ekanayaka

Address review comments

370. By Free Ekanayaka

Reference bug #1625500

Revision history for this message
Free Ekanayaka (free.ekanayaka) wrote :

> Having the assumption that the trigger for leader changing is because the old
> leader is dying feels too broad.

Agreed, however I believe this is "safe enough" in practice for now, and any alternative I see will be significantly more expensive.

> This needs a bug link too.

Agreed: Bug #1625500.

Revision history for this message
🤖 Landscape Builder (landscape-builder) :
review: Abstain (executing tests)
Revision history for this message
🤖 Landscape Builder (landscape-builder) wrote :
review: Approve (test results)
Revision history for this message
Adam Collard (adam-collard) wrote :

+1 thanks for the fixes

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches