abh1sar commented on code in PR #11601:
URL: https://github.com/apache/cloudstack/pull/11601#discussion_r2340761213
##########
extensions/Proxmox/proxmox.sh:
##########
@@ -285,6 +285,88 @@ status() {
echo "{\"status\": \"success\", \"power_state\": \"$powerstate\"}"
}
+get_node_host() {
+ check_required_fields node
+ local net_json host
+
+ if ! net_json="$(call_proxmox_api GET "/nodes/${node}/network")"; then
+ echo ""
+ return 1
+ fi
+
+ # Prefer a static non-bridge IP
+ host="$(echo "$net_json" | jq -r '
+ .data
+ | map(select(
+ (.type // "") != "bridge" and
+ (.type // "") != "bond" and
+ (.method // "") == "static" and
+ ((.address // .cidr // "") != "")
+ ))
+ | map(.address // (.cidr | split("/")[0]))
+ | .[0] // empty
+ ' 2>/dev/null)"
+
+ # Fallback: first interface with a CIDR
+ if [[ -z "$host" ]]; then
+ host="$(echo "$net_json" | jq -r '
+ .data
+ | map(select((.cidr // "") != ""))
+ | map(.cidr | split("/")[0])
+ | .[0] // empty
+ ' 2>/dev/null)"
+ fi
+
+ echo "$host"
+ }
+
+ get_console() {
+ check_required_fields node vmid
+
+ # Request VNC proxy from Proxmox
+ local api_resp port ticket
+ if ! api_resp="$(call_proxmox_api POST
"/nodes/${node}/qemu/${vmid}/vncproxy")"; then
Review Comment:
call_proxmox_api is not returning the error code from the curl command
currently, so this if block will never get executed.
Alternatively, we can check if the api_resp is empty here.
Ideally we should return and handle curl exit code from call_proxmox_api.
I'll leave it up to you.
##########
scripts/vm/hypervisor/external/provisioner/provisioner.sh:
##########
@@ -99,6 +99,14 @@ status() {
echo '{"status": "success", "power_state": "poweron"}'
}
+get_console() {
+ parse_json "$1" || exit 1
+ local response
+ jq -n --arg msg "Operation not supported" \
Review Comment:
Also, should we add this method to hyperv.py?
##########
extensions/Proxmox/proxmox.sh:
##########
@@ -285,6 +285,88 @@ status() {
echo "{\"status\": \"success\", \"power_state\": \"$powerstate\"}"
}
+get_node_host() {
+ check_required_fields node
+ local net_json host
+
+ if ! net_json="$(call_proxmox_api GET "/nodes/${node}/network")"; then
+ echo ""
+ return 1
+ fi
+
+ # Prefer a static non-bridge IP
+ host="$(echo "$net_json" | jq -r '
+ .data
+ | map(select(
+ (.type // "") != "bridge" and
+ (.type // "") != "bond" and
+ (.method // "") == "static" and
+ ((.address // .cidr // "") != "")
+ ))
+ | map(.address // (.cidr | split("/")[0]))
+ | .[0] // empty
+ ' 2>/dev/null)"
+
+ # Fallback: first interface with a CIDR
+ if [[ -z "$host" ]]; then
+ host="$(echo "$net_json" | jq -r '
+ .data
+ | map(select((.cidr // "") != ""))
+ | map(.cidr | split("/")[0])
+ | .[0] // empty
+ ' 2>/dev/null)"
+ fi
+
+ echo "$host"
+ }
+
+ get_console() {
+ check_required_fields node vmid
+
+ # Request VNC proxy from Proxmox
+ local api_resp port ticket
+ if ! api_resp="$(call_proxmox_api POST
"/nodes/${node}/qemu/${vmid}/vncproxy")"; then
+ jq -n --arg msg "API call failed for node=$node vmid=$vmid" \
+ '{status:"error", message:$msg, code:"API_CALL_FAILED"}'
+ return 1
Review Comment:
Would it be better to call `exit 1` instead for errors?
##########
scripts/vm/hypervisor/external/provisioner/provisioner.sh:
##########
@@ -99,6 +99,14 @@ status() {
echo '{"status": "success", "power_state": "poweron"}'
}
+get_console() {
+ parse_json "$1" || exit 1
+ local response
+ jq -n --arg msg "Operation not supported" \
Review Comment:
Can we write `echo "{"status": "error", "message": "Operation not
supported", "code": "OPERATION_NOT_SUPPORTED"}` instead to make it simpler as
we are just printing static strings?
##########
scripts/vm/hypervisor/external/provisioner/provisioner.sh:
##########
@@ -99,6 +99,14 @@ status() {
echo '{"status": "success", "power_state": "poweron"}'
}
+get_console() {
+ parse_json "$1" || exit 1
+ local response
+ jq -n --arg msg "Operation not supported" \
+ '{status:"error", message:$msg, code:"OPERATION_NOT_SUPPORTED"}'
+ return 1
Review Comment:
`return 1` doesn't do anything here. We can replace it with `exit 1` if it
makes sense
##########
extensions/Proxmox/proxmox.sh:
##########
@@ -285,6 +285,88 @@ status() {
echo "{\"status\": \"success\", \"power_state\": \"$powerstate\"}"
}
+get_node_host() {
+ check_required_fields node
+ local net_json host
+
+ if ! net_json="$(call_proxmox_api GET "/nodes/${node}/network")"; then
+ echo ""
+ return 1
+ fi
+
+ # Prefer a static non-bridge IP
+ host="$(echo "$net_json" | jq -r '
+ .data
+ | map(select(
+ (.type // "") != "bridge" and
+ (.type // "") != "bond" and
+ (.method // "") == "static" and
+ ((.address // .cidr // "") != "")
+ ))
+ | map(.address // (.cidr | split("/")[0]))
+ | .[0] // empty
+ ' 2>/dev/null)"
+
+ # Fallback: first interface with a CIDR
+ if [[ -z "$host" ]]; then
+ host="$(echo "$net_json" | jq -r '
+ .data
+ | map(select((.cidr // "") != ""))
+ | map(.cidr | split("/")[0])
+ | .[0] // empty
+ ' 2>/dev/null)"
+ fi
+
+ echo "$host"
+ }
+
+ get_console() {
+ check_required_fields node vmid
+
+ # Request VNC proxy from Proxmox
+ local api_resp port ticket
+ if ! api_resp="$(call_proxmox_api POST
"/nodes/${node}/qemu/${vmid}/vncproxy")"; then
+ jq -n --arg msg "API call failed for node=$node vmid=$vmid" \
+ '{status:"error", message:$msg, code:"API_CALL_FAILED"}'
+ return 1
+ fi
+
+ port="$(echo "$api_resp" | jq -re '.data.port // empty' 2>/dev/null ||
true)"
+ ticket="$(echo "$api_resp" | jq -re '.data.ticket // empty' 2>/dev/null ||
true)"
+
+ if [[ -z "$port" || -z "$ticket" ]]; then
+ jq -n --arg msg "Proxmox response missing port/ticket" \
+ --arg raw "$api_resp" \
+ '{status:"error", message:$msg, code:"BAD_UPSTREAM_RESPONSE",
upstream:$raw}'
+ return 1
+ fi
+
+ # Derive host from node’s network info
+ local host
+ host="$(get_node_host)"
+ if [[ -z "$host" ]]; then
+ jq -n --arg msg "Could not determine host IP for node $node" \
+ '{status:"error", message:$msg, code:"HOST_RESOLUTION_ERROR"}'
+ return 1
+ fi
+
+ # Success JSON to stdout
+ jq -n \
+ --arg host "$host" \
+ --arg port "$port" \
+ --arg password "$ticket" \
+ '{
+ status: "success",
+ message: "Console retrieved",
+ console: {
+ host: $host,
Review Comment:
Shouldn't `$host` here be same as `$url`? Why do we need `get_node_host()`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
