Add alive_check configuration

amandahla commented 2 months ago

Update: I changed the check from ready to alive because the alive one behaves as expected by restarting the pod once is failing. From pebble documentation, I assumed that:

Setting only the ready check would mean that if fails, Pebble would show alive as failed too.
Once ready check fails, a restart action action would be triggered. https://github.com/canonical/pebble#layer-specification

Thanks @javierdelapuente for raising this. Pebble issue: https://github.com/canonical/pebble/issues/439

Overview

Since there is no way of delaying health checks before Synapse start, we need an easy way of changing timeout/threshold/retry.

Reference: https://github.com/canonical/pebble/issues/145

Note: The test_synapse_pebble_layer_change is skipped because it seems that the checks are not replaced. Real environments: :heavy_check_mark: Harness: https://github.com/canonical/operator/issues/1112

Rationale

This way we can easily change checks if Synapse is being impacted (constantly restarting)

Juju Events Changes

Module Changes

Library Changes

Checklist

[x] The charm style guide was applied
[x] The contributing guide was applied
[x] The changes are compliant with ISD054 - Manging Charm Complexity
[x] The documentation is generated using src-docs
[ ] The documentation for charmhub is updated.
[x] The PR is tagged with appropriate label (urgent, trivial, complex)

No CH doc.

dimaqq commented 2 months ago

Some high level thoughts:

Does the heath check interval really need configuration? As in, who is in the better position to understand synapse/mjolnir behaviour, the charm author or the operator? Who would know what values to apply best?

The code to parse this custom format somehow bothers me... because the stringified 10a,3,30s format is like invented for this charm only. Would it be easier to supply each value separately and rely on defaults? Or would it be more pragmatic to parse this string with a regexp?

Given that the feature is marked experimental, I figure it's fine.

github-actions[bot] commented 2 months ago

Test coverage for e188cba29be75e88893c1d211d6799725ce310ef

Name                            Stmts   Miss Branch BrPart  Cover   Missing
---------------------------------------------------------------------------
src/actions/__init__.py             2      0      0      0   100%
src/actions/register_user.py       21      0      2      0   100%
src/actions/reset_instance.py      20      0      2      0   100%
src/admin_access_token.py           9      0      0      0   100%
src/backup.py                     175      5     24      2    96%   353-354, 423-424, 480->482, 483
src/backup_observer.py            134     16     14      0    89%   132-135, 140-143, 179-182, 211-214
src/charm.py                      335     17     86      9    94%   141->143, 146, 286-287, 315, 322, 403-407, 410-411, 439-441, 461, 491-494, 556-557
src/charm_state.py                137      6     34      4    94%   264, 289, 295, 301, 305-306
src/charm_types.py                 34      0     10      0   100%
src/database_client.py             57      1     12      4    93%   35, 47->exit, 69->exit, 88->98
src/database_observer.py           39      0      4      1    98%   70->72
src/exceptions.py                   3      0      0      0   100%
src/irc_bridge.py                  36      9      6      0    79%   31, 71-86
src/media_observer.py              41      4      2      1    88%   61-63, 82
src/mjolnir.py                     94      3     32      3    95%   82, 86->98, 108-112
src/observability.py               20      0      2      0   100%
src/pebble.py                     205     29     38     11    84%   58->63, 139-143, 174-175, 187->exit, 198-202, 221-222, 242-243, 296->301, 315-316, 333, 335, 337, 339, 341, 348-351, 372, 540-560
src/redis_observer.py              35      3      4      0    92%   62-65
src/s3_parameters.py               22      0      4      0   100%
src/saml_observer.py               31      0      6      0   100%
src/smtp_observer.py               56      4     14      2    91%   82-86, 89, 108->113
src/synapse/__init__.py             3      0      0      0   100%
src/synapse/admin.py               19      2      2      0    90%   40-41
src/synapse/api.py                175      3     24      3    97%   176, 229, 402
src/synapse/workload.py           306     33     68     14    87%   302-303, 324-325, 403->406, 409, 424->exit, 428-429, 449-450, 490-491, 527-528, 547-549, 551, 573-574, 629, 645, 692->695, 719-720, 739, 747->749, 749->751, 756-757, 772, 792-793, 810, 819-820, 839->844, 845
src/user.py                        23      0      4      0   100%
---------------------------------------------------------------------------
TOTAL                            2032    135    394     54    92%

Static code analysis report

Working... ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 100% 0:00:01
Run started:2024-07-10 08:21:36.587546

Test results:
    No issues identified.

Code scanned:
    Total lines of code: 11120
    Total lines skipped (#nosec): 4
    Total potential issues skipped due to specifically being disabled (e.g., #nosec BXXX): 0

Run metrics:
    Total issues (by severity):
        Undefined: 0
        Low: 0
        Medium: 0
        High: 0
    Total issues (by confidence):
        Undefined: 0
        Low: 0
        Medium: 0
        High: 0
Files skipped (0):

canonical / synapse-operator