Repository navigation
Conversation
…ll ensure that, if Node's clock is rewound, RDS still sees a version increment when the resource is required to be updated in RDS
…ures the time always increases even when the system clock is rewound
| ? bst::chrono::duration_cast<bst::chrono::microseconds>(nmos::tai_clock::now() - nmos::time_point_from_tai(nmos::parse_version(received_time->second))).count() / 1000.0 | ||
| ? bst::chrono::duration_cast<bst::chrono::microseconds>( | ||
| // processing_dur = now - received time | ||
| ( tai_clock::time_point(tai_clock::duration(std::chrono::duration_cast<std::chrono::nanoseconds>(std::chrono::steady_clock::now().time_since_epoch()).count())) |
There was a problem hiding this comment.
Can you explain this? Doesn't the steady_clock have a different epoch than tai_clock?
There was a problem hiding this comment.
tai_clock now uses the boost implementation. According to the note in https://github.com/sony/nmos-cpp/blob/master/Development/nmos/api_utils.cpp#L710-L711, on Windows, the boost version of time_point::now (system_clock::now) sometimes returns the same value after a small time increment. Therefore, we are using std::chrono::steady_clock::now() here.
|
|
||
| Recent activity on the project (newest first): | ||
|
|
||
| - Added support for making existing timing/version semantics safe in environments where NTP rewinds system time. |
There was a problem hiding this comment.
⬇️ This PR isn't really much to do with NTP at all, it's about handling system clock being stepped back by a correction, which might come from NTP, PTP or anything. I'd change this line and the PR title.
garethsb
left a comment
There was a problem hiding this comment.
Co-authored-by: Gareth's 🤖 buddy
This PR tries to stop resource version values going backwards when the system clock is stepped back, for example by an NTP correction.
- The registry side isn't fixed. The description says the registry (RDS) uses
steady_clockfor "the request's received time". ButX-Received-Timeis read in only one place, the Server-Timing calculation inmake_api_finally_handler. Heartbeat expiry useshealth_now(), which is based on the system clock viatai_clock. A clock step on the registry therefore still holds off expiry (stepped back) or expires every Node at once (stepped forward). The diagram's registry scenario isn't addressed. - Mismatched clock conversions.
X-Received-Timenow holds asteady_clockvalue encoded as a TAI version string.support_apiuses the raw.count(), while the finally handler usesduration_cast<nanoseconds>. Both should use the same cast, with a comment saying the value issteady_clocktime, not TAI. (That would also answer my earlier question about epochs.) - Some version writes are missed.
annotation_api.cpp(IS-13 PATCH), the version updates in thenmos-cpp-nodeexample, and downstream calls still usemake_version()ortai_now(). Thestrictly_increasing_update(resources, …)approach only guarantees increasing versions if every write goes through it. - API break.
set_resource_subscription,set_resource_versionandset_connection_resource_activeall gain aresourcesparameter. (I know this would affect NVIDIA/nvnmos,nvnmos_impl.cppcalls the old signatures six times.)
For points 3 and 4, a per-resource rule would be simpler. IS-04 only needs each resource's version to increase; it doesn't need to be newer than every other resource's update time. A helper that returns the later of the new time and the resource's current version plus one nanosecond would keep the existing signatures:
inline tai strictly_increasing_version(const nmos::resource& resource, tai update = tai_now())
{
const auto current = nmos::parse_version(nmos::fields::version(resource.data));
return update > current ? update : tai_from_time_point(time_point_from_tai(current) + tai_clock::duration(1));
}set_resource_subscription, set_resource_version and the annotation PATCH could each call this internally. nvnmos and other downstream code would then benefit without changes.
The PR also has small unrelated edits (using web::json::value;, web::json::value::string changed to value::string, a leftover at variable). It includes merge commits from master, so it would be better to rewrite history (maybe rebase on current master) before merging.
The following scenarios illustrate the issue that arises when the system clock is rewound. In both cases, the time used is
system_clock. To overcome this issue, we will need to usesteady_clockto obtain the request's received time at the RDS. On the Node side, we will need to usenmos::strictly_increasing_updatefor the resource's version, which will always increment.