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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
