fix: cache the server version info of kubernetes #936

Sodawyx · 2022-07-31T11:55:43Z

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:
/kind bug
/kind documentation
/kind enhancement
/kind good-first-issue
/kind feature
/kind question
/kind design
/sig ai
/sig iot
/sig network
/sig storage

What this PR does / why we need it:

cache the server version info of the kubernetes, and fix the issue of coreDNS is abnormal when edge node restart

Which issue(s) this PR fixes:

Fixes #880

Special notes for your reviewer:

Does this PR introduce a user-facing change?

other Note

openyurt-bot · 2022-07-31T11:55:47Z

@Sodawyx: GitHub didn't allow me to assign the following users: your_reviewer.

Note that only openyurtio members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:
/kind bug
/kind documentation
/kind enhancement
/kind good-first-issue
/kind feature
/kind question
/kind design
/sig ai
/sig iot
/sig network
/sig storage

What this PR does / why we need it:

cache the server version info of the kubernetes, and fix the issue of coreDNS is abnormal when edge node restart

Which issue(s) this PR fixes:

Fixes #880

Special notes for your reviewer:

Does this PR introduce a user-facing change?
other Note

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Congrool · 2022-08-01T00:51:07Z

Thanks for your contribution, I'll take a look.

pkg/yurthub/cachemanager/cache_manager.go

pkg/yurthub/proxy/local/local.go

pkg/yurthub/cachemanager/cache_manager.go

Sodawyx · 2022-08-01T06:27:58Z

Hi, I have fixed the codes and commit again! Please have a look and take a review!

rambohe-ch · 2022-08-01T09:19:21Z

@Sodawyx beside /version request, we need to handle apps/discovery.k8s.io/v1 and apps/discovery.k8s.io/v1beta1 resource requests.

codecov · 2022-08-01T09:21:16Z

Codecov Report

❗ No coverage uploaded for pull request base (master@ccf6a85). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #936   +/-   ##
=========================================
  Coverage          ?   39.69%           
=========================================
  Files             ?       83           
  Lines             ?    11234           
  Branches          ?        0           
=========================================
  Hits              ?     4459           
  Misses            ?     6444           
  Partials          ?      331

Flag	Coverage Δ
unittests	`39.69% <0.00%> (?)`

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

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

rambohe-ch · 2022-08-01T09:27:50Z

@Sodawyx I think it's not a good idea to modify local proxy and cache manager, maybe we can handle the above requests by adding new handlers only. take /version for example:

// 1. add http handler for `/version` in yurthub proxy server

// 2. the function for version handler as following:
func versionHandler(w http.ResponseWriter, req *http.Request) {
      // 1. when local cache has version info
      if inLocalCache() {
          return storageWrapper.GetRaw("serverversion")
      }

      // 2. no version info in local cache
     v := client.GetServerVersion

     // 3. cache server version info in local cache
     storageWrapper.UpdateRaw(v)

    return v
}

Congrool · 2022-08-01T09:58:49Z

@rambohe-ch Sounds good, it can simplify the logic in CacheManager. Since proxyServer and yurthub server listen at different port, it may need another redirection in local proxy.

And for apps/discovery.k8s.io/v1, apps/discovery.k8s.io/v1beta1, does coreDNS need it to get start? If not, I think we can deal with it in another pr. Because the original idea of this pr is to enable coreDNS to run when cloud-edge disconnects.

rambohe-ch · 2022-08-01T10:21:18Z

@rambohe-ch Sounds good, it can simplify the logic in CacheManager. Since proxyServer and yurthub server listen at different port, it may need another redirection in local proxy.

And for apps/discovery.k8s.io/v1, apps/discovery.k8s.io/v1beta1, does coreDNS need it to get start? If not, I think we can deal with it in another pr. Because the original idea of this pr is to enable coreDNS to run when cloud-edge disconnects.

@Congrool we need to add new handler for ProxyServer not yurthub server. and apps/discovery.k8s.io/v1 and apps/discovery.k8s.io/v1beta1 are needed for CoreDNS startup.

Congrool · 2022-08-01T10:25:31Z

@Congrool we need to add new handler for ProxyServer not yurthub server. and apps/discovery.k8s.io/v1 and apps/discovery.k8s.io/v1beta1 are needed for CoreDNS startup.

Well, I mean all requests from edge will be sent to proxy server. If we want to handle them in yurthub server, we need another redirection to send them to yurthub server.

Congrool · 2022-08-01T10:31:15Z

By the way, if we handle /version in yurthub server, it's somewhere counterintuitive. If some client visit the /version of yurthub server, it may expect to get the version info of yurthub itself but actually get APIServer version.

So why not add the /version handler at proxy server?

rambohe-ch · 2022-08-01T12:11:45Z

@Congrool yes, we need to add /version handler in yurthub proxy server for getting version info of kube-apiserver. and i have updated the above example as following:

Congrool · 2022-08-01T13:07:15Z

OK, my misundertanding😢.
agree+1

rambohe-ch · 2022-08-02T03:43:10Z

@Sodawyx small modifications for comments:

apps/discovery.k8s.io/v1 --> apis/discovery.k8s.io/v1 
apps/discovery.k8s.io/v1beta1 --> apis/discovery.k8s.io/v1beta1

Sodawyx · 2022-08-02T10:39:47Z

@Sodawyx small modifications for comments:

apps/discovery.k8s.io/v1 --> apis/discovery.k8s.io/v1 
apps/discovery.k8s.io/v1beta1 --> apis/discovery.k8s.io/v1beta1

Yeah, I got it!

Sodawyx · 2022-08-03T03:19:34Z

Hi @Congrool @rambohe-ch , I have update the code in the new commits.

pkg/yurthub/proxy/util/util.go

Congrool · 2022-08-03T04:29:42Z

I think the disscussion above means to add a new http handler for proxy server to handle /version request.
Something like:

	mux := http.NewServeMux()
	mux.HandleFunc("/version", versionHandleFunc)
        mux.HandleFunc("/apis/discovery.k8s.io/v1", discoveryFunc)
        ...

rambohe-ch · 2022-08-03T06:40:08Z

I think the disscussion above means to add a new http handler for proxy server to handle /version request. Something like:
	mux := http.NewServeMux()
	mux.HandleFunc("/version", versionHandleFunc)
        mux.HandleFunc("/apis/discovery.k8s.io/v1", discoveryFunc)
        ...

@Sodawyx yes, i agree with the @Congrool's opinion that we should add new handlers based on proxy handler like as following:

var nonResourceReqPaths = []string{
    "/version",
    "/apis/discovery.k8s.io/v1",
    "/apis/discovery.k8s.io/v1beta1",
}

func wrapNonResourceHandler(proxyHandler http.Handler) http.Handler {
	wrapMux := mux.NewRouter()

	// register handler for non resource requests
        for i := range nonResourceReqPaths {
             wrapMux.HandleFunc(nonResourceReqPaths[i], nonResourceHandler).Methods("GET")
        }

	// register handler for other requests
	wrapMux.PathPrefix("/").Handler(proxyHandler)
	return wrapMux
}

Sodawyx · 2022-08-03T08:17:16Z

Very appreciate for your suggestions, I will try to improve it.

Sodawyx · 2022-08-04T06:48:30Z

Hi @Congrool and @rambohe-ch, I have update a commit according to your suggestions. Please take a review and I will implement the unit test later!

pkg/yurthub/server/nonresourceproxy.go

rambohe-ch · 2022-08-08T03:21:56Z

@Sodawyx please add unit tests for newly added code.

pkg/yurthub/server/nonresourceproxy.go

pkg/yurthub/server/nonresource_test.go

rambohe-ch · 2022-08-09T03:07:46Z

pkg/yurthub/server/server.go

+	restCfg := rest.GetRestConfig(false)
+	clientSet, err := kubernetes.NewForConfig(restCfg)
+	if err != nil {
+		klog.Warningf("cannot create the client set: %v", clientSet)


if the kube-client can not created, it's more reasonable to return here, not just log warning.

If the network is disconnected, the program will be continue to execute and query the cache for the non-resource info. And I think the error should not be return.

I think kubernetes.NewForConfig is used for creating client only, and do not access the remote kube-apiserver.

I think you are right! And I will return the error.

pkg/yurthub/server/nonresource.go

rambohe-ch · 2022-08-09T07:26:28Z

@Sodawyx please fix errors from github ci pipeline.

pkg/yurthub/server/nonresource.go

pkg/yurthub/server/nonresource_test.go

rambohe-ch · 2022-08-14T03:41:19Z

/lgtm
/approve

openyurt-bot · 2022-08-14T03:41:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rambohe-ch, Sodawyx

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:

~~OWNERS~~ [rambohe-ch]

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

openyurt-bot · 2022-08-14T03:41:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rambohe-ch, Sodawyx

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:

~~OWNERS~~ [rambohe-ch]

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

rambohe-ch · 2022-08-30T14:16:01Z

/backport release-v0.7

github-actions · 2022-08-30T14:16:33Z

Successfully created backport PR #977 for release-v0.7.

fix: cache the server version info of kubernetes

1112bf3

openyurt-bot requested review from charleszheng44 and Fei-Guo July 31, 2022 11:55

openyurt-bot added the size/M size/M: 30-99 label Jul 31, 2022

Congrool reviewed Aug 1, 2022

View reviewed changes

fix: fix the errors according to the comments

4f79e7e

fix: fix the bug of cache the non-resource info update

41292c2

openyurt-bot added size/L size/L: 100-499 and removed size/M size/M: 30-99 labels Aug 3, 2022

fix: delete the change before

ec2880c

Congrool reviewed Aug 3, 2022

View reviewed changes

pkg/yurthub/proxy/util/util.go Outdated Show resolved Hide resolved

fix: update the change

c9bf2e1

Congrool reviewed Aug 4, 2022

View reviewed changes

rambohe-ch reviewed Aug 8, 2022

View reviewed changes

pkg/yurthub/server/nonresourceproxy.go Outdated Show resolved Hide resolved

fix: update the change

c90db6b