✨ Add a condition to report when hub and agent clock out of sync.

[xuezhaojun]

Summary

When hub and agent clock out of sync, the managed cluster status will show as 'Unknown' even the agent functions well.

Here we add a new condition to report clock out-of-sync, when we detect the clock of hub and a agent is not synced.

The strategy:
Every time hub find the renewTime updated, we compare the now with renewTime. The 2 values should close to each other, if the now is faster than renewTime too much (over a lease duration), we will report clock out-of-sync.

depends on: open-cluster-management-io/api#296

Related issue(s)

Fixes #

[codecov]

Codecov Report

Attention: 52 lines in your changes are missing coverage. Please review.

Comparison is base (f89d535) 61.75% compared to head (09fe3b2) 61.63%.
Report is 1 commits behind head on main.

Files	Patch %	Lines
pkg/registration/hub/lease/clocksynccontroller.go	42.85%	47 Missing and 5 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #312      +/-   ##
==========================================
- Coverage   61.75%   61.63%   -0.13%     
==========================================
  Files         132      133       +1     
  Lines       13992    14083      +91     
==========================================
+ Hits         8641     8680      +39     
- Misses       4585     4632      +47     
- Partials      766      771       +5     

Flag	Coverage Δ
unit	61.63% <42.85%> (-0.13%)	⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[skeeey]

I remembered we intentionally didn't use local time here, because we want to keep the time sync between the managed cluster and hub cluster

/cc @qiujian16

[xuezhaojun]

I remembered we intentionally didn't use local time here, because we want to keep the time sync between the managed cluster and hub cluster

/cc @qiujian16

That's new to me, do we know the motivation of keeping clocks sync between hub and agents?
And if we intentionally avoid using local time, we should mention this as a prerequisite of using ocm.

[xuezhaojun]

Find the comment: open-cluster-management-io/registration#40 (comment)

It seems the initial implement is based on probeTimestamp but we change to using observedLease directly in this PR: open-cluster-management-io/registration#43

[xuezhaojun]

Hi, @qiujian16 , I tried the bareInformer approach as well, does that means:

1. we need a independent controller watch leaseinformer using the following bare setting:

 informer.AddEventHandler(cache.ResourceEventHandlerFuncs{
        UpdateFunc: func(oldObj, newObj interface{}) {
            oldLease := oldObj.(*corev1.Lease)
            newLease := newObj.(*corev1.Lease)
            if oldLease.Spec.RenewTime.Before(newLease.Spec.RenewTime) {
              c.equeue(newLease)
            }
        },
    })

2. In the new controller, every reconcile is triggered by a renewTime update event, and when it occurs, we assume the now on the agent-side which is lease's renewTime, should be close to(in 1 leaseDuration) the now on the hub-side which we can get by time.Now()

The concern of this approach is, the controller has a default 10 hours resync setting, this resync is not triggered by the renew update event.

This may cause misreport of out-of-sync condition.

[qiujian16]

I think you should have a separated controller to update this condition. The reconcile logic is not quite the same as what lease did before.

[xuezhaojun]

I think you should have a separated controller to update this condition. The reconcile logic is not quite the same as what lease did before.

Got it, do you think we can seprete a controller but still use savedLease approach to avoid misreport by 10 hours resync of controller-runtime?

[qiujian16]

My understanding is if you make it in the different controller, you would not need a local cache. isn't it?

[qiujian16]

/approve

/assign @elgnay

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qiujian16, xuezhaojun

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• pkg/registration/OWNERS [qiujian16]
• test/OWNERS [qiujian16]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[qiujian16]

/lgtm