Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

open the VNC console in a new window #5593

Merged
merged 1 commit into from
Jul 2, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
import * as React from 'react';
import { K8sResourceKind, PodKind } from '@console/internal/module/k8s';
import { PodModel, ServiceModel } from '@console/internal/models';
import { getVMStatus } from '../../statuses/vm/vm-status';
import { VMImportKind } from '../../types/vm-import/ovirt/vm-import';
import { V1alpha1DataVolume } from '../../types/vm/disk/V1alpha1DataVolume';
import {
DataVolumeModel,
VirtualMachineImportModel,
VirtualMachineInstanceMigrationModel,
VirtualMachineInstanceModel,
VirtualMachineModel,
} from '../../models';
import { VMIKind, VMKind } from '../../types/vm';
import { Firehose, FirehoseResult } from '@console/internal/components/utils';
import { VMConsolesWrapper } from '../vms/vm-console';
import { getLoadedData } from '../../utils';
import { ConsoleEmptyState } from './vm-console-empty-state';
import { ConsoleType } from '../../constants/vm/console-type';

const ConnectedVMConsole: React.FC<ConnectedVMConsoleProps> = ({
type,
vm,
vmis,
pods,
migrations,
dataVolumes,
vmImports,
}) => {
const loadedVM = getLoadedData(vm);
Copy link
Contributor

Choose a reason for hiding this comment

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

I dont see any kind of error/loading handling. I really dislike getLoadedData selector because of that exact reason - it just pushes you to forget about the handling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

getLoadedData returns null in case of an error? later down the tree theres a component for many cases:
VMCannotBeStarted, VMIsStarting, VMIsDown

Copy link
Contributor

@rawagner rawagner Jun 30, 2020

Choose a reason for hiding this comment

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

I dont understand what kind of handling you can have to differentiate between loaded, loading and error states when getLoadedData can have two values - data or null. That's 3 states but only 2 values ?

Copy link
Contributor Author

@glekner glekner Jun 30, 2020

Choose a reason for hiding this comment

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

I thought getVMStatus handles it pretty nicely? @suomiy your thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems to me like you would end up with <VMIsDown /> while loading/error. There's just no state for loading or error

Copy link
Member

Choose a reason for hiding this comment

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

IMO it is not that important to handle this in VMConsolesTab.
It might be useful a bit in standaloneconsole. I don't mind either way. The user will probably visit the VM page when the console stops working (vm down) to debug it.

const loadedVMIs = getLoadedData(vmis);
const loadedPods = getLoadedData(pods);
const loadedMigrations = getLoadedData(migrations);
const loadedDataVolumes = getLoadedData(dataVolumes);
const loadedImports = getLoadedData(vmImports);
const vmi = loadedVMIs?.[0];

const vmStatusBundle = getVMStatus({
vm: loadedVM,
vmi,
pods: loadedPods,
migrations: loadedMigrations,
dataVolumes: loadedDataVolumes,
vmImports: loadedImports,
});

return (
<VMConsolesWrapper
vm={loadedVM}
vmi={vmi}
vmStatusBundle={vmStatusBundle}
pods={loadedPods}
type={type}
showOpenInNewWindow={false}
/>
);
};

type ConnectedVMConsoleProps = {
type: ConsoleType;
vm?: FirehoseResult<VMKind>;
vmis?: FirehoseResult<VMIKind[]>;
pods?: FirehoseResult<PodKind[]>;
migrations?: FirehoseResult<K8sResourceKind[]>;
dataVolumes?: FirehoseResult<V1alpha1DataVolume[]>;
vmImports?: FirehoseResult<VMImportKind[]>;
services?: FirehoseResult;
};

const FirehoseVMConsole: React.FC<FirehoseVMConsoleProps> = ({
type,
namespace,
name,
isKubevirt,
}) => {
const resources = [
{
kind: VirtualMachineModel.kind,
name,
namespace,
prop: 'vm',
},
{
kind: VirtualMachineInstanceModel.kind,
namespace,
isList: true,
prop: 'vmis',
optional: true,
fieldSelector: `metadata.name=${name}`,
},
{
kind: PodModel.kind,
namespace,
isList: true,
prop: 'pods',
},
{
kind: VirtualMachineInstanceMigrationModel.kind,
isList: true,
namespace,
prop: 'migrations',
},
{
kind: VirtualMachineImportModel.kind,
isList: true,
namespace,
prop: 'vmImports',
optional: true,
},
{
kind: DataVolumeModel.kind,
Copy link
Member

Choose a reason for hiding this comment

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

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

isList: true,
namespace,
prop: 'dataVolumes',
},
{ kind: ServiceModel.kind, namespace, prop: 'services' },
];
return isKubevirt ? (
<Firehose resources={resources}>
Copy link
Contributor

Choose a reason for hiding this comment

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

would be nice to use k8s-watch-hook, not a blocker though

<ConnectedVMConsole type={type} />
</Firehose>
) : (
<ConsoleEmptyState isKubevirt={isKubevirt} />
);
};

type FirehoseVMConsoleProps = {
type: ConsoleType;
namespace: string;
name: string;
isKubevirt: boolean;
};

export default FirehoseVMConsole;
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
.kv-cloud-vm-console-empty {
display: flex;
justify-content: center;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import * as React from 'react';
import { Title, EmptyState, EmptyStateIcon, EmptyStateBody, Spinner } from '@patternfly/react-core';
import { PlugIcon } from '@patternfly/react-icons';
import './vm-console-empty-state.scss';

export interface ConsoleEmptyStateProps {
isKubevirt: boolean;
}

export const ConsoleEmptyState: React.SFC<ConsoleEmptyStateProps> = ({ isKubevirt }) => (
<div className="kv-cloud-vm-console-empty">
<EmptyState>
{isKubevirt === undefined ? (
<>
<EmptyStateIcon variant="container" component={Spinner} />
<Title size="lg" headingLevel="h4">
Loading
</Title>
</>
) : (
<>
<EmptyStateIcon icon={PlugIcon} />
<Title size="lg" headingLevel="h4">
Kubevirt Plugin was not found
</Title>
<EmptyStateBody>
Accessing the VNC Console is not possible. Please install Kubevirt
</EmptyStateBody>
</>
)}
</EmptyState>
</div>
);
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import * as React from 'react';
import ConnectedVMConsole from '@console/kubevirt-plugin/src/components/connected-vm-console/connected-vm-console';
import { ConsoleType } from '@console/kubevirt-plugin/src/constants/vm/console-type';
import { FLAG_KUBEVIRT } from '@console/kubevirt-plugin/src/plugin';
import { useFlag } from '@console/shared/src/hooks/flag';

export const VMConsolePage: React.FC<VMConsolePageProps> = ({ match, location }) => {
const isKubevirt = useFlag(FLAG_KUBEVIRT);
const params = new URLSearchParams(location.search);
const type = ConsoleType.fromString(params.get('type'));
const { name, ns: namespace } = match.params;

return (
<ConnectedVMConsole isKubevirt={isKubevirt} type={type} namespace={namespace} name={name} />
);
};

type VMConsolePageProps = {
location: Location;
match: any;
};
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,14 @@ export const menuActionDeleteVMI = (kindObj: K8sKind, vmi: VMIKind): KebabOption
accessReview: asAccessReview(kindObj, vmi, 'delete'),
});

export const menuActionOpenConsole = (kindObj: K8sKind, vmi: VMIKind): KebabOption => ({
label: `Open Console`,
Copy link
Member

Choose a reason for hiding this comment

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

good idea - better accessibility

callback: () =>
Copy link
Member

Choose a reason for hiding this comment

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

can we do the new tab here instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it does open a new tab, if not its a user browser preference.

Copy link
Member

Choose a reason for hiding this comment

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

I see, shouldn't we rather always open it a new tab to be consistent?

window.open(
`/k8s/ns/${getNamespace(vmi)}/virtualmachineinstances/${getName(vmi)}/standaloneconsole`,
),
});

export const vmMenuActions = [
menuActionStart,
menuActionStop,
Expand All @@ -287,6 +295,7 @@ export const vmMenuActions = [
menuActionCancelMigration,
menuActionClone,
menuActionCdEdit,
menuActionOpenConsole,
Kebab.factory.ModifyLabels,
Kebab.factory.ModifyAnnotations,
menuActionDeleteVMorCancelImport,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export type VMTabProps = {
customData: {
kindObj: K8sKind;
};
showOpenInNewWindow?: boolean;
};

export type VMLikeEntityTabProps = {
Expand Down