Skip to content

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

add a psql subcommand to cncluster script for convenient DB inspection #2672

wants to merge 3 commits into from
+138 −1

Conversation

mblaze-da
Copy link
Contributor

[static]

I've tested this by running the new subcommand against different databases in the cluster on a scratchnet.

sequencer-0-pg

scratchnetc % cncluster psql sv-1 global-domain-0-sequencer                                                          
INFO: Retrieving DB connection info from pod description...
INFO: Retrieving DB password from secret [sequencer-0-pg-secrets]...
INFO: Deploying debug pod on version 0.4.19
pod/splice-debug created
INFO: Waiting for debug pod to become ready
pod/splice-debug condition met
INFO: Opening terminal on debug pod
psql (16.10 (Ubuntu 16.10-0ubuntu0.24.04.1), server 14.19 (Debian 14.19-1.pgdg13+1))
Type "help" for help.

cantonnet=#

The optional search path parameter can be specified and it is set as shown in the following example:

sv-app postgres

scratchnetc % cncluster psql sv-1 sv-app test
INFO: Retrieving DB connection info from pod description...
INFO: Retrieving DB password from secret [postgres-secrets]...
INFO: Deploying debug pod on version 0.4.19
pod/splice-debug created
INFO: Waiting for debug pod to become ready
pod/splice-debug condition met
INFO: Opening terminal on debug pod
psql (16.10 (Ubuntu 16.10-0ubuntu0.24.04.1), server 14.19 (Debian 14.19-1.pgdg13+1))
Type "help" for help.

cantonnet=# show search_path;
 search_path 
-------------
 test
(1 row)

@mblaze-da mblaze-da linked an issue Oct 13, 2025 that may be closed by this pull request
Add a cncluster psql #2217
Open
julientinguely-da
julientinguely-da approved these changes Oct 13, 2025
View reviewed changes
Copy link
Contributor

@julientinguely-da julientinguely-da left a 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 :)

build-tools/cncluster
effect: "NoSchedule"
EOF

# shellcheck disable=SC2086
Copy link
Contributor

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")

Copy link
Contributor Author

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.

build-tools/cncluster
_update_cluster_config "$deployment_config" "operatorDeployment"
}

subcommand_whitelist[psql]=""
Copy link
Contributor

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 \
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch

build-tools/cncluster
_info "Retrieving DB connection info from pod description..."
local DB_INIT_COMMAND
DB_INIT_COMMAND=$(
kubectl describe pod \
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@julientinguely-da
Copy link
Contributor

Also you can add fixes https://github.com/hyperledger-labs/splice/issues/2217 on top of the PR description such that it links the PR to the issue and automatically close it when merged.

@mblaze-da
fix a hardcoded namespace in cncluster psql
b3f8ac9 
[static]

Signed-off-by: Mateusz Błażejewski <[email protected]>
martinflorian-da
martinflorian-da reviewed Oct 13, 2025
View reviewed changes
martinflorian-da
martinflorian-da reviewed Oct 13, 2025
View reviewed changes
build-tools/cncluster Outdated
shift 2
;;
-d=*|--dbname=*)
local DB_NAME="${1#*=}"
Copy link
Contributor

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"

Copy link
Contributor

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

martinflorian-da
martinflorian-da reviewed Oct 13, 2025
View reviewed changes
build-tools/cncluster
subcmd_debug_shell " \
/bin/env
PGPASSWORD=$DB_PASSWORD \
${SEARCH_PATH:+PGOPTIONS=--search_path=$SEARCH_PATH} \
Copy link
Contributor

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...

@isegall-da
Copy link
Contributor

Should we add some commentary and examples in some README?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@martinflorian-da martinflorian-da martinflorian-da left review comments

@julientinguely-da julientinguely-da julientinguely-da approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add a cncluster psql

4 participants

@mblaze-da @julientinguely-da @isegall-da @martinflorian-da