-
Notifications
You must be signed in to change notification settings - Fork 79
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
fix(cluster): write install script for worker pool #2009
base: 02-08-feat_clusters_add_worker_pool_type
Are you sure you want to change the base?
fix(cluster): write install script for worker pool #2009
Conversation
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.
PR Summary
This PR implements installation scripts and configurations for worker pools in the Rivet cluster management system, focusing on proper service connectivity and resource management.
- Added new
worker
pool type insdks/api/fern/definition/provision/common.yml
with corresponding SDK updates - Implemented worker pool configuration in
packages/common/config/src/config/server/rivet/cluster_provision.rs
with proper service URLs and port mappings - Added FoundationDB configuration support in
packages/edge/infra/client/isolate-v8-runner/src/main.rs
for worker pools - Added
datacenter_name_id
field to server info responses for better datacenter identification - Configured tunnel services with specific ports (CRDB: 5040, Redis: 5041/5042, ClickHouse: 5043/5044, S3: 5045, NATS: 5046, Prometheus: 5047) in
packages/core/services/cluster/src/workflows/server/install/install_scripts/components/traefik.rs
131 file(s) reviewed, 28 comment(s)
Edit PR Review Bot Settings | Greptile
--mount=type=cache,target=/usr/local/cargo/git,id=univseral-cargo-git \ | ||
--mount=type=cache,target=/usr/local/cargo/registry,id=univseral-cargo-registry \ | ||
--mount=type=cache,target=/app/target,id=univseral-target \ |
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.
syntax: typo in cache mount ids: 'univseral' should be 'universal'
connection: "fdb:[email protected]:4500".to_string(), | ||
cluster_description: "fdb".to_string(), | ||
cluster_id: "fdb".to_string(), | ||
addresses: Addresses::Static(vec!["127.0.0.1:4500".to_string()]), |
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.
style: default address includes port 4500 but should use the FDB constant from default_ports to stay DRY
async move { | ||
let joined = servers | ||
.into_iter() | ||
.filter_map(|server| server.lan_ip) |
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.
logic: No handling for empty server list - could cause invalid connection string if all servers are filtered out
cluster_id = fdb_config.cluster_id, | ||
); | ||
|
||
fs::write(temp_path, connection.as_bytes()).await?; |
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.
logic: Multiple concurrent service discovery callbacks could race when writing to the same file
rand = "0.8" | ||
reqwest = { version = "0.11", features = ["json"] } | ||
serde = { version = "1.0", features = ["derive"] } | ||
tokio = { version = "1.40", features = ["full"] } |
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.
style: tokio 1.40 is very new and may cause compatibility issues. Consider using a more established version like 1.32 which is used in other parts of the codebase.
FROM scratch | ||
COPY --from=rust /app/dist/ / |
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.
logic: Using scratch base image without libfdb_c.so will cause runtime errors since the FoundationDB client library is required. Need to copy libfdb_c.so from builder stage.
COPY --from=rust /app/dist/ / | ||
|
||
# Allows `docker create` to work even though this fails | ||
CMD [""] |
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.
logic: Empty CMD will cause container to fail immediately. Should set proper entrypoint to run rivet-edge-server.
@@ -0,0 +1,8 @@ | |||
* |
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.
style: Consider adding a comment header explaining the purpose of this dockerignore file
@@ -33,6 +34,7 @@ util.workspace = true | |||
uuid = "1.11.0" | |||
|
|||
server-spec = { workspace = true, optional = true } | |||
token-create.workspace = true |
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.
style: token-create is not marked as optional despite being likely only needed for certain features like the other dependencies
base_path: util::url::to_string_without_slash(&edge.intercom_endpoint), | ||
bearer_access_token: Some(token), | ||
..Default::default() | ||
}; |
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.
style: Consider adding error handling for URL parsing failures
24ae5de
to
02f4f81
Compare
741ef37
to
0618a18
Compare
02f4f81
to
7bdc5b6
Compare
0618a18
to
72e6e1e
Compare
72e6e1e
to
d4e7b15
Compare
7bdc5b6
to
e6dd614
Compare
d4e7b15
to
b439967
Compare
e6dd614
to
c333240
Compare
c333240
to
0bb9a93
Compare
b439967
to
1ca83db
Compare
0bb9a93
to
9f0f985
Compare
1ca83db
to
257ad3e
Compare
257ad3e
to
9936cb1
Compare
9f0f985
to
9763a09
Compare
9763a09
to
9f0f985
Compare
9936cb1
to
257ad3e
Compare
257ad3e
to
9936cb1
Compare
9f0f985
to
9763a09
Compare
9936cb1
to
257ad3e
Compare
9763a09
to
9f0f985
Compare
Deploying rivet with
|
Latest commit: |
257ad3e
|
Status: | ✅ Deploy successful! |
Preview URL: | https://c8c5ec4d.rivet.pages.dev |
Branch Preview URL: | https://graphite-base-2020.rivet.pages.dev |
257ad3e
to
9936cb1
Compare
9f0f985
to
9763a09
Compare
Deploying rivet-hub with
|
Latest commit: |
9936cb1
|
Status: | ✅ Deploy successful! |
Preview URL: | https://9930d981.rivet-hub-7jb.pages.dev |
Branch Preview URL: | https://02-11-fix-cluster-write-inst.rivet-hub-7jb.pages.dev |
Deploying rivet with
|
Latest commit: |
9936cb1
|
Status: | ✅ Deploy successful! |
Preview URL: | https://b9ab0ff9.rivet.pages.dev |
Branch Preview URL: | https://02-11-fix-cluster-write-inst.rivet.pages.dev |
Changes