Skip to content

Conversation

@davidjumani
Copy link
Contributor

@davidjumani davidjumani commented Mar 16, 2020

Description

This feature provides novnc console for virtual machines.

It is developed based on noVNC 1.1.0
It includes changes in cloudstack :
(1) add a new websocket proxy to interact with the noVNC console
(2) a ConsoleProxyClient to read / write data from the vm vnc to the websocket.

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • 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)

Screenshots (if appropriate):

How Has This Been Tested?

(1) Works on kvm / ubuntu / centos
(2) Works on xs 75
(3) Works on VMware 6.5

@davidjumani davidjumani changed the title Adding novnc noVNC console integration Mar 16, 2020
@blueorangutan
Copy link

Packaging result: ✖centos6 ✔centos7 ✔debian. JID-1053

@davidjumani davidjumani force-pushed the adding-novnc branch 2 times, most recently from 49b94fc to 7aba8b1 Compare March 16, 2020 09:45
@blueorangutan
Copy link

Packaging result: ✖centos6 ✔centos7 ✔debian. JID-1054

@blueorangutan
Copy link

Packaging result: ✖centos6 ✔centos7 ✔debian. JID-1065

@davidjumani
Copy link
Contributor Author

@ustcweizhou I've created this to use a java websocket proxy which supports client metrics reporting. Could you provide your comments

@weizhouapache
Copy link
Member

@davidjumani cooooool.
I am currently busy on some other stuff. I will look at it when I am free.
@rhtyd has some ideas. maybe he can help you.

@davidjumani davidjumani marked this pull request as ready for review March 20, 2020 04:34
@davidjumani
Copy link
Contributor Author

@rhtyd It's all good from my end, any comments ?

@nvazquez
Copy link
Contributor

@blueorangutan package

@blueorangutan
Copy link

@nvazquez a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result: ✔centos7 ✔debian. JID-1094

Copy link
Contributor

@nvazquez nvazquez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @davidjumani. I have tested manually and works nice. I have left some few comments and also wondering if we could include a jar instead of the noVNC codebase on the console proxy VM and serve it. Otherwise LGTM

if (param.getHypervHost() != null || !ConsoleProxyManager.NoVncConsoleDefault.value()) {
sb.append("/ajax?token=" + encryptor.encryptObject(ConsoleProxyClientParam.class, param));
} else {
sb.append("/resource/noVNC/vnc_lite.html?port=" + ConsoleProxyManager.DEFAULT_NOVNC_PORT + "&token="
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not using vnc.html which provides more functionalities? Briefly tested invoking it and got this extra functionalities:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wanted a simpler, minimalist UI, can add this when we get multi lingual keyboard working

public static final String CERTIFICATE_NAME = "CPVMCertificate";

public static final ConfigKey<Boolean> NoVncConsoleDefault = new ConfigKey<Boolean>("Advanced", Boolean.class, "novnc.console.default", "true",
"If true, noVNC console will be default console for virtual machines", true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be disabled by default at least until all hypervisors tested working properly

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested it on KVM, VMware and Xen and it works fine!

@borisstoyanov
Copy link
Contributor

@blueorangutan package

@blueorangutan
Copy link

@borisstoyanov a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result: ✔centos7 ✔debian. JID-1104

@vladimirpetrov
Copy link
Contributor

@blueorangutan test

1 similar comment
@borisstoyanov
Copy link
Contributor

@blueorangutan test

@blueorangutan
Copy link

@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

@vladimirpetrov
Copy link
Contributor

@blueorangutan help

@blueorangutan
Copy link

@vladimirpetrov I understand these words: "help", "hello", "thanks", "package", "test"
Test command usage: test [mgmt os] [hypervisor] [additional tests]
Mgmt OS options: ['centos6', 'centos7', 'ubuntu']
Hypervisor options: ['kvm-centos6', 'kvm-centos7', 'kvm-ubuntu', 'xenserver-71', 'xenserver-65sp1', 'xenserver-62sp1', 'vmware-67u3', 'vmware-65u2', 'vmware-60u2', 'vmware-55u3', 'vmware-51u1', 'vmware-50u1']
Additional tests: list of space separated tests with paths relative to the test/integration directory, for example: component/test_acl_listvm.py component/test_volumes.py
Note: when additional tests are passed, you need to specify mgmt server os and hypervisor or use the matrix command.

Blessed contributors for kicking Trillian test jobs: ['rhtyd', 'nvazquez', 'PaulAngus', 'borisstoyanov', 'DaanHoogland', 'shwstppr', 'andrijapanicsb', 'Spaceman1984', 'Pearl1594', 'davidjumani', 'harikrishna-patnala', 'vladimirpetrov']

@apache apache deleted a comment from blueorangutan Apr 3, 2020
@apache apache deleted a comment from vladimirpetrov Apr 3, 2020
@vladimirpetrov
Copy link
Contributor

@blueorangutan package

@davidjumani
Copy link
Contributor Author

Verified with multi keyboard layouts! Have to configure the appropriate keyboard layout in the guest VM
keyboards

@vladimirpetrov
Copy link
Contributor

@blueorangutan package

@blueorangutan
Copy link

@vladimirpetrov a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@vladimirpetrov
Copy link
Contributor

@blueorangutan package

@blueorangutan
Copy link

@vladimirpetrov a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result: ✔centos7 ✔debian. JID-1168

@DaanHoogland DaanHoogland added this to the 4.15.0.0 milestone May 8, 2020
@apache apache deleted a comment from blueorangutan May 14, 2020
@andrijapanicsb
Copy link
Contributor

@blueorangutan test

@blueorangutan
Copy link

@andrijapanicsb a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

@blueorangutan
Copy link

Trillian test result (tid-1531)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 37093 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr3967-t1531-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 83 look OK, 0 have error(s)
Only failed tests results shown below:

Test Result Time (s) Test File

@andrijapanicsb andrijapanicsb changed the title noVNC console integration [WIP - do not merge] noVNC console integration May 15, 2020
@blueorangutan
Copy link

Packaging result: ✔centos7 ✔debian. JID-1229

@blueorangutan
Copy link

Packaging result: ✔centos7 ✔debian. JID-1230

@DaanHoogland DaanHoogland marked this pull request as draft May 18, 2020 10:48
@DaanHoogland DaanHoogland changed the title [WIP - do not merge] noVNC console integration noVNC console integration May 18, 2020
@davidjumani davidjumani marked this pull request as ready for review May 19, 2020 11:02
@andrijapanicsb andrijapanicsb self-requested a review May 19, 2020 11:20
Copy link
Contributor

@andrijapanicsb andrijapanicsb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

tested manually as well.

@andrijapanicsb
Copy link
Contributor

Travis log length exceeded - otherwise LGMT

Merging based on 2 x LGTMs, manual testing by 2 persons (Vladimir and me) and the regression test results passing 100%.

@andrijapanicsb andrijapanicsb merged commit 1756b0f into apache:master May 19, 2020
@davidjumani davidjumani deleted the adding-novnc branch May 19, 2020 14:33
@DaanHoogland
Copy link
Contributor

@davidjumani please have a look at https://builds.apache.org/job/cloudstack-pr-analysis/10377/artifact/target/rat.txt or equivalent of any new PR. it seems we missed some licensing checks on this PR.

@DaanHoogland DaanHoogland mentioned this pull request May 25, 2020
5 tasks
soreana pushed a commit to soreana/cloudstack that referenced this pull request Nov 3, 2020
* Adding noVNC repo

* Adding support for noVNC

* Adding Ctl+Esc

* Removing device name from novnc header
DaanHoogland pushed a commit to shapeblue/cloudstack that referenced this pull request May 20, 2022
* Adding noVNC repo

* Adding support for noVNC

* Adding Ctl+Esc

* Removing device name from novnc header
shwstppr pushed a commit to shapeblue/cloudstack that referenced this pull request Jan 17, 2023
* Adding noVNC repo

* Adding support for noVNC

* Adding Ctl+Esc

* Removing device name from novnc header
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants