[DPE-4307] HA process interrupt tests

[juditnovak]

This change unites 2 tasks:

• DPE-4307 [OpenSearch Dashboards][VM] - Testing - HA process interrupt tests
  • HA tests are covering both OSD process intrrunpt and Opensearch Dashboards process interrupt
  • An issues was discovered and fixed:
    • OSD "hanigng" (simulated by sending SIGSTOP to the node process) was not handled by the charm.
    • Healthcheck is extended, and a restart mechansim is added for the case when the application doesn't respond
• DPE-5516 [OpenSearch Dashboards][VM][BUGFIX] Service unavailable until first update-status
  • Reason:
    • I believe that the issue here was not to leave time for the service to establish, but triggering an update-status as soon as the process (i.e. snap level) was considered up.
  • Fix:
    • A "grace period" was added, giving the application a chance to get back after a restart.
  • Proof of functionality:
    • See earlier pipeline (not including the fix) with OSD showing regular Service unavailable status.
    • Gone on current 🟢 pipelines.

[juditnovak]

We had an ugly bug here. (@phvalguima is my witness :-) )

Reminder: OSD is a service that doesn't even come up (at all!) except in perfect conditions.
Meaning functional connection to Opensearch, etc. No, we do NOT even have a Status Page.
All we get is

The Prometheus Exproter process is very similar. It can't come up as long as there is no functional OSD available. It's failing right after re-start otherwise.

Now this resulted in an ugly, flaky behavior whenever OSD was considered active on a snap level -- yet the service was not running (for example: due to missing Opensearch connection). Meaning that the first part of the condition above was True -- while the 2nd part was True/False ... depending on the moment.

Debugging underlying data structures (representing snap service state):

The conclusion is this: we shouldn't check Prometheus Exporter state here.

One proposal could be to do it in a separate check. (I can propose a solution even in parallel on a separte branch. If we want to address the matter, it would be much nicer in a separate task.).
In my opinion, as long as we are not providing similar safety measures on Openserach (where monitoring is a WAY more crucial), it may be best to keep comlexity low and trus automatic snap restarts to cover Prometheus Exporter failures.

[phvalguima]

@juditnovak yes, I think we need to break the concepts: alive() here means the main service is up and running.

[Mehdi-Bendriss]

Thanks Judit - I have a minor comment regarding the check on the status as part of the charm logic.

[phvalguima]

There is a potential loop here:

The reconcile method may end calling for a restart

And, in the _restart, we end up calling reconcile again before the actual restart.

A charm leader in a deployment with no lock assigned would end up in a continuous loop where the charm calls:
reconcile() > detects unit is down > calls acquire_lock() > which emits a relation_changed > assigns itself the lock > calls the _restart > re-calls the reconcile and restarts the loop

My recommendation is to restart the workload right away on _restart method.

[juditnovak]

@phvalguima You are right. Restart attempt on service down is removed from reconcile(). The check is not needed.

[phvalguima]

Avoid a loop here, as described on: #114 (review)

There is no point in speeding up an update-status, it will happen anyways. In this case, it is better to leave some time between this _restart call and the next update-status for the system to settle.

[phvalguima]

Looking at this code again, may be worthy to structure it instead as follows:

        start_time = time.time()
        unit_healthy, _ = self.health_manager.unit_healthy()
        while not unit_healthy and time.time() - start_time < SERVICE_AVAILABLE_TIMEOUT:
            time.sleep(5)
            unit_healthy, _ = self.health_manager.unit_healthy()
        if unit_healthy:
            # We clean the status and finish
            clear_status(self.unit, [MSG_STARTING, MSG_STARTING_SERVER])
        else:
            # this call will potentially retrigger a restart
            self.on.update_status.emit()

[phvalguima]

FYI, the proposal above reintroduces the risk of continuous restart loop if the unit is failing because of some other problem...