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 2 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
45 changes: 36 additions & 9 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
Expand Up @@ -153,22 +153,23 @@ const osUpdater = ({ id, prevState, dispatch, getState }: UpdateOptions) => {
if (iGetCommonData(state, id, VMWizardProps.isProviderImport)) {
return;
}
if (iGetCommonData(state, id, VMWizardProps.isProviderImport)) {
Copy link
Member

Choose a reason for hiding this comment

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

twice?

Copy link
Member Author

Choose a reason for hiding this comment

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

wanted to be extra sure :-) , removed 👍

return;
}
if (!hasVMSettingsValueChanged(prevState, state, id, VMSettingsField.OPERATING_SYSTEM)) {
return;
}

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 Expand Up @@ -211,6 +212,31 @@ const baseImageUpdater = ({ id, prevState, dispatch, getState }: UpdateOptions)
);
};

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.

looks good, but we should move this updater to storage-tab-state-update.ts so the relevant will be together

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 storage 👍

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));
}
};

const cloneCommonBaseDiskImageUpdater = ({ id, prevState, dispatch, getState }: UpdateOptions) => {
const state = getState();
if (iGetCommonData(state, id, VMWizardProps.isProviderImport)) {
Expand Down Expand Up @@ -305,6 +331,7 @@ export const updateVmSettingsState = (options: UpdateOptions) =>
osUpdater,
baseImageUpdater,
cloneCommonBaseDiskImageUpdater,
windowsToolsUpdater,
workloadConsistencyUpdater,
provisioningSourceUpdater,
nativeK8sUpdater,
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
44 changes: 30 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 @@ -48,6 +48,7 @@ export const OSFlavor: React.FC<OSFlavorProps> = React.memo(
userTemplate,
operatinSystemField,
cloneBaseDiskImageField,
mountWindowsGuestToolsField,
flavorField,
workloadProfile,
cnvBaseImages,
Expand All @@ -60,6 +61,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 @@ -184,21 +186,34 @@ 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>
{(cloneBaseDiskImage || mountWindowsGuestTools) && (
Copy link
Member

Choose a reason for hiding this comment

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

not having the Virtualized HW step simplifies things a bit :)

<Text className="kv-create-vm__input-checkbox">
Copy link
Member

Choose a reason for hiding this comment

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

i know it works, but the name does not reflect the reality :)

either

  • make more generic name
  • make 2nd class

Copy link
Member Author

Choose a reason for hiding this comment

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

:-) yea

Copy link
Member Author

Choose a reason for hiding this comment

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

changed to kv-create-vm__input-text-help-msg 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

any reason you can't use pf-c-form__helper-text here instead?

View the mounted disk 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 probably should pluralize this depending on the number of checkboxes checked

Copy link
Member Author

Choose a reason for hiding this comment

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

pluralized 🥞

<Button
isDisabled={!goToStorageStep}
isInline
onClick={goToStorageStep}
variant={ButtonVariant.link}
>
<strong>storage</strong>
</Button>{' '}
step
</Text>
)}
<FormFieldRow
field={flavorField}
fieldType={FormFieldType.SELECT}
Expand Down Expand Up @@ -228,6 +243,7 @@ type OSFlavorProps = {
flavorField: any;
operatinSystemField: any;
cloneBaseDiskImageField: any;
mountWindowsGuestToolsField: any;
userTemplate: string;
workloadProfile: string;
cnvBaseImages: any;
Expand Down
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