Re: [PR] Draft: Add logs to CPVM connection process [cloudstack]

[via GitHub]

github-actions[bot] commented on PR #8924:
URL: https://github.com/apache/cloudstack/pull/8924#issuecomment-2062092698

   This pull request has merge conflicts. Dear author, please fix the conflicts 
and sync your branch with the base branch.


-- 
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-unsubscr...@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] Draft: Add logs to CPVM connection process [cloudstack]

[via GitHub]

DaanHoogland commented on code in PR #8924:
URL: https://github.com/apache/cloudstack/pull/8924#discussion_r1568395543


##
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyNoVNCHandler.java:
##
@@ -104,20 +104,20 @@ public void onConnect(final Session session) throws 
IOException, InterruptedExce
 try {
 port = Integer.parseInt(portStr);
 } catch (NumberFormatException e) {
-s_logger.warn("Invalid number parameter in query string: " + 
portStr);
+s_logger.error("Invalid port value in query string: {}. Expected a 
number.", portStr, e);
 throw new IllegalArgumentException(e);
 }
 
 if (ajaxSessionIdStr != null) {
 try {
 ajaxSessionId = Long.parseLong(ajaxSessionIdStr);
 } catch (NumberFormatException e) {
-s_logger.warn("Invalid number parameter in query string: " + 
ajaxSessionIdStr);
+s_logger.error("Invalid ajaxSessionId (sess) value in query 
string: {}. Expected a number.", ajaxSessionIdStr, e);

Review Comment:
   same here; stacktrace at `error`



##
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyNoVNCHandler.java:
##
@@ -129,23 +129,25 @@ public void onConnect(final Session session) throws 
IOException, InterruptedExce
 param.setClientTag(tag);
 param.setTicket(ticket);
 param.setClientDisplayName(displayName);
-param.setClientTunnelUrl(console_url);
-param.setClientTunnelSession(console_host_session);
-param.setLocale(vm_locale);
+param.setClientTunnelUrl(consoleUrl);
+param.setClientTunnelSession(consoleHostSession);
+param.setLocale(vmLocale);
 param.setHypervHost(hypervHost);
 param.setUsername(username);
 param.setPassword(password);
 param.setWebsocketUrl(websocketUrl);
 param.setSessionUuid(sessionUuid);
+param.setSourceIP(sourceIP);
 if (queryMap.containsKey("extraSecurityToken")) {
 
param.setExtraSecurityToken(queryMap.get("extraSecurityToken"));
 }
 if (queryMap.containsKey("extra")) {
 
param.setClientProvidedExtraSecurityToken(queryMap.get("extra"));
 }
 viewer = ConsoleProxy.getNoVncViewer(param, ajaxSessionIdStr, 
session);
+s_logger.debug("Viewer has been created successfully.");
 } catch (Exception e) {
-s_logger.warn("Failed to create viewer due to " + e.getMessage(), 
e);
+s_logger.error("Failed to create viewer due to {}", 
e.getMessage(), e);

Review Comment:
   same here, also note in spite of it still being a point of discussion in 
case of system degradation; a clean return follows so this is not considdered 
such a case it seems. see #8746 



##
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/vnc/NoVncClient.java:
##
@@ -175,8 +176,9 @@ public byte[] authenticateTunnel(String password)
 is.readFully(buf);
 String reason = new String(buf, RfbConstants.CHARSET);
 
-s_logger.error("Authentication to VNC server is failed. 
Reason: " + reason);
-throw new RuntimeException("Authentication to VNC server is 
failed. Reason: " + reason);
+String msg = String.format("Authentication to VNC server has 
failed. Reason: %s", reason);
+s_logger.error(msg);

Review Comment:
   :+1: 



##
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyNoVncClient.java:
##
@@ -300,8 +301,10 @@ private void connectClientToVNCServer(String tunnelUrl, 
String tunnelSession, St
 ConsoleProxy.ensureRoute(getClientHostAddress());
 client.connectTo(getClientHostAddress(), getClientHostPort());
 }
+
+s_logger.info("Connection to VNC server has been established 
successfully.");
 } catch (Throwable e) {
-s_logger.error("Unexpected exception", e);
+s_logger.error("Unexpected exception while connecting to VNC 
server.", e);

Review Comment:
   ```suggestion
   s_logger.error("Unexpected exception while connecting to VNC 
server, {}", e.getLocalizedMessage());
   s_logger.debug("Exception while connecting to VNC server.", e);
   ```



##
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyNoVNCHandler.java:
##
@@ -104,20 +104,20 @@ public void onConnect(final Session session) throws 
IOException, InterruptedExce
 try {
 port = Integer.parseInt(portStr);
 } catch (NumberFormatException e) {
-s_logger.warn("Invalid number parameter in query string: " + 
portStr);
+