Implement listwatchresources API on the simulator #165

196Ikuchil · 2022-05-08T16:17:23Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Implement new API endpoint /api/v1/listwatchresources on the simulator.

This endpoint send all resources's event data to a client by Server-Sent Events.
We can specify ' lastResourceVersion`.
If we will not specify it, the endpoint calls list API and returns the result before starting watch.

Which issue(s) this PR fixes:

Fixes #120

Special notes for your reviewer:

The implementation of the WebUI side is divided into separate PRs.

/label tide/merge-method-squash

k8s-ci-robot · 2022-05-08T16:17:26Z

@196Ikuchil: The label(s) /label do-not-merge/work-in-progress] cannot be applied. These labels are supported: api-review, tide/merge-method-merge, tide/merge-method-rebase, tide/merge-method-squash, team/katacoda, refactor

In response to this:

What type of PR is this?

/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #120

Special notes for your reviewer:

Work in progress

/label tide/merge-method-squash
/label do-not-merge/work-in-progress]

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.

196Ikuchil · 2022-05-26T16:32:08Z

/retitle Update all resources periodically on WebUI

k8s-ci-robot · 2022-05-26T16:36:14Z

@196Ikuchil: The label(s) /remove-label do-not-merge/work-in-progress cannot be applied. These labels are supported: api-review, tide/merge-method-merge, tide/merge-method-rebase, tide/merge-method-squash, team/katacoda, refactor

In response to this:

/remove-label do-not-merge/work-in-progress

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.

196Ikuchil · 2022-05-26T16:38:16Z

This PR includes #178.
So, we need to merge it first.
/hold

196Ikuchil · 2022-06-30T14:08:33Z

watcher/watcher.go

+	go s.WatchAndHandleEvent(pcsEventProxy, pcsWatcher, ctx.Done())
+
+	<-ctx.Done()
+	return xerrors.Errorf("terminated to watch: %w", ctx.Err())


<-ctx.Done()

WatchResources will return an error when the frontend browser is reloaded or call cancel() method.
This is to allow the front end to handle stream connection disconnections.

test code

<-ctx.Done() klog.Info("done") ## this return xerrors.Errorf("terminated to watch: %w", ctx.Err())

When the browser reloading

Server's log

I0630 13:38:17.417764 20024 watcher.go:121] done E0630 13:38:17.417823 20024 watcher.go:39] closed to watch resources: terminated to watch: github.com/kubernetes-sigs/kube-scheduler-simulator/watcher.(*Service).WatchResources /workspace/kube-scheduler-simulator/watcher/watcher.go:122 - context canceled {"time":"2022-06-30T13:38:17.417908424Z","id":"","remote_ip":"127.0.0.1","host":"localhost:1212","method":"GET","uri":"/api/v1/watchresources?podsLastResourceVersion=498&nodesLastResourceVersion=498&pvsLastResourceVersion=498&pvcsLastResourceVersion=498&scsLastResourceVersion=498&pcsLastResourceVersion=498","user_agent":"Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Firefox/102.0","status":200,"error":"code=500, message=Internal Server Error","latency":67046223926,"latency_human":"1m7.046223926s","bytes_in":0,"bytes_out":0}

When cancel() calling

We expect the method to be invoked like this.

watcherAPI .watchResources(createLastResourceVersions() as LastResourceVersions) .then((response) => { if (!response.body) { return; } const stream = response.body.getReader(); const utf8Decoder = new TextDecoder("utf-8"); let buffer = ""; return stream.read().then(function processText({ done, value }): any { if (isfinish) { stream.cancel(); ## this return }

The result of when invoked the cancel().

I0630 13:49:16.241880 23480 watcher.go:121] done E0630 13:49:16.241923 23480 watcher.go:39] closed to watch resources: terminated to watch: github.com/kubernetes-sigs/kube-scheduler-simulator/watcher.(*Service).WatchResources /workspace/kube-scheduler-simulator/watcher/watcher.go:122 - context canceled {"time":"2022-06-30T13:49:16.241962479Z","id":"","remote_ip":"127.0.0.1","host":"localhost:1212","method":"GET","uri":"/api/v1/watchresources?podsLastResourceVersion=214&nodesLastResourceVersion=214&pvsLastResourceVersion=214&pvcsLastResourceVersion=214&scsLastResourceVersion=214&pcsLastResourceVersion=214","user_agent":"Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Firefox/102.0","status":200,"error":"code=500, message=Internal Server Error","latency":16756917245,"latency_human":"16.756917245s","bytes_in":0,"bytes_out":3346}

Nice 👍👍👍

Can you add a comment for this? (A brief summary is enough.)

sanposhiho

Rough reviews only for backend side

docs/api.md

server/di/di.go

server/handler/watcher.go

watcher/watcher.go

sanposhiho · 2022-07-06T00:40:16Z

watcher/watcher.go

+	go s.WatchAndHandleEvent(pcsEventProxy, pcsWatcher, ctx.Done())
+
+	<-ctx.Done()
+	return xerrors.Errorf("terminated to watch: %w", ctx.Err())


Nice 👍👍👍

sanposhiho · 2022-07-06T00:41:13Z

watcher/watcher.go

+	go s.WatchAndHandleEvent(pcsEventProxy, pcsWatcher, ctx.Done())
+
+	<-ctx.Done()
+	return xerrors.Errorf("terminated to watch: %w", ctx.Err())


Can you add a comment for this? (A brief summary is enough.)

watcher/watcher.go

sanposhiho · 2022-07-06T00:48:40Z

docs/api.md

+|parameter|requirement|description|
+| ----- | --- | -------- |
+|podsLastResourceVersion|MUST|integer|
+|nodesLastResourceVersion|MUST|integer|
+|pvsLastResourceVersion|MUST|integer|
+|pvcsLastResourceVersion|MUST|integer|
+|scsLastResourceVersion|MUST|integer|
+|pcsLastResourceVersion|MUST|integer|


Making these parameters required sounds weird to me.
How about changing it to optional and when not specifying them, start watching with 0.

hmm, I don't think so.

Our /watchresources API use NewRetryWatcher in internal logic.
The NewRetryWatcher will return error if we pass the "0" to a lastResourceVersion. If these values are optional, Isn't this likely to be a cause of error?
https://github.com/kubernetes/client-go/blob/3b969f96803febcac81f3a4272a9914270fbb3a3/tools/watch/retrywatcher.go#L66

Also, we shouldn't modify these value.

Resource versions must be treated as opaque by clients and passed unmodified back to the server.
https://kubernetes.io/docs/reference/using-api/api-concepts/#resource-versions

Therefore, I think it is generally expected that this watch API will be called after the list API is called. And at least, I'd liket to require it in this simulator.

BTW, string might be better than integer.

You must not assume resource versions are numeric or collatable.

Our /watchresources API use NewRetryWatcher in internal logic.
The NewRetryWatcher will return error if we pass the "0" to a lastResourceVersion. If these values are optional, Isn't this likely to be a cause of error?

That makes sense.

But, still the API looks unkind to clients.
What I expect to see in this API is something like kubectl get Pods -w. (= After listing/getting the requested object, watch for changes.)

How about deleting these last resource versions params completely?
In most use cases, the clients need to list all resources directly from kube-apiserver and then call /watchresources after that right?
If so, we don't need to let clients send the last resource versions to server. Just listing all supported objects and start to watch for changes looks more kind to clients.

If so, we don't need to let clients send the last resource versions to server. Just listing all supported objects and start to watch for changes looks more kind to clients.

Good question.
Your proposed feature is already provided as this method.
https://pkg.go.dev/k8s.io/client-go/tools/cache#NewInformer

I use NewRetryWatcher because it allows me to specify the lastresourceversion at the start of the watch.

In the future, we will be able to operate several simulators at once.
At that time, it is not desirable to keep WATCHING all simulators.
That is, watch needs to stop once, and it needs to be possible to resume watch from the middle of the previous one.
And then, In order to restart the watch from the middle, the lastresourceversion needs to be specifiable from the client.

My new idea was to make the lastresourcecersion optional parameter, and if not specified, to run the list on the server side, return the result as an Added event to the client, and then start the watch.

The disadvantage of this is that if the client is already running a listing and does not specify lastresourceversion, the resource passed to the client will be duplicated.

WDYT?

See also this article.
https://kubernetes.io/docs/reference/using-api/api-concepts/#efficient-detection-of-changes

Ah~, okay. I think now I fully understand your idea.

My new idea was to make the lastresourcecersion optional parameter, and if not specified, to run the list on the server side, return the result as an Added event to the client, and then start the watch.

After understood your idea completely, I guess your previous idea, making lastresourcecersion required and using /watchresources only to notice the update, also makes some sense.

But, hmmmm, it's a difficult question. I think there are pros/cons to each strategy.

making lastresourcecersion optional

This is your new idea.

pros
web UI only need to communicate with /watchresources API to fetch the resources. Not need to call kube-apiserver's list API anymore.
That makes the web UI's implementation much simpler, since the client performs the same logic regardless of whether it is the first time access or not. What web UI always needs to do to fetch resources/update resources is only using /watchresources API.

cons
When a user accesses the web UI for the first time, web UI needs to build the resource's view by processing many Added event for all resources one by one.
So, If there are too many resources, the building may take some time.

making lastresourcecersion required

This is your previous idea.

pros

When a user accesses the web UI for the first time, web UI calls list API of kube-apiserver.
By processing another logic for the first time access, the time that takes for first view will be decreased especially when the number of resources is large.

cons

Web UI needs to have two logics to show resources; one is for first-time access, and another is for /watchresources .

So, to summarize, your new idea vs your previous idea looks like maintainability vs performance.

196Ikuchil · 2022-07-09T09:58:05Z

Fixed. (Also some conflicts.)

sanposhiho

Only server side

docs/api.md

simulator/resourcewatcher/resourcewatcher.go

sanposhiho · 2022-07-09T11:52:10Z

simulator/resourcewatcher/resourcewatcher.go

+				case watch.Deleted:
+					eventType = watch.Deleted
+				default:
+					// we may receive watch.Bookmark and watch.Error events.


should watch.Error be logged as error logs?

sanposhiho · 2022-07-09T11:55:28Z

simulator/resourcewatcher/resourcewatcher_test.go

@@ -0,0 +1,484 @@
+package resourcewatcher_test


I think this test does not need to be written in a separate test package.
Sometimes separate test package strategy is useful to check the behavior from outside of the package, but in most cases, this only increases the test complexity/difficulties.
This time, we need to create export_test.go for the test and it's a non-good signal.

The reason I changed the package name was because of a circular reference to a file in the mock file. (I don't remember exactly, but I'm pretty sure it occurred in ResponseStream or somewhere)

Hm, it doesn't make sense to change test to be in separate package only for solving import cycles.

I'm not sure but seems like StreamWriter is one cause...? (Because it uses WatchEvent in its definition.)
If so, one idea to solve this is just to create another package for StreamWriter and move the related implementation into that new package.

simulator/server/server.go

simulator/server/handler/watcher.go

196Ikuchil

Hi @sanposhiho
I updated it.

196Ikuchil · 2022-07-24T05:43:40Z

docs/api.md

+| pvsLastResourceVersion   | OPTIONAL    | If not specified, some existing resource data is returned as `ADDED` Events before starting watch. |
+| pvcsLastResourceVersion  | OPTIONAL    | If not specified, some existing resource data is returned as `ADDED` Events before starting watch. |
+| scsLastResourceVersion   | OPTIONAL    | If not specified, some existing resource data is returned as `ADDED` Events before starting watch. |
+| pcsLastResourceVersion   | OPTIONAL    | If not specified, some existing resource data is returned as `ADDED` Events before starting watch. |


I changed these queries to OPTIONAL.

196Ikuchil · 2022-07-24T05:45:34Z

simulator/resourcewatcher/resourcewatcher.go

+func (s *Service) ListAndWatch(p ResourceEventProxy, stopCh <-chan struct{}) error {
+	lw := createListWatch(p)
+	// If the lastResourceVersion isn't specified by client, call the list and return the result as ADDED event first.
+	if p.LastResourceVersion() == "" {


If a client will not set the lastresourceversion, the value will be "".
And then the server calls list method of the resource before starting the watch.

196Ikuchil · 2022-07-24T05:51:23Z

web/store/helpers/storeHelper.ts

+type ResourceState<T extends resourceObject> = Array<T>;
+
+// createResourceState returns a new resource store's state.
+export function createResourceState<T extends resourceObject>(


Almost resources will use this page's methods to handle Events. (Pod is an exception.)
This is because the Pod needs to consider to unscheduled pod.

sanposhiho

only for backend

simulator/docs/api.md

simulator/resourcewatcher/resourcewatcher.go

196Ikuchil · 2022-07-25T12:04:43Z

@sanposhiho Thanks! Fixed.

sanposhiho

@196Ikuchil
Looks good. Because this PR is so big, could you create the new PR for the frontend and change this PR to only includes the backend changes?
After you do that, I'm gonna approve this.

/lgtm

196Ikuchil · 2022-08-13T14:32:46Z

/retitle Implement listwatchresources API on the simulator

196Ikuchil · 2022-08-13T14:54:16Z

@sanposhiho
Thank you for the comment.
I move some implementations of webUI to the new PR #211.

sanposhiho · 2022-08-13T16:10:55Z

/lgtm
/apprive

sanposhiho · 2022-08-13T16:10:59Z

/approve

k8s-ci-robot · 2022-08-13T16:11:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 196Ikuchil, sanposhiho

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~~ [sanposhiho]

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. kind/feature Categorizes issue or PR as related to a new feature. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. labels May 8, 2022

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 8, 2022

k8s-ci-robot requested a review from adtac May 8, 2022 16:17

k8s-ci-robot added the sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. label May 8, 2022

k8s-ci-robot requested a review from sanposhiho May 8, 2022 16:17

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 8, 2022

196Ikuchil force-pushed the auto_update_webui branch from 03758b8 to 6d3c581 Compare May 15, 2022 11:17

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

196Ikuchil force-pushed the auto_update_webui branch from 8ac3525 to 7129e95 Compare May 15, 2022 15:07

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 16, 2022

196Ikuchil force-pushed the auto_update_webui branch from 6907053 to 32444ff Compare May 16, 2022 11:39

196Ikuchil marked this pull request as draft May 16, 2022 12:44

196Ikuchil force-pushed the auto_update_webui branch 2 times, most recently from 404ddc8 to d5d6e0b Compare May 16, 2022 15:51

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 17, 2022

196Ikuchil force-pushed the auto_update_webui branch from 3f30a33 to f931237 Compare May 25, 2022 12:01

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

196Ikuchil force-pushed the auto_update_webui branch from f931237 to fa9641f Compare May 26, 2022 16:25

k8s-ci-robot changed the title ~~[WIP]Update all resources periodically on WebUI~~ Update all resources periodically on WebUI May 26, 2022

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 26, 2022

196Ikuchil marked this pull request as ready for review May 26, 2022 16:39

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 May 26, 2022

196Ikuchil commented Jun 30, 2022

View reviewed changes

sanposhiho reviewed Jul 6, 2022

View reviewed changes

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 8, 2022

sanposhiho reviewed Jul 9, 2022

View reviewed changes

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 10, 2022

196Ikuchil commented Jul 24, 2022

View reviewed changes

sanposhiho reviewed Jul 24, 2022

View reviewed changes

sanposhiho reviewed Aug 13, 2022

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 13, 2022

196Ikuchil force-pushed the auto_update_webui branch from ea1c4a2 to 9d05cc2 Compare August 13, 2022 13:41

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Aug 13, 2022

196Ikuchil force-pushed the auto_update_webui branch from 9d05cc2 to ea1c4a2 Compare August 13, 2022 13:51

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

feat:add list and watch all resources api on the simulator

7af062a

196Ikuchil force-pushed the auto_update_webui branch from ea1c4a2 to 7af062a Compare August 13, 2022 14:20

k8s-ci-robot changed the title ~~Update all resources periodically on WebUI~~ Implement listwatchresources API on the simulator Aug 13, 2022

196Ikuchil mentioned this pull request Aug 13, 2022

Implement to update all resources periodically on Frontend #211

Merged

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 13, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 13, 2022

k8s-ci-robot merged commit 030391e into kubernetes-sigs:master Aug 13, 2022

Implement listwatchresources API on the simulator #165

Implement listwatchresources API on the simulator #165

Conversation

196Ikuchil commented May 8, 2022 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

k8s-ci-robot commented May 8, 2022

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

196Ikuchil commented May 26, 2022

k8s-ci-robot commented May 26, 2022

196Ikuchil commented May 26, 2022

Choose a reason for hiding this comment

<-ctx.Done()

test code

When the browser reloading

When cancel() calling

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanposhiho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

196Ikuchil Jul 13, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanposhiho Jul 16, 2022 • edited

Choose a reason for hiding this comment

making lastresourcecersion optional

making lastresourcecersion required

sanposhiho Jul 16, 2022 • edited

Choose a reason for hiding this comment

196Ikuchil commented Jul 9, 2022

sanposhiho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

196Ikuchil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanposhiho left a comment

Choose a reason for hiding this comment

196Ikuchil commented Jul 25, 2022

sanposhiho left a comment

Choose a reason for hiding this comment

196Ikuchil commented Aug 13, 2022

196Ikuchil commented Aug 13, 2022

sanposhiho commented Aug 13, 2022

sanposhiho commented Aug 13, 2022

k8s-ci-robot commented Aug 13, 2022

196Ikuchil commented May 8, 2022 •

edited

`<-ctx.Done()`

When `cancel()` calling

196Ikuchil Jul 13, 2022 •

edited

sanposhiho Jul 16, 2022 •

edited

sanposhiho Jul 16, 2022 •

edited