-
Notifications
You must be signed in to change notification settings - Fork 6
[HWORKS-2117] Make rondb-helm works for RonDB 24.10 #46
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
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.
Pull Request Overview
This PR updates the rondb-helm chart to support RonDB 24.10 by upgrading image tags, updating the command-line configuration, and bumping version numbers.
- Updated image tags in values.yaml for both rondb and toolbox.
- Changed the command from "rdrs" to "rdrs2" in templates/rdrs.yaml and removed GRPC port configurations.
- Bumped Chart version and appVersion in Chart.yaml.
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.
File | Description |
---|---|
values.yaml | Updated image tags to target RonDB 24.10 and toolbox tag to 0.8. |
templates/rdrs.yaml | Updated command configuration and removed GRPC ports. |
Chart.yaml | Updated chart and application version numbers. |
Files not reviewed (1)
- files/configs/rest_api.json: Language not supported
Comments suppressed due to low confidence (2)
templates/rdrs.yaml:59
- Please confirm that the change from 'rdrs' to 'rdrs2' is intentional. If this update is not correct, it could lead to command execution errors.
rdrs2 --config {{ include "rondb.dataDir" $ }}/rest_api.json
templates/rdrs.yaml:152
- The removal of GRPC port definitions might affect services relying on GRPC endpoints. Please verify that this omission is an intentional part of the RonDB 24.10 update.
- - name: grpc
@@ -5,11 +5,11 @@ name: rondb | |||
|
|||
# This is the chart version. This version number should be incremented each time you make changes | |||
# to the chart and its templates, including the app version. | |||
version: 0.6.5 | |||
version: 0.7.0 |
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.
A general comment, are we planning to create a branch for rondb-helm to be based on 22.10 since i assume we might need to do some fixes in the future for 4.2 which uses the rondb-helm chart 0.6.5 . If so, i think we should adapt the version of the rondb-helm chart to reflect that as well @kouzant @KernelMaker
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 discussed with Antonios last week, and the conclusion was that there will be a standalone branch for 22.10 in the future. The default branch will serve as the main development branch for 24.10 going forward.
We haven’t finalized when or where exactly to create the new branch for 22.10 yet, but I assume it should start from the last commit before this patch is merged? @kouzant @maismail
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.
Yeah exactly. As @KernelMaker said we'll create the 0.6 branch. We waited until Serverless migration in case we had to do some changes so we don't need to backport
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.
Ok, make sense, thanks!
65e44c5
to
73fc83b
Compare
Rebased onto the latest main(commit id:bba23141a86dc66000ea38869608477c9a77a345), which now includes updates from the 0.6 branch |
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.
@KernelMaker the latest commit introduced a TargetGroupBinding
for rdrs-grpc
Could you please remove that block?
Also, it looks like something is wrong with the datanodes https://github.com/logicalclocks/rondb-helm/actions/runs/14337188971 |
1. Archive error and trace logs if `ndbmtds.sh` detects them on restart 2. Remove GRPC from `rdrs-aws-TargetGroupBinding.yaml`
Added one more commit: Author: KernelMaker [email protected]
|
files/scripts/ndbmtds.sh
Outdated
# CAUTION: | ||
# If additional files are configured to be stored in this directory in the future, | ||
# be careful with this move operation — it may affect unrelated files. | ||
files=($(find "$LOG_DIR" -maxdepth 1 -type f -name 'ndb_*')) |
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.
Is there a common suffix we can filter tracefiles? I'm thinking of your comment in case we/somebody adds more files under log directory. If not, then that's good enough
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.
Here are the log file names I got from the test env:
ndb_4_error.log, ndb_4_trace.log.1, ndb_4_trace.log.1_t1, ndb_4_trace.log.2, ndb_4_trace.log.2_t1, ndb_4_trace.log.3, ndb_4_trace.log.3_t1, ndb_4_trace.log.4, ndb_4_trace.log.4_t1, ndb_4_trace.log.next
They don’t have a common suffix.. maybe ndb_*log*
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.
Yeah I think filter just the logs is safer, in case something else accidentally ends in this directory
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.
[HWORKS-2117] Make rondb-helm work with RonDB 24.10 [PATCH 3]
Update regex from ndb_*
to ndb*_log*
for safer archiving of trace and error logs.
Update regex from `ndb_*` to `ndb*_log*` for safer archiving of trace and error logs.
No description provided.