extension: proxmox: rewrite in python3 - #13832
Conversation
There was a problem hiding this comment.
Pull request overview
This PR rewrites the Proxmox extension implementation from a Bash script to a Python 3 script, aiming to improve maintainability and reduce reliance on shell tooling.
Changes:
- Removed the legacy
proxmox.shBash-based extension implementation. - Added a new
proxmox.pyPython-based implementation covering lifecycle actions (prepare/create/start/stop/reboot/delete/status/statuses), console retrieval, and snapshot operations.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| extensions/Proxmox/proxmox.sh | Removed the previous Bash implementation of the Proxmox extension. |
| extensions/Proxmox/proxmox.py | Added a Python implementation for Proxmox extension operations, including VM lifecycle, console access, and snapshot management. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #13832 +/- ##
============================================
- Coverage 19.64% 19.64% -0.01%
+ Complexity 19790 19787 -3
============================================
Files 6368 6368
Lines 574889 574889
Branches 70353 70353
============================================
- Hits 112962 112952 -10
- Misses 449656 449666 +10
Partials 12271 12271
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
extensions/Proxmox/proxmox.py:107
- This script uses multiple Python 3.9/3.10+ language features (e.g.,
dict[str, Any]/list[str],str.removeprefix,@dataclass(slots=True), PEP 604 unions likeint | None, andzip(..., strict=False)). On distros where/usr/bin/python3is < 3.10, the script will fail to even start due to syntax errors. Either refactor to a Python version that matches CloudStack’s supported platforms (e.g., 3.8+) or explicitly document/enforce a minimum Python version for this extension in packaging/runtime checks.
@dataclass(slots=True)
class ProxmoxSettings:
url: str
user: str
token: str
secret: str
extensions/Proxmox/proxmox.py:88
_normalize_url()keeps any port present in the configured URL, butcall_api()always appends:8006. If the input is already likehttps://pve.example:8006, requests becomehttps://pve.example:8006:8006/...and will fail. Normalize the URL down toscheme://host(dropping any provided port/path) socall_api()can safely append the Proxmox API port exactly once.
def _normalize_url(url: str) -> str:
url = url.strip()
if not url.startswith(("http://", "https://")):
url = "https://" + url
return url.rstrip("/")
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
Suppressed comments (5)
extensions/Proxmox/proxmox.sh:20
- The wrapper executes proxmox.py directly, which requires the file to be marked executable and to have a working env-based shebang resolution; otherwise it can fail with 'Permission denied' or 'Exec format error'. More robust approach (mandatory): invoke it via python3 explicitly (e.g., python3 "$SCRIPT_PATH/proxmox.py" "$@").
SCRIPT_PATH="$( cd -- "$(dirname "$0")" >/dev/null 2>&1 ; pwd -P )"
"$SCRIPT_PATH/proxmox.py" "$@"
extensions/Proxmox/proxmox.py:43
- fail() raises SystemExit and is called from within ProxmoxManager methods (e.g., parse_json/create validations). This makes the class harder to unit test/reuse and mixes process-exit behavior into lower-level logic. Recommended fix (mandatory): have ProxmoxManager raise ProxmoxError (or a validation exception) and keep SystemExit/printing centralized in main().
def fail(message: str) -> None:
print(json.dumps({"status": "error", "error": message}))
raise SystemExit(1)
extensions/Proxmox/proxmox.py:147
- fail() raises SystemExit and is called from within ProxmoxManager methods (e.g., parse_json/create validations). This makes the class harder to unit test/reuse and mixes process-exit behavior into lower-level logic. Recommended fix (mandatory): have ProxmoxManager raise ProxmoxError (or a validation exception) and keep SystemExit/printing centralized in main().
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}")
extensions/Proxmox/proxmox.py:655
- The usage message is missing the optional wait_time argument that the script actually accepts (sys.argv[3]). Consider updating it to reflect the real CLI signature so operator errors are easier to diagnose.
if len(sys.argv) < 3:
fail("Usage: proxmox.py <operation> '<json-file-path>'")
extensions/Proxmox/proxmox.py:141
- ssl._create_unverified_context() is a private API. If you want to keep the TLS-bypass feature, consider constructing an SSLContext via public APIs (e.g., create_default_context + verify_mode changes) to avoid relying on underscored implementation details.
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
)
| return _string(value, "-") | ||
|
|
||
|
|
||
| @dataclass(slots=True) |
| class ProxmoxManager: | ||
| def __init__(self, config_path: str, wait_time: int | None = None): |
| nic_map = _mapping(nic) | ||
| mac_addresses.append(_string(nic_map.get("mac"))) | ||
| vlan = _string(nic_map.get("broadcastUri")) | ||
| vlans.append(vlan.removeprefix("vlan://")) |
| for idx, (mac, vlan) in enumerate( | ||
| zip(self.data.mac_addresses, self.data.vlans, strict=False) | ||
| ): |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
extensions/Proxmox/proxmox.sh:20
- proxmox.sh executes proxmox.py directly, which depends on the executable bit being set on the Python file. Invoking it via python3 (and using exec) makes the wrapper robust and preserves the child exit code/signals consistently.
SCRIPT_PATH="$( cd -- "$(dirname "$0")" >/dev/null 2>&1 ; pwd -P )"
"$SCRIPT_PATH/proxmox.py" "$@"
extensions/Proxmox/proxmox.py:334
- vm_not_present() treats any "not found"-like error as absence. If vmid is missing/empty, the generated URL contains a double-slash (".../qemu//status/current"), which is very likely to 404 and be interpreted as "VM not present". That makes stop/delete incorrectly return success when the request is actually invalid. Fail fast when vmid is missing.
def vm_not_present(self) -> bool:
try:
self.call_api(
"GET", f"/nodes/{self.data.node}/qemu/{self.data.vmid}/status/current"
)
| def _normalize_url(url: str) -> str: | ||
| url = url.strip() | ||
| if not url.startswith(("http://", "https://")): | ||
| url = "https://" + url | ||
| return url.rstrip("/") |
|
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. Bash is just good as glue: e.g. the bash implementation relies on other "software" such as jq for parsing json, curl to make http requests. (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.) |
|
@resmo great initiative, but I'll side with @shwstppr on this one. 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. |
Description
this PR rewrites the proxmox extension from shell to modern python3 for better maintainability.
Due to the fact, that ubuntu 22.04 already used py3.10 and all other have newer python3 or the possibility install a later version (rhel9), I'd like to keep py3.10 syntax.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?