-
Notifications
You must be signed in to change notification settings - Fork 299
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
feat: scylladb database provider implementation #320
Conversation
cmd/program/program.go
Outdated
@@ -103,6 +103,7 @@ var ( | |||
sqliteDriver = []string{"github.com/mattn/go-sqlite3"} | |||
redisDriver = []string{"github.com/redis/go-redis/v9"} | |||
mongoDriver = []string{"go.mongodb.org/mongo-driver"} | |||
scyllaDriver = []string{"github.com/gocql/gocql"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scyllaDriver = []string{"github.com/gocql/gocql"} | |
scyllaDriver = []string{"github.com/scylladb/gocql"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we have to replace the module, I managed to add a GoCQL driver and then a a replaceable right after it.
Should be clean enough for now.
Co-authored-by: Yaniv Kaul <[email protected]>
Co-authored-by: Dmitry Kropachev <[email protected]>
Ready to review @Melkeydev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had a new release, rebased, and checked the updated Docker configuration. I also noticed that a test is failing.
make itest
Running integration tests...
2024/11/17 22:17:22 github.com/testcontainers/testcontainers-go - Connected to docker:
Server Version: 27.3.1
API Version: 1.46
Operating System: Debian GNU/Linux 12 (bookworm)
Total Memory: 64206 MB
Testcontainers for Go Version: v0.34.0
Resolved Docker Host: unix:///var/run/docker.sock
Resolved Docker Socket Path: /var/run/docker.sock
Test SessionID: 3edd946ba504e85fbb20c0ce8590efde82e51b2ba5456422a77900931f8d8910
Test ProcessID: 95e3c3a0-f2c2-49c7-b01c-1ef279c3d674
2024/11/17 22:17:22 🐳 Creating container for image testcontainers/ryuk:0.10.2
2024/11/17 22:17:22 ✅ Container created: a1b7597d722b
2024/11/17 22:17:22 🐳 Starting container: a1b7597d722b
2024/11/17 22:17:23 ✅ Container started: a1b7597d722b
2024/11/17 22:17:23 ⏳ Waiting for container id a1b7597d722b image: testcontainers/ryuk:0.10.2. Waiting for: &{Port:8080/tcp timeout:<nil> PollInterval:100ms skipInternalCheck:false}
2024/11/17 22:17:23 🔔 Container is ready: a1b7597d722b
2024/11/17 22:17:23 🐳 Creating container for image scylladb/scylla:6.2
2024/11/17 22:17:23 ✅ Container created: d8ce0a90298b
2024/11/17 22:17:23 🐳 Starting container: d8ce0a90298b
2024/11/17 22:17:23 ✅ Container started: d8ce0a90298b
2024/11/17 22:17:23 ⏳ Waiting for container id d8ce0a90298b image: scylladb/scylla:6.2. Waiting for: &{timeout:<nil> deadline:<nil> Strategies:[0xc00036ccc0 0xc00036ccf0 0xc000358900]}
2024/11/17 22:17:27 🔔 Container is ready: d8ce0a90298b
=== RUN TestNew
2024/11/17 22:17:28 Failed to connect to ScyllaDB cluster: no connections were made when creating the session
FAIL test1/internal/database 5.312s
FAIL
make: *** [Makefile:40: itest] Error 1
I just ran it locally and seems to work flawlessly. Also the pipeline didn't accused anything... Maybe could be related to cc: @gvieira18 Configure ScyllaDB settingsBefore starting the cluster, ensure the fs.aio-max-nr value is sufficient (e.g. You can use the Makefile setup command to configure the make setup If you prefer to configure it manually, run one of the following commands to check the current value: sysctl --all | grep --word-regexp -- 'aio-max-nr' sysctl fs.aio-max-nr cat /proc/sys/fs/aio-max-nr If the value is lower than required, you can use one of these commands: # Update config non-persistent
sysctl --write fs.aio-max-nr=1048576 Some logs from my local test:
|
@DanielHe4rt I get the same behavior here, my # go clean -testcache && make itest
Running integration tests...
2024/11/18 15:26:03 github.com/testcontainers/testcontainers-go - Connected to docker:
Server Version: 27.3.1
API Version: 1.46
Operating System: Debian GNU/Linux 12 (bookworm)
Total Memory: 15996 MB
Testcontainers for Go Version: v0.34.0
Resolved Docker Host: unix:///var/run/docker.sock
Resolved Docker Socket Path: /var/run/docker.sock
Test SessionID: 7926d623964bdc80c86869a0e083128a04c8b911a4aae9d79b89bead644b26e3
Test ProcessID: 9e9eee67-f1b5-4d4f-89fb-43cc1069178d
2024/11/18 15:26:03 🐳 Creating container for image testcontainers/ryuk:0.10.2
2024/11/18 15:26:03 ✅ Container created: 20e7ad71202d
2024/11/18 15:26:03 🐳 Starting container: 20e7ad71202d
2024/11/18 15:26:03 ✅ Container started: 20e7ad71202d
2024/11/18 15:26:03 ⏳ Waiting for container id 20e7ad71202d image: testcontainers/ryuk:0.10.2. Waiting for: &{Port:8080/tcp timeout:<nil> PollInterval:100ms skipInternalCheck:false}
2024/11/18 15:26:04 🔔 Container is ready: 20e7ad71202d
2024/11/18 15:26:04 🐳 Creating container for image scylladb/scylla:6.2
2024/11/18 15:26:04 ✅ Container created: 503de84e8751
2024/11/18 15:26:04 🐳 Starting container: 503de84e8751
2024/11/18 15:26:04 ✅ Container started: 503de84e8751
2024/11/18 15:26:04 ⏳ Waiting for container id 503de84e8751 image: scylladb/scylla:6.2. Waiting for: &{timeout:<nil> deadline:<nil> Strategies:[0xc000275b60 0xc000275b90 0xc000269140]}
2024/11/18 15:26:43 🔔 Container is ready: 503de84e8751
=== RUN TestNew
--- PASS: TestNew (0.00s)
=== RUN TestHealth
--- PASS: TestHealth (0.01s)
=== RUN TestClose
--- PASS: TestClose (0.00s)
PASS
2024/11/18 15:26:43 🐳 Stopping container: 503de84e8751
2024/11/18 15:26:49 ✅ Container stopped: 503de84e8751
2024/11/18 15:26:49 🐳 Terminating container: 503de84e8751
2024/11/18 15:26:49 🚫 Container terminated: 503de84e8751
ok simple-test/internal/database 46.535s |
@DanielHe4rt
Regarding the testcontainerd case, on my machine, the tests are failing. I'm not sure why, as I configured it as you suggested: sudo sysctl --all | grep --word-regexp -- 'aio-max-nr'
fs.aio-max-nr = 1048576 I thought maybe my local environment was broken in some way, so I tested other databases. In this case, the test passed. If I am having this issue, other users might experience the same. We need to investigate this further and update the documentation accordingly. Running the database in a container and testing the health endpoint works as expected: curl localhost:8080/health
{"message":"It's healthy","scylla_cluster_nodes_down":"0","scylla_cluster_nodes_up":"1","scylla_cluster_size":"1","scylla_current_datacenter":"datacenter1","scylla_current_time":"2024-11-18 18:58:32.016 +0000 UTC","scylla_health_check_duration":"8.134206ms","scylla_keyspaces":"6","status":"up"} I am sending my logs: make itest
Running integration tests...
2024/11/18 20:12:38 github.com/testcontainers/testcontainers-go - Connected to docker:
Server Version: 27.3.1
API Version: 1.46
Operating System: Debian GNU/Linux 12 (bookworm)
Total Memory: 64206 MB
Testcontainers for Go Version: v0.34.0
Resolved Docker Host: unix:///var/run/docker.sock
Resolved Docker Socket Path: /var/run/docker.sock
Test SessionID: 698c302e2118be82e763b4dd71b363ce392a72516252e6d06b4574d0842d8fb2
Test ProcessID: 01047aa4-6e3f-4ee9-843f-11867b3548e3
2024/11/18 20:12:38 🐳 Creating container for image testcontainers/ryuk:0.10.2
2024/11/18 20:12:38 ✅ Container created: 5cb859125e16
2024/11/18 20:12:38 🐳 Starting container: 5cb859125e16
2024/11/18 20:12:38 ✅ Container started: 5cb859125e16
2024/11/18 20:12:38 ⏳ Waiting for container id 5cb859125e16 image: testcontainers/ryuk:0.10.2. Waiting for: &{Port:8080/tcp timeout:<nil> PollInterval:100ms skipInternalCheck:false}
2024/11/18 20:12:38 🔔 Container is ready: 5cb859125e16
2024/11/18 20:12:38 🐳 Creating container for image scylladb/scylla:6.2
2024/11/18 20:12:38 ✅ Container created: 79a6c52d563a
2024/11/18 20:12:38 🐳 Starting container: 79a6c52d563a
2024/11/18 20:12:39 ✅ Container started: 79a6c52d563a
2024/11/18 20:12:39 ⏳ Waiting for container id 79a6c52d563a image: scylladb/scylla:6.2. Waiting for: &{timeout:<nil> deadline:<nil> Strategies:[0xc0002f5cb0 0xc0002f5ce0 0xc0002e0ac0]}
2024/11/18 20:12:43 🔔 Container is ready: 79a6c52d563a
=== RUN TestNew
--- PASS: TestNew (0.00s)
=== RUN TestHealth
2024/11/18 20:12:43 Failed to connect to ScyllaDB cluster: no connections were made when creating the session
FAIL test3/internal/database 4.995s
FAIL
make: *** [Makefile:40: itest] Error 1 |
Please add scylladb to workflows: name: Linting Generated Blueprints Core
and name: Integrations Test for the Generated Blueprints
|
Signed-off-by: Gabriel do Carmo Vieira <[email protected]>
Signed-off-by: Gabriel do Carmo Vieira <[email protected]>
Seems that @gvieira18 made the changes. @Ujstor can you review it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR is really good. There are two more things to address:
Update the documentation with information on how a local machine/server needs to be configured to run ScyllaDB in case of issues similar to mine.
Second one, I need to handle this myself. I'll try testing the container-test part that is failing for me on another machine. In the pipeline, the tests are passing, so I think the issue is on my side.
return strings.Contains(string(data), "COMPLETED") | ||
}), | ||
), | ||
Name: name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd not pin the container name, as it would mean that it won't be possible to run multiple tests in parallel. Testcontainers (Docker) creates random names for each container without name.
return nil, err | ||
} | ||
|
||
hosts = fmt.Sprintf("localhost:%v", mappedPort.Port()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bug: we'd need to initialise the rest of the fields (username, password, consistencyLevel) in order to bootstrap the cluster correctly. Please check how to define the credentials in the docker image, so that the container request sets them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DanielHe4rt would you be interested in contributing a ScyllaDB module for testcontainers-go? This way here we'd use the module instead of the GenericContainer approach. In the end is the same, but easier for the end user to interact with a built-in module.
I can help you out with the creation of the module. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually with a task to work on it under the Testcontainers with Go :p
Quite amazing to have a core-contributor looking for new contributions around! We can chat and make this a thing, even start some Livestream while we work on it.
I just sent you a LinkedIn connection, we can chat there if you want!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bug: we'd need to initialise the rest of the fields (username, password, consistencyLevel) in order to bootstrap the cluster correctly. Please check how to define the credentials in the docker image, so that the container request sets them.
@DanielHe4rt Was there a particular reason for not including this in the fix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scylla doesn't need these extra fields. Initially it just need the Host and Port since we're using only one instance instead a 3 Node Cluster.
There's no bug there since by default all these options has a fallback. It's fully up to the user, the boilerplate will only fulfill the basics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I'm not a Scylla user, so I wrongly understood the credentials needed to be set. LGTM then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mdelapenya, thanks for the review. @DanielHe4rt, make the changes that were pointed out in the review. Tests need to work OOB. After that, we are good to merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.
Feature
This pull request introduces support for ScyllaDB as a new database option in the project. It includes modifications to various parts of the codebase to integrate ScyllaDB, including database drivers, templates, and Docker configurations.
Description of Changes:
go mod edit
helper to get the updated driverChecklist