using lease for cluster remote server healthz checker. #249
Conversation
| if url == nil { | ||
| for _, server := range kcm.remoteServers { | ||
| if kcm.checker.IsHealthy(server) { | ||
| s = server |
There was a problem hiding this comment.
how about add a break statement?
pkg/yurthub/proxy/proxy.go
Outdated
| @@ -27,6 +27,7 @@ import ( | |||
| "github.com/openyurtio/openyurt/pkg/yurthub/proxy/remote" | |||
| "github.com/openyurtio/openyurt/pkg/yurthub/proxy/util" | |||
| "github.com/openyurtio/openyurt/pkg/yurthub/transport" | |||
| util2 "github.com/openyurtio/openyurt/pkg/yurthub/util" | |||
| @@ -122,6 +122,17 @@ func ReqInfoString(info *apirequest.RequestInfo) string { | |||
| return fmt.Sprintf("%s %s for %s", info.Verb, info.Resource, info.Path) | |||
| } | |||
|
|
|||
| func IsKubeletLeaseReq(req *http.Request) bool { | |||
zyjhtangtang
force-pushed
the
master
branch
2 times, most recently
from
99fe6ba to
b7bcfff
Compare
| return checker.isHealthy() | ||
| } | ||
| //if there is not checker for server, default healthy. | ||
| return true |
There was a problem hiding this comment.
why the status of server is healthy when no checker for server?
There was a problem hiding this comment.
if there is not checker for server, default healthy, so certificate can be made up before healthcheker run.
| } | ||
|
|
||
| func (hcm *healthCheckerManager) getChecker() *checker { | ||
| hcm.RLock() |
There was a problem hiding this comment.
how about delete lock of healthCheckerManager?
|
|
||
| return false | ||
| func (hcm *healthCheckerManager) sync() { |
There was a problem hiding this comment.
Please add comments to explain the business of checking healthy status of server by lease
There was a problem hiding this comment.
Ensure that the node heartbeat can be reported when there is a healthy remote server
| serverHealthzURL := *url | ||
| if serverHealthzURL.Path == "" || serverHealthzURL.Path == "/" { | ||
| serverHealthzURL.Path = "/healthz" | ||
| func (hcm *healthCheckerManager) backoffEnsureLease(checker *checker) (*coordinationv1.Lease, bool) { |
There was a problem hiding this comment.
Please define Lease process in a interface
| Timeout: hcm.heartbeatTimeoutSeconds, | ||
| } | ||
| return cfg | ||
|
|
||
| } | ||
|
|
||
| func PingClusterHealthz(client *http.Client, addr string) (bool, error) { |
There was a problem hiding this comment.
PingClusterHealthz() has not been used now. how about delete it?
There was a problem hiding this comment.
yurtctl uses this method to detect the health status of yurthub when setup yurthub.
| lease.Spec.RenewTime = &metav1.MicroTime{Time: hcm.clock.Now()} | ||
|
|
||
| if lease.OwnerReferences == nil || len(lease.OwnerReferences) == 0 { | ||
| if node, err := checker.client.CoreV1().Nodes().Get(hcm.holderIdentity, metav1.GetOptions{}); err == nil { |
There was a problem hiding this comment.
can we get node.UID from yurt-hub env instead of getting from kube-apiserver?
There was a problem hiding this comment.
the env of yurt-hub have not node.UID.
| return lease | ||
| } | ||
|
|
||
| func (hcm *healthCheckerManager) storageLease(lease *coordinationv1.Lease) error { |
There was a problem hiding this comment.
i think we only need to update lease by using storageWrapper directly, not need to verify resourceVersion.
| return err | ||
| } | ||
| klog.Infof("%d. run health checker for remote servers", trace) | ||
| healthChecker.Run() |
There was a problem hiding this comment.
why not add healthChecker.Run() after healthchecker.NewHealthChecker()?
| if !rp.cacheMgr.CanCacheFor(req) { | ||
| klog.Errorf("can not cache for %s", util.ReqString(req)) | ||
| rw.WriteHeader(http.StatusBadGateway) | ||
| return |
There was a problem hiding this comment.
how about write some error message to rw?
There was a problem hiding this comment.
Refer to the default errHandler, do not write error messages. only return 502
pkg/yurthub/proxy/remote/remote.go
Outdated
| var errInfo error | ||
| obj, errQuery := rp.cacheMgr.QueryCache(req) | ||
| if errQuery == storage.ErrStorageNotFound { | ||
| reqInfo, _ := apirequest.RequestInfoFrom(req.Context()) |
There was a problem hiding this comment.
how about use info directly?
| } | ||
| } | ||
| rw.WriteHeader(http.StatusBadGateway) | ||
| } |
There was a problem hiding this comment.
how about write some error message to rw?
There was a problem hiding this comment.
Refer to the default errHandler, do not write error messages. only return 502
pkg/yurthub/proxy/util/util.go
Outdated
| var timeout time.Duration | ||
| if info.Verb == "watch" { | ||
| timeout = time.Duration(*opts.TimeoutSeconds+watchTimeoutMargin) * time.Second | ||
| } else if *opts.TimeoutSeconds > getAndListTimeoutReduce { |
There was a problem hiding this comment.
please add ut testcases for get/list request
zyjhtangtang
force-pushed
the
master
branch
3 times, most recently
from
6eb7888 to
8b5cccf
Compare
zyjhtangtang
changed the title
using lease for cluster remote server healthz checker.
Ⅰ. Describe what this PR does
In yurthub, the existing cloud-side network check uses the health check interface of apiserver. Because the data packet of the health check interface is very small, when the cloud edge weak network, the health check interface can request data normally, but other requests cannot. It is inaccurate for health check. Therefore, the lease request is used to do a health check.
（1）Yurthub sends a lease request to report the heartbeat of the node, and serves as the basis for the health of the cloud-side network.
（2）Yurthub caches lease data, and kubelet's lease requests all use locally cached data.
（3）Add interface Run() for HealthChecker.
（4）The initialization certificate no longer depends on the health check.
（5）Adjust the startup sequence of the health check module in yurthub.
（6）When the request from the cloud fails, the data is fetched from the local cache.
（7）Add UT.
Ⅱ. Does this pull request fix one issue?
NONE
Optimize cloud-side network inspection for yurthub.
Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews