Draft implementation for watch support #28

ityuhui · 2020-09-09T10:06:39Z

This is a draft implementation for watch support. We can discuss the design here.

This PR should not be merged because a part of code should be merged to generator repo https://github.com/OpenAPITools/openapi-generator/tree/master/modules/openapi-generator/src/main/resources/C-libcurl

ityuhui · 2020-09-09T10:14:18Z

Hi @clearday4

Cloud you please review the design (especially for the examples/watch_list_pod) too?

ityuhui · 2020-09-09T10:16:11Z

Append the output of examples/watch_list_pod

watch event raw string:
{"type":"ADDED","object":{"kind":"Pod","apiVersion":"v1","metadata":{"name":"test-p5","namespace":"default","selfLink":"/api/v1/namespaces/default/pods/test-p5","uid":"4a59346d-4351-4a94-b5b0-5c93afb74d3e","resourceVersion":"36874225","creationTimestamp":"2020-03-18T09:21:54Z"},"spec":{"volumes":[{"name":"default-token-8p4h4","secret":{"secretName":"default-token-8p4h4","defaultMode":420}}],"containers":[{"name":"my-container","image":"ubuntu:16.04","command":["sleep"],"args":["3600"],"resources":{},"volumeMounts":[{"name":"default-token-8p4h4","readOnly":true,"mountPath":"/var/run/secrets/kubernetes.io/serviceaccount"}],"terminationMessagePath":"/dev/termination-log","terminationMessagePolicy":"File","imagePullPolicy":"IfNotPresent"}],"restartPolicy":"Always","terminationGracePeriodSeconds":30,"dnsPolicy":"ClusterFirst","serviceAccountName":"default","serviceAccount":"default","nodeName":"yhu1a","securityContext":{},"schedulerName":"default-scheduler","tolerations":[{"key":"node.kubernetes.io/not-ready","operator":"Exists","effect":"NoExecute","tolerationSeconds":300},{"key":"node.kubernetes.io/unreachable","operator":"Exists","effect":"NoExecute","tolerationSeconds":300}],"priority":0,"enableServiceLinks":true},"status":{"phase":"Running","conditions":[{"type":"Initialized","status":"True","lastProbeTime":null,"lastTransitionTime":"2020-03-18T09:21:54Z"},{"type":"Ready","status":"True","lastProbeTime":null,"lastTransitionTime":"2020-09-09T10:08:52Z"},{"type":"ContainersReady","status":"True","lastProbeTime":null,"lastTransitionTime":"2020-09-09T10:08:52Z"},{"type":"PodScheduled","status":"True","lastProbeTime":null,"lastTransitionTime":"2020-03-18T09:21:54Z"}],"hostIP":"9.111.254.254","podIP":"10.244.0.5","podIPs":[{"ip":"10.244.0.5"}],"startTime":"2020-03-18T09:21:54Z","containerStatuses":[{"name":"my-container","state":{"running":{"startedAt":"2020-09-09T10:08:52Z"}},"lastState":{"terminated":{"exitCode":0,"reason":"Completed","startedAt":"2020-09-09T09:08:51Z","finishedAt":"2020-09-09T10:08:51Z","containerID":"docker://23702d68fda5f98a2035f03139692e32ebb4c9968f2997a5c83a4904b8e59776"}},"ready":true,"restartCount":4054,"image":"ubuntu:16.04","imageID":"docker-pullable://ubuntu@sha256:bb5b48c7750a6a8775c74bcb601f7e5399135d0a06de004d000e05fd25c1a71c","containerID":"docker://b81c2afbd7cb0549d8cd5fe175c83778eef64450252bb44be36ce3d4140e8f8e","started":true}],"qosClass":"BestEffort"}}}

type: ADDED
pod:
        name: test-p5

brendandburns · 2020-09-09T16:12:48Z

kubernetes/watch/watch_util.h

+extern "C" {
+#endif
+
+void kubernets_watch_handler(void** pData, long* pDataLen, void (*event_hander)(const char*));


Do we want to typedef the event handler callback?

Sure. It's better. I will do it.

brendandburns · 2020-09-09T16:14:39Z

kubernetes/src/apiClient.c

@@ -452,6 +456,10 @@ size_t writeDataCallback(void *buffer, size_t size, size_t nmemb, void *userp) {
    apiClient->dataReceived = (char *)realloc( apiClient->dataReceived, apiClient->dataReceivedLen + size_this_time + 1);
    memcpy(apiClient->dataReceived + apiClient->dataReceivedLen, buffer, size_this_time);
    apiClient->dataReceivedLen += size_this_time;
+    ((char*)apiClient->dataReceived)[apiClient->dataReceivedLen] = '\0'; // the space size of (apiClient->dataReceived) = dataReceivedLen + 1
+    if (apiClient->watch_func) {


I think the way to do this without modifying the API client is to split the api invoke into two phases:

Prepare the call

Execute the call

That way the watch code can run #1 from the generated code, but then take over the call to get access to the raw stream.

It's not going to be maintainable long term to add this to the generated code as it is Kubernetes specific.

Hi @brendandburns

Do you mean splitting the generator function apiClient_invoke to 2 parts ?

I do not find a way to "take over" the call to get access to the raw stream in kubernetes-client/c because curl_easy_perform in apiClient_invoke of watch request will never return to caller. So I have to add the callback function into writeDataCallback

I think apiClient->watch_func is not Kubernetes specific, other swagger client can use this function to support watch,
watch/watch_util.{h,c} will not be merged into openapi c generator because they are Kubernetes specific.

Yes, I'm suggesting breaking apiClient_invoke into two parts:

// the whole invoke function, currently a single function is split into two parts. apiClient_invoke(...) { // Get the curl call ready CURL *curlRequest = apiClient_prepare(...); // actually make the curl call apiClient_call(curlRequest); } // Can be called separately to just prepare but not execute a call. CURL *apiClient_prepare(...) {} // Probably not used by users. apiClient_call(...) {}

Note that this separation will also be useful for port-forward/logs/etc which don't follow REST standards.

Hi @brendandburns

After splitting apiClient_invoke to apiClient_prepare and apiClient_call, how can watch code take over to access to the raw stream ?

In current apiClient_invoke

res = curl_easy_perform(handle); // <--- curl_easy_perform will never return when "watch"

Do you mean watch code implements its writeDataCallback, non-watch request uses generator's writeDataCallback

I agree. how about using curl_multi_perform instead of using curl_easy_perform?
or set timeout like curl_easy_setopt(curl_, CURLOPT_TIMEOUT, timeout_)
currently curl_easy_perform will be blocked and never return.

@brendandburns
Suppose to use curl_multi_perform in apiClient_call()

apiClient_call(...) { ... do { int numfds=0; int res = curl_multi_wait(cm, NULL, 0, MAX_WAIT_MSECS, &numfds); if(res != CURLM_OK) { fprintf(stderr, "error: curl_multi_wait() returned %d\n", res); return EXIT_FAILURE; } curl_multi_perform(cm, &still_running); } while(still_running); ... }

How does the watch code take over to access the raw stream ?

I will try to implement it now.

@clearday4

I agree. how about using curl_multi_perform instead of using curl_easy_perform?
or set timeout like curl_easy_setopt(curl_, CURLOPT_TIMEOUT, timeout_)
currently curl_easy_perform will be blocked and never return.

Setting timeout is not a solution, after curl_easy_perform returns after timeout, the http connection will disconnect, watch events will never come, this is not right for the watch function.
Actually, curl_easy_perform blocking is accaptable here, because only one http request is working at this time. Although using curl_multi_perform , we still need poll the request in a loop to get the watch event stream.

Hi @brendandburns

I tried to implement with curl_multi_* , but I found it still need inject a function to apiClient_call to access the raw stream, it will do more changes to api client.

For the watch, because we issue only one request at one time, curl_multi_* is not better than curl_easy_* , the function curl_multi_wait will return as long as there is data comming, so we need poll in a loop while(still_running)

So I suggest adopting my current solution:

Only add a callback function pointerdata_callback_func, it does small change to api client.

Other functions in watch/watch_util.{h,c} are committed to this repo (kubernetes-client/c)

If somebody has time to try a new solution with curl_multi_*, we can update/change this part in the future.

cc @clearday4

Your suggestions look good to me. Let's do it!

clearday4 · 2020-09-10T05:30:04Z

Hi @clearday4

Cloud you please review the design (especially for the examples/watch_list_pod) too?

Sure.
I'm pleased to review this function.
I will add a comment after review.

ityuhui · 2020-09-21T07:29:21Z

The callback function pointer data_callback_func will be added to api client by this PR OpenAPITools/openapi-generator#7467

brendandburns · 2020-09-22T04:32:53Z

Great, let's wait for that to merge and then we can get this PR merged too.

ityuhui · 2020-09-30T08:35:56Z

Hi @brendandburns

The code is updated after data_callback_func is merged from openapi-generator.
Could you please review the new code change ? Thanks

brendandburns · 2020-10-01T04:03:35Z

kubernetes/watch/watch_util.c

+void kubernets_watch_handler(void** pData, long* pDataLen, KUBE_WATCH_EVENT_HANDLER_FUNC event_hander)
+{
+    char* data = *(char**)pData;
+    //printf("dataLen=%ld, strlen(data)=%ld\n", *pDataLen, strlen(data));


Delete this debug line.

brendandburns · 2020-10-01T04:04:08Z

One small nit and then it's ready to merge.

ityuhui · 2020-10-01T12:12:29Z

The code change is updated.

brendandburns · 2020-10-01T15:21:58Z

/lgtm
/approve

Cool!

k8s-ci-robot · 2020-10-01T15:22:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brendandburns, ityuhui

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~~ [brendandburns,ityuhui]

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

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 9, 2020

k8s-ci-robot requested a review from brendandburns September 9, 2020 10:06

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 9, 2020

brendandburns reviewed Sep 9, 2020

View reviewed changes

ityuhui force-pushed the yh-watch-design branch from e983e6e to c780f06 Compare September 20, 2020 06:23

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 29, 2020

ityuhui force-pushed the yh-watch-design branch from c780f06 to b2ba27c Compare September 30, 2020 08:20

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 30, 2020

ityuhui force-pushed the yh-watch-design branch from b2ba27c to 2a034b5 Compare September 30, 2020 08:28

brendandburns changed the title ~~[WIP] Draft implementation for watch support~~ Draft implementation for watch support Oct 1, 2020

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 1, 2020

brendandburns reviewed Oct 1, 2020

View reviewed changes

[Watch] Support watch

be3b335

ityuhui force-pushed the yh-watch-design branch from 2a034b5 to be3b335 Compare October 1, 2020 12:10

k8s-ci-robot assigned brendandburns Oct 1, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 1, 2020

k8s-ci-robot merged commit d8d4bc8 into kubernetes-client:master Oct 1, 2020

ityuhui deleted the yh-watch-design branch October 3, 2020 01:57

ityuhui mentioned this pull request Oct 3, 2020

About watch function #21

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Draft implementation for watch support #28

Draft implementation for watch support #28

ityuhui commented Sep 9, 2020 •

edited

Loading

ityuhui commented Sep 9, 2020

ityuhui commented Sep 9, 2020

brendandburns Sep 9, 2020

ityuhui Sep 10, 2020

brendandburns Sep 9, 2020

ityuhui Sep 10, 2020 •

edited

Loading

brendandburns Sep 11, 2020 •

edited

Loading

brendandburns Sep 11, 2020

ityuhui Sep 14, 2020 •

edited

Loading

clearday4 Sep 15, 2020 •

edited

Loading

ityuhui Sep 15, 2020 •

edited

Loading

ityuhui Sep 15, 2020 •

edited

Loading

ityuhui Sep 18, 2020 •

edited

Loading

brendandburns Sep 19, 2020

clearday4 commented Sep 10, 2020

ityuhui commented Sep 21, 2020

brendandburns commented Sep 22, 2020

ityuhui commented Sep 30, 2020

brendandburns Oct 1, 2020

ityuhui Oct 1, 2020

brendandburns commented Oct 1, 2020

ityuhui commented Oct 1, 2020

brendandburns commented Oct 1, 2020

k8s-ci-robot commented Oct 1, 2020

Draft implementation for watch support #28

Draft implementation for watch support #28

Conversation

ityuhui commented Sep 9, 2020 • edited Loading

ityuhui commented Sep 9, 2020

ityuhui commented Sep 9, 2020

brendandburns Sep 9, 2020

Choose a reason for hiding this comment

ityuhui Sep 10, 2020

Choose a reason for hiding this comment

brendandburns Sep 9, 2020

Choose a reason for hiding this comment

ityuhui Sep 10, 2020 • edited Loading

Choose a reason for hiding this comment

brendandburns Sep 11, 2020 • edited Loading

Choose a reason for hiding this comment

brendandburns Sep 11, 2020

Choose a reason for hiding this comment

ityuhui Sep 14, 2020 • edited Loading

Choose a reason for hiding this comment

clearday4 Sep 15, 2020 • edited Loading

Choose a reason for hiding this comment

ityuhui Sep 15, 2020 • edited Loading

Choose a reason for hiding this comment

ityuhui Sep 15, 2020 • edited Loading

Choose a reason for hiding this comment

ityuhui Sep 18, 2020 • edited Loading

Choose a reason for hiding this comment

brendandburns Sep 19, 2020

Choose a reason for hiding this comment

clearday4 commented Sep 10, 2020

ityuhui commented Sep 21, 2020

brendandburns commented Sep 22, 2020

ityuhui commented Sep 30, 2020

brendandburns Oct 1, 2020

Choose a reason for hiding this comment

ityuhui Oct 1, 2020

Choose a reason for hiding this comment

brendandburns commented Oct 1, 2020

ityuhui commented Oct 1, 2020

brendandburns commented Oct 1, 2020

k8s-ci-robot commented Oct 1, 2020

ityuhui commented Sep 9, 2020 •

edited

Loading

ityuhui Sep 10, 2020 •

edited

Loading

brendandburns Sep 11, 2020 •

edited

Loading

ityuhui Sep 14, 2020 •

edited

Loading

clearday4 Sep 15, 2020 •

edited

Loading

ityuhui Sep 15, 2020 •

edited

Loading

ityuhui Sep 15, 2020 •

edited

Loading

ityuhui Sep 18, 2020 •

edited

Loading