Copilot commented on code in PR #14292:
URL: https://github.com/apache/cloudstack/pull/14292#discussion_r4204230113
##########
server/src/main/java/org/apache/cloudstack/ca/CAManagerImpl.java:
##########
@@ -339,6 +340,31 @@ private boolean provisionKvmHostViaSsh(Host host, String
caProvider) {
}
}
+ /**
+ * Live-reloads the VNC TLS certificate on running VMs that have VNC TLS
enabled via SSH, since a
+ * libvirtd/cloudstack-agent restart alone does not affect VMs already
running. Per-VM failures are tolerated and logged.
+ */
+ private void reloadVncTlsCertificateOnRunningVmsViaSsh(final Connection
sshConnection, final String sudoPrefix, final String hostIp) {
+ final String cmd = sudoPrefix + "virsh -c qemu:///system list --name
--state-running | while read -r vm; do " +
+ "[ -z \"$vm\" ] && continue; " +
+ "vnc_info=$(" + sudoPrefix + "virsh -c qemu:///system
qemu-monitor-command \"$vm\" '{\"execute\":\"query-vnc\"}' 2>/dev/null | grep
-o '\"auth\":\"[^\"]*' | cut -d'\"' -f4); " +
Review Comment:
A failed `qemu-monitor-command` is redirected away, and the pipeline ending
in `cut` still normally returns success, leaving `vnc_info` empty. That failure
is then reported by the `else` branch as “VNC TLS is disabled”, so transient VM
races or permission/libvirt errors are not logged as per-VM failures as
promised. Preserve the query status and distinguish an unsuccessful query from
a successful query whose auth is not TLS-enabled.
##########
server/src/main/java/org/apache/cloudstack/ca/CAManagerImpl.java:
##########
@@ -339,6 +340,31 @@ private boolean provisionKvmHostViaSsh(Host host, String
caProvider) {
}
}
+ /**
+ * Live-reloads the VNC TLS certificate on running VMs that have VNC TLS
enabled via SSH, since a
+ * libvirtd/cloudstack-agent restart alone does not affect VMs already
running. Per-VM failures are tolerated and logged.
+ */
+ private void reloadVncTlsCertificateOnRunningVmsViaSsh(final Connection
sshConnection, final String sudoPrefix, final String hostIp) {
+ final String cmd = sudoPrefix + "virsh -c qemu:///system list --name
--state-running | while read -r vm; do " +
Review Comment:
The `virsh list | while` pipeline reports the status of the `while` loop,
not the `virsh list` process. If listing domains fails (for example because
libvirt is unavailable), the loop can still exit 0, so this helper logs success
and proceeds without reloading any VM. Enable `pipefail` or capture and check
the listing command separately so host-level failures are surfaced.
##########
server/src/main/java/org/apache/cloudstack/ca/CAManagerImpl.java:
##########
@@ -339,6 +340,31 @@ private boolean provisionKvmHostViaSsh(Host host, String
caProvider) {
}
}
+ /**
+ * Live-reloads the VNC TLS certificate on running VMs that have VNC TLS
enabled via SSH, since a
+ * libvirtd/cloudstack-agent restart alone does not affect VMs already
running. Per-VM failures are tolerated and logged.
+ */
+ private void reloadVncTlsCertificateOnRunningVmsViaSsh(final Connection
sshConnection, final String sudoPrefix, final String hostIp) {
+ final String cmd = sudoPrefix + "virsh -c qemu:///system list --name
--state-running | while read -r vm; do " +
+ "[ -z \"$vm\" ] && continue; " +
+ "vnc_info=$(" + sudoPrefix + "virsh -c qemu:///system
qemu-monitor-command \"$vm\" '{\"execute\":\"query-vnc\"}' 2>/dev/null | grep
-o '\"auth\":\"[^\"]*' | cut -d'\"' -f4); " +
+ "if [[ \"$vnc_info\" == vencrypt+x509* ]]; then " +
Review Comment:
This command is executed through SSH's default remote shell, which is
commonly POSIX `/bin/sh`; `[[ ... ]]` is a Bash-only conditional. On hosts
using `dash` (for example Debian/Ubuntu), this condition reports `[[: not
found` and takes the `else` branch, so no running VM ever receives
`display-reload`. Use a POSIX `[ ... ]` prefix check or explicitly invoke `bash
-c` for the whole command.
##########
server/src/test/java/org/apache/cloudstack/ca/CAManagerImplTest.java:
##########
@@ -281,6 +281,10 @@ public void testProvisionKvmHostViaSsh() throws Exception {
MockedStatic<SSHCmdHelper> sshCmdHelperMock =
Mockito.mockStatic(SSHCmdHelper.class)) {
sshCmdHelperMock.when(() ->
SSHCmdHelper.acquireAuthorizedConnectionWithPublicKey(Mockito.any(Connection.class),
Mockito.anyString(), Mockito.anyString()))
.thenReturn(true);
+ sshCmdHelperMock.when(() ->
SSHCmdHelper.sshExecuteCmdWithResult(Mockito.any(Connection.class),
Mockito.contains("virsh")))
+ .thenReturn(new SSHCmdHelper.SSHCmdResult(0, "", ""));
Review Comment:
This test stubs every `virsh` invocation with an empty successful result and
only checks that one command was called. It therefore does not exercise the new
behavior for a running `vencrypt+x509` VM, the `display-reload` payload, or a
per-VM failure, so regressions in the command construction can pass; capture
the generated command and add representative success/failure cases.
--
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]
