sablierapp / sablier

Start your containers on demand, shut them down automatically when there's no activity. Docker, Docker Swarm Mode and Kubernetes compatible.
https://sablierapp.dev/
GNU Affero General Public License v3.0
1.36k stars 46 forks source link

Websocket PoC #242

Open tomaszduda23 opened 9 months ago

tomaszduda23 commented 9 months ago

It could be used to implement websocket https://github.com/acouvreur/sablier/issues/21

acouvreur commented 9 months ago

I would like to merge this feature into beta first, can you try to rebase your branch into beta first please ?

tomaszduda23 commented 9 months ago

This is just proof of concept to show how it could be done. It was never completed. I could make it work but https://github.com/acouvreur/sablier/pull/241 needs to be merged first.

acouvreur commented 9 months ago

I've squashed your commits from the previous PR, sorry if that causes issues on your side :D

acouvreur commented 9 months ago

This is just proof of concept to show how it could be done. It was never completed. I could make it work but #241 needs to be merged first.

Maybe we can mark the PR as draft?

DrummyFloyd commented 6 months ago

do this PR was merge into Beta branch ?

tomaszduda23 commented 6 months ago

I've not completed it yet.

DrummyFloyd commented 6 months ago

@tomaszduda23 do you need help =) i'll be glad to help

tomaszduda23 commented 6 months ago

The help would be great. You can open new PR based on that. I can help to review. This PR just checked if this can be implemented. The logic which check if connection is still active is missing. The idea was:

  1. Create goroutine during websocket request
  2. the goroutine would get notification from Read/Write/Close and make sure that backend is scaled up The goroutine is needed since http request is much heavier than tcp request(websocket).
DrummyFloyd commented 6 months ago

what is your test case scenario ?

will get a bit of time for it asap

tomaszduda23 commented 6 months ago

There are E2E tests in the repo https://github.com/acouvreur/sablier/blob/dda8caec9ca4a9fade363b74a2776714b48d91da/e2e/e2e_test.go#L51 For websocket the end point would be /echo. The test should look like:

  1. Call /echo
  2. Send data to websocket periodically for 3 min (current test use 1min scale down timeout)
  3. Check if successful

The first call is done by http. It is handled by current implementation. After that data are exchanged over tcp here you can see only print messages. The rest of implementation is missing.

DrummyFloyd commented 6 months ago

@tomaszduda23 @acouvreur when the HiJack function occur ? if i get it

  1. you have a traefik route which point to an ws endpoint => whoami/echo
  2. sablier should scale up the pod/container to 1 if it's down => Seems OK to this part but return 200 instead of 101
  3. go routine should check if there is any activity on the WS endpoint 3.1 if activity => no scale down (eg: wscat -c localhost:8080/echo) 3.2 if no activity scaledown ATM, it's scaledown no matterwhat, because Sablier is only IngressRoute & middlewares nor IngressRouteTCP & MiddlewareTCP => https://github.com/traefik/traefik/issues/10593 , and this trick will make work websocket trhough sablier

    am i right ? will try to do this week end

tomaszduda23 commented 6 months ago

http is based on TCP. After http negotiation "hijack" happens and exactly the same connection is used for websocket. You do not need tcp middleware at all. The connection part is already implemented. You just need to deal with those:

fmt.Println("=== websocket close") <- scale down
fmt.Println("=== websocket write", len(b)) <- keep up
fmt.Println("=== websocket read", len) <- keep up

Scale up is already implemented too.

Make the tests first. Than the rest will be clear when you see the logs.

tomaszduda23 commented 6 months ago
  • sablier should scale up the pod/container to 1 if it's down => Seems OK to this part but return 200 instead of 101

in case of websocket only blocking strategy make sense. If user select dynamic strategy and service is scaled down I would return 503 probably. It would still work with dynamic if websocket is not the first connection required by service. It should be fine e.g. for visual studio code.

DrummyFloyd commented 6 months ago

@tomaszduda23 thank you for you help

iit seems to work , will make a bit a of cleaning , before opening a new PR =)