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.

Sign up for GitHub

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

Jump to bottom

Bug 1829542: Add mount guest agent tools to vm wizard #5490

Merged
merged 9 commits into from
Aug 8, 2020
Merged

Bug 1829542: Add mount guest agent tools to vm wizard #5490

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
7b54b96
add mount win tools checkbox
yaacov Aug 5, 2020
d046361
don't mount win tools in import mode
yaacov Aug 5, 2020
2ec411c
use pluralize for number of auto generated disks
yaacov Aug 6, 2020
6750c7a
rename text calss from checkbox to text
yaacov Aug 6, 2020
37cd478
remove extra chcek
yaacov Aug 6, 2020
9341363
move win tools updater to storage
yaacov Aug 6, 2020
eac892b
call wintools before storagebus updater
yaacov Aug 6, 2020
7ab0ad2
use different pluralize method
yaacov Aug 6, 2020
51130d4
move updating magic to match its invocation
yaacov Aug 7, 2020
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
4 changes: 4 additions & 0 deletions ...ugin/src/components/create-vm-wizard/redux/initial-state/vm-settings-tab-initial-state.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,10 @@ export const getInitialVmSettings = (data: CommonData): VMSettings => {
value: false,
isHidden: hiddenByOperatingSystem,
},
[VMSettingsField.MOUNT_WINDOWS_GUEST_TOOLS]: {
value: false,
isHidden: asHidden(true, VMSettingsField.OPERATING_SYSTEM),
},
[VMSettingsField.FLAVOR]: {
isRequired: asRequired(true),
},
Expand Down
47 changes: 41 additions & 6 deletions ...irt-plugin/src/components/create-vm-wizard/redux/state-update/storage-tab-state-update.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,26 @@
import { ValidationErrorType } from '@console/shared/src';
import { VMSettingsField, VMWizardStorage, VMWizardStorageType } from '../../types';
import { VMSettingsField, VMWizardStorage, VMWizardStorageType, VMWizardProps } from '../../types';
import { InternalActionType, UpdateOptions } from '../types';
import { hasStoragesChanged, iGetProvisionSourceStorage } from '../../selectors/immutable/storage';
import { getNewProvisionSourceStorage } from '../initial-state/storage-tab-initial-state';
import {
getNewProvisionSourceStorage,
windowsToolsStorage,
} from '../initial-state/storage-tab-initial-state';
import { VolumeWrapper } from '../../../../k8s/wrapper/vm/volume-wrapper';
import { DataVolumeWrapper } from '../../../../k8s/wrapper/vm/data-volume-wrapper';
import { StorageUISource } from '../../../modals/disk-modal/storage-ui-source';
import { getNextIDResolver } from '../../../../utils/utils';
import { getStorages } from '../../selectors/selectors';
import { vmWizardInternalActions } from '../internal-actions';
import { getTemplateValidation } from '../../selectors/template';
import { getVolumeContainerImage, isWinToolsImage } from '../../../../selectors/vm';
import { TemplateValidations } from '../../../../utils/validations/template/template-validations';
import { DiskWrapper } from '../../../../k8s/wrapper/vm/disk-wrapper';
import { hasVMSettingsValueChanged } from '../../selectors/immutable/vm-settings';
import {
hasVMSettingsValueChanged,
iGetVmSettingValue,
} from '../../selectors/immutable/vm-settings';
import { iGetCommonData } from '../../selectors/immutable/selectors';

export const prefillInitialDiskUpdater = ({ id, prevState, dispatch, getState }: UpdateOptions) => {
const state = getState();
Expand Down Expand Up @@ -137,7 +145,34 @@ export const internalStorageDiskBusUpdater = ({
}
};

const windowsToolsUpdater = ({ id, prevState, dispatch, getState }: UpdateOptions) => {
Copy link
Member

Choose a reason for hiding this comment

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

it should be declared/called before the internalStorageDiskBusUpdater so the validation to it applies as well

Copy link
Member Author

Choose a reason for hiding this comment

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

moved to before internalStorageDiskBusUpdater

Copy link
Member

Choose a reason for hiding this comment

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

please move the code block as well. We are striving for our updaters to be declared in the same order they are executed in. It is much easier to read the file from top to the bottom to asses what is happening in sequence, than jumping across different sections of the file.

There are few exceptions (eg prefillVmTemplateUpdater) where the updater would get too long and such updaters are moved to new files.

const state = getState();
if (iGetCommonData(state, id, VMWizardProps.isProviderImport)) {
return;
}
if (!hasVMSettingsValueChanged(prevState, state, id, VMSettingsField.MOUNT_WINDOWS_GUEST_TOOLS)) {
return;
}
const mountWindowsGuestTools = iGetVmSettingValue(
state,
id,
VMSettingsField.MOUNT_WINDOWS_GUEST_TOOLS,
);
const windowsTools = getStorages(state, id).find(
(storage) => !!isWinToolsImage(getVolumeContainerImage(storage.volume)),
);

if (mountWindowsGuestTools && !windowsTools) {
dispatch(vmWizardInternalActions[InternalActionType.UpdateStorage](id, windowsToolsStorage));
}
if (!mountWindowsGuestTools && windowsTools) {
dispatch(vmWizardInternalActions[InternalActionType.RemoveStorage](id, windowsTools.id));
}
};

export const updateStorageTabState = (options: UpdateOptions) =>
[prefillInitialDiskUpdater, internalStorageDiskBusUpdater].forEach((updater) => {
updater && updater(options);
});
[prefillInitialDiskUpdater, internalStorageDiskBusUpdater, windowsToolsUpdater].forEach(
(updater) => {
updater && updater(options);
},
);
19 changes: 7 additions & 12 deletions ...plugin/src/components/create-vm-wizard/redux/state-update/vm-settings-tab-state-update.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { FLAGS } from '@console/shared';
import { isWinToolsImage, getVolumeContainerImage } from '../../../../selectors/vm';
import {
hasVmSettingsChanged,
hasVMSettingsValueChanged,
Expand All @@ -20,8 +19,6 @@ import {
import { iGetRelevantTemplate } from '../../../../selectors/immutable/template/combined';
import { CUSTOM_FLAVOR, TEMPLATE_DATAVOLUME_ANNOTATION } from '../../../../constants/vm';
import { ProvisionSource } from '../../../../constants/vm/provision-source';
import { windowsToolsStorage } from '../initial-state/storage-tab-initial-state';
import { getStorages } from '../../selectors/selectors';
import { prefillVmTemplateUpdater } from './prefill-vm-template-state-update';
import { iGetAnnotation } from '../../../../selectors/immutable/common';

Expand Down Expand Up @@ -159,16 +156,14 @@ const osUpdater = ({ id, prevState, dispatch, getState }: UpdateOptions) => {

const os = iGetVmSettingValue(state, id, VMSettingsField.OPERATING_SYSTEM);
const isWindows = os?.startsWith('win');
const windowsTools = getStorages(state, id).find(
(storage) => !!isWinToolsImage(getVolumeContainerImage(storage.volume)),
);

if (isWindows && !windowsTools) {
dispatch(vmWizardInternalActions[InternalActionType.UpdateStorage](id, windowsToolsStorage));
}
if (!isWindows && windowsTools) {
dispatch(vmWizardInternalActions[InternalActionType.RemoveStorage](id, windowsTools.id));
}
dispatch(
Copy link
Member

@atiratree atiratree May 21, 2020 •
edited

Choose a reason for hiding this comment

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

IMO, we should also consider the vm import part here.

can we change the value to false when isProviderImport? But keep it visible in case some import providers (possibly vmWare) wish to use it.

In case of ovirt provider it is actually impossible to use this field so can we hide it in ovirt-state-update.ts?

We should also register field reset for MOUNT_WINDOWS_GUEST_TOOLS in each provider - look for vmFieldUpdate

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't have an env to test and check this scenarios ...

@suomiy can you take it as a follow up PR ?

Copy link
Member

Choose a reason for hiding this comment

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

should be few lines, just use vmFieldUpdate in the same way like for other VM fields and also register isHidden in ovirt provider in the same spot/update.

I can test your PR, once you have it.

Copy link
Member Author

Choose a reason for hiding this comment

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

added a gate to not mount when in import view

vmWizardInternalActions[InternalActionType.UpdateVmSettingsField](
id,
VMSettingsField.MOUNT_WINDOWS_GUEST_TOOLS,
{ isHidden: asHidden(!isWindows, VMSettingsField.OPERATING_SYSTEM), value: isWindows },
),
);
};

const baseImageUpdater = ({ id, prevState, dispatch, getState }: UpdateOptions) => {
Expand Down
1 change: 1 addition & 0 deletions ...tend/packages/kubevirt-plugin/src/components/create-vm-wizard/strings/renderable-field.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ export const titleResolver: RenderableFieldResolver = {
[VMSettingsField.USER_TEMPLATE]: 'Template',
[VMSettingsField.OPERATING_SYSTEM]: 'Operating System',
[VMSettingsField.CLONE_COMMON_BASE_DISK_IMAGE]: 'Clone available operating system source',
[VMSettingsField.MOUNT_WINDOWS_GUEST_TOOLS]: 'Mount Windows guest tools',
[VMSettingsField.FLAVOR]: 'Flavor',
[VMSettingsField.MEMORY]: 'Memory',
[VMSettingsField.CPU]: 'CPUs',
Expand Down
48 changes: 34 additions & 14 deletions ...ckages/kubevirt-plugin/src/components/create-vm-wizard/tabs/vm-settings-tab/os-flavor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
Button,
ButtonVariant,
} from '@patternfly/react-core';
import { pluralize } from '@console/internal/components/utils';
import { ValidationErrorType, asValidationObject } from '@console/shared/src/utils/validation';
import {
concatImmutableLists,
Expand Down Expand Up @@ -48,6 +49,7 @@ export const OSFlavor: React.FC<OSFlavorProps> = React.memo(
userTemplate,
operatinSystemField,
cloneBaseDiskImageField,
mountWindowsGuestToolsField,
flavorField,
workloadProfile,
cnvBaseImages,
Expand All @@ -60,6 +62,7 @@ export const OSFlavor: React.FC<OSFlavorProps> = React.memo(
const display = iGet(operatinSystemField, 'display');
const displayOnly = !!display;
const cloneBaseDiskImage = iGetFieldValue(cloneBaseDiskImageField);
const mountWindowsGuestTools = iGetFieldValue(mountWindowsGuestToolsField);

const params = {
userTemplate,
Expand Down Expand Up @@ -145,6 +148,22 @@ export const OSFlavor: React.FC<OSFlavorProps> = React.memo(
);
const baseImage = operatingSystemBaseImages.find((image) => image.id === os);

const numOfMountedDisks = cloneBaseDiskImage + mountWindowsGuestTools; // using boolean addition operator to count true
Copy link
Member

Choose a reason for hiding this comment

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

wow, this seems so hacky :)

const mountedDisksHelpMsg = numOfMountedDisks > 0 && (
<Text className="kv-create-vm__input-text-help-msg">
View the mounted {pluralize(numOfMountedDisks, 'disk', 'disks', false)} in the{' '}
Copy link
Member

Choose a reason for hiding this comment

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

we also have pluralize function in our package which is easier to use and does not depend on details-page file

Copy link
Member Author

Choose a reason for hiding this comment

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

using our own pluralize :-)

<Button
isDisabled={!goToStorageStep}
isInline
onClick={goToStorageStep}
variant={ButtonVariant.link}
>
<strong>storage</strong>
</Button>{' '}
step
</Text>
);

return (
<>
<FormFieldRow
Expand Down Expand Up @@ -184,21 +203,21 @@ export const OSFlavor: React.FC<OSFlavorProps> = React.memo(
onChange={(v) => onChange(VMSettingsField.CLONE_COMMON_BASE_DISK_IMAGE, v)}
/>
</FormField>
{cloneBaseDiskImage && (
<Text>
View the cloned disk in the{' '}
<Button
isDisabled={!goToStorageStep}
isInline
onClick={goToStorageStep}
variant={ButtonVariant.link}
>
<strong>storage</strong>
</Button>{' '}
step
</Text>
)}
</FormFieldRow>
<FormFieldRow
field={mountWindowsGuestToolsField}
fieldType={FormFieldType.INLINE_CHECKBOX}
loadingResources={loadingResources}
>
<FormField>
<Checkbox
className="kv-create-vm__input-checkbox"
id={getFieldId(VMSettingsField.MOUNT_WINDOWS_GUEST_TOOLS)}
onChange={(v) => onChange(VMSettingsField.MOUNT_WINDOWS_GUEST_TOOLS, v)}
/>
</FormField>
</FormFieldRow>
{mountedDisksHelpMsg}
<FormFieldRow
field={flavorField}
fieldType={FormFieldType.SELECT}
Expand Down Expand Up @@ -228,6 +247,7 @@ type OSFlavorProps = {
flavorField: any;
operatinSystemField: any;
cloneBaseDiskImageField: any;
mountWindowsGuestToolsField: any;
userTemplate: string;
workloadProfile: string;
cnvBaseImages: any;
Expand Down
4 changes: 4 additions & 0 deletions ...kubevirt-plugin/src/components/create-vm-wizard/tabs/vm-settings-tab/vm-settings-tab.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,7 @@
.kv-create-vm__input-checkbox {
margin-top: calc(var(--pf-global--spacer--md) * -1);
}

.kv-create-vm__input-text-help-msg {
margin-top: calc(var(--pf-global--spacer--md) * -1);
}
1 change: 1 addition & 0 deletions .../kubevirt-plugin/src/components/create-vm-wizard/tabs/vm-settings-tab/vm-settings-tab.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ export class VMSettingsTabComponent extends React.Component<VMSettingsTabCompone
operatinSystemField={this.getField(VMSettingsField.OPERATING_SYSTEM)}
flavorField={this.getField(VMSettingsField.FLAVOR)}
cloneBaseDiskImageField={this.getField(VMSettingsField.CLONE_COMMON_BASE_DISK_IMAGE)}
mountWindowsGuestToolsField={this.getField(VMSettingsField.MOUNT_WINDOWS_GUEST_TOOLS)}
userTemplate={this.getFieldValue(VMSettingsField.USER_TEMPLATE)}
workloadProfile={this.getFieldValue(VMSettingsField.WORKLOAD_PROFILE)}
cnvBaseImages={cnvBaseImages}
Expand Down
1 change: 1 addition & 0 deletions frontend/packages/kubevirt-plugin/src/components/create-vm-wizard/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ export enum VMSettingsField {
USER_TEMPLATE = 'USER_TEMPLATE',
OPERATING_SYSTEM = 'OPERATING_SYSTEM',
CLONE_COMMON_BASE_DISK_IMAGE = 'CLONE_COMMON_BASE_DISK_IMAGE',
MOUNT_WINDOWS_GUEST_TOOLS = 'MOUNT_WINDOWS_GUEST_TOOLS',
FLAVOR = 'FLAVOR',
MEMORY = 'MEMORY',
CPU = 'CPU',
Expand Down
18 changes: 10 additions & 8 deletions .../packages/kubevirt-plugin/src/components/create-vm-wizard/utils/renderable-field-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,14 +36,15 @@ const renderableFieldOrder: { [key in RenderableField]: number } = {
[VMSettingsField.USER_TEMPLATE]: 20,
[VMSettingsField.OPERATING_SYSTEM]: 21,
[VMSettingsField.CLONE_COMMON_BASE_DISK_IMAGE]: 22,
[VMSettingsField.FLAVOR]: 23,
[VMSettingsField.MEMORY]: 24,
[VMSettingsField.CPU]: 25,
[VMSettingsField.WORKLOAD_PROFILE]: 26,
[VMSettingsField.PROVISION_SOURCE_TYPE]: 27,
[VMSettingsField.CONTAINER_IMAGE]: 28,
[VMSettingsField.IMAGE_URL]: 29,
[VMSettingsField.START_VM]: 30,
[VMSettingsField.MOUNT_WINDOWS_GUEST_TOOLS]: 23,
[VMSettingsField.FLAVOR]: 24,
[VMSettingsField.MEMORY]: 25,
[VMSettingsField.CPU]: 26,
[VMSettingsField.WORKLOAD_PROFILE]: 27,
[VMSettingsField.PROVISION_SOURCE_TYPE]: 28,
[VMSettingsField.CONTAINER_IMAGE]: 29,
[VMSettingsField.IMAGE_URL]: 30,
[VMSettingsField.START_VM]: 31,
};

const idResolver: RenderableFieldResolver = {
Expand All @@ -69,6 +70,7 @@ const idResolver: RenderableFieldResolver = {
[VMSettingsField.USER_TEMPLATE]: 'template-dropdown',
[VMSettingsField.OPERATING_SYSTEM]: 'operating-system-dropdown',
[VMSettingsField.CLONE_COMMON_BASE_DISK_IMAGE]: 'clone-common-base-image-checkbox',
[VMSettingsField.MOUNT_WINDOWS_GUEST_TOOLS]: 'mount-windows-guest-tools-checkbox',
[VMSettingsField.FLAVOR]: 'flavor-dropdown',
[VMSettingsField.MEMORY]: 'resources-memory',
[VMSettingsField.CPU]: 'resources-cpu',
Expand Down