CA-394343: After clock jump the xapi assumed the host is HOST_OFFLINE

[minglumlu]

Prior to this commit, the xapi on the coordinator host records the 'Unix.gettimeofday' as the timestamps of the heartbeat with other pool supporter hosts. When the system clock is updated with a huge jump forward, the timestamps would be far back into the past. This would cause the xapi assumes that the hosts are offline as long time no heartbeats.

In this commit, the timestamps are changed to get from a monotonic clock. In this way, the system clock changes will not impact the heartbeats' timestamps any more.

Additionally, Host_metrics.last_updated is only set when the object is created. It's useless in check_host_liveness at all.

---

This change is 

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Additional details and impacted files

@@           Coverage Diff            @@
##           master   #5700     +/-   ##
========================================
+ Coverage    44.8%   45.5%   +0.6%     
========================================
  Files          16      13      -3     
  Lines        2210    1624    -586     
========================================
- Hits          992     739    -253     
+ Misses       1218     885    -333     

see 8 files with indirect coverage changes

Flag	Coverage Δ
python2.7	45.5% <ø> (ø)
python3.11	?

Flags with carried forward coverage won't be shown. Click here to find out more.

[lindig]

Robustness against clock changes is desirable but I question that it can be expected for large clock changes. That's like designing for power outages - it needs to be an explicit goal right from the start.

[minglumlu]

Robustness against clock changes is desirable but I question that it can be expected for large clock changes. That's like designing for power outages - it needs to be an explicit goal right from the start.

Yes. The robustness is the major concern of this change. The automation testing relies on the large clock changes heavily, although there may not be common cases in production. But I think improving the robustness in xapi would resolve the potential issues in either case.

[psafont]

The biggest is the drop of the retrieval of the timestamp from the metrics, I don't think that code was useful

[coveralls]

coverage: 44.887%. remained the same
when pulling 9603537 on minglumlu:private/mingl/CA-394343
into e61e0ac on xapi-project:master.

[minglumlu]

Rebased with the master branch to resolve the conflict.

[gangj]

I think in case of None, it means the host has never tickle heartbeat? Then we should consider it as expired?

[gangj]

287 let start_db_gc_thread () =
288   Thread.create
289     (fun () ->
290       Debug.with_thread_named "db_gc"
291         (fun () ->
292           while true do
293             try Thread.delay db_GC_TIMER ; single_pass ()
294             with e ->
295               debug "Exception in DB GC thread: %s" (ExnHelper.string_of_exn e)
296           done
297         )
298         ()
299     )
300     ()
301

For
293 try Thread.delay db_GC_TIMER ; single_pass ()
As we use a timer to check heartbeat and a default timer for each host, I think here we should start single_pass immediately.

[gangj]

Additionally, Host_metrics.last_updated is only set when the object is
created. It's useless in check_host_liveness at all.

I guess it was updated periodically with some time interval in some old version xapi which did not tickle_heartbeat yet, so it was considered as the heartbeat for the new added heartbeat mechanism, add then it was removed in the new heartbeat version.
As all hosts will tickle_heartbeat now, it is useless now.

[psafont]

Is there any testing left to do? This seems ready to merge otherwise

[gangj]

Reviewable status: 0 of 3 files reviewed, 9 unresolved discussions (waiting on @minglumlu and @psafont)

---

ocaml/xapi/db_gc.ml line 95 at r1 (raw file):

Previously, gangj (Gang Ji) wrote…

287 let start_db_gc_thread () =
288   Thread.create
289     (fun () ->
290       Debug.with_thread_named "db_gc"
291         (fun () ->
292           while true do
293             try Thread.delay db_GC_TIMER ; single_pass ()
294             with e ->
295               debug "Exception in DB GC thread: %s" (ExnHelper.string_of_exn e)
296           done
297         )
298         ()
299     )
300     ()
301

For
293 try Thread.delay db_GC_TIMER ; single_pass ()
As we use a timer to check heartbeat and a default timer for each host, I think here we should start single_pass immediately.

How about this change?

[psafont]

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @gangj and @minglumlu)

[psafont]

Reviewed all commit messages.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @gangj)

---

ocaml/xapi/db_gc.ml line 95 at r1 (raw file):

Previously, gangj (Gang Ji) wrote…

How about this change?

I'd rather keep the logic the same. This can be enhanced in another PR

[psafont]

Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gangj)

[gangj]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @psafont)

---

ocaml/xapi/db_gc.ml line 95 at r1 (raw file):

Previously, psafont (Pau Ruiz Safont) wrote…

I'd rather keep the logic the same. This can be enhanced in another PR

With this delay, the first time a host's liveness is checked after xapi startup is not !Xapi_globs.host_assumed_dead_interval, it is (db_GC_TIMER + !Xapi_globs.host_assumed_dead_interval). I agree it is a small issue.

May I know which logic do you mean to "keep the logic the same" as?