Skip to content

feat: http sync service#1187

Merged
hacdias merged 88 commits intomasterfrom
sync-service
Sep 27, 2021
Merged

feat: http sync service#1187
hacdias merged 88 commits intomasterfrom
sync-service

Conversation

@hacdias
Copy link
Copy Markdown
Member

@hacdias hacdias commented Jan 15, 2021 •
edited
Loading

Closes #1186.

Read more on: testground/sync-service#4

This PR introduces a new web service to Testground: the sync service. The sync service will act as the central service to which the tests should connect to get sync features (barriers, publishing, subscribing, entry signaling, event signaling). Previously, the tests were connecting directly to the Redis instance through the SDK. However, that brings us some issues:

  • In case we want to switch from Redis to a different technology (unlikely in the short term), we would need to change the implementation of all SDKs in all languages. refactor: replace Redis by custom implementation sync-service#8
  • The support of Redis on non-backend technologies (such as browsers) is non-existent, which would not allow us to successfully build a JavaScript SDK that successfully works in the browser.
  • By accessing Redis directly, a test may, on purpose, mess up with the purpose of Redis.

This new service is built to battle those issues, by:

  • Providing an HTTP server to which each instance should connect. From that point on, a WebSocket connection should be created. This WebSocket will live as long as the test instance is alive. All connections to the sync service shall be done through this single WebSocket to avoid useless connections.
  • The implementation of the sync service is abstracted by the sync.Service interface, which makes it easy to swap to a different backend in the future.

Regarding testing, I simply moved the tests from sdk-go that now belong here, and adapted them. Do you think we should do more unit tests or are the current integration tests enough?

To do

Before merging

  • Update sdk-go version to definite one on tests and tg
  • Update infra repository to use the new sync service Updating TaaS is deferred.

@hacdias
start implementing sync service
34cd56d 
- signal event
- signal entry
= publish

License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias
improve structure and separate client from interface
6731bd2 
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias
add barrier tos erice
6b6a1c9 
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias
wip: sub
ca6c781
@hacdias
add subscription to service
3ae579f
@hacdias
subscribe handler'
a0bb87c
@hacdias
add http server
4e235ba 
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias
sync service with websockets
cbcb8ff
@hacdias
remove timeouts from barrier and subscribe
835d7be
@hacdias
Copy link
Copy Markdown
Member Author

hacdias commented Jan 29, 2021

@nonsense I implemented the WebSockets as we discussed. Tomorrow, I will start updating the client in order to support the new sync server. Plus, I still need to add tests here.

I wanted to ask you to take a quick look to validate the architecture. A small recap of what I did so far was:

  • sync.Service is an interface that should implement the actual sync actions using a certain backend. The current implementation, DefaultService uses Redis and it is mostly based on the original sync package from the Go SDK.
  • We have an HTTP server that accepts connections, which must upgrade to WebSockets. For each connection, we have a connection that is responsible for consuming the new requests and consuming (sending) the responses that we generate. When we receive a new request, we spin a new goroutine to handle that request. Usually, those are short lived goroutines except for Subscribe and Barrier.

One of my concerns was spinning up goroutines for each request but, as I mentioned, most of them are short-lived. And for the long-lived ones, it's unlikely that a test is calling Barriers and Subscribes at the same time.

@hacdias
refactor: cleanup service, add comments
dcfee7d 
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias
redis test
93b2701
@hacdias
chore: pkg/sync is its own module (#1191)
2ab2628
@hacdias
fix dockerfile with pkg/sync/go.mod
3a701db 
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias
chore: add dockerfile for sync-service
a65ee20 
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias
enlist sync service
6a85d53
@hacdias
Copy link
Copy Markdown
Member Author

hacdias commented Feb 12, 2021

I now noticed testground/pkg/runners and testground/pkg/sidecar both depend on sdk-go for the sync client. The usage of the watch client is making my life complicated as I'm not sure what's the use for it (or even the need).

I plan on moving the GC collection to the sync service.

@nonsense do you have any recommendation on what to do with the Watch Client? What was it used for? is it useful? Should the sidecar connect directly to Redis? Should I create some interface through the sync service for the watching thing?

@nonsense
Copy link
Copy Markdown
Member

nonsense commented Feb 12, 2021

The WatchClient is used by the testground daemon to fetch information about the run of individual testplan instances for a given run, and display that information on TaaS on the dashboard, i.e. clients 998/1000 ; miners 10/10 in terms of failed/ok testplan instances.

The testground daemon would have to connect to the new sync service to fetch that information, when we introduce the sync service, that would replace the sync library and the direct connection to Redis.

@hacdias
fixes related to the new sync service
6ec312c 
- fixes
- tested with some plans and it seems to work correctly
- NOTE: there is note on the logs saying websocket closed unexpectedly
    - IS  related to the client used by the sidecar.

License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias
update sync service and add TODOs
ec91bf4 
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias
cleanup, remove signalEvent
f862c23 
'
@hacdias
remove signal event handler
110b24f
@hacdias
improvements and fixes
3217c23 
NOTE:
- Im probably printing something inside doRun that is causing this error
    write error: write /Users/henriquedias/testground/data/daemon/c0jsphucie6mgsgepg6g.out: file already closed
- TODO: remove code from signalEvent from testrond/pkg/sync as .Subscribe/.Publish can do the same

License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias
fix: TaaS displaying correct outcome, other fixes
f6bff31 
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias
make tidy && make lint
ead14b3 
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias
fix: check if err == nil
be5b548 
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias
Copy link
Copy Markdown
Member Author

hacdias commented Feb 16, 2021 •
edited
Loading

Current issues

### Issue 1

Every time a test fails, I get something like this on the console.

Feb 16 15:31:37.247686	INFO	1.3661s INCOMPLETE << sidecar      >>
2021-02-16 16:31:37.24772 +0100 CET m=+263.132244532 write error: write /Users/henriquedias/testground/data/daemon/XXXXXXXXXXXX.out: file already closed

So I guess I'm still somehow writing to that file while I shouldn't (see logs).

Apparently this also happens on my computer on the master branch so it may be something to do with my local configuration.

### Issue 2 (splitbrain)

Splitbrain hangs and then fails with deadline exceeded. It appears that pubsub might be only sending data to one sub.

PubSub system is made for only one sub. Make it support more subs. Take into account that multiple subs might've started in different points in time, hence lastid might differ.

Rework barrier logic: currently we are asking for same barrier the number of instances waiting on that barrier.

@hacdias
group barriers for the same key in one
efc6f39 
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias
rework subs worker
438dc04 
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias
tidy
3986f02 
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias
use new sync service, ping pong not passing
08ab4da 
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias hacdias requested a review from nonsense April 15, 2021 14:26
nonsense
nonsense reviewed Apr 16, 2021
View reviewed changes
nonsense
nonsense reviewed Apr 16, 2021
View reviewed changes
nonsense
nonsense reviewed Apr 16, 2021
View reviewed changes
hacdias and others added 3 commits April 17, 2021 08:32
@hacdias
fix: race on docker runner
c5fd5d3 
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias
fix: race on runner k8s
7b7f16e 
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@nonsense
add missing SYNC_SERVICE_HOST to cluster:k8s runner
5f78e1f
nonsense
nonsense reviewed Apr 20, 2021
View reviewed changes
@hacdias hacdias requested a review from nonsense April 21, 2021 13:38
@hacdias hacdias force-pushed the sync-service branch 2 times, most recently from 19a349a to f01af2b Compare May 15, 2021 08:25
@hacdias hacdias mentioned this pull request May 15, 2021
Merged
hacdias
hacdias commented Jul 2, 2021
View reviewed changes
Makefile
./integration_tests/10_docker_splitbrain_reject.sh
./integration_tests/11_docker_splitbrain_drop.sh
./integration_tests/12_docker_example-js_pingpong.sh
# ./integration_tests/12_docker_example-js_pingpong.sh
Copy link
Copy Markdown
Member Author

@hacdias hacdias Jul 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SDK JS is a work in progress. We can re-add the tests afterwards.

hacdias added 2 commits July 2, 2021 11:16
@hacdias
fix: do not remove some Redis related vars
a8c9ad1 
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias
rm comp
ba976f5 
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias hacdias merged commit b0d9aa8 into master Sep 27, 2021
@hacdias hacdias deleted the sync-service branch September 27, 2021 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nonsense nonsense Awaiting requested review from nonsense

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Sync service refactor sdk-go FetchAllEvents and SubscribeEvents

2 participants

@hacdias @nonsense