open the VNC console in a new window #5593

glekner · 2020-05-27T09:26:14Z

design: openshift/openshift-origin-design#425
JIRA: https://issues.redhat.com/browse/CNV-5111

This enables opening an independent new window for a VM/VMI console.

Opening a new window disables the main window's console as they can cause disconnects between each other. closing the new window returns the console to the main window.
closing the main window doesn't affect the new window :)
If the new window fails to open due to some Adblocker, an alert is shown.
We're introducing a new route /vm-console with 3 URL Parameters:
name, namespace, console
if console=SerialConsole it will open the serial console, for every other string or missing parameter it will open VncConsole
If name or namespace or both are missing or misspelled, an error is shown

If Kubevirt is not installed the following error is shown:

Video showing the new feature:

openshift-ci-robot · 2020-05-27T09:26:17Z

@glekner: This pull request references Bugzilla bug 1837677, which is invalid:

expected the bug to target the "4.5.0" release, but it targets "2.5.0" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 1837677: open the VNC console in a new window

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot · 2020-05-27T09:29:37Z

@glekner: This pull request references Bugzilla bug 1837677, which is invalid:

expected the bug to target the "4.5.0" release, but it targets "2.5.0" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 1837677: open the VNC console in a new window

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

glekner · 2020-05-27T09:30:14Z

@suomiy @rawagner @yaacov can you review

yaacov · 2020-05-27T10:08:29Z

frontend/public/components/app.jsx

@@ -229,6 +230,7 @@ render(
  <Provider store={store}>
    <Router history={history} basename={window.SERVER_FLAGS.basePath}>
      <Switch>
+        <Route path="/vm-console" component={CloudVMConsole} />


IMHO our route table is a better place for a route we use, see for example:
https://github.com/openshift/console/blob/master/frontend/packages/kubevirt-plugin/src/plugin.tsx#L158

Mounting the route here will mount the layout aswell, we want to render the console alone

@vojtechszocs @spadgett

Are we okay with rendering a kubevirt component here? Are we going to add an extension point for this in the future?

Mounting the route here will mount the layout aswell, we want to render the console alone

With #5645 a plugin can add new page & route that is rendered outside the standard Console <App> layout.

I think the CloudVMConsole page fits this use case, so we shouldn't need to modify the core app.jsx.

atiratree · 2020-05-27T11:12:02Z

frontend/packages/console-app/src/components/cloud-vm-console/cloud-vm-console.tsx

+
+const CloudVMConsole: React.FC<CloudVMConsoleProps> = ({ location, flags }) => {
+  const params = new URLSearchParams(location.search);
+  const consoleType = params.get('console');


can we make the constants a bit simpler? like vnc | serial | rdp

and maybe name it just type ?

atiratree · 2020-05-27T11:12:25Z

frontend/packages/console-app/src/components/cloud-vm-console/cloud-vm-console.tsx

+  return (
+    <ConnectedVMConsole
+      isKubevirt={flags.KUBEVIRT}
+      consoleType={consoleType === 'SerialConsole' ? 'SerialConsole' : 'VncConsole'}


Can we create an enum for this?

atiratree · 2020-05-27T11:18:04Z

frontend/packages/kubevirt-plugin/src/components/vms/vm-console.tsx

+      '',
+      `width=1024,height=768,left=200,top=200`,
+    );
+


IMHO we shouldn't create an additional logic for this. We should have the same behaviour when somebody opens the console in the new tab by themselves.

Can we just create a simple link with href="/vm-console" target="_blank" rel="noopener noreferrer" ?

Current console will get Disconnected by default in the current window

From my experience they interfere each other.
also having a variable for the window helps us know if it got blocked by an Adblocker or not.

What do you mean by interfering? You can still open a new tab with /vm-console.

You don't have to care about adblock when you have link for a new tab

atiratree · 2020-05-27T11:20:02Z

frontend/packages/kubevirt-plugin/src/components/vms/vm-console.tsx

@@ -63,7 +65,13 @@ const VMConsoles: React.FC<VMConsolesProps> = ({
  serial,
  rdp,
  LoadingComponent,


we could also add the url console type param for /k8s/ns/default/virtualmachines/vm-example/consoles

atiratree · 2020-05-27T11:25:59Z

frontend/packages/kubevirt-plugin/src/components/connected-vm-console/connected-vm-console.tsx

+      optional: true,
+    },
+    {
+      kind: DataVolumeModel.kind,


these tagging resources are becoming quite a big pain. We should refactor how we use them.

vojtechszocs · 2020-06-04T17:15:57Z

frontend/public/components/app.jsx

@@ -229,6 +230,7 @@ render(
  <Provider store={store}>
    <Router history={history} basename={window.SERVER_FLAGS.basePath}>
      <Switch>
+        <Route path="/vm-console" component={CloudVMConsole} />


Mounting the route here will mount the layout aswell, we want to render the console alone

With #5645 a plugin can add new page & route that is rendered outside the standard Console <App> layout.

I think the CloudVMConsole page fits this use case, so we shouldn't need to modify the core app.jsx.

vojtechszocs · 2020-06-04T17:22:42Z

frontend/packages/console-app/src/components/cloud-vm-console/cloud-vm-console.tsx

@@ -0,0 +1,27 @@
+import * as React from 'react';
+import { connectToFlags, FlagsObject } from '@console/internal/reducers/features';
+import ConnectedVMConsole from '@console/kubevirt-plugin/src/components/connected-vm-console/connected-vm-console';


@glekner Let's leverage #5645 and move all VM-related console code to kubevirt-plugin.

vojtechszocs · 2020-06-04T17:29:57Z

frontend/packages/console-app/src/components/cloud-vm-console/cloud-vm-console.tsx

+  return (
+    <ConnectedVMConsole
+      isKubevirt={flags.KUBEVIRT}
+      type={type === ConsoleType.SERIAL ? ConsoleType.SERIAL : ConsoleType.VNC}


Why not simply use value pass-through?

Suggested change

type={type === ConsoleType.SERIAL ? ConsoleType.SERIAL : ConsoleType.VNC}

type={type}

The value should be already coerced via ConsoleType.fromString logic.

vojtechszocs · 2020-06-04T17:32:29Z

frontend/packages/console-app/src/components/cloud-vm-console/cloud-vm-console.tsx

+  location: Location;
+};
+
+export default connectToFlags('KUBEVIRT')(CloudVMConsole);


Shouldn't we import & use the KubeVirt flag name as a constant?

vojtechszocs · 2020-06-04T17:33:41Z

frontend/packages/kubevirt-plugin/src/components/connected-vm-console/connected-vm-console.tsx

+  });
+
+  return (
+    <VmConsolesWrapper


Suggested change

<VmConsolesWrapper

<VMConsolesWrapper

Also why "consoles" (plural) ?

Patternfly's AccessConsoles is plural, so I guess someone thought its in place. ( I didnt name the components)

vojtechszocs · 2020-06-04T17:36:41Z

...tend/packages/kubevirt-plugin/src/components/connected-vm-console/vm-console-empty-state.tsx

+                  <>
+                    <TextListItem component={TextListItemVariants.dt}>name</TextListItem>
+                    <TextListItem component={TextListItemVariants.dd}>
+                      Missing name of the VM/VMI


Nit: I'd omit the "Missing" prefix since it's already mentioned above ("Missing Required URL Parameters").

Suggested change

Missing name of the VM/VMI

Name of the VM/VMI

vojtechszocs · 2020-06-04T17:37:47Z

...tend/packages/kubevirt-plugin/src/components/connected-vm-console/vm-console-empty-state.tsx

+                  <>
+                    <TextListItem component={TextListItemVariants.dt}>namespace</TextListItem>
+                    <TextListItem component={TextListItemVariants.dd}>
+                      Missing namespace of the VM/VMI


Suggested change

Missing namespace of the VM/VMI

Namespace of the VM/VMI

vojtechszocs · 2020-06-04T17:39:13Z

frontend/packages/kubevirt-plugin/src/components/vms/vm-console.scss

+.vm-console-actions {
+  padding: 0 30px;
+}
+.vm-console-bottom-gutter {


Nit: please separate CSS declarations with newline.

vojtechszocs · 2020-06-04T17:41:25Z

frontend/packages/kubevirt-plugin/src/components/vms/vm-console.tsx

+        >
+          <SerialConsoleConnector {...serial} />
+          <VncConsole {...vnc} />
+          {desktopViewverSelector}


Suggested change

{desktopViewverSelector}

{desktopViewerSelector}

vojtechszocs · 2020-06-04T17:42:02Z

frontend/packages/kubevirt-plugin/src/components/vms/vm-console.tsx

  );
 };

-const VmConsolesWrapper: React.FC<VmConsolesWrapperProps> = (props) => {
-  const { vm: vmProp, vmi, pods, vmStatusBundle } = props;
+export const VmConsolesWrapper: React.FC<VmConsolesWrapperProps> = (props) => {


Suggested change

export const VmConsolesWrapper: React.FC<VmConsolesWrapperProps> = (props) => {

export const VMConsoleWrapper: React.FC<VmConsolesWrapperProps> = (props) => {

openshift-ci-robot · 2020-06-05T23:57:24Z

@glekner: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

In response to this:

open the VNC console in a new window

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

atiratree · 2020-06-10T12:37:22Z

frontend/packages/kubevirt-plugin/src/constants/vm/console-type.ts

+
+export class ConsoleType extends ObjectEnum<string> {
+  static readonly VNC = new ConsoleType('VncConsole');
+  static readonly SERIAL = new ConsoleType('SerialConsole');


Maybe we should allow navigating to DesktopViewer as well. And default to VNC when not available.

We also might select the vnc or serial if we detect one of them is not available.

thoughts?

cc @mareklibra

atiratree · 2020-06-10T12:39:02Z

frontend/packages/kubevirt-plugin/src/components/vms/vm-console.tsx

+        </Button>
+      )}
+      <AccessConsoles
+        preselectedType={type?.toString() || VNC_CONSOLE_TYPE}


here select the right type if the desired one is not available

atiratree · 2020-06-10T12:41:24Z

frontend/packages/kubevirt-plugin/src/components/vms/vm-console.tsx

+        <Button
+          className="vm-console-bottom-gutter"
+          variant="secondary"
+          onClick={() => window.open(`/vm-console?namespace=${namespace}&name=${vmName}`)}


can we use this here?

href={href} target="_blank" rel="noopener noreferrer"

atiratree · 2020-06-10T12:45:33Z

frontend/packages/kubevirt-plugin/src/components/connected-vm-console/cloud-vm-console.tsx

+
+const CloudVMConsole: React.FC<CloudVMConsoleProps> = ({ location, flags }) => {
+  const params = new URLSearchParams(location.search);
+  const type = ConsoleType.fromString(params.get('console'));


Can we use a shorter name without uppercases for the URL?

console=vnc instead of console=VncConsole
and console=serial

We could define both in ConsoleType to have them together in one enum instance and use the second one for pf react-console

atiratree · 2020-06-10T12:46:21Z

frontend/packages/kubevirt-plugin/src/components/connected-vm-console/connected-vm-console.tsx

+import { ConsoleEmptyState } from './vm-console-empty-state';
+import { ConsoleType } from '../../constants/vm/console-type';
+
+const CloudVMConsole: React.FC<CloudVMConsoleProps> = ({


What is the reason for calling this Cloud? ^^

Just took inspiration from the CloudTerminal

IMO it is better not to overuse words like this, because people will try to find meaning in them after they try to touch this code in one year

atiratree · 2020-07-01T11:41:17Z

frontend/packages/kubevirt-plugin/src/components/vms/vm-console.tsx

+            component="a"
+            target="_blank"
+            variant="secondary"
+            href={`/k8s/ns/${namespace}/virtualmachineinstances/${vmName}/standaloneconsole?type=${consoleType.toString()}`}


NPE: consoleType could be null if no console is supported by VM

atiratree · 2020-07-01T11:50:16Z

frontend/packages/kubevirt-plugin/src/components/vms/vm-console.tsx

+    return null;
+  };
+
+  const consoleType = typeNotSupported || type === undefined ? getAvailableType() : type;


nit: IMO it is always better to use type == null unless we have a special use case that has different behaviour for null vs undefined

yaacov · 2020-07-02T08:25:44Z

/test e2e-gcp-console
/test analyze

rawagner · 2020-07-02T10:53:58Z

/lgtm

openshift-bot · 2020-07-02T11:19:17Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-02T11:32:17Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-02T11:45:18Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-02T11:59:01Z

/retest

Please review the full test history for this PR and help us cut down flakes.

yaacov · 2020-07-02T12:08:15Z

/test analyze
/test e2e-gcp-console

openshift-bot · 2020-07-02T12:11:24Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-02T12:24:17Z

/retest

Please review the full test history for this PR and help us cut down flakes.

rawagner · 2020-07-02T12:27:12Z

/lgtm cancel

Analyze is failing, probably because VMConsolePage is not loaded lazily which brings a lot of stuff from kubevirt plugin. So either lazy load the component or wait for route extension.

glekner · 2020-07-02T13:07:51Z

/retest

rawagner · 2020-07-02T14:12:40Z

/lgtm

openshift-ci-robot · 2020-07-02T14:13:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: glekner, rawagner, suomiy

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~frontend/OWNERS~~ [rawagner]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot added the bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. label May 27, 2020

openshift-ci-robot added bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. component/core Related to console core functionality labels May 27, 2020

openshift-ci-robot requested review from benjaminapetersen and rawagner May 27, 2020 09:26

openshift-ci-robot added the component/kubevirt Related to kubevirt-plugin label May 27, 2020

glekner mentioned this pull request May 27, 2020

Bug 1837677: open the VNC console in a new window #5548

Closed

yaacov reviewed May 27, 2020

View reviewed changes

atiratree suggested changes May 27, 2020

View reviewed changes

glekner force-pushed the cloud-vm-console branch 2 times, most recently from ee498d0 to 6b41f7d Compare May 27, 2020 14:33

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 30, 2020

glekner force-pushed the cloud-vm-console branch from 6b41f7d to b359680 Compare May 31, 2020 14:50

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 31, 2020

yaacov mentioned this pull request Jun 2, 2020

Add optional rootWindow option to route plugin #5645

Closed

glekner force-pushed the cloud-vm-console branch from b359680 to db67238 Compare June 4, 2020 14:11

vojtechszocs suggested changes Jun 4, 2020

View reviewed changes

glekner force-pushed the cloud-vm-console branch from db67238 to 120a8c5 Compare June 4, 2020 22:24

glekner changed the title ~~Bug 1837677: open the VNC console in a new window~~ open the VNC console in a new window Jun 5, 2020

openshift-ci-robot removed bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Jun 5, 2020

glekner force-pushed the cloud-vm-console branch 2 times, most recently from c09e6b6 to 111b73a Compare June 9, 2020 17:58

atiratree suggested changes Jun 10, 2020

View reviewed changes

glekner force-pushed the cloud-vm-console branch 2 times, most recently from b927585 to 9f54dec Compare June 10, 2020 14:50

glekner force-pushed the cloud-vm-console branch from 59226e8 to 20298df Compare July 1, 2020 11:12

atiratree reviewed Jul 1, 2020

View reviewed changes

glekner force-pushed the cloud-vm-console branch 2 times, most recently from d225b96 to 84579bc Compare July 1, 2020 17:30

atiratree approved these changes Jul 2, 2020

View reviewed changes

openshift-ci-robot assigned rawagner Jul 2, 2020

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 2, 2020

openshift-ci-robot removed lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 2, 2020

Fixes 1837677: open the VNC console in a new window

87b9640

glekner force-pushed the cloud-vm-console branch from 84579bc to 87b9640 Compare July 2, 2020 12:57

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 2, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 2, 2020

openshift-merge-robot merged commit 4f21818 into openshift:master Jul 2, 2020

spadgett added this to the v4.6 milestone Jul 8, 2020

	type={type === ConsoleType.SERIAL ? ConsoleType.SERIAL : ConsoleType.VNC}
	type={type}

	export const VmConsolesWrapper: React.FC<VmConsolesWrapperProps> = (props) => {
	export const VMConsoleWrapper: React.FC<VmConsolesWrapperProps> = (props) => {

open the VNC console in a new window #5593

open the VNC console in a new window #5593

Conversation

glekner commented May 27, 2020 • edited

openshift-ci-robot commented May 27, 2020

openshift-ci-robot commented May 27, 2020

glekner commented May 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci-robot commented Jun 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaacov commented Jul 2, 2020

rawagner commented Jul 2, 2020

openshift-bot commented Jul 2, 2020

openshift-bot commented Jul 2, 2020

openshift-bot commented Jul 2, 2020

openshift-bot commented Jul 2, 2020

yaacov commented Jul 2, 2020

openshift-bot commented Jul 2, 2020

openshift-bot commented Jul 2, 2020

rawagner commented Jul 2, 2020

glekner commented Jul 2, 2020

rawagner commented Jul 2, 2020

openshift-ci-robot commented Jul 2, 2020

glekner commented May 27, 2020 •

edited