You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by "DaanHoogland (via GitHub)" <gi...@apache.org> on 2024/04/17 08:17:43 UTC
Re: [PR] Draft: Add logs to CPVM connection process [cloudstack]
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);
+ s_logger.error("Invalid port value in query string: {}. Expected a number.", portStr, e);
Review Comment:
please don't add the stacktrace at `error` level.
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyNoVncClient.java:
##########
@@ -155,7 +156,7 @@ private boolean sendReadBytesToNoVNC(byte[] b, int readBytes) {
session.getRemote().sendBytes(ByteBuffer.wrap(b, 0, readBytes));
updateFrontEndActivityTime();
} catch (WebSocketException | IOException e) {
- s_logger.debug("Connection exception", e);
+ s_logger.error("VNC server connection exception.", e);
Review Comment:
stacktrace in `error`
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyNoVNCHandler.java:
##########
@@ -155,31 +157,35 @@ public void onConnect(final Session session) throws IOException, InterruptedExce
}
private boolean checkSessionSourceIp(final Session session, final String sourceIP) throws IOException {
- // Verify source IP
+ s_logger.debug("Verifying session source IP.");
String sessionSourceIP = session.getRemoteAddress().getAddress().getHostAddress();
s_logger.info("Get websocket connection request from remote IP : " + sessionSourceIP);
- if (ConsoleProxy.isSourceIpCheckEnabled && (sessionSourceIP == null || ! sessionSourceIP.equals(sourceIP))) {
+ if (ConsoleProxy.isSourceIpCheckEnabled && (sessionSourceIP == null || !sessionSourceIP.equals(sourceIP))) {
s_logger.warn("Failed to access console as the source IP to request the console is " + sourceIP);
session.disconnect();
return false;
}
+ s_logger.debug("Session source IP has been verified successfully.");
return true;
}
@OnWebSocketClose
public void onClose(Session session, int statusCode, String reason) throws IOException, InterruptedException {
+ s_logger.debug("Closing WebSocket session.");
if (viewer != null) {
ConsoleProxy.removeViewer(viewer);
}
+ s_logger.debug("WebSocket session closed successfully.");
}
@OnWebSocketFrame
public void onFrame(Frame f) throws IOException {
+ s_logger.trace("Sending client frame of {} bytes.", f.getPayloadLength());
viewer.sendClientFrame(f);
}
@OnWebSocketError
public void onError(Throwable cause) {
- s_logger.error("Error on websocket", cause);
+ s_logger.error("Error on WebSocket.", cause);
Review Comment:
```suggestion
s_logger.error("Error on closing WebSocket.");
s_logger.debug("Error on WebSocket.", cause);
```
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyNoVncClient.java:
##########
@@ -72,9 +72,9 @@ public boolean isHostConnected() {
@Override
public boolean isFrontEndAlive() {
- if (!connectionAlive || System.currentTimeMillis()
- - getClientLastFrontEndActivityTime() > ConsoleProxy.VIEWER_LINGER_SECONDS * 1000) {
- s_logger.info("Front end has been idle for too long");
+ long unusedTime = System.currentTimeMillis() - getClientLastFrontEndActivityTime();
+ if (!connectionAlive || unusedTime > ConsoleProxy.VIEWER_LINGER_SECONDS * 1000) {
+ s_logger.info("Front end has been idle for too long ({}ms).", unusedTime);
Review Comment:
```suggestion
s_logger.info("Front end has been idle for too long ({} ms).", unusedTime);
```
--
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