Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Websocket PoC #242

Draft
wants to merge 7 commits into
base: beta
Choose a base branch
from
Draft

Websocket PoC #242

wants to merge 7 commits into from

Conversation

tomaszduda23
Copy link
Contributor

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

@acouvreur
Copy link
Member

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

@tomaszduda23
Copy link
Contributor Author

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
Copy link
Member

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

@acouvreur
Copy link
Member

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?

@acouvreur acouvreur marked this pull request as draft January 5, 2024 21:23
@acouvreur acouvreur added help wanted Extra attention is needed traefik related to traefik plugin labels Jan 5, 2024
@tomaszduda23 tomaszduda23 changed the base branch from main to beta January 5, 2024 21:58
@github-actions github-actions bot added the Stale label Mar 7, 2024
@github-actions github-actions bot closed this Mar 12, 2024
@DrummyFloyd
Copy link

do this PR was merge into Beta branch ?

@tomaszduda23
Copy link
Contributor Author

I've not completed it yet.

@DrummyFloyd
Copy link

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

@tomaszduda23
Copy link
Contributor Author

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
Copy link

DrummyFloyd commented Apr 4, 2024

what is your test case scenario ?

will get a bit of time for it asap

@acouvreur acouvreur reopened this Apr 5, 2024
@tomaszduda23
Copy link
Contributor Author

tomaszduda23 commented Apr 5, 2024

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.

@github-actions github-actions bot removed the Stale label Apr 6, 2024
@DrummyFloyd
Copy link

DrummyFloyd commented Apr 12, 2024

@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 => middlewareTCP custom Plugins traefik/traefik#10593 , and this trick will make work websocket trhough sablier

am i right ?
will try to do this week end

@tomaszduda23
Copy link
Contributor Author

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
Copy link
Contributor Author

tomaszduda23 commented Apr 12, 2024

  • 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
Copy link

@tomaszduda23
thank you for you help

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

DrummyFloyd added a commit to DrummyFloyd/sablier that referenced this pull request Apr 13, 2024
DrummyFloyd added a commit to DrummyFloyd/sablier that referenced this pull request Apr 13, 2024
DrummyFloyd added a commit to DrummyFloyd/sablier that referenced this pull request Apr 13, 2024
DrummyFloyd added a commit to DrummyFloyd/sablier that referenced this pull request Apr 13, 2024
DrummyFloyd added a commit to DrummyFloyd/sablier that referenced this pull request Apr 13, 2024
@acouvreur acouvreur force-pushed the beta branch 2 times, most recently from 6039eda to 052f7db Compare April 21, 2024 04:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed traefik related to traefik plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants