NO-JIRA: feat: add ssh-node target for quick SSH access to cluster nodes#55
NO-JIRA: feat: add ssh-node target for quick SSH access to cluster nodes#55clobrano wants to merge 1 commit intoopenshift-eng:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
@clobrano: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: clobrano The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
clobrano
force-pushed
the
feature/simplify-ssh-into-node
branch
from
68a1e2e to
9ac5588
Compare
Add 'make ssh-node' command to simplify SSH access to cluster nodes. Usage: make ssh-node node=master-0 # Interactive SSH session make ssh-node node=1 # SSH to master-1 make ssh-node node=0 cmd='pcs status' # Run command and return Features: - Supports multiple node name formats (master-0, master_0, node0, 0) - Handles known_hosts issues from cluster redeploys - Uses ProxyJump through the hypervisor automatically
clobrano
force-pushed
the
feature/simplify-ssh-into-node
branch
from
9ac5588 to
3c99bea
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
fonta-rh
left a comment
fonta-rh
left a comment
There was a problem hiding this comment.
This is great! For attacking both nodes, ad hoc ansible like this works well:
ansible -a "sudo podman inspect sushy-tools" cluster_vms -i openshift-clusters/inventory.ini
But adapting it work constantly on a single node was a pain, so this is a great addition!
Out of the review comments, I only think the unreachable echos are necessary, the rest is just nitpicking
| @@ -0,0 +1,92 @@ | |||
| #!/bin/bash | |||
|
|
|||
| set -euo pipefail | |||
There was a problem hiding this comment.
This is going to make a few lines of helpful errors unreachable, I'll point them out below
| # Extract node IP from inventory | ||
| NODE_IP=$(grep -E "master_0|master_1" "${INVENTORY_FILE}" | grep "${NODE_PATTERN}" | grep -oP "ansible_host='\\K[^']+") | ||
|
|
||
| if [[ -z "${NODE_IP}" ]]; then |
There was a problem hiding this comment.
Might be unreachable: if line 55 grep doesn't find anything, it will have exit code 1 and script should exit.
Adding "|| true" to the command on 55 should be enough, we're checking for content of grep output anyway
| # Extract hypervisor info from inventory | ||
| HYPERVISOR=$(grep -E "^[^#]*@" "${INVENTORY_FILE}" | head -1 | awk '{print $1}') | ||
|
|
||
| if [[ -z "${HYPERVISOR}" ]]; then |
There was a problem hiding this comment.
Might be unreachable: if line 64 grep doesn't find anything, it will have exit code 1 and script should exit
| fi | ||
|
|
||
| # Extract hypervisor info from inventory | ||
| HYPERVISOR=$(grep -E "^[^#]*@" "${INVENTORY_FILE}" | head -1 | awk '{print $1}') |
There was a problem hiding this comment.
This is great to parse the ProxyJump, but it might also hit the @ in "ansible_ssh_common_args" if the inventory were ordered differently, so although it works with the current inventory structure, it's a bit fragile.
Claude suggests parsing just in the metal_machine subsection
HYPERVISOR=$(awk '/^\[metal_machine\]/{found=1;next} /^\[/{found=0} found && /@/{print $1; exit}' "${INVENTORY_FILE}")
| @echo " make ssh-node node=1 cmd=\"sudo pcs status\"" | ||
| @exit 1 | ||
| endif | ||
| @./openshift-clusters/scripts/ssh-node.sh $(node) $(cmd) |
There was a problem hiding this comment.
the $(cmd) might be expanded as an "empty" argument to make if it's not provided. To allow for no command safely, might need something like this:
@./openshift-clusters/scripts/ssh-node.sh $(node) $(if $(cmd),$(cmd))
| esac | ||
|
|
||
| # Extract node IP from inventory | ||
| NODE_IP=$(grep -E "master_0|master_1" "${INVENTORY_FILE}" | grep "${NODE_PATTERN}" | grep -oP "ansible_host='\\K[^']+") |
There was a problem hiding this comment.
I don't think this'll happen, but maybe we need to parse for the full word (master_0 or master_1, as this will match master10, master100, and also things like ostest_master_10)
Suggestion might be changing grep -E to grep -W to parse only full words
| SSH_OPTS=( | ||
| -o StrictHostKeyChecking=no | ||
| -o UserKnownHostsFile=/dev/null | ||
| -o LogLevel=ERROR |
There was a problem hiding this comment.
I would move this to WARN to make debugging easier
3 participants
Add 'make ssh-node' command to simplify SSH access to cluster nodes.
Usage:
make ssh-node node=master-0 # Interactive SSH session
make ssh-node node=1 # SSH to master-1
make ssh-node node=0 cmd='pcs status' # Run command and return
Features: