Added NoVNC console page #956

bond95 · 2019-02-15T09:54:48Z

Fixes: #490
Fixes: #966

Refactored Mike's code
Fixed console page
Improved NoVNC component
Changed ConsoleConfirmationModal architecture
Added Loader component
Added console info modal

Patterfly-react PR: patternfly/patternfly-react#1402

lwrigh

The updates are looking good! My one comment is on adding the external link icon next to the console types that open up in a new window.

lwrigh · 2019-02-18T06:03:35Z

What do you guys think about the possibility of adding a full screen button into the console page? We could place it in the lower right hand corner of the console screen.

gregsheremeta · 2019-02-18T13:19:50Z

What do you guys think about the possibility of adding a full screen button into the console page? We could place it in the lower right hand corner of the console screen.

I'm not sure the novnc component supports full screen mode. @bond95 ?

bond95 · 2019-02-18T14:53:41Z

@gregsheremeta well, yeah, component doesn't have any props for it, but it doesn't mean that we can't add it. But I think it would be better to do in separate PR.

gregsheremeta · 2019-02-18T16:33:57Z

@gregsheremeta well, yeah, component doesn't have any props for it, but it doesn't mean that we can't add it. But I think it would be better to do in separate PR.

Agree. I was just wondering if novnc supports it at all.

bond95 · 2019-02-20T10:26:39Z

@lwrigh added icons

lwrigh · 2019-02-21T02:27:55Z

@lwrigh added icons

Looks good to me 👍

sjd78

First pass - mostly questions and some nit-picks. I'll take a deeper look later.

config/paths.js

config/webpack.config.dev.js

config/webpack.config.prod.js

package.json

scripts/start.js

src/components/Loader.js

src/constants/console.js

src/components/Toolbar/index.js

src/components/Toolbar/style.css

src/components/Pages/index.js

sjd78 · 2019-02-21T23:32:45Z

@bond95, @gregsheremeta - Try opening a VNC browser console and leave it open for awhile. Seems to crash my tab every time, I just haven't been able to time it out consistently.

Update: And now I've had a VNC console to a command line session open without errors for >4 hours. 🤷‍♂️

sjd78 · 2019-02-22T03:49:59Z

@lwrigh, @gregsheremeta, @bond95 - Thoughts on the breadcrumb title? Right now it is the VM's console ID. Maybe just the word "Console" instead?

sjd78 · 2019-02-22T04:45:57Z

@gregsheremeta - Thought ... this could probably be adapted to webadmin via ui-extensions without all that much extra work.

lwrigh · 2019-02-22T05:12:13Z

@lwrigh, @gregsheremeta, @bond95 - Thoughts on the breadcrumb title? Right now it is the VM's console ID. Maybe just the word "Console" instead?

Just having 'Console' as the third breadcrumb makes sense to me.

sjd78

Overall the structure looks good and works well enough.

A few general comments:

The way the ConsoleConfirmationModal is tightly tied to the redux store seems to work but seems very unique to this component. Feels a bit over complicated, but I don't have any specific suggestions on how to simplify it.
For the customized version of @patterfly/react-console, it would be helpful to do 2 things:
- Put the custom code in a distinct folder like src/patternfly-overrides and refernce it there. That will make it a lot more obvious as to where the code came from and that it could be replace in future by an updated component.
- Put comments in the components you needed to change summarizing what you needed to add or change. That will help simplify the review/verification. (I had to look at the sources in the patternfly-react repo and visually compare differences to see what was tweaked for web-ui).
Using the React portal to push the VNC toolbar to the right place is great work! It would be nice to adopt that style for all the page toolbars so they don't have to be tied in via the routes.

src/components/VmConsole/VncConsole/index.js

src/components/VmConsole/VncConsole/VncActions.js

src/components/VmConsole/VncConsole/index.js

src/components/VmConsole/index.js

src/routes.js

src/components/VmConsole/index.js

src/components/VmActions/index.js

src/components/VmActions/Action.js

src/components/VmConsole/VmConsoleSelector.js

Fixed comments, moved VncConsole to src/react-console

sjd78 · 2019-02-27T18:41:20Z

@bond95 - Need to be rebased after #967 merged.

sjd78

Very close. Only a few style updates and localization work are still needed.

scripts/start.js

src/components/Loader.js

src/components/VmActions/index.js

src/components/VmConsole/index.js

sjd78 · 2019-02-28T17:43:50Z

src/react-console/VncActions.js

+  if (!toolbarContainer) {
+    return toolbar
+  }
+  return document.getElementById(toolbarContainer) &&


It would be more appropriate for toolbarContainer to already be a createPortal compatible element by now...probably passed in as a component already.

Actually I don't see big difference, but I prefer more this way, because in this case props more clear and easier to read.

I haven't seen any other patternfly-react or react-bootstrap code do containers/portals this way. The component always accepts another element to use as the target.

According to what I saw, patternfly-react has both id and element, so user can choose which one to use, and it seems to me better solution.

I looked again and found exactly 1 instance of getElementById use in a component in patternfly-react. That use actually looks for a component in one param (scrollable) and if it isn't there, uses a different param (scrollableElementId). Either way, it's up to Marek in the patternfly-react patch to approve what will be integrated with react-console and ultimately adopted here.

feel free to get his opinion now ...

sjd78 · 2019-02-28T17:45:52Z

src/components/VmConsole/index.js

+            path={proxyTicket}
+            host={websocket.get('host')}
+            port={websocket.get('port')}
+            toolbarContainer='vm-console-toolbar-sendkeys'


Grab the DOM element directly here instead of passing in the element id. Don't make the VncConsole component have to do any work fetching. Make it easy by passing in the element that can be the portal target for the container.

src/react-console/index.js

sjd78 · 2019-02-28T18:12:02Z

@bond95, @gregsheremeta, @lwrigh - FYI, an easy way to tell if you got all of the strings into the translation bundle is to run the app with the dummy locale. A script will generate a dummy locale for you that surrounds all of the english with some arrows. No arrows on a piece of text means the text is data or needs to be localized.

  yarn intl:dummy

Then open the app with the locale on the URL:

   http://localhost:3000?locale=aa

For example:

sjd78 · 2019-03-01T17:25:27Z

@bond95 - I'm still getting #966 when I try to connect to a console of a VM that is powering up.

sjd78

3 issues to address remain:

VmConsole's use of VncConsole needs localized text
Recheck the CSS in src/index-nomodules.css
attempting to open a console on a VM with a broken/stopped/non-responsive guest agent doesn't work #966 is a valid issue and should probably be addressed before we merge

src/index-nomodules.css

src/intl/messages.js

…es to console modal settings

…ix some issues (including pool console button style)

…el, added message about bad certs after disconnect,fixed bug in force VMs update

sjd78 · 2019-04-30T16:18:44Z

@bond95 , @leistnerova - How is this PR looking? Latest round of code changes look ok. Does it just need another round of testing?

leistnerova · 2019-05-02T11:04:55Z

I'll do a quick test of last changes, not the whole testing.

leistnerova · 2019-05-02T11:05:06Z

ci build please

bond95 · 2019-05-06T09:08:53Z

@sjd78 @lwrigh WDYT?

lwrigh · 2019-05-06T13:37:34Z

I would suggest changing the text to read, "Cannot connect to websocket proxy server. Please check your websocket proxy certificate or ask your administrator for help. For further information please refer to the console manual(linked). Press the 'Connect' button to reconnect to the console.".

bond95 · 2019-05-07T09:19:47Z

@lwrigh Done

lwrigh · 2019-05-07T12:00:05Z

Nice looks good to me!

leistnerova · 2019-05-09T15:06:34Z

LGTM

sjd78 · 2019-05-09T21:54:38Z

Just noticed while redoing a demo video: At least on Chrome, if you click directly from the console to the details in the breadcrumbs, the charts on the details page don't render:

My best guess is that c3 gets broken somewhere when you don't explicitly click the disconnect button. Eventually my browser tab (Chrome) will straight crash as well.

@bond95, @lwrigh, @leistnerova - Can you guys reproduce?
@sgratch, @michalskrivanek - Punt to an issue or resolve in this PR (assuming it can be reproduced)?

bond95 · 2019-05-10T09:25:54Z

@bond95, @lwrigh, @leistnerova - Can you guys reproduce?

@sjd78 Unfortunately, no, everything works fine for me in Chrome Version 73.0.3683.86 (Official Build).

sjd78 · 2019-05-15T13:23:38Z

@sgratch, @michalskrivanek - Punt to an issue or resolve in this PR (assuming it can be reproduced)?

I'll do some additional testing permutations (Chrome on a Mac connecting to dev server web-ui and connecting to web-ui installed to my dev ovirt-engine instance). If it still comes up, I'll log a separate issue. I'm ok with merging this as-is.

bond95 · 2019-05-15T13:25:47Z

I'll do some additional testing permutations (Chrome on a Mac connecting to dev server web-ui and connecting to web-ui installed to my dev ovirt-engine instance). If it still comes up, I'll log a separate issue. I'm ok with merging this as-is.

Great, thank you.

leistnerova · 2019-05-15T15:21:46Z

@sjd78 I also did not reproduce is, tested in Chrome 74.0 on Fedora

sjd78 · 2019-05-17T17:01:55Z

@sgratch, @michalskrivanek - Punt to an issue or resolve in this PR (assuming it can be reproduced)?

I'll do some additional testing permutations (Chrome on a Mac connecting to dev server web-ui and connecting to web-ui installed to my dev ovirt-engine instance). If it still comes up, I'll log a separate issue. I'm ok with merging this as-is.

I tested Chrome 74.0 on Mac against the web-ui dev server and against web-ui installed to my dev ovirt-engine and was able to reproduce the c3 no charts problem and the browser tab crashing problem. The tab appears to crash because of a massive memory leak. I also tested with Firefox on fedora against my dev ovirt-engine with web-ui installed and recreated the memory leak there. Firefox's tab doesn't crash as politely as the Chrome tab.

Here is a video of the Chrome 74.0 on Mac C3 fail followed by memory leak and tab crash:
https://youtu.be/ZrOGGyPQpPA

leistnerova · 2019-05-28T14:41:08Z

It seems I have the reproduction steps:

have running VM
check browser console, go back to VM detail
Shutdown with Force (normal shutdown seems to work)
Run VM again
go to browser console, wait a while
when going back to VM detail, charts already loaded
go to browser console, back to detail
charts broken

It doesn't happen 100%, but mostly. Reproduced in Google Chrome 74.0 and also Firefox 66.0

michalskrivanek · 2019-05-29T07:08:44Z

@leistnerova thanks! @sjd78 does the console code terminate properly when VM is shut down? if not...could be novnc bug or our code around it. Generally as soon as VM is down the console should die with it.

bond95 · 2019-05-29T09:55:47Z

@leistnerova thanks! @sjd78 does the console code terminate properly when VM is shut down? if not...could be novnc bug or our code around it. Generally as soon as VM is down the console should die with it.

Console code must be terminated with VmConsole component. When user go to detail page from console page, VmConsole is unmounting and console terminate as well. According to my experiments problem can be in charts, because this bug cause frequent update of charts, which can be cause of growing memory usage.

sgratch · 2019-06-04T16:49:06Z

@bond95 @sjd78 if both #966 and #490 is handled by this PR then can we please merge this as discussed and open a new issue for #956 (comment) ?

bond95 · 2019-06-05T10:15:17Z

@sgratch yes, of course, I'll merge this.

Pull Request: oVirt/ovirt-web-ui#956 Change-Id: I9fefb7a09922c3ad05908be70850a89b5017782f Signed-off-by: Scott J Dickerson <[email protected]>

bond95 requested review from sjd78, gregsheremeta and lwrigh February 15, 2019 09:54

bond95 mentioned this pull request Feb 15, 2019

VM list page needs static ID on Suspend button #937

Closed

lwrigh reviewed Feb 18, 2019

View reviewed changes

bond95 mentioned this pull request Feb 18, 2019

feat(VncConsole): More possibilities to customize component patternfly/patternfly-react#1402

Merged

bond95 force-pushed the console-page branch from a1bee37 to cef5189 Compare February 20, 2019 10:27

sjd78 requested changes Feb 21, 2019

View reviewed changes

sjd78 mentioned this pull request Feb 21, 2019

attempting to open a console on a VM with a broken/stopped/non-responsive guest agent doesn't work #966

Closed

sjd78 previously requested changes Feb 22, 2019

View reviewed changes

sjd78 self-requested a review February 27, 2019 18:23

sjd78 requested changes Feb 28, 2019

View reviewed changes

bond95 force-pushed the console-page branch from 7f8633f to 2056b2f Compare March 1, 2019 13:10

bond95 requested a review from sjd78 March 1, 2019 13:18

sjd78 requested changes Mar 1, 2019

View reviewed changes

src/index-nomodules.css Outdated Show resolved Hide resolved

src/intl/messages.js Outdated Show resolved Hide resolved

bond95 force-pushed the console-page branch from 2056b2f to e7a5c40 Compare March 4, 2019 10:23

bond95 added 4 commits April 18, 2019 14:01

Fixed bug with RDP, added images with higher resolution and did chang…

78d452a

…es to console modal settings

Added connected handler and containerId props to novnc console, and f…

3bdf180

…ix some issues (including pool console button style)

Fixed bug in console confirmation modal, fixed focus after ctrl+alt+d…

69e0610

…el, added message about bad certs after disconnect,fixed bug in force VMs update

Added handling empty of websocket proxy path

bfce39d

bond95 force-pushed the console-page branch from d7d6a9e to bfce39d Compare April 18, 2019 12:13

sjd78 approved these changes Apr 23, 2019

View reviewed changes

Changed connection failure text

1991f95

bond95 force-pushed the console-page branch from c0f869b to 1991f95 Compare May 7, 2019 09:19

bond95 mentioned this pull request Jun 5, 2019

Memory leak in C3 charts when navigating on to and off of VM details #1031

Closed

bond95 merged commit 3f38ef0 into oVirt:master Jun 5, 2019

Added NoVNC console page #956

Added NoVNC console page #956

Conversation

bond95 commented Feb 15, 2019 • edited by sjd78 Loading

lwrigh left a comment

Choose a reason for hiding this comment

lwrigh commented Feb 18, 2019

gregsheremeta commented Feb 18, 2019

bond95 commented Feb 18, 2019

gregsheremeta commented Feb 18, 2019

bond95 commented Feb 20, 2019

lwrigh commented Feb 21, 2019

sjd78 left a comment

Choose a reason for hiding this comment

sjd78 commented Feb 21, 2019 • edited Loading

sjd78 commented Feb 22, 2019

sjd78 commented Feb 22, 2019

lwrigh commented Feb 22, 2019

sjd78 left a comment

Choose a reason for hiding this comment

sjd78 commented Feb 27, 2019

sjd78 left a comment

Choose a reason for hiding this comment

sjd78 Feb 28, 2019

Choose a reason for hiding this comment

bond95 Mar 1, 2019

Choose a reason for hiding this comment

sjd78 Mar 1, 2019

Choose a reason for hiding this comment

bond95 Mar 1, 2019

Choose a reason for hiding this comment

sjd78 Mar 1, 2019

Choose a reason for hiding this comment

gregsheremeta Mar 1, 2019

Choose a reason for hiding this comment

sjd78 Feb 28, 2019

Choose a reason for hiding this comment

sjd78 commented Feb 28, 2019

sjd78 commented Mar 1, 2019

sjd78 left a comment

Choose a reason for hiding this comment

sjd78 commented Apr 30, 2019

leistnerova commented May 2, 2019

leistnerova commented May 2, 2019

bond95 commented May 6, 2019

lwrigh commented May 6, 2019

bond95 commented May 7, 2019

lwrigh commented May 7, 2019

leistnerova commented May 9, 2019

sjd78 commented May 9, 2019

bond95 commented May 10, 2019

sjd78 commented May 15, 2019

bond95 commented May 15, 2019

leistnerova commented May 15, 2019

sjd78 commented May 17, 2019 • edited Loading

leistnerova commented May 28, 2019

michalskrivanek commented May 29, 2019

bond95 commented May 29, 2019

sgratch commented Jun 4, 2019

bond95 commented Jun 5, 2019

bond95 commented Feb 15, 2019 •

edited by sjd78

Loading

sjd78 commented Feb 21, 2019 •

edited

Loading

sjd78 commented May 17, 2019 •

edited

Loading