DAOS-17427 control: Restart excluded rank after suicide

[tanabarr]

When an engine detects that it has been removed from the system group
map by receiving a CART event, it will now notify its local control plane
with a RAS engine_self_terminated event before terminating its own
process. After receiving this self-termination event, the local
control plane will restart the engine so it can rejoin the system.

The goal of this change is to improve overall system resilience by
automatically recovering engines that are excluded because of
temporary issues such as network instability. Once the engines rejoin,
the rank will still need to be reintegrated into pools as a separate
follow‑up step.

Rate-limiting prevents restart storms: a configurable minimum delay
(default 300 seconds) between restarts per rank ensures system
stability. Two new server config file parameters control behavior:
disable_engine_auto_restart (boolean, default false) completely
disables automatic restarts, while engine_auto_restart_min_delay
(integer seconds) sets the minimum time between consecutive restart
attempts.

Functional tests for the automatic engine restart feature included
with cases to verify disabling, rate-limiting and configuration support.

Features: pool control

Steps for the author:

• Commit message follows the guidelines.
• Appropriate Features or Test-tag pragmas were used.
• Appropriate Functional Test Stages were run.
• At least two positive code reviews including at least one code owner from each category referenced in the PR.
• Testing is complete. If necessary, forced-landing label added and a reason added in a comment.

After all prior steps are complete:

• Gatekeeper requested (daos-gatekeeper added as a reviewer).

[github-actions]

Ticket title is 'Handle engine suicides by automatically restarting the engines'
Status is 'In Review'
https://daosio.atlassian.net/browse/DAOS-17427

[daosbuild1]

Test stage Functional Hardware Medium completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-16279/1/execution/node/1466/log

[liw]

Not requesting changes.

[liw]

[Discussion] Looking at the code changes I think I'm back to the previously discussed point: Whether the engine kills itself, or only informs the server, who will decide what to do. If the engine kills itself, why do we need the RAS event... The server can simply decide whether to restart a killed engine, even without the RAS event, cannot it...

[Question] If the ds_notify_rank_suicide call fails, then the engine will still terminate, but will the server restart the engine?

[tanabarr]

[Discussion] I implemented kill+ras mainly because it is guaranteed that the engine process will be killed, the RAS event signifies that it is a suicide as opposed to other termination, do we really want to always restart a terminated rank regardless of reason?

[Question] Yes, if the notify call fails then the engine will be terminated but not restarted. if instead we don't terminate the engine on group map change and just send ras and put the engine into a blocking state then if a notify call fails then the engine will effectively hang.

[liw]

do we really want to always restart a terminated rank regardless of reason?

Hmm, it's indeed hard to say for sure. I'd understand if we'd like to begin conservatively, only restarting for certain cases.

[kjacque]

I agree we should start conservatively. For engines that crash, or exit for some unknown reason, we don't know if the engine would be OK if we restarted it. Better to let the admin investigate and manually restart the ranks in that case.

[knard38]

@tanabarr, I had the same issue with the CI regarding the "Unit test beds with memcheck".
At the end, I have merged with master and restarted the CI.
Now, this stage is successfully run.

[daosbuild3]

Test stage Functional Hardware Large MD on SSD completed with status FAILURE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net//job/daos-stack/job/daos/view/change-requests/job/PR-16279/5/execution/node/1303/log

[daosbuild3]

Test stage Functional Hardware Medium MD on SSD completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-16279/5/testReport/

[tanabarr]

PR CI run with Features: pool control to give an extra coverage in case restarting engine automatically causes any tests to fail. BoundaryTest and ListVerboseTest failures are unrelated.

@kjacque @knard38 @mjmac @liw can I get reviews please?

[tanabarr]

conflicts resolved, doc-only change, CI run no. 5 is still relevant and should be used for PR review

[daltonbohning]

It seems the new functional tests aren't even running in CI?
There are many lint errors in ftest that need to be resolved or possibly ignored

[daltonbohning]

We have a way to handle this in the framework by calling

self.register_cleanup(reset_engine_restart_state)

After whatever operation puts the system into the invalid state. So maybe instead of defining this tearDown, we can do that after calling exclude_rank_and_wait_restart the first time

[daltonbohning]

It is better to fail because skipping will be silent and easily ignored

Suggested change

	self.skipTest("Test requires at least 2 ranks")
	self.fail("Test requires at least 2 ranks")

[tanabarr]

done

[daltonbohning]

It would be better if get_rank_incarnation raised an exception instead of silently returning None

[tanabarr]

done

[daltonbohning]

Where does the arbitrary 2s sleep come from? This will eventually be a problem and we will have to revisit

[tanabarr]

will remove if you don't think it necessary for a system restart

[tanabarr]

done

[daltonbohning]

If we don't need it at all then that is great, but I was more meaning that 2s seems arbitrary. And historically arbitrary sleeps have been an issue that we have to revisit later

[daosbuild3]

Test stage Functional on EL 9 completed with status FAILURE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net//job/daos-stack/job/daos/view/change-requests/job/PR-16279/28/execution/node/942/log

[daltonbohning]

Similar to some other cases: how do we know 2s is enough? If there really is not a deterministic way to know at this code level, this is fine, but eventually this kind of thing needs to be revisited.

[daosbuild3]

Test stage Functional on EL 9 completed with status FAILURE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos/job/PR-16279/33/display/redirect

[mjmac]

Should wait_for_resp be true now, in order to ensure that the event processing doesn't race with the SIGKILL? I don't really see a downside...

[mjmac]

There is still a race here, I think. If multiple restart requests come in at the same time, there's a window where the manager goroutine and the AfterFunc goroutine could both wind up calling performRestart().

If you rewrite this method to make the decision under the lock, I think you eliminate the race and also easily deal with floods of restart requests:

      rank, instance := req.rank, req.instance

      mgr.mu.Lock()
      if last, ok := mgr.lastRestart[rank]; ok {
          if elapsed := time.Since(last); elapsed < mgr.getMinDelay() {
              // Fast debounce for subsequent requests inside of the delay window
              if _, pending := mgr.pendingRestart[rank]; pending {
                  mgr.mu.Unlock()
                  mgr.log.Debugf("rank %d already has a deferred restart pending; dropping",
                      rank)
                  return
              }

              // First restart request inside of the delay window claims it
              remaining := mgr.getMinDelay() - elapsed
              mgr.pendingRestart[rank] = time.AfterFunc(remaining, func() {
                  mgr.requestRestart(rank, instance)
              })
              mgr.mu.Unlock()
              mgr.log.Noticef("rank %d restart rate limited: will restart in %s",
                  rank, remaining.Round(time.Second))
              return
          }
      }
      
      // If this is the first restart or it's outside of the delay window, start the process (over)
      mgr.lastRestart[rank] = time.Now()
      delete(mgr.pendingRestart, rank)
      mgr.mu.Unlock()

      if err := waitForEngineStopped(ctx, []Engine{instance}); err != nil {
          mgr.log.Errorf("rank %d did not stop before restart: %s", rank, err)
          return
      }
      mgr.log.Noticef("restart manager is restarting rank %d", rank)
      instance.requestStart(ctx)