-
Notifications
You must be signed in to change notification settings - Fork 59
add a psql subcommand to cncluster script for convenient DB inspection #2672
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
[static] Signed-off-by: Mateusz Błażejewski <mateusz.blazejewski@digitalasset.com>
julientinguely-da
left a comment
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.
Really nice, thanks! Looks good overall, just a few minor comments/suggestions. Gonna try it out now :)
| effect: "NoSchedule" | ||
| EOF | ||
|
|
||
| # shellcheck disable=SC2086 |
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.
let's try to remove the shellcheck disable=SC2086 by adding the parentheses (like "$SHELL_COMMAND")
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.
Then it would not work. We need this to expand.
| _update_cluster_config "$deployment_config" "operatorDeployment" | ||
| } | ||
|
|
||
| subcommand_whitelist[psql]="" |
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.
nit: could we add the ### on top to match with the other methods
build-tools/cncluster
Outdated
| local PASSWORD_REF | ||
| PASSWORD_REF=$( | ||
| kubectl get deployment \ | ||
| --namespace sv-1 \ |
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 think you forgot to parametrize the namespace here
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.
good catch
| _info "Retrieving DB connection info from pod description..." | ||
| local DB_INIT_COMMAND | ||
| DB_INIT_COMMAND=$( | ||
| kubectl describe pod \ |
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.
suggestion: couldn't we use kubectl get pod instead of describe here, would seem less brittle?
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 don't think it would make it much more reliable. We'd still need to sed the psql command out of a larger script in the "command" field. We'd just exchange a grep -i psql for a more complicated JSON path expression.
|
Also you can add |
[static] Signed-off-by: Mateusz Błażejewski <mateusz.blazejewski@digitalasset.com>
build-tools/cncluster
Outdated
| shift 2 | ||
| ;; | ||
| -d=*|--dbname=*) | ||
| local DB_NAME="${1#*=}" |
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 so at least on the deployment I tested on (sv-app) this is wrong: we don't want the default db (cantonnet), but the one that the init container is creating sv_sv_1:
until errmsg=$(psql -h 10.60.65.197 -p 5432 --username=cnadmin --dbname=cantonnet -c 'create database sv_sv_1' 2\u003e\u00261); do\n if [[ $errmsg == *\"already exists\"* ]]; then\n echo \"Database sv_sv_1 already exists. Done.\"\n break\n fi\n\n echo \"trying to create postgres database sv_sv_1, last error: $errmsg\";\n sleep 2;\ndone\n"
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 for the horrible paste, but I hope the point comes across; create database sv_sv_1 is the interesting one)
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.
fixed
build-tools/cncluster
Outdated
| subcmd_debug_shell " \ | ||
| /bin/env | ||
| PGPASSWORD=$DB_PASSWORD \ | ||
| ${SEARCH_PATH:+PGOPTIONS=--search_path=$SEARCH_PATH} \ |
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 wonder if there is a nice way to infer the search path...
I often just guess/know it (is it perhaps always the same as database name for our deployments?).
But maybe there is some nice postgres way to just add all existing schemas to the search path or something...
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.
Well, I tried to set the search path to include all schemas but I couldn't get it to work. Listing all schemas is simple:
PGPASSWORD=$DB_PASSWORD psql \
--host=$DB_HOST \
--port=$DB_PORT \
--username=$DB_USERNAME \
--dbname=$DB_NAME \
--tuples-only \
--pset format=unaligned \
--command "SELECT string_agg(schema_name, ',') FROM information_schema.schemata"
as well as setting the search path:
PGOPTIONS=--search_path=$(...)
What I couldn't figure out was how to delay the evaluation of the schema listing till pod runtime while at the same time keeping the session interactive. I could get either one or the other to work. Not both at the same time. I gave up and implemented a simple mapping that uses DB_NAME with exceptions for participant, sequencer and mediator, as we've discussed on Slack. TBH I don't want to invest much more time into improving this as I don't think it is that important.
Perhaps this could be achieved by modifying the pod configuration but I did not want to duplicate any code from the debug_shell subcommand.
I tested it with participant and sv-app and it seems to be working.
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.
Sounds like a perfectly reasonable decision to me!
[static] Signed-off-by: Mateusz Błażejewski <mateusz.blazejewski@digitalasset.com>
|
Should we add some commentary and examples in some README? |
…sed on application name [static] Signed-off-by: Mateusz Błażejewski <mateusz.blazejewski@digitalasset.com>
|
@isegall-da there is a section about connecting to PG DBs with the help of |
@mblaze-da Yep. Agree that it would be nice to do this as a follow-up. |
martinflorian-da
left a comment
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 love it and will use it today; thanks a lot!
|
(tested the use-cases that I need, they work! :) ) |
[static]
I've tested this by running the new subcommand against different databases in the cluster on a scratchnet.
The optional search path parameter can be specified and it is set as shown in the following example: