Martin Sivák has posted comments on this change. Change subject: use monotonic time when checking the timeouts ......................................................................
Patch Set 10: Code-Review-1 (4 comments) http://gerrit.ovirt.org/#/c/26384/10/ovirt_hosted_engine_ha/agent/state_machine.py File ovirt_hosted_engine_ha/agent/state_machine.py: Line 67: """ Line 68: # Collect stats Line 69: stats = { Line 70: "collect_start": int(monotonic.time()), Line 71: "time_epoch": int(time.time() - monotonic.time()) Please make just a single call to time and monotonic here to avoid getting two different timestamps. Line 72: } Line 73: Line 74: # Do not refresh if the time has not changed Line 75: if (old_data.stats is not None and http://gerrit.ovirt.org/#/c/26384/10/ovirt_hosted_engine_ha/agent/states.py File ovirt_hosted_engine_ha/agent/states.py: Line 553: return super(EngineUpBadHealth, self).consume(fsm, new_data, logger) Line 554: Line 555: def score(self, logger): Line 556: # If engine has bad health status, let another host try Line 557: time_str = time.ctime() The timeout should be always set here, is it not? I never needed the if, because the timeout decorator made sure the value was initialized. I do not want to see anything that calls out to the system in the states btw. So the time.ctime() violates the original design and testability. Line 558: if self.data.timeout_start_time: Line 559: time_str = time.ctime(self.data.timeout_start_time Line 560: + self.data.stats.time_epoch) Line 561: Line 626: return EngineUnexpectedlyDown(new_data) Line 627: Line 628: def score(self, logger): Line 629: time_str = time.ctime() Line 630: if self.data.timeout_start_time: The same as in the previous comment. Why the if? Line 631: time_str = time.ctime(self.data.timeout_start_time Line 632: + self.data.stats.time_epoch) Line 633: Line 634: logger.info('Score is 0 due to unexpected vm shutdown at %s', http://gerrit.ovirt.org/#/c/26384/10/ovirt_hosted_engine_ha/lib/metadata.py File ovirt_hosted_engine_ha/lib/metadata.py: Line 84: It is used to determine the host liveness independently on the system time. Line 85: The logic just compares the last timestamp with the current one and Line 86: checks if it raises. If the host is not in the map it's considered not alive Line 87: """ Line 88: host_liveness_dict = {} No way. This has to be reported using some data API. This won't work with code that is executed using some other binary (like as a part of vdsm or cli). I hate globals in library modules. Line 89: Line 90: Line 91: def is_host_alive(host_str, data): Line 92: last_time = host_liveness_dict.get(host_str, None) -- To view, visit http://gerrit.ovirt.org/26384 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9794beb4082f52d29835be5b6a182ab448c248f2 Gerrit-PatchSet: 10 Gerrit-Project: ovirt-hosted-engine-ha Gerrit-Branch: master Gerrit-Owner: Jiří Moskovčák <[email protected]> Gerrit-Reviewer: Greg Padgett <[email protected]> Gerrit-Reviewer: Jiří Moskovčák <[email protected]> Gerrit-Reviewer: Martin Sivák <[email protected]> Gerrit-Reviewer: Sandro Bonazzola <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
