You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by "JoaoJandre (via GitHub)" <gi...@apache.org> on 2023/08/07 18:59:48 UTC
[GitHub] [cloudstack] JoaoJandre opened a new pull request, #7826: Fix CPVM not releasing CPU after closing console
JoaoJandre opened a new pull request, #7826:
URL: https://github.com/apache/cloudstack/pull/7826
### Description
When using VNC over Nio socket, even after closing the console, the CPVM stays in a while true and will not stop using resources until a restart. This PR introduces a new verification to check if the session is still alive.
### Types of changes
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [X] Bug fix (non-breaking change which fixes an issue)
- [ ] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
### Feature/Enhancement Scale or Bug Severity
#### Feature/Enhancement Scale
- [ ] Major
- [ ] Minor
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [X] Major
- [ ] Minor
- [ ] Trivial
### Screenshots (if appropriate):
### How Has This Been Tested?
--
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
[GitHub] [cloudstack] weizhouapache commented on a diff in pull request #7826: cpvm: Fix CPVM not releasing CPU after closing console
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on code in PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826#discussion_r1294308690
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyNoVncClient.java:
##########
@@ -113,25 +113,29 @@ public void run() {
if (client.isVncOverWebSocketConnectionOpen()) {
updateFrontEndActivityTime();
}
- connectionAlive = client.isVncOverWebSocketConnectionAlive();
+ connectionAlive = session.isOpen();
Review Comment:
thanks @GutoVeronezi
so `session.isOpen()` is good enough to determine if the connection is still alive ?
--
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
[GitHub] [cloudstack] rohityadavcloud commented on a diff in pull request #7826: cpvm: Fix CPVM not releasing CPU after closing console
Posted by "rohityadavcloud (via GitHub)" <gi...@apache.org>.
rohityadavcloud commented on code in PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826#discussion_r1294700463
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyNoVncClient.java:
##########
@@ -113,32 +114,36 @@ public void run() {
if (client.isVncOverWebSocketConnectionOpen()) {
updateFrontEndActivityTime();
}
- connectionAlive = client.isVncOverWebSocketConnectionAlive();
- try {
- Thread.sleep(1);
- } catch (Exception e) {
- s_logger.warn("Error on sleep for vnc over websocket", e);
- }
+ connectionAlive = session.isOpen();
+ Thread.sleep(1);
} else if (client.isVncOverNioSocket()) {
byte[] bytesArr;
int nextBytes = client.getNextBytes();
bytesArr = new byte[nextBytes];
client.readBytes(bytesArr, nextBytes);
+ s_logger.trace(String.format("Read [%s] bytes from client [%s]", nextBytes, clientId));
Review Comment:
should this be wrapped under isTraceEnabled (or similar)?
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyNoVncClient.java:
##########
@@ -113,32 +114,36 @@ public void run() {
if (client.isVncOverWebSocketConnectionOpen()) {
updateFrontEndActivityTime();
}
- connectionAlive = client.isVncOverWebSocketConnectionAlive();
- try {
- Thread.sleep(1);
- } catch (Exception e) {
- s_logger.warn("Error on sleep for vnc over websocket", e);
- }
+ connectionAlive = session.isOpen();
+ Thread.sleep(1);
} else if (client.isVncOverNioSocket()) {
byte[] bytesArr;
int nextBytes = client.getNextBytes();
bytesArr = new byte[nextBytes];
client.readBytes(bytesArr, nextBytes);
+ s_logger.trace(String.format("Read [%s] bytes from client [%s]", nextBytes, clientId));
if (nextBytes > 0) {
session.getRemote().sendBytes(ByteBuffer.wrap(bytesArr));
updateFrontEndActivityTime();
+ } else {
+ connectionAlive = session.isOpen();
}
} else {
b = new byte[100];
readBytes = client.read(b);
+ s_logger.trace(String.format("Read [%s] bytes from client [%s]", readBytes, clientId));
Review Comment:
should this be wrapped under isTraceEnabled (or similar)?
--
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
[GitHub] [cloudstack] blueorangutan commented on pull request #7826: cpvm: Fix CPVM not releasing CPU after closing console
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826#issuecomment-1681790269
@DaanHoogland a [SF] Trillian-Jenkins test job (rocky8 mgmt + kvm-rocky8) has been kicked to run smoke tests
--
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
[GitHub] [cloudstack] weizhouapache commented on a diff in pull request #7826: cpvm: Fix CPVM not releasing CPU after closing console
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on code in PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826#discussion_r1295459985
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyNoVncClient.java:
##########
@@ -113,32 +114,36 @@ public void run() {
if (client.isVncOverWebSocketConnectionOpen()) {
updateFrontEndActivityTime();
}
- connectionAlive = client.isVncOverWebSocketConnectionAlive();
- try {
- Thread.sleep(1);
- } catch (Exception e) {
- s_logger.warn("Error on sleep for vnc over websocket", e);
- }
+ connectionAlive = session.isOpen();
+ Thread.sleep(1);
} else if (client.isVncOverNioSocket()) {
byte[] bytesArr;
int nextBytes = client.getNextBytes();
bytesArr = new byte[nextBytes];
client.readBytes(bytesArr, nextBytes);
+ s_logger.trace(String.format("Read [%s] bytes from client [%s]", nextBytes, clientId));
if (nextBytes > 0) {
session.getRemote().sendBytes(ByteBuffer.wrap(bytesArr));
updateFrontEndActivityTime();
+ } else {
+ connectionAlive = session.isOpen();
}
} else {
b = new byte[100];
readBytes = client.read(b);
+ s_logger.trace(String.format("Read [%s] bytes from client [%s]", readBytes, clientId));
if (readBytes == -1 || (readBytes > 0 && !sendReadBytesToNoVNC(b, readBytes))) {
connectionAlive = false;
}
}
}
- connectionAlive = false;
+ s_logger.info(String.format("Connection with client [%s] is dead.", clientId));
} catch (IOException e) {
s_logger.error("Error on VNC client", e);
+ } catch (InterruptedException e) {
Review Comment:
@JoaoJandre
to be clear, I think we should use
```
try {
Thread.sleep(1);
} catch (InterruptedException e) {
s_logger.warn("Error on sleep for vnc over websocket", e);
}
```
--
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
[GitHub] [cloudstack] JoaoJandre commented on a diff in pull request #7826: Fix CPVM not releasing CPU after closing console
Posted by "JoaoJandre (via GitHub)" <gi...@apache.org>.
JoaoJandre commented on code in PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826#discussion_r1291234040
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyNoVncClient.java:
##########
@@ -113,25 +113,29 @@ public void run() {
if (client.isVncOverWebSocketConnectionOpen()) {
updateFrontEndActivityTime();
}
- connectionAlive = client.isVncOverWebSocketConnectionAlive();
+ connectionAlive = session.isOpen();
Review Comment:
When I tested, ` isVncOverWebSocketConnectionAlive()` would always return true. So I don't see a reason to use it.
--
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
[GitHub] [cloudstack] JoaoJandre commented on a diff in pull request #7826: cpvm: Fix CPVM not releasing CPU after closing console
Posted by "JoaoJandre (via GitHub)" <gi...@apache.org>.
JoaoJandre commented on code in PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826#discussion_r1294899479
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyNoVncClient.java:
##########
@@ -113,32 +114,36 @@ public void run() {
if (client.isVncOverWebSocketConnectionOpen()) {
updateFrontEndActivityTime();
}
- connectionAlive = client.isVncOverWebSocketConnectionAlive();
- try {
- Thread.sleep(1);
- } catch (Exception e) {
- s_logger.warn("Error on sleep for vnc over websocket", e);
- }
+ connectionAlive = session.isOpen();
+ Thread.sleep(1);
} else if (client.isVncOverNioSocket()) {
byte[] bytesArr;
int nextBytes = client.getNextBytes();
bytesArr = new byte[nextBytes];
client.readBytes(bytesArr, nextBytes);
+ s_logger.trace(String.format("Read [%s] bytes from client [%s]", nextBytes, clientId));
Review Comment:
I think that using `isTraceEnabled` is bad for readability. Furthermore, as we have a Log4J patch prepared. As soon as we merge that patch, none of these checks will be needed anymore.
--
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
[GitHub] [cloudstack] GutoVeronezi commented on a diff in pull request #7826: cpvm: Fix CPVM not releasing CPU after closing console
Posted by "GutoVeronezi (via GitHub)" <gi...@apache.org>.
GutoVeronezi commented on code in PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826#discussion_r1293777831
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyNoVncClient.java:
##########
@@ -113,25 +113,29 @@ public void run() {
if (client.isVncOverWebSocketConnectionOpen()) {
updateFrontEndActivityTime();
}
- connectionAlive = client.isVncOverWebSocketConnectionAlive();
+ connectionAlive = session.isOpen();
Review Comment:
@weizhouapache
I did a quick test with VMware 7.0 U3; when the session times out on the VMware side, it closes the websocket and the resources on the CPVM are freed.
It would be good if others could try other use cases as well.
--
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
[GitHub] [cloudstack] blueorangutan commented on pull request #7826: Fix CPVM not releasing CPU after closing console
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826#issuecomment-1670105661
@rohityadavcloud a [SF] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.
--
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
[GitHub] [cloudstack] DaanHoogland commented on pull request #7826: Fix CPVM not releasing CPU after closing console
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826#issuecomment-1674313721
@blueorangutan package
--
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
[GitHub] [cloudstack] rohityadavcloud commented on pull request #7826: cpvm: Fix CPVM not releasing CPU after closing console
Posted by "rohityadavcloud (via GitHub)" <gi...@apache.org>.
rohityadavcloud commented on PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826#issuecomment-1679073423
I'm able to reproduce the issue with 4.18, restarting the cloud service on cpvm fixes this as a workaround.
PR LGTM, left some comments - but otherwise we should merge this.
<img width="1098" alt="Screenshot 2023-08-15 at 8 19 20 PM" src="https://github.com/apache/cloudstack/assets/95203/b2624ce0-f47a-4ca3-b7e8-eacca35c08fa">
--
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
[GitHub] [cloudstack] DaanHoogland merged pull request #7826: cpvm: Fix CPVM not releasing CPU after closing console
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland merged PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826
--
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
[GitHub] [cloudstack] JoaoJandre commented on pull request #7826: cpvm: Fix CPVM not releasing CPU after closing console
Posted by "JoaoJandre (via GitHub)" <gi...@apache.org>.
JoaoJandre commented on PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826#issuecomment-1677942718
> @JoaoJandre can you fix the conflicts ? thanks
Done
--
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
[GitHub] [cloudstack] weizhouapache commented on a diff in pull request #7826: cpvm: Fix CPVM not releasing CPU after closing console
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on code in PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826#discussion_r1296306587
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyNoVncClient.java:
##########
@@ -113,32 +114,36 @@ public void run() {
if (client.isVncOverWebSocketConnectionOpen()) {
updateFrontEndActivityTime();
}
- connectionAlive = client.isVncOverWebSocketConnectionAlive();
- try {
- Thread.sleep(1);
- } catch (Exception e) {
- s_logger.warn("Error on sleep for vnc over websocket", e);
- }
+ connectionAlive = session.isOpen();
+ Thread.sleep(1);
} else if (client.isVncOverNioSocket()) {
byte[] bytesArr;
int nextBytes = client.getNextBytes();
bytesArr = new byte[nextBytes];
client.readBytes(bytesArr, nextBytes);
+ s_logger.trace(String.format("Read [%s] bytes from client [%s]", nextBytes, clientId));
if (nextBytes > 0) {
session.getRemote().sendBytes(ByteBuffer.wrap(bytesArr));
updateFrontEndActivityTime();
+ } else {
+ connectionAlive = session.isOpen();
}
} else {
b = new byte[100];
readBytes = client.read(b);
+ s_logger.trace(String.format("Read [%s] bytes from client [%s]", readBytes, clientId));
if (readBytes == -1 || (readBytes > 0 && !sendReadBytesToNoVNC(b, readBytes))) {
connectionAlive = false;
}
}
}
- connectionAlive = false;
+ s_logger.info(String.format("Connection with client [%s] is dead.", clientId));
} catch (IOException e) {
s_logger.error("Error on VNC client", e);
+ } catch (InterruptedException e) {
Review Comment:
@JoaoJandre
what's your opinion ?
--
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
[GitHub] [cloudstack] codecov[bot] commented on pull request #7826: Fix CPVM not releasing CPU after closing console
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826#issuecomment-1668543519
## [Codecov](https://app.codecov.io/gh/apache/cloudstack/pull/7826?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#7826](https://app.codecov.io/gh/apache/cloudstack/pull/7826?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (140f033) into [main](https://app.codecov.io/gh/apache/cloudstack/commit/32448e1ac7bb6cb80f052f644a9dde25e150d64a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (32448e1) will **increase** coverage by `0.00%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## main #7826 +/- ##
=========================================
Coverage 14.28% 14.28%
Complexity 9994 9994
=========================================
Files 2747 2747
Lines 258905 258905
Branches 40318 40318
=========================================
+ Hits 36987 36988 +1
+ Misses 217125 217121 -4
- Partials 4793 4796 +3
```
[see 1 file with indirect coverage changes](https://app.codecov.io/gh/apache/cloudstack/pull/7826/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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
[GitHub] [cloudstack] DaanHoogland commented on pull request #7826: Fix CPVM not releasing CPU after closing console
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826#issuecomment-1672708486
code looks good @JoaoJandre , this is still work in progress?
--
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
[GitHub] [cloudstack] blueorangutan commented on pull request #7826: Fix CPVM not releasing CPU after closing console
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826#issuecomment-1671644194
@rohityadavcloud a [SF] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
--
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
[GitHub] [cloudstack] blueorangutan commented on pull request #7826: Fix CPVM not releasing CPU after closing console
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826#issuecomment-1670198243
Packaging result [SF]: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: el9 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 6712
--
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
[GitHub] [cloudstack] blueorangutan commented on pull request #7826: Fix CPVM not releasing CPU after closing console
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826#issuecomment-1672593139
<b>[SF] Trillian test result (tid-7342)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 49470 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7826-t7342-kvm-centos7.zip
Smoke tests completed. 107 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_01_migrate_VM_and_root_volume | `Error` | 79.74 | test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks | `Error` | 55.42 | test_vm_life_cycle.py
--
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
[GitHub] [cloudstack] DaanHoogland commented on pull request #7826: cpvm: Fix CPVM not releasing CPU after closing console
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826#issuecomment-1678567557
@blueorangutan test rocky8 kvm-rocky8 keepEnv
--
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
[GitHub] [cloudstack] DaanHoogland commented on pull request #7826: cpvm: Fix CPVM not releasing CPU after closing console
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826#issuecomment-1681787751
@blueorangutan test rocky8 kvm-rocky8 keepEnv
--
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
[GitHub] [cloudstack] JoaoJandre commented on pull request #7826: Fix CPVM not releasing CPU after closing console
Posted by "JoaoJandre (via GitHub)" <gi...@apache.org>.
JoaoJandre commented on PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826#issuecomment-1670040505
> @JoaoJandre could you also check #6970 cc @alexandru-bagu - if this is a better fix?
@rohityadavcloud, actually there are two adjacent problems happening in the CPVM: the first one is the high CPU consumption on the CPVM; and the second one is processes not being released after closing the console session. This PR aims to fix the second situation; and PR #6970 aims to fix the first situation; therefore, I think that both issues should be treated in separated PRs (as it is currently).
--
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
[GitHub] [cloudstack] JoaoJandre commented on pull request #7826: Fix CPVM not releasing CPU after closing console
Posted by "JoaoJandre (via GitHub)" <gi...@apache.org>.
JoaoJandre commented on PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826#issuecomment-1668426141
I'm still validating what happens when using vnc over websocket
--
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
[GitHub] [cloudstack] weizhouapache commented on a diff in pull request #7826: Fix CPVM not releasing CPU after closing console
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on code in PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826#discussion_r1291254580
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyNoVncClient.java:
##########
@@ -113,25 +113,29 @@ public void run() {
if (client.isVncOverWebSocketConnectionOpen()) {
updateFrontEndActivityTime();
}
- connectionAlive = client.isVncOverWebSocketConnectionAlive();
+ connectionAlive = session.isOpen();
Review Comment:
@JoaoJandre
did you test with vmware 7.x ?
is it possible that the websocket server (esxi host) closes the console due to some reasons (e.g. timeout, or vm is stopped) ?
--
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
[GitHub] [cloudstack] DaanHoogland commented on pull request #7826: cpvm: Fix CPVM not releasing CPU after closing console
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826#issuecomment-1678497455
@blueorangutan package
--
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
[GitHub] [cloudstack] weizhouapache commented on a diff in pull request #7826: cpvm: Fix CPVM not releasing CPU after closing console
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on code in PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826#discussion_r1294309271
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyNoVncClient.java:
##########
@@ -113,32 +114,36 @@ public void run() {
if (client.isVncOverWebSocketConnectionOpen()) {
updateFrontEndActivityTime();
}
- connectionAlive = client.isVncOverWebSocketConnectionAlive();
- try {
- Thread.sleep(1);
- } catch (Exception e) {
- s_logger.warn("Error on sleep for vnc over websocket", e);
- }
+ connectionAlive = session.isOpen();
+ Thread.sleep(1);
} else if (client.isVncOverNioSocket()) {
byte[] bytesArr;
int nextBytes = client.getNextBytes();
bytesArr = new byte[nextBytes];
client.readBytes(bytesArr, nextBytes);
+ s_logger.trace(String.format("Read [%s] bytes from client [%s]", nextBytes, clientId));
if (nextBytes > 0) {
session.getRemote().sendBytes(ByteBuffer.wrap(bytesArr));
updateFrontEndActivityTime();
+ } else {
+ connectionAlive = session.isOpen();
}
} else {
b = new byte[100];
readBytes = client.read(b);
+ s_logger.trace(String.format("Read [%s] bytes from client [%s]", readBytes, clientId));
if (readBytes == -1 || (readBytes > 0 && !sendReadBytesToNoVNC(b, readBytes))) {
connectionAlive = false;
}
}
}
- connectionAlive = false;
+ s_logger.info(String.format("Connection with client [%s] is dead.", clientId));
} catch (IOException e) {
s_logger.error("Error on VNC client", e);
+ } catch (InterruptedException e) {
Review Comment:
@JoaoJandre
maybe better to keep the origin code and do not rhrow exception in case of InterruptedException
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyNoVncClient.java:
##########
@@ -113,32 +114,36 @@ public void run() {
if (client.isVncOverWebSocketConnectionOpen()) {
updateFrontEndActivityTime();
}
- connectionAlive = client.isVncOverWebSocketConnectionAlive();
- try {
- Thread.sleep(1);
- } catch (Exception e) {
- s_logger.warn("Error on sleep for vnc over websocket", e);
- }
+ connectionAlive = session.isOpen();
+ Thread.sleep(1);
} else if (client.isVncOverNioSocket()) {
byte[] bytesArr;
int nextBytes = client.getNextBytes();
bytesArr = new byte[nextBytes];
client.readBytes(bytesArr, nextBytes);
+ s_logger.trace(String.format("Read [%s] bytes from client [%s]", nextBytes, clientId));
if (nextBytes > 0) {
session.getRemote().sendBytes(ByteBuffer.wrap(bytesArr));
updateFrontEndActivityTime();
+ } else {
+ connectionAlive = session.isOpen();
}
} else {
b = new byte[100];
readBytes = client.read(b);
+ s_logger.trace(String.format("Read [%s] bytes from client [%s]", readBytes, clientId));
if (readBytes == -1 || (readBytes > 0 && !sendReadBytesToNoVNC(b, readBytes))) {
connectionAlive = false;
}
}
}
- connectionAlive = false;
+ s_logger.info(String.format("Connection with client [%s] is dead.", clientId));
} catch (IOException e) {
s_logger.error("Error on VNC client", e);
+ } catch (InterruptedException e) {
Review Comment:
@JoaoJandre
maybe better to keep the origin code and do not throw exception in case of InterruptedException
--
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
[GitHub] [cloudstack] weizhouapache commented on pull request #7826: cpvm: Fix CPVM not releasing CPU after closing console
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826#issuecomment-1682927362
@blueorangutan package
--
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
[GitHub] [cloudstack] JoaoJandre commented on pull request #7826: Fix CPVM not releasing CPU after closing console
Posted by "JoaoJandre (via GitHub)" <gi...@apache.org>.
JoaoJandre commented on PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826#issuecomment-1673738570
> code looks good @JoaoJandre , this is still work in progress?
I was just doing some testing, but the PR is ready now
--
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
[GitHub] [cloudstack] rohityadavcloud commented on pull request #7826: Fix CPVM not releasing CPU after closing console
Posted by "rohityadavcloud (via GitHub)" <gi...@apache.org>.
rohityadavcloud commented on PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826#issuecomment-1669244023
@JoaoJandre can you raise this for 4.18 branch? I hit this issue with VMware recently with 4.18 but didn't know what caused it.
--
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
[GitHub] [cloudstack] rohityadavcloud commented on pull request #7826: Fix CPVM not releasing CPU after closing console
Posted by "rohityadavcloud (via GitHub)" <gi...@apache.org>.
rohityadavcloud commented on PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826#issuecomment-1671640579
Manual test on all supported hypervisors types may be needed cc @weizhouapache
@blueorangutan test
--
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
[GitHub] [cloudstack] blueorangutan commented on pull request #7826: Fix CPVM not releasing CPU after closing console
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826#issuecomment-1674315793
@DaanHoogland a [SF] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.
--
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
[GitHub] [cloudstack] blueorangutan commented on pull request #7826: cpvm: Fix CPVM not releasing CPU after closing console
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826#issuecomment-1683044316
@weizhouapache a [SF] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.
--
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
[GitHub] [cloudstack] weizhouapache commented on pull request #7826: cpvm: Fix CPVM not releasing CPU after closing console
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826#issuecomment-1677352222
@JoaoJandre
can you fix the conflicts ? thanks
--
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
[GitHub] [cloudstack] rohityadavcloud commented on pull request #7826: cpvm: Fix CPVM not releasing CPU after closing console
Posted by "rohityadavcloud (via GitHub)" <gi...@apache.org>.
rohityadavcloud commented on PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826#issuecomment-1677109811
@blueorangutan test alma vmware-70u3
--
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
[GitHub] [cloudstack] rohityadavcloud commented on pull request #7826: Fix CPVM not releasing CPU after closing console
Posted by "rohityadavcloud (via GitHub)" <gi...@apache.org>.
rohityadavcloud commented on PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826#issuecomment-1670105373
Thanks for replying that makes sense. In that case we should aspire to get both merged.
@blueorangutan package
--
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
[GitHub] [cloudstack] rohityadavcloud commented on pull request #7826: Fix CPVM not releasing CPU after closing console
Posted by "rohityadavcloud (via GitHub)" <gi...@apache.org>.
rohityadavcloud commented on PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826#issuecomment-1669629972
@JoaoJandre could you also check https://github.com/apache/cloudstack/pull/6970 cc @alexandru-bagu - if this is a better fix?
cc @DaanHoogland @weizhouapache
--
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
[GitHub] [cloudstack] weizhouapache commented on a diff in pull request #7826: Fix CPVM not releasing CPU after closing console
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on code in PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826#discussion_r1291014920
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyNoVncClient.java:
##########
@@ -113,25 +113,29 @@ public void run() {
if (client.isVncOverWebSocketConnectionOpen()) {
updateFrontEndActivityTime();
}
- connectionAlive = client.isVncOverWebSocketConnectionAlive();
+ connectionAlive = session.isOpen();
Review Comment:
@JoaoJandre @nvazquez
would be good to use ?
```suggestion
connectionAlive = client.isVncOverWebSocketConnectionAlive() && session.isOpen();
```
--
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
[GitHub] [cloudstack] weizhouapache commented on a diff in pull request #7826: cpvm: Fix CPVM not releasing CPU after closing console
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on code in PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826#discussion_r1295075504
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyNoVncClient.java:
##########
@@ -113,32 +114,36 @@ public void run() {
if (client.isVncOverWebSocketConnectionOpen()) {
updateFrontEndActivityTime();
}
- connectionAlive = client.isVncOverWebSocketConnectionAlive();
- try {
- Thread.sleep(1);
- } catch (Exception e) {
- s_logger.warn("Error on sleep for vnc over websocket", e);
- }
+ connectionAlive = session.isOpen();
+ Thread.sleep(1);
} else if (client.isVncOverNioSocket()) {
byte[] bytesArr;
int nextBytes = client.getNextBytes();
bytesArr = new byte[nextBytes];
client.readBytes(bytesArr, nextBytes);
+ s_logger.trace(String.format("Read [%s] bytes from client [%s]", nextBytes, clientId));
if (nextBytes > 0) {
session.getRemote().sendBytes(ByteBuffer.wrap(bytesArr));
updateFrontEndActivityTime();
+ } else {
+ connectionAlive = session.isOpen();
}
} else {
b = new byte[100];
readBytes = client.read(b);
+ s_logger.trace(String.format("Read [%s] bytes from client [%s]", readBytes, clientId));
if (readBytes == -1 || (readBytes > 0 && !sendReadBytesToNoVNC(b, readBytes))) {
connectionAlive = false;
}
}
}
- connectionAlive = false;
+ s_logger.info(String.format("Connection with client [%s] is dead.", clientId));
} catch (IOException e) {
s_logger.error("Error on VNC client", e);
+ } catch (InterruptedException e) {
Review Comment:
@JoaoJandre
I agree if you change Exception to InterruptedException
I think InterruptedException should not terminate the process, it should be silently ignored as other processes.
--
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
[GitHub] [cloudstack] blueorangutan commented on pull request #7826: cpvm: Fix CPVM not releasing CPU after closing console
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826#issuecomment-1678553134
Packaging result [SF]: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: el9 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 6778
--
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
[GitHub] [cloudstack] github-actions[bot] commented on pull request #7826: cpvm: Fix CPVM not releasing CPU after closing console
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826#issuecomment-1677398296
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-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #7826: cpvm: Fix CPVM not releasing CPU after closing console
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826#issuecomment-1683089467
Packaging result [SF]: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: el9 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 6822
--
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
[GitHub] [cloudstack] blueorangutan commented on pull request #7826: Fix CPVM not releasing CPU after closing console
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826#issuecomment-1674382892
Packaging result [SF]: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: el9 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 6746
--
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
[GitHub] [cloudstack] blueorangutan commented on pull request #7826: cpvm: Fix CPVM not releasing CPU after closing console
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826#issuecomment-1677110744
@rohityadavcloud a [SF] Trillian-Jenkins test job (alma8 mgmt + vmware-70u3) has been kicked to run smoke tests
--
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
[GitHub] [cloudstack] rohityadavcloud commented on pull request #7826: cpvm: Fix CPVM not releasing CPU after closing console
Posted by "rohityadavcloud (via GitHub)" <gi...@apache.org>.
rohityadavcloud commented on PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826#issuecomment-1677110153
@blueorangutan test alma8 vmware-70u3
--
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
[GitHub] [cloudstack] blueorangutan commented on pull request #7826: cpvm: Fix CPVM not releasing CPU after closing console
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826#issuecomment-1678497640
@DaanHoogland a [SF] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.
--
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
[GitHub] [cloudstack] JoaoJandre commented on a diff in pull request #7826: cpvm: Fix CPVM not releasing CPU after closing console
Posted by "JoaoJandre (via GitHub)" <gi...@apache.org>.
JoaoJandre commented on code in PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826#discussion_r1294889711
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyNoVncClient.java:
##########
@@ -113,32 +114,36 @@ public void run() {
if (client.isVncOverWebSocketConnectionOpen()) {
updateFrontEndActivityTime();
}
- connectionAlive = client.isVncOverWebSocketConnectionAlive();
- try {
- Thread.sleep(1);
- } catch (Exception e) {
- s_logger.warn("Error on sleep for vnc over websocket", e);
- }
+ connectionAlive = session.isOpen();
+ Thread.sleep(1);
} else if (client.isVncOverNioSocket()) {
byte[] bytesArr;
int nextBytes = client.getNextBytes();
bytesArr = new byte[nextBytes];
client.readBytes(bytesArr, nextBytes);
+ s_logger.trace(String.format("Read [%s] bytes from client [%s]", nextBytes, clientId));
if (nextBytes > 0) {
session.getRemote().sendBytes(ByteBuffer.wrap(bytesArr));
updateFrontEndActivityTime();
+ } else {
+ connectionAlive = session.isOpen();
}
} else {
b = new byte[100];
readBytes = client.read(b);
+ s_logger.trace(String.format("Read [%s] bytes from client [%s]", readBytes, clientId));
if (readBytes == -1 || (readBytes > 0 && !sendReadBytesToNoVNC(b, readBytes))) {
connectionAlive = false;
}
}
}
- connectionAlive = false;
+ s_logger.info(String.format("Connection with client [%s] is dead.", clientId));
} catch (IOException e) {
s_logger.error("Error on VNC client", e);
+ } catch (InterruptedException e) {
Review Comment:
@weizhouapache catching all exceptions is a bad practice, there's no reason to use that in this situation. But I can remove the exception being thrown and just keep the log.
--
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
[GitHub] [cloudstack] JoaoJandre commented on a diff in pull request #7826: cpvm: Fix CPVM not releasing CPU after closing console
Posted by "JoaoJandre (via GitHub)" <gi...@apache.org>.
JoaoJandre commented on code in PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826#discussion_r1297538965
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyNoVncClient.java:
##########
@@ -113,32 +114,36 @@ public void run() {
if (client.isVncOverWebSocketConnectionOpen()) {
updateFrontEndActivityTime();
}
- connectionAlive = client.isVncOverWebSocketConnectionAlive();
- try {
- Thread.sleep(1);
- } catch (Exception e) {
- s_logger.warn("Error on sleep for vnc over websocket", e);
- }
+ connectionAlive = session.isOpen();
+ Thread.sleep(1);
} else if (client.isVncOverNioSocket()) {
byte[] bytesArr;
int nextBytes = client.getNextBytes();
bytesArr = new byte[nextBytes];
client.readBytes(bytesArr, nextBytes);
+ s_logger.trace(String.format("Read [%s] bytes from client [%s]", nextBytes, clientId));
if (nextBytes > 0) {
session.getRemote().sendBytes(ByteBuffer.wrap(bytesArr));
updateFrontEndActivityTime();
+ } else {
+ connectionAlive = session.isOpen();
}
} else {
b = new byte[100];
readBytes = client.read(b);
+ s_logger.trace(String.format("Read [%s] bytes from client [%s]", readBytes, clientId));
if (readBytes == -1 || (readBytes > 0 && !sendReadBytesToNoVNC(b, readBytes))) {
connectionAlive = false;
}
}
}
- connectionAlive = false;
+ s_logger.info(String.format("Connection with client [%s] is dead.", clientId));
} catch (IOException e) {
s_logger.error("Error on VNC client", e);
+ } catch (InterruptedException e) {
Review Comment:
@weizhouapache I think it's fine either way, there are pros and cons, but I think that not terminating the process is a better option.
--
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
[GitHub] [cloudstack] weizhouapache commented on pull request #7826: cpvm: Fix CPVM not releasing CPU after closing console
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826#issuecomment-1683044111
@blueorangutan package
--
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
[GitHub] [cloudstack] blueorangutan commented on pull request #7826: cpvm: Fix CPVM not releasing CPU after closing console
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7826:
URL: https://github.com/apache/cloudstack/pull/7826#issuecomment-1682963720
<b>[SF] Trillian test result (tid-7457)</b>
Environment: kvm-rocky8 (x2), Advanced Networking with Mgmt server r8
Total time taken: 45588 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7826-t7457-kvm-rocky8.zip
Smoke tests completed. 107 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_01_invalid_upgrade_kubernetes_cluster | `Failure` | 0.01 | test_kubernetes_clusters.py
test_02_upgrade_kubernetes_cluster | `Failure` | 0.01 | test_kubernetes_clusters.py
test_03_deploy_and_scale_kubernetes_cluster | `Failure` | 0.01 | test_kubernetes_clusters.py
test_04_autoscale_kubernetes_cluster | `Failure` | 0.00 | test_kubernetes_clusters.py
test_05_basic_lifecycle_kubernetes_cluster | `Failure` | 0.00 | test_kubernetes_clusters.py
test_06_delete_kubernetes_cluster | `Failure` | 0.00 | test_kubernetes_clusters.py
test_07_deploy_kubernetes_ha_cluster | `Failure` | 0.01 | test_kubernetes_clusters.py
test_08_upgrade_kubernetes_ha_cluster | `Failure` | 0.01 | test_kubernetes_clusters.py
test_09_delete_kubernetes_ha_cluster | `Failure` | 0.01 | test_kubernetes_clusters.py
test_10_vpc_tier_kubernetes_cluster | `Failure` | 0.01 | test_kubernetes_clusters.py
--
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