You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by "resmo (via GitHub)" <gi...@apache.org> on 2026/08/09 09:00:33 UTC
[PR] extension: proxmox: rewrite in python3 [cloudstack]
resmo opened a new pull request, #13832:
URL: https://github.com/apache/cloudstack/pull/13832
### Description
this PR rewrites the proxmox extension from shell to modern python3 for better maintainability.
<!-- For new features, provide link to FS, dev ML discussion etc. -->
<!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. -->
<!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged -->
<!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" -->
<!-- Fixes: # -->
<!--- ******************************************************************************* -->
<!--- NOTE: AUTOMATION USES THE DESCRIPTIONS TO SET LABELS AND PRODUCE DOCUMENTATION. -->
<!--- PLEASE PUT AN 'X' in only **ONE** box -->
<!--- ******************************************************************************* -->
### Types of changes
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Bug fix (non-breaking change which fixes an issue)
- [x] Enhancement (improves an existing feature and functionality)
- [x] Cleanup (Code refactoring and cleanup, that may add test cases)
- [ ] Build/CI
- [ ] Test (unit or integration test code)
### Feature/Enhancement Scale or Bug Severity
#### Feature/Enhancement Scale
- [ ] Major
- [x] Minor
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [ ] Major
- [x] Minor
- [ ] Trivial
### Screenshots (if appropriate):
### How Has This Been Tested?
<!-- Please describe in detail how you tested your changes. -->
<!-- Include details of your testing environment, and the tests you ran to -->
#### How did you try to break this feature and the system with this change?
<!-- see how your change affects other areas of the code, etc. -->
<!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md) document -->
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] extension: proxmox: rewrite in python3 [cloudstack]
Posted by "shwstppr (via GitHub)" <gi...@apache.org>.
shwstppr commented on PR #13832:
URL: https://github.com/apache/cloudstack/pull/13832#issuecomment-5289495447
Without getting into the merits of the programming language/script to use, the idea behind using bash for this extension was to showcase that any executable can be used. This is why Proxmox extension was created in bash and HyperV extension was created in Python. I would still prefer it in bash or maybe something other than python.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] extension: proxmox: rewrite in python3 [cloudstack]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13832:
URL: https://github.com/apache/cloudstack/pull/13832#discussion_r3764461389
##########
extensions/Proxmox/proxmox.sh:
##########
@@ -16,539 +16,4 @@
# specific language governing permissions and limitations
# under the License.
-parse_json() {
- local json_string="$1"
- echo "$json_string" | jq '.' > /dev/null || { echo '{"status": "error", "error": "Invalid JSON input"}'; exit 1; }
-
- local -A details
- while IFS="=" read -r key value; do
- details[$key]="$value"
- done < <(echo "$json_string" | jq -r '{
- "extension_url": (.externaldetails.extension.url // ""),
- "extension_user": (.externaldetails.extension.user // ""),
- "extension_token": (.externaldetails.extension.token // ""),
- "extension_secret": (.externaldetails.extension.secret // ""),
- "host_url": (.externaldetails.host.url // ""),
- "host_user": (.externaldetails.host.user // ""),
- "host_token": (.externaldetails.host.token // ""),
- "host_secret": (.externaldetails.host.secret // ""),
- "node": (.externaldetails.host.node // ""),
- "network_bridge": (.externaldetails.host.network_bridge // ""),
- "verify_tls_certificate": (.externaldetails.host.verify_tls_certificate // "true"),
- "vm_name": (.externaldetails.virtualmachine.vm_name // ""),
- "template_id": (.externaldetails.virtualmachine.template_id // ""),
- "template_type": (.externaldetails.virtualmachine.template_type // ""),
- "iso_path": (.externaldetails.virtualmachine.iso_path // ""),
- "iso_os_type": (.externaldetails.virtualmachine.iso_os_type // "l26"),
- "disk_size_gb": (.externaldetails.virtualmachine.disk_size_gb // "64"),
- "storage": (.externaldetails.virtualmachine.storage // "local-lvm"),
- "is_full_clone": (.externaldetails.virtualmachine.is_full_clone // "false"),
- "snap_name": (.parameters.snap_name // ""),
- "snap_description": (.parameters.snap_description // ""),
- "snap_save_memory": (.parameters.snap_save_memory // ""),
- "vmid": (."cloudstack.vm.details".details.proxmox_vmid // ""),
- "vm_internal_name": (."cloudstack.vm.details".name // ""),
- "vmmemory": (."cloudstack.vm.details".minRam // ""),
- "vmcpus": (."cloudstack.vm.details".cpus // ""),
- "vlans": ([."cloudstack.vm.details".nics[]?.broadcastUri // "" | sub("vlan://"; "")] | join(",")),
- "mac_addresses": ([."cloudstack.vm.details".nics[]?.mac // ""] | join(","))
- } | to_entries | .[] | "\(.key)=\(.value)"')
-
- for key in "${!details[@]}"; do
- declare -g "$key=${details[$key]}"
- done
-
- # set url, user, token, secret to host values if present, otherwise use extension values
- url="${host_url:-$extension_url}"
- user="${host_user:-$extension_user}"
- token="${host_token:-$extension_token}"
- secret="${host_secret:-$extension_secret}"
-
- check_required_fields url user token secret node
-}
-
-urlencode() {
- encoded_data=$(python3 -c "import urllib.parse; print(urllib.parse.quote('''$1'''))")
- echo "$encoded_data"
-}
-
-check_required_fields() {
- local missing=()
- for varname in "$@"; do
- local value="${!varname}"
- if [[ -z "$value" ]]; then
- missing+=("$varname")
- fi
- done
-
- if [[ ${#missing[@]} -gt 0 ]]; then
- echo "{\"error\":\"Missing required fields: ${missing[*]}\"}"
- exit 1
- fi
-}
-
-validate_name() {
- local entity="$1"
- local name="$2"
- if [[ ! "$name" =~ ^[a-zA-Z0-9-]+$ ]]; then
- echo "{\"error\":\"Invalid $entity name '$name'. Only alphanumeric characters and dashes (-) are allowed.\"}"
- exit 1
- fi
-}
-
-call_proxmox_api() {
- local method=$1
- local path=$2
- local data=$3
-
- curl_opts=(
- -s
- --fail
- -X "$method"
- -H "Authorization: PVEAPIToken=${user}!${token}=${secret}"
- )
-
- if [[ "$verify_tls_certificate" == "false" ]]; then
- curl_opts+=(-k)
- fi
-
- if [[ -n "$data" ]]; then
- curl_opts+=(-d "$data")
- fi
-
- response=$(curl "${curl_opts[@]}" "https://${url}:8006/api2/json${path}")
- local status=$?
- if [[ $status -ne 0 ]]; then
- echo "{\"errors\":{\"curl\":\"API call failed with status $status: $(echo "$response" | jq -Rsa . | jq -r .)\"}}"
- return $status
- fi
- echo "$response"
- return 0
-}
-
-wait_for_proxmox_task() {
- local upid="$1"
- local timeout="${2:-$wait_time}"
- local interval="${3:-1}"
-
- local start_time
- start_time=$(date +%s)
-
- while true; do
- local now
- now=$(date +%s)
- if (( now - start_time > timeout )); then
- echo '{"status": "error", "error":"Timeout while waiting for async task"}'
- exit 1
- fi
-
- local status_response
- status_response=$(call_proxmox_api GET "/nodes/${node}/tasks/$(urlencode "$upid")/status")
-
- if [[ -z "$status_response" || "$status_response" == *'"errors":'* ]]; then
- local msg
- msg=$(echo "$status_response" | jq -r '.message // "Unknown error"')
- echo "{\"status\": \"error\", \"error\": \"$msg\"}"
- exit 1
- fi
-
- local task_status
- task_status=$(echo "$status_response" | jq -r '.data.status')
-
- if [[ "$task_status" == "stopped" ]]; then
- local exit_status
- exit_status=$(echo "$status_response" | jq -r '.data.exitstatus')
- if [[ "$exit_status" != "OK" ]]; then
- echo "{\"error\":\"Task failed with exit status: $exit_status\"}"
- exit 1
- fi
- return 0
- fi
-
- sleep "$interval"
- done
-}
-
-execute_and_wait() {
- local method="$1"
- local path="$2"
- local data="$3"
- local response upid msg
-
- response=$(call_proxmox_api "$method" "$path" "$data")
- upid=$(echo "$response" | jq -r '.data // ""')
-
- if [[ -z "$upid" ]]; then
- msg=$(echo "$response" | jq -r '.message // "Unknown error"')
- echo "{\"error\":\"Failed to execute API or retrieve UPID. Message: $msg\"}"
- exit 1
- fi
-
- wait_for_proxmox_task "$upid"
-}
-
-vm_not_present() {
- response=$(call_proxmox_api GET "/cluster/nextid?vmid=$vmid")
- vmid_result=$(echo "$response" | jq -r '.data // empty')
- if [[ "$vmid_result" == "$vmid" ]]; then
- return 0
- else
- return 1
- fi
-}
-
-prepare() {
- response=$(call_proxmox_api GET "/cluster/nextid")
- vmid=$(echo "$response" | jq -r '.data // ""')
-
- echo "{\"details\":{\"proxmox_vmid\": \"$vmid\"}}"
-}
-
-create() {
- if [[ -z "$vm_name" ]]; then
- if [[ -z "$vm_internal_name" ]]; then
- echo '{"error":"Missing required fields: vm_internal_name"}'
- exit 1
- fi
- vm_name="$vm_internal_name"
- fi
- validate_name "VM" "$vm_name"
- check_required_fields vmid network_bridge vmcpus vmmemory
-
- if [[ "${template_type^^}" == "ISO" ]]; then
- check_required_fields iso_path
- local data="vmid=$vmid"
- data+="&name=$vm_name"
- data+="&ide2=$(urlencode "$iso_path,media=cdrom")"
- data+="&ostype=$iso_os_type"
- data+="&scsihw=virtio-scsi-single"
- data+="&scsi0=$(urlencode "$storage:$disk_size_gb,iothread=on")"
- data+="&sockets=1"
- data+="&cores=$vmcpus"
- data+="&numa=0"
- data+="&cpu=x86-64-v2-AES"
- data+="&memory=$((vmmemory / 1024 / 1024))"
-
- execute_and_wait POST "/nodes/${node}/qemu/" "$data"
- cleanup_vm=1
-
- else
- check_required_fields template_id
- local data="newid=$vmid"
- data+="&name=$vm_name"
- clone_flag=$(( is_full_clone == "true" ))
- data+="&storage=$storage&full=$clone_flag"
- execute_and_wait POST "/nodes/${node}/qemu/${template_id}/clone" "$data"
- cleanup_vm=1
-
- data="cores=$vmcpus"
- data+="&memory=$((vmmemory / 1024 / 1024))"
- execute_and_wait POST "/nodes/${node}/qemu/${vmid}/config" "$data"
- fi
-
- IFS=',' read -ra vlan_array <<< "$vlans"
- IFS=',' read -ra mac_array <<< "$mac_addresses"
- for i in "${!vlan_array[@]}"; do
- network="net${i}=$(urlencode "virtio=${mac_array[i]},bridge=${network_bridge},tag=${vlan_array[i]},firewall=0")"
- call_proxmox_api PUT "/nodes/${node}/qemu/${vmid}/config/" "$network" > /dev/null
- done
-
- execute_and_wait POST "/nodes/${node}/qemu/${vmid}/status/start"
-
- cleanup_vm=0
- echo '{"status": "success", "message": "Instance created"}'
-}
-
-start() {
- execute_and_wait POST "/nodes/${node}/qemu/${vmid}/status/start"
- echo '{"status": "success", "message": "Instance started"}'
-}
-
-delete() {
- if vm_not_present; then
- echo '{"status": "success", "message": "Instance deleted"}'
- return 0
- fi
- execute_and_wait DELETE "/nodes/${node}/qemu/${vmid}"
- echo '{"status": "success", "message": "Instance deleted"}'
-}
-
-stop() {
- if vm_not_present; then
- echo '{"status": "success", "message": "Instance stopped"}'
- return 0
- fi
- execute_and_wait POST "/nodes/${node}/qemu/${vmid}/status/stop"
- echo '{"status": "success", "message": "Instance stopped"}'
-}
-
-reboot() {
- execute_and_wait POST "/nodes/${node}/qemu/${vmid}/status/reboot"
- echo '{"status": "success", "message": "Instance rebooted"}'
-}
-
-status() {
- local status_response vm_status powerstate
- status_response=$(call_proxmox_api GET "/nodes/${node}/qemu/${vmid}/status/current")
- vm_status=$(echo "$status_response" | jq -r '.data.status')
- case "$vm_status" in
- running) powerstate="poweron" ;;
- stopped) powerstate="poweroff" ;;
- *) powerstate="unknown" ;;
- esac
-
- 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
-
- local api_resp port ticket
- if ! api_resp="$(call_proxmox_api POST "/nodes/${node}/qemu/${vmid}/vncproxy")"; then
- echo "$api_resp" | jq -c '{status:"error", error:(.errors.curl // (.errors|tostring))}'
- exit 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 raw "$api_resp" \
- '{status:"error", error:"Proxmox response missing port/ticket", upstream:$raw}'
- exit 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", error:$msg}'
- exit 1
- fi
-
- jq -n \
- --arg host "$host" \
- --arg port "$port" \
- --arg password "$ticket" \
- --argjson passwordonetimeuseonly true \
- '{
- status: "success",
- message: "Console retrieved",
- console: {
- host: $host,
- port: $port,
- password: $password,
- passwordonetimeuseonly: $passwordonetimeuseonly,
- protocol: "vnc"
- }
- }'
-}
-
-statuses() {
- local response
- response=$(call_proxmox_api GET "/nodes/${node}/qemu")
-
- if [[ -z "$response" ]]; then
- echo '{"status":"error","message":"empty response from Proxmox API"}'
- return 1
- fi
-
- if ! echo "$response" | jq empty >/dev/null 2>&1; then
- echo '{"status":"error","message":"invalid JSON response from Proxmox API"}'
- return 1
- fi
-
- echo "$response" | jq -c '
- def map_state(s):
- if s=="running" then "poweron"
- elif s=="stopped" then "poweroff"
- else "unknown" end;
-
- {
- status: "success",
- power_state: (
- .data
- | map(select(.template != 1))
- | map({ ( (.name // (.vmid|tostring)) ): map_state(.status) })
- | add // {}
- )
- }'
-}
-
-list_snapshots() {
- snapshot_response=$(call_proxmox_api GET "/nodes/${node}/qemu/${vmid}/snapshot")
- echo "$snapshot_response" | jq '
- def to_date:
- if . == "-" then "-"
- elif . == null then "-"
- else (. | tonumber | strftime("%Y-%m-%d %H:%M:%S"))
- end;
-
- {
- status: "success",
- printmessage: "true",
- message: [.data[] | {
- name: .name,
- snaptime: ((.snaptime // "-") | to_date),
- description: .description,
- parent: (.parent // "-"),
- vmstate: (.vmstate // "-")
- }]
- }
- '
-}
-
-create_snapshot() {
- check_required_fields snap_name
- validate_name "Snapshot" "$snap_name"
-
- local data vmstate
- data="snapname=$snap_name"
- if [[ -n "$snap_description" ]]; then
- data+="&description=$snap_description"
- fi
- if [[ -n "$snap_save_memory" && "$snap_save_memory" == "true" ]]; then
- vmstate="1"
- else
- vmstate="0"
- fi
- data+="&vmstate=$vmstate"
-
- execute_and_wait POST "/nodes/${node}/qemu/${vmid}/snapshot" "$data"
- echo '{"status": "success", "message": "Instance Snapshot created"}'
-}
-
-restore_snapshot() {
- check_required_fields snap_name
- validate_name "Snapshot" "$snap_name"
-
- execute_and_wait POST "/nodes/${node}/qemu/${vmid}/snapshot/${snap_name}/rollback"
-
- status_response=$(call_proxmox_api GET "/nodes/${node}/qemu/${vmid}/status/current")
- vm_status=$(echo "$status_response" | jq -r '.data.status')
- if [ "$vm_status" = "stopped" ];then
- execute_and_wait POST "/nodes/${node}/qemu/${vmid}/status/start"
- fi
-
- echo '{"status": "success", "message": "Instance Snapshot restored"}'
-}
-
-delete_snapshot() {
- check_required_fields snap_name
- validate_name "Snapshot" "$snap_name"
-
- execute_and_wait DELETE "/nodes/${node}/qemu/${vmid}/snapshot/${snap_name}"
- echo '{"status": "success", "message": "Instance Snapshot deleted"}'
-}
-
-action=$1
-parameters_file="$2"
-wait_time=$3
-
-if [[ -z "$action" || -z "$parameters_file" ]]; then
- echo '{"status": "error", "error": "Missing required arguments"}'
- exit 1
-fi
-
-if [[ ! -r "$parameters_file" ]]; then
- echo '{"status": "error", "error": "File not found or unreadable"}'
- exit 1
-fi
-
-# Read file content as parameters (assumes space-separated arguments)
-parameters=$(<"$parameters_file")
-
-parse_json "$parameters" || exit 1
-
-cleanup_vm=0
-cleanup() {
- if (( cleanup_vm == 1 )); then
- execute_and_wait DELETE "/nodes/${node}/qemu/${vmid}"
- fi
-}
-
-trap cleanup EXIT
-
-case $action in
- prepare)
- prepare
- ;;
- create)
- create
- ;;
- delete)
- delete
- ;;
- start)
- start
- ;;
- stop)
- stop
- ;;
- reboot)
- reboot
- ;;
- status)
- status
- ;;
- statuses)
- statuses
- ;;
- getconsole)
- get_console
- ;;
- ListSnapshots)
- list_snapshots
- ;;
- CreateSnapshot)
- create_snapshot
- ;;
- RestoreSnapshot)
- restore_snapshot
- ;;
- DeleteSnapshot)
- delete_snapshot
- ;;
- *)
- echo '{"status": "error", "error": "Invalid action"}'
- exit 1
- ;;
-esac
-
-exit 0
+./proxmox.py "$@"
Review Comment:
`proxmox.sh` executes `./proxmox.py`, which depends on the current working directory being `extensions/Proxmox`. When CloudStack runs the extension by absolute path from another working directory, this will fail with “No such file or directory”. Resolve the Python script path relative to the wrapper’s location and `exec` it (optionally via `python3` to avoid relying on executable bits).
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] extension: proxmox: rewrite in python3 [cloudstack]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13832:
URL: https://github.com/apache/cloudstack/pull/13832#discussion_r3743258311
##########
extensions/Proxmox/proxmox.py:
##########
@@ -0,0 +1,697 @@
+#!/usr/bin/env python3
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
Review Comment:
This PR removes `extensions/Proxmox/proxmox.sh` but other parts of the repo still reference/install/register that path (e.g. `engine/schema/.../schema-42040to42100.sql:480` registers `Proxmox/proxmox.sh`, and `debian/cloudstack-management.install:25` installs `/etc/cloudstack/extensions/Proxmox/proxmox.sh`). Without updating those references to `Proxmox/proxmox.py`, the Proxmox extension will not be discoverable/packaged correctly.
##########
extensions/Proxmox/proxmox.py:
##########
@@ -0,0 +1,697 @@
+#!/usr/bin/env python3
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+import datetime as _dt
+import json
+import re
+import ssl
+import sys
+import time
+from dataclasses import dataclass
+from pathlib import Path
+from typing import Any
+from urllib import error, parse, request
+
+DEFAULT_WAIT_SECONDS = 600
+PROXMOX_API_PORT = 8006
+PROXMOX_API_PREFIX = "/api2/json"
+VM_NAME_PATTERN = re.compile(r"^[a-zA-Z0-9-]+$")
+
+
+class ProxmoxError(RuntimeError):
+ """Raised when the Proxmox API or payload validation fails."""
+
+
+def fail(message: str) -> None:
+ print(json.dumps({"status": "error", "error": message}))
+ raise SystemExit(1)
+
+
+def succeed(data: dict[str, Any]) -> None:
+ print(json.dumps(data))
+ raise SystemExit(0)
+
+
+def _is_mapping(value: Any) -> bool:
+ return isinstance(value, dict)
+
+
+def _mapping(value: Any) -> dict[str, Any]:
+ return value if isinstance(value, dict) else {}
+
+
+def _string(value: Any, default: str = "") -> str:
+ if value is None:
+ return default
+ if isinstance(value, str):
+ return value
+ return str(value)
+
+
+def _bool_text(value: Any, default: bool = False) -> bool:
+ if value is None:
+ return default
+ if isinstance(value, bool):
+ return value
+ return _string(value).strip().lower() in {"1", "true", "yes", "on"}
+
+
+def _int_text(value: Any, default: int = 0) -> int:
+ if value is None or value == "":
+ return default
+ try:
+ return int(value)
+ except (TypeError, ValueError):
+ return default
+
+
+def _normalize_url(url: str) -> str:
+ url = url.strip()
+ if not url.startswith(("http://", "https://")):
+ url = "https://" + url
+ return url.rstrip("/")
+
+
+def _format_snapshot_time(value: Any) -> str:
+ if value in (None, "", "-"):
+ return "-"
+ try:
+ return _dt.datetime.fromtimestamp(int(float(value))).strftime(
+ "%Y-%m-%d %H:%M:%S"
+ )
+ except (TypeError, ValueError, OSError, OverflowError):
+ return _string(value, "-")
+
+
+@dataclass(slots=True)
+class ProxmoxSettings:
+ url: str
+ user: str
+ token: str
+ secret: str
+ node: str
+ network_bridge: str
+ verify_tls_certificate: bool
+ vm_name: str
+ vm_internal_name: str
+ vmid: str
+ vmcpus: int
+ vmmemory: int
+ template_type: str
+ template_id: str
+ iso_path: str
+ iso_os_type: str
+ disk_size_gb: str
+ storage: str
+ is_full_clone: bool
+ snap_name: str
+ snap_description: str
+ snap_save_memory: bool
+ mac_addresses: list[str]
+ vlans: list[str]
+
+
+class ProxmoxManager:
+ def __init__(self, config_path: str, wait_time: int | None = None):
+ self.config_path = config_path
+ self.wait_time = (
+ wait_time if wait_time and wait_time > 0 else DEFAULT_WAIT_SECONDS
Review Comment:
`proxmox.py` uses Python 3.10+ syntax/features (e.g. `wait_time: int | None` and other PEP 604 unions; `@dataclass(slots=True)`; `zip(..., strict=False)`). CloudStack packaging/tests reference Python 3.6 compatibility in other components (e.g. `packaging/el8/cloud.spec:342` and `scripts/vm/.../test_base.py:63`), so this script may fail to even start on supported distros unless the minimum Python is raised consistently.
##########
extensions/Proxmox/proxmox.py:
##########
@@ -0,0 +1,697 @@
+#!/usr/bin/env python3
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+import datetime as _dt
+import json
+import re
+import ssl
+import sys
+import time
+from dataclasses import dataclass
+from pathlib import Path
+from typing import Any
+from urllib import error, parse, request
+
+DEFAULT_WAIT_SECONDS = 600
+PROXMOX_API_PORT = 8006
+PROXMOX_API_PREFIX = "/api2/json"
+VM_NAME_PATTERN = re.compile(r"^[a-zA-Z0-9-]+$")
+
+
+class ProxmoxError(RuntimeError):
+ """Raised when the Proxmox API or payload validation fails."""
+
+
+def fail(message: str) -> None:
+ print(json.dumps({"status": "error", "error": message}))
+ raise SystemExit(1)
+
+
+def succeed(data: dict[str, Any]) -> None:
+ print(json.dumps(data))
+ raise SystemExit(0)
+
+
+def _is_mapping(value: Any) -> bool:
+ return isinstance(value, dict)
+
Review Comment:
`_is_mapping()` is defined but never used, which adds dead code and can confuse future maintenance.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] extension: proxmox: rewrite in python3 [cloudstack]
Posted by "resmo (via GitHub)" <gi...@apache.org>.
resmo commented on PR #13832:
URL: https://github.com/apache/cloudstack/pull/13832#issuecomment-5290121955
> Without getting into the merits of the programming language/script to use, the idea behind using bash for this extension was to showcase that any executable can be used. This is why Proxmox extension was created in bash and HyperV extension was created in Python. I would still prefer it in bash or maybe something other than python.
Thanks @shwstppr for explanation, just to make it clear, I don't say your work or the show case is bad or wrong.
Showcases are good to show a case but I don't think that we should expand the scope of programming languages in apache/cloudstack just to "prove" we can do that. This is "software" of course you can do many things.
I don't know many people writing error prove bash and the ones do, avoid bash by any chance: maintability and extensibilty, integration, error handling and testabilty is way better in any other language.
Large parts of apache/cloudstack is in java or python. These are the langauges the community knows best. If we don't have a very, very good reason to not using one of these, use one of these.
(I burnt my fingers once created a project (gh/git-ftp/git-ftp) in bash and learned the hard way how easy it is to write "good looking but wrong" code in bash.)
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] extension: proxmox: rewrite in python3 [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #13832:
URL: https://github.com/apache/cloudstack/pull/13832#issuecomment-5292855371
I think we need a (more open) extensions repo, with liberal addition rights, for instance containing a file with external urls that refer extension repos???
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] extension: proxmox: rewrite in python3 [cloudstack]
Posted by "resmo (via GitHub)" <gi...@apache.org>.
resmo commented on PR #13832:
URL: https://github.com/apache/cloudstack/pull/13832#issuecomment-5396849816
Dear Apache CloudStack collegues
We moved this extension in pypi under Apache License with install instructions. Please visit https://github.com/ngine-io/cloudstack-extension-proxmox for more details and source.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] extension: proxmox: rewrite in python3 [cloudstack]
Posted by "kiranchavala (via GitHub)" <gi...@apache.org>.
kiranchavala commented on PR #13832:
URL: https://github.com/apache/cloudstack/pull/13832#issuecomment-5289370821
cc @shwstppr @harikrishna-patnala @abh1sar
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] extension: proxmox: rewrite in python3 [cloudstack]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #13832:
URL: https://github.com/apache/cloudstack/pull/13832#issuecomment-5230734592
## [Codecov](https://app.codecov.io/gh/apache/cloudstack/pull/13832?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 3.41%. Comparing base ([`5a67f19`](https://app.codecov.io/gh/apache/cloudstack/commit/5a67f1930ccbd2f568c07df53f9a6981ad72f629?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`0ef2aa1`](https://app.codecov.io/gh/apache/cloudstack/commit/0ef2aa11529ced11a94932c6ac9895aa4b89ed2e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on main.
> :exclamation: There is a different number of reports uploaded between BASE (5a67f19) and HEAD (0ef2aa1). Click for more details.
>
> <details><summary>HEAD has 1 upload less than BASE</summary>
>
>| Flag | BASE (5a67f19) | HEAD (0ef2aa1) |
>|------|------|------|
>|unittests|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #13832 +/- ##
=============================================
- Coverage 19.64% 3.41% -16.24%
=============================================
Files 6368 487 -5881
Lines 574889 41867 -533022
Branches 70353 7912 -62441
=============================================
- Hits 112962 1429 -111533
+ Misses 449656 40238 -409418
+ Partials 12271 200 -12071
```
| [Flag](https://app.codecov.io/gh/apache/cloudstack/pull/13832/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [uitests](https://app.codecov.io/gh/apache/cloudstack/pull/13832/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `3.41% <ø> (ø)` | |
| [unittests](https://app.codecov.io/gh/apache/cloudstack/pull/13832/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/cloudstack/pull/13832?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] extension: proxmox: rewrite in python3 [cloudstack]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13832:
URL: https://github.com/apache/cloudstack/pull/13832#discussion_r3765169595
##########
extensions/Proxmox/proxmox.py:
##########
@@ -0,0 +1,698 @@
+#!/usr/bin/env python3
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import annotations
+import datetime as _dt
+import json
+import re
+import ssl
+import sys
+import time
+from dataclasses import dataclass
+from pathlib import Path
+from typing import Any
+from urllib import error, parse, request
+
+DEFAULT_WAIT_SECONDS = 600
+PROXMOX_API_PORT = 8006
+PROXMOX_API_PREFIX = "/api2/json"
+VM_NAME_PATTERN = re.compile(r"^[a-zA-Z0-9-]+$")
+
+
+class ProxmoxError(RuntimeError):
+ """Raised when the Proxmox API or payload validation fails."""
+
+
+def fail(message: str) -> None:
+ print(json.dumps({"status": "error", "error": message}))
+ raise SystemExit(1)
+
+
+def succeed(data: dict[str, Any]) -> None:
+ print(json.dumps(data))
+ raise SystemExit(0)
+
+
+def _is_mapping(value: Any) -> bool:
+ return isinstance(value, dict)
+
+
+def _mapping(value: Any) -> dict[str, Any]:
+ return value if isinstance(value, dict) else {}
+
+
+def _string(value: Any, default: str = "") -> str:
+ if value is None:
+ return default
+ if isinstance(value, str):
+ return value
+ return str(value)
+
+
+def _bool_text(value: Any, default: bool = False) -> bool:
+ if value is None:
+ return default
+ if isinstance(value, bool):
+ return value
+ return _string(value).strip().lower() in {"1", "true", "yes", "on"}
+
+
+def _int_text(value: Any, default: int = 0) -> int:
+ if value is None or value == "":
+ return default
+ try:
+ return int(value)
+ except (TypeError, ValueError):
+ return default
+
+
+def _normalize_url(url: str) -> str:
+ url = url.strip()
+ if not url.startswith(("http://", "https://")):
+ url = "https://" + url
+ return url.rstrip("/")
+
+
+def _format_snapshot_time(value: Any) -> str:
+ if value in (None, "", "-"):
+ return "-"
+ try:
+ return _dt.datetime.fromtimestamp(int(float(value))).strftime(
+ "%Y-%m-%d %H:%M:%S"
+ )
+ except (TypeError, ValueError, OSError, OverflowError):
+ return _string(value, "-")
+
+
+@dataclass(slots=True)
+class ProxmoxSettings:
+ url: str
+ user: str
+ token: str
+ secret: str
+ node: str
+ network_bridge: str
+ verify_tls_certificate: bool
+ vm_name: str
+ vm_internal_name: str
+ vmid: str
+ vmcpus: int
+ vmmemory: int
+ template_type: str
+ template_id: str
+ iso_path: str
+ iso_os_type: str
+ disk_size_gb: str
+ storage: str
+ is_full_clone: bool
+ snap_name: str
+ snap_description: str
+ snap_save_memory: bool
+ mac_addresses: list[str]
+ vlans: list[str]
+
+
+class ProxmoxManager:
+ def __init__(self, config_path: str, wait_time: int | None = None):
Review Comment:
This implementation requires Python 3.10+ due to PEP 604 union types (int | None) and dataclass(slots=True). If the runtime environment may be older (common on some distros), this will fail at import time. Recommended fix: either (mandatory) document/enforce Python >= 3.10 for this extension, or (mandatory) refactor type hints to Optional[...] and remove dataclass slots to keep compatibility.
##########
extensions/Proxmox/proxmox.py:
##########
@@ -0,0 +1,698 @@
+#!/usr/bin/env python3
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import annotations
+import datetime as _dt
+import json
+import re
+import ssl
+import sys
+import time
+from dataclasses import dataclass
+from pathlib import Path
+from typing import Any
+from urllib import error, parse, request
+
+DEFAULT_WAIT_SECONDS = 600
+PROXMOX_API_PORT = 8006
+PROXMOX_API_PREFIX = "/api2/json"
+VM_NAME_PATTERN = re.compile(r"^[a-zA-Z0-9-]+$")
+
+
+class ProxmoxError(RuntimeError):
+ """Raised when the Proxmox API or payload validation fails."""
+
+
+def fail(message: str) -> None:
+ print(json.dumps({"status": "error", "error": message}))
+ raise SystemExit(1)
+
+
+def succeed(data: dict[str, Any]) -> None:
+ print(json.dumps(data))
+ raise SystemExit(0)
+
+
+def _is_mapping(value: Any) -> bool:
+ return isinstance(value, dict)
+
+
+def _mapping(value: Any) -> dict[str, Any]:
+ return value if isinstance(value, dict) else {}
+
+
+def _string(value: Any, default: str = "") -> str:
+ if value is None:
+ return default
+ if isinstance(value, str):
+ return value
+ return str(value)
+
+
+def _bool_text(value: Any, default: bool = False) -> bool:
+ if value is None:
+ return default
+ if isinstance(value, bool):
+ return value
+ return _string(value).strip().lower() in {"1", "true", "yes", "on"}
+
+
+def _int_text(value: Any, default: int = 0) -> int:
+ if value is None or value == "":
+ return default
+ try:
+ return int(value)
+ except (TypeError, ValueError):
+ return default
+
+
+def _normalize_url(url: str) -> str:
+ url = url.strip()
+ if not url.startswith(("http://", "https://")):
+ url = "https://" + url
+ return url.rstrip("/")
+
+
+def _format_snapshot_time(value: Any) -> str:
+ if value in (None, "", "-"):
+ return "-"
+ try:
+ return _dt.datetime.fromtimestamp(int(float(value))).strftime(
+ "%Y-%m-%d %H:%M:%S"
+ )
+ except (TypeError, ValueError, OSError, OverflowError):
+ return _string(value, "-")
+
+
+@dataclass(slots=True)
+class ProxmoxSettings:
+ url: str
+ user: str
+ token: str
+ secret: str
+ node: str
+ network_bridge: str
+ verify_tls_certificate: bool
+ vm_name: str
+ vm_internal_name: str
+ vmid: str
+ vmcpus: int
+ vmmemory: int
+ template_type: str
+ template_id: str
+ iso_path: str
+ iso_os_type: str
+ disk_size_gb: str
+ storage: str
+ is_full_clone: bool
+ snap_name: str
+ snap_description: str
+ snap_save_memory: bool
+ mac_addresses: list[str]
+ vlans: list[str]
+
+
+class ProxmoxManager:
+ def __init__(self, config_path: str, wait_time: int | None = None):
+ self.config_path = config_path
+ self.wait_time = (
+ wait_time if wait_time and wait_time > 0 else DEFAULT_WAIT_SECONDS
+ )
+ self.data = self.parse_json()
+ self._ssl_context = (
+ ssl.create_default_context()
+ if self.data.verify_tls_certificate
+ else ssl._create_unverified_context() # noqa: SLF001 - intentional for admin-controlled TLS bypass
+ )
+
+ def parse_json(self) -> ProxmoxSettings:
+ try:
+ payload = json.loads(Path(self.config_path).read_text(encoding="utf-8"))
+ except FileNotFoundError:
+ fail(f"JSON file not found: {self.config_path}")
+ except json.JSONDecodeError:
+ fail("Invalid JSON in file")
+ except OSError as exc:
+ fail(f"Unable to read JSON file: {exc}")
+ if not isinstance(payload, dict):
+ fail("Invalid JSON input")
+ externaldetails = _mapping(payload.get("externaldetails"))
+ extension = _mapping(externaldetails.get("extension"))
+ host = _mapping(externaldetails.get("host"))
+ vm = _mapping(externaldetails.get("virtualmachine"))
+ details_root = _mapping(payload.get("cloudstack.vm.details"))
+ vm_details = _mapping(details_root.get("details"))
+ parameters = _mapping(payload.get("parameters"))
+ url = _string(host.get("url") or extension.get("url"))
+ user = _string(host.get("user") or extension.get("user"))
+ token = _string(host.get("token") or extension.get("token"))
+ secret = _string(host.get("secret") or extension.get("secret"))
+ node = _string(host.get("node"))
+ network_bridge = _string(host.get("network_bridge"))
+ verify_tls = _bool_text(host.get("verify_tls_certificate", "true"), True)
+ vm_name = _string(vm.get("vm_name") or details_root.get("name"))
+ vm_internal_name = _string(details_root.get("name"))
+ vmid = _string(
+ vm_details.get("proxmox_vmid")
+ or details_root.get("details", {}).get("proxmox_vmid")
+ )
+ vmcpus = _int_text(details_root.get("cpus"), 0)
+ vmmemory = _int_text(details_root.get("minRam"), 0)
+ template_type = _string(vm.get("template_type"))
+ template_id = _string(vm.get("template_id"))
+ iso_path = _string(vm.get("iso_path"))
+ iso_os_type = _string(vm.get("iso_os_type", "l26"))
+ disk_size_gb = _string(vm.get("disk_size_gb", "64"))
+ storage = _string(vm.get("storage", "local-lvm"))
+ is_full_clone = _bool_text(vm.get("is_full_clone", "false"), False)
+ snap_name = _string(parameters.get("snap_name"))
+ snap_description = _string(parameters.get("snap_description"))
+ snap_save_memory = _bool_text(parameters.get("snap_save_memory", False), False)
+ nics = details_root.get("nics", [])
+ if not isinstance(nics, list):
+ nics = []
+ mac_addresses: list[str] = []
+ vlans: list[str] = []
+ for nic in nics:
+ nic_map = _mapping(nic)
+ mac_addresses.append(_string(nic_map.get("mac")))
+ vlan = _string(nic_map.get("broadcastUri"))
+ vlans.append(vlan.removeprefix("vlan://"))
Review Comment:
These lines further constrain the minimum Python version: str.removeprefix requires Python 3.9+, and zip(..., strict=...) requires Python 3.10+. If you need broader Python 3 compatibility, replace removeprefix with a startswith/slice pattern and avoid the strict= argument (zip already truncates to the shortest input by default).
##########
extensions/Proxmox/proxmox.py:
##########
@@ -0,0 +1,698 @@
+#!/usr/bin/env python3
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import annotations
+import datetime as _dt
+import json
+import re
+import ssl
+import sys
+import time
+from dataclasses import dataclass
+from pathlib import Path
+from typing import Any
+from urllib import error, parse, request
+
+DEFAULT_WAIT_SECONDS = 600
+PROXMOX_API_PORT = 8006
+PROXMOX_API_PREFIX = "/api2/json"
+VM_NAME_PATTERN = re.compile(r"^[a-zA-Z0-9-]+$")
+
+
+class ProxmoxError(RuntimeError):
+ """Raised when the Proxmox API or payload validation fails."""
+
+
+def fail(message: str) -> None:
+ print(json.dumps({"status": "error", "error": message}))
+ raise SystemExit(1)
+
+
+def succeed(data: dict[str, Any]) -> None:
+ print(json.dumps(data))
+ raise SystemExit(0)
+
+
+def _is_mapping(value: Any) -> bool:
+ return isinstance(value, dict)
+
+
+def _mapping(value: Any) -> dict[str, Any]:
+ return value if isinstance(value, dict) else {}
+
+
+def _string(value: Any, default: str = "") -> str:
+ if value is None:
+ return default
+ if isinstance(value, str):
+ return value
+ return str(value)
+
+
+def _bool_text(value: Any, default: bool = False) -> bool:
+ if value is None:
+ return default
+ if isinstance(value, bool):
+ return value
+ return _string(value).strip().lower() in {"1", "true", "yes", "on"}
+
+
+def _int_text(value: Any, default: int = 0) -> int:
+ if value is None or value == "":
+ return default
+ try:
+ return int(value)
+ except (TypeError, ValueError):
+ return default
+
+
+def _normalize_url(url: str) -> str:
+ url = url.strip()
+ if not url.startswith(("http://", "https://")):
+ url = "https://" + url
+ return url.rstrip("/")
+
+
+def _format_snapshot_time(value: Any) -> str:
+ if value in (None, "", "-"):
+ return "-"
+ try:
+ return _dt.datetime.fromtimestamp(int(float(value))).strftime(
+ "%Y-%m-%d %H:%M:%S"
+ )
+ except (TypeError, ValueError, OSError, OverflowError):
+ return _string(value, "-")
+
+
+@dataclass(slots=True)
Review Comment:
This implementation requires Python 3.10+ due to PEP 604 union types (int | None) and dataclass(slots=True). If the runtime environment may be older (common on some distros), this will fail at import time. Recommended fix: either (mandatory) document/enforce Python >= 3.10 for this extension, or (mandatory) refactor type hints to Optional[...] and remove dataclass slots to keep compatibility.
##########
extensions/Proxmox/proxmox.py:
##########
@@ -0,0 +1,698 @@
+#!/usr/bin/env python3
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import annotations
+import datetime as _dt
+import json
+import re
+import ssl
+import sys
+import time
+from dataclasses import dataclass
+from pathlib import Path
+from typing import Any
+from urllib import error, parse, request
+
+DEFAULT_WAIT_SECONDS = 600
+PROXMOX_API_PORT = 8006
+PROXMOX_API_PREFIX = "/api2/json"
+VM_NAME_PATTERN = re.compile(r"^[a-zA-Z0-9-]+$")
+
+
+class ProxmoxError(RuntimeError):
+ """Raised when the Proxmox API or payload validation fails."""
+
+
+def fail(message: str) -> None:
+ print(json.dumps({"status": "error", "error": message}))
+ raise SystemExit(1)
+
+
+def succeed(data: dict[str, Any]) -> None:
+ print(json.dumps(data))
+ raise SystemExit(0)
+
+
+def _is_mapping(value: Any) -> bool:
+ return isinstance(value, dict)
+
+
+def _mapping(value: Any) -> dict[str, Any]:
+ return value if isinstance(value, dict) else {}
+
+
+def _string(value: Any, default: str = "") -> str:
+ if value is None:
+ return default
+ if isinstance(value, str):
+ return value
+ return str(value)
+
+
+def _bool_text(value: Any, default: bool = False) -> bool:
+ if value is None:
+ return default
+ if isinstance(value, bool):
+ return value
+ return _string(value).strip().lower() in {"1", "true", "yes", "on"}
+
+
+def _int_text(value: Any, default: int = 0) -> int:
+ if value is None or value == "":
+ return default
+ try:
+ return int(value)
+ except (TypeError, ValueError):
+ return default
+
+
+def _normalize_url(url: str) -> str:
+ url = url.strip()
+ if not url.startswith(("http://", "https://")):
+ url = "https://" + url
+ return url.rstrip("/")
+
+
+def _format_snapshot_time(value: Any) -> str:
+ if value in (None, "", "-"):
+ return "-"
+ try:
+ return _dt.datetime.fromtimestamp(int(float(value))).strftime(
+ "%Y-%m-%d %H:%M:%S"
+ )
+ except (TypeError, ValueError, OSError, OverflowError):
+ return _string(value, "-")
+
+
+@dataclass(slots=True)
+class ProxmoxSettings:
+ url: str
+ user: str
+ token: str
+ secret: str
+ node: str
+ network_bridge: str
+ verify_tls_certificate: bool
+ vm_name: str
+ vm_internal_name: str
+ vmid: str
+ vmcpus: int
+ vmmemory: int
+ template_type: str
+ template_id: str
+ iso_path: str
+ iso_os_type: str
+ disk_size_gb: str
+ storage: str
+ is_full_clone: bool
+ snap_name: str
+ snap_description: str
+ snap_save_memory: bool
+ mac_addresses: list[str]
+ vlans: list[str]
+
+
+class ProxmoxManager:
+ def __init__(self, config_path: str, wait_time: int | None = None):
+ self.config_path = config_path
+ self.wait_time = (
+ wait_time if wait_time and wait_time > 0 else DEFAULT_WAIT_SECONDS
+ )
+ self.data = self.parse_json()
+ self._ssl_context = (
+ ssl.create_default_context()
+ if self.data.verify_tls_certificate
+ else ssl._create_unverified_context() # noqa: SLF001 - intentional for admin-controlled TLS bypass
+ )
+
+ def parse_json(self) -> ProxmoxSettings:
+ try:
+ payload = json.loads(Path(self.config_path).read_text(encoding="utf-8"))
+ except FileNotFoundError:
+ fail(f"JSON file not found: {self.config_path}")
+ except json.JSONDecodeError:
+ fail("Invalid JSON in file")
+ except OSError as exc:
+ fail(f"Unable to read JSON file: {exc}")
+ if not isinstance(payload, dict):
+ fail("Invalid JSON input")
+ externaldetails = _mapping(payload.get("externaldetails"))
+ extension = _mapping(externaldetails.get("extension"))
+ host = _mapping(externaldetails.get("host"))
+ vm = _mapping(externaldetails.get("virtualmachine"))
+ details_root = _mapping(payload.get("cloudstack.vm.details"))
+ vm_details = _mapping(details_root.get("details"))
+ parameters = _mapping(payload.get("parameters"))
+ url = _string(host.get("url") or extension.get("url"))
+ user = _string(host.get("user") or extension.get("user"))
+ token = _string(host.get("token") or extension.get("token"))
+ secret = _string(host.get("secret") or extension.get("secret"))
+ node = _string(host.get("node"))
+ network_bridge = _string(host.get("network_bridge"))
+ verify_tls = _bool_text(host.get("verify_tls_certificate", "true"), True)
+ vm_name = _string(vm.get("vm_name") or details_root.get("name"))
+ vm_internal_name = _string(details_root.get("name"))
+ vmid = _string(
+ vm_details.get("proxmox_vmid")
+ or details_root.get("details", {}).get("proxmox_vmid")
+ )
+ vmcpus = _int_text(details_root.get("cpus"), 0)
+ vmmemory = _int_text(details_root.get("minRam"), 0)
+ template_type = _string(vm.get("template_type"))
+ template_id = _string(vm.get("template_id"))
+ iso_path = _string(vm.get("iso_path"))
+ iso_os_type = _string(vm.get("iso_os_type", "l26"))
+ disk_size_gb = _string(vm.get("disk_size_gb", "64"))
+ storage = _string(vm.get("storage", "local-lvm"))
+ is_full_clone = _bool_text(vm.get("is_full_clone", "false"), False)
+ snap_name = _string(parameters.get("snap_name"))
+ snap_description = _string(parameters.get("snap_description"))
+ snap_save_memory = _bool_text(parameters.get("snap_save_memory", False), False)
+ nics = details_root.get("nics", [])
+ if not isinstance(nics, list):
+ nics = []
+ mac_addresses: list[str] = []
+ vlans: list[str] = []
+ for nic in nics:
+ nic_map = _mapping(nic)
+ mac_addresses.append(_string(nic_map.get("mac")))
+ vlan = _string(nic_map.get("broadcastUri"))
+ vlans.append(vlan.removeprefix("vlan://"))
+ if not url or not user or not token or not secret or not node:
+ missing = [
+ name
+ for name, value in (
+ ("url", url),
+ ("user", user),
+ ("token", token),
+ ("secret", secret),
+ ("node", node),
+ )
+ if not value
+ ]
+ fail(f"Missing required fields: {' '.join(missing)}")
+ return ProxmoxSettings(
+ url=_normalize_url(url),
+ user=user,
+ token=token,
+ secret=secret,
+ node=node,
+ network_bridge=network_bridge,
+ verify_tls_certificate=verify_tls,
+ vm_name=vm_name,
+ vm_internal_name=vm_internal_name,
+ vmid=vmid,
+ vmcpus=vmcpus,
+ vmmemory=vmmemory,
+ template_type=template_type,
+ template_id=template_id,
+ iso_path=iso_path,
+ iso_os_type=iso_os_type,
+ disk_size_gb=disk_size_gb,
+ storage=storage,
+ is_full_clone=is_full_clone,
+ snap_name=snap_name,
+ snap_description=snap_description,
+ snap_save_memory=snap_save_memory,
+ mac_addresses=mac_addresses,
+ vlans=vlans,
+ )
+
+ def _auth_header(self) -> str:
+ return f"PVEAPIToken={self.data.user}!{self.data.token}={self.data.secret}"
+
+ def call_api(
+ self, method: str, path: str, data: dict[str, Any] | str | None = None
+ ) -> dict[str, Any]:
+ url = f"{self.data.url}:{PROXMOX_API_PORT}{PROXMOX_API_PREFIX}{path}"
+ headers = {"Authorization": self._auth_header()}
+ body: bytes | None
+ if data is None:
+ body = None
+ elif isinstance(data, str):
+ body = data.encode("utf-8")
+ headers["Content-Type"] = "application/x-www-form-urlencoded"
+ else:
+ body = parse.urlencode(data, doseq=True).encode("utf-8")
+ headers["Content-Type"] = "application/x-www-form-urlencoded"
+ req = request.Request(url, data=body, headers=headers, method=method.upper())
+ try:
+ with request.urlopen(req, context=self._ssl_context, timeout=120) as resp:
+ raw = resp.read().decode("utf-8", errors="replace")
+ except error.HTTPError as exc:
+ raw = exc.read().decode("utf-8", errors="replace")
+ raise ProxmoxError(
+ self._extract_error_message(raw, exc.code, exc.reason)
+ ) from exc
+ except error.URLError as exc:
+ raise ProxmoxError(str(getattr(exc, "reason", exc))) from exc
+ if not raw.strip():
+ return {}
+ try:
+ parsed = json.loads(raw)
+ except json.JSONDecodeError as exc:
+ raise ProxmoxError(
+ f"Invalid JSON response from Proxmox API: {raw}"
+ ) from exc
+ if not isinstance(parsed, dict):
+ raise ProxmoxError("Invalid response from Proxmox API")
+ return parsed
+
+ @staticmethod
+ def _extract_error_message(raw: str, status_code: int, reason: str | None) -> str:
+ fallback = f"HTTP {status_code}{': ' + reason if reason else ''}"
+ try:
+ parsed = json.loads(raw)
+ except json.JSONDecodeError:
+ return f"{fallback}: {raw.strip() or 'Unknown error'}"
+ if isinstance(parsed, dict):
+ for key in ("message", "error"):
+ value = parsed.get(key)
+ if value:
+ return _string(value)
+ errors = parsed.get("errors")
+ if errors:
+ return _string(errors)
+ return f"{fallback}: {raw.strip() or 'Unknown error'}"
+
+ def execute_and_wait(
+ self, method: str, path: str, data: dict[str, Any] | str | None = None
+ ) -> None:
+ response = self.call_api(method, path, data)
+ upid = _string(response.get("data"))
+ if not upid:
+ message = _string(response.get("message"), "Unknown error")
+ error_detail = _string(response.get("error"))
+ if error_detail:
+ message = error_detail
+ raise ProxmoxError(
+ f"Failed to execute API or retrieve UPID. Message: {message}"
+ )
+ self.wait_for_task(upid)
+
+ def wait_for_task(
+ self, upid: str, timeout: int | None = None, interval: int = 1
+ ) -> None:
+ timeout = self.wait_time if timeout is None or timeout <= 0 else timeout
+ deadline = time.monotonic() + timeout
+ task_path = f"/nodes/{self.data.node}/tasks/{parse.quote(upid, safe='')}/status"
+ while True:
+ if time.monotonic() > deadline:
+ raise ProxmoxError("Timeout while waiting for async task")
+ response = self.call_api("GET", task_path)
+ status_data = _mapping(response.get("data"))
+ task_status = _string(status_data.get("status")).lower()
+ if task_status == "stopped":
+ exit_status = _string(status_data.get("exitstatus"))
+ if exit_status and exit_status != "OK":
+ raise ProxmoxError(f"Task failed with exit status: {exit_status}")
+ return
+ time.sleep(interval)
+
+ def vm_not_present(self) -> bool:
+ try:
+ self.call_api(
+ "GET", f"/nodes/{self.data.node}/qemu/{self.data.vmid}/status/current"
+ )
+ return False
+ except ProxmoxError as exc:
+ message = str(exc).lower()
+ return any(
+ token in message
+ for token in (
+ "not found",
+ "does not exist",
+ "no such vm",
+ "unknown vm",
+ "unable to find a virtual machine",
+ "vmid not found",
+ )
+ )
+
+ def prepare(self) -> None:
+ response = self.call_api("GET", "/cluster/nextid")
+ vmid = _string(response.get("data"))
+ if not vmid:
+ raise ProxmoxError(
+ _string(
+ response.get("message"), "Unable to retrieve next available VM ID"
+ )
+ )
+ succeed({"details": {"proxmox_vmid": vmid}})
+
+ def create(self) -> None:
+ vm_name = self.data.vm_name or self.data.vm_internal_name
+ if not vm_name:
+ fail("Missing required fields: vm_internal_name")
+ if not VM_NAME_PATTERN.fullmatch(vm_name):
+ fail(
+ f"Invalid VM name '{vm_name}'. Only alphanumeric characters and dashes (-) are allowed."
+ )
+ required = {
+ "vmid": self.data.vmid,
+ "network_bridge": self.data.network_bridge,
+ "vmcpus": self.data.vmcpus,
+ "vmmemory": self.data.vmmemory,
+ }
+ missing = [name for name, value in required.items() if value in (None, "", 0)]
+ if missing:
+ fail(f"Missing required fields: {' '.join(missing)}")
+ cleanup_vm = False
+ try:
+ if self.data.template_type.strip().upper() == "ISO":
+ if not self.data.iso_path:
+ fail("Missing required field in JSON: iso_path")
+ if not self.data.disk_size_gb:
+ fail("Missing required field in JSON: disk_size_gb")
+ payload = {
+ "vmid": self.data.vmid,
+ "name": vm_name,
+ "ide2": f"{self.data.iso_path},media=cdrom",
+ "ostype": self.data.iso_os_type,
+ "scsihw": "virtio-scsi-single",
+ "scsi0": f"{self.data.storage}:{self.data.disk_size_gb},iothread=on",
+ "sockets": 1,
+ "cores": self.data.vmcpus,
+ "numa": 0,
+ "cpu": "x86-64-v2-AES",
+ "memory": self.data.vmmemory // 1024 // 1024,
+ }
+ cleanup_vm = True
+ self.execute_and_wait("POST", f"/nodes/{self.data.node}/qemu/", payload)
+ else:
+ if not self.data.template_id:
+ fail("Missing required field in JSON: template_id")
+ clone_payload = {
+ "newid": self.data.vmid,
+ "name": vm_name,
+ "storage": self.data.storage,
+ "full": 1 if self.data.is_full_clone else 0,
+ }
+ cleanup_vm = True
+ self.execute_and_wait(
+ "POST",
+ f"/nodes/{self.data.node}/qemu/{self.data.template_id}/clone",
+ clone_payload,
+ )
+ config_payload = {
+ "cores": self.data.vmcpus,
+ "memory": self.data.vmmemory // 1024 // 1024,
+ }
+ self.execute_and_wait(
+ "POST",
+ f"/nodes/{self.data.node}/qemu/{self.data.vmid}/config",
+ config_payload,
+ )
+ for idx, (mac, vlan) in enumerate(
+ zip(self.data.mac_addresses, self.data.vlans, strict=False)
+ ):
Review Comment:
These lines further constrain the minimum Python version: str.removeprefix requires Python 3.9+, and zip(..., strict=...) requires Python 3.10+. If you need broader Python 3 compatibility, replace removeprefix with a startswith/slice pattern and avoid the strict= argument (zip already truncates to the shortest input by default).
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] extension: proxmox: rewrite in python3 [cloudstack]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13832:
URL: https://github.com/apache/cloudstack/pull/13832#discussion_r3765642485
##########
extensions/Proxmox/proxmox.py:
##########
@@ -0,0 +1,701 @@
+#!/usr/bin/env python3
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import annotations
+import datetime as _dt
+import json
+import re
+import ssl
+import sys
+import time
+from dataclasses import dataclass
+from pathlib import Path
+from typing import Any
+from urllib import error, parse, request
+
+DEFAULT_WAIT_SECONDS = 600
+PROXMOX_API_PREFIX = "/api2/json"
+VM_NAME_PATTERN = re.compile(r"^[a-zA-Z0-9-]+$")
+
+
+class ProxmoxError(RuntimeError):
+ """Raised when the Proxmox API or payload validation fails."""
+
+
+def fail(message: str) -> None:
+ print(json.dumps({"status": "error", "error": message}))
+ raise SystemExit(1)
+
+
+def succeed(data: dict[str, Any]) -> None:
+ print(json.dumps(data))
+ raise SystemExit(0)
+
+
+def _is_mapping(value: Any) -> bool:
+ return isinstance(value, dict)
+
+
+def _mapping(value: Any) -> dict[str, Any]:
+ return value if isinstance(value, dict) else {}
+
+
+def _string(value: Any, default: str = "") -> str:
+ if value is None:
+ return default
+ if isinstance(value, str):
+ return value
+ return str(value)
+
+
+def _bool_text(value: Any, default: bool = False) -> bool:
+ if value is None:
+ return default
+ if isinstance(value, bool):
+ return value
+ return _string(value).strip().lower() in {"1", "true", "yes", "on"}
+
+
+def _int_text(value: Any, default: int = 0) -> int:
+ if value is None or value == "":
+ return default
+ try:
+ return int(value)
+ except (TypeError, ValueError):
+ return default
+
+
+def _normalize_url(url: str) -> str:
+ url = url.strip()
+ if not url.startswith(("http://", "https://")):
+ url = "https://" + url
+ return url.rstrip("/")
Review Comment:
The previous implementation always called the Proxmox API on port 8006 ("https://${url}:8006/..."). call_api() now uses self.data.url as-is, so a typical host value like "pve.example.com" will produce "https://pve.example.com/api2/json..." and fail. Consider normalizing the base URL to include :8006 when no port is provided.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] extension: proxmox: rewrite in python3 [cloudstack]
Posted by "NuxRo (via GitHub)" <gi...@apache.org>.
NuxRo commented on PR #13832:
URL: https://github.com/apache/cloudstack/pull/13832#issuecomment-5290352424
@resmo great initiative, but I'll side with @shwstppr on this one.
The decision to do the Proxmox extension in shell was collective and not taken lightly.
That said, I would not have a problem at all, on the contrary, I would even encourage alternative extensions in whatever language. There's no rule to say there must only be one of a kind.
You could submit it as proxmox-py or something like this.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] extension: proxmox: rewrite in python3 [cloudstack]
Posted by "resmo (via GitHub)" <gi...@apache.org>.
resmo closed pull request #13832: extension: proxmox: rewrite in python3
URL: https://github.com/apache/cloudstack/pull/13832
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org