Skip to content

Show only the allowed storage interfaces on Disk-modal #4028

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

Merged
openshift-merge-robot merged 1 commit into from
Jan 23, 2020
Merged

Show only the allowed storage interfaces on Disk-modal #4028

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions ...bevirt-plugin/src/components/create-vm-wizard/redux/validations/storage-tab-validation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import { getStoragesWithWrappers } from '../../selectors/selectors';
import { iGetStorages } from '../../selectors/immutable/storage';
import { iGetProvisionSource } from '../../selectors/immutable/vm-settings';
import { ProvisionSource } from '../../../../constants/vm/provision-source';
import { TemplateValidations } from '../../../../utils/validations/template/template-validations';
import { getTemplateValidation } from '../../selectors/template';

export const validateStorages = (options: UpdateOptions) => {
const { id, prevState, dispatch, getState } = options;
Expand All @@ -28,6 +30,8 @@ export const validateStorages = (options: UpdateOptions) => {
}

const storages = getStoragesWithWrappers(state, id);
const templateValidation = getTemplateValidation(state, id);
const allowedBusses = (templateValidation || new TemplateValidations()).getAllowedBusses();

const validatedStorages = storages.map(
({
Expand Down Expand Up @@ -56,6 +60,7 @@ export const validateStorages = (options: UpdateOptions) => {
{
usedDiskNames,
usedPVCNames,
allowedBusses,
},
),
};
Expand Down
6 changes: 4 additions & 2 deletions ...rt-plugin/src/components/create-vm-wizard/redux/validations/vm-settings-tab-validation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ import { vmSettingsOrder } from '../initial-state/vm-settings-tab-initial-state'
import { TemplateValidations } from '../../../../utils/validations/template/template-validations';
import { combineIntegerValidationResults } from '../../../../utils/validations/template/utils';
import { getValidationUpdate } from './utils';
import { getTemplateValidations } from './utils/templates-validations';
import { getTemplateValidations } from '../../selectors/template';

const validateVm: VmSettingsValidator = (field, options) => {
const { getState, id } = options;
Expand Down Expand Up @@ -103,8 +103,10 @@ const memoryValidation: VmSettingsValidator = (field, options): ValidationObject
if (memValueGB == null || memValueGB === '') {
return null;
}
const { id, getState } = options;
const state = getState();
const memValueBytes = memValueGB * 1024 ** 3;
const validations = getTemplateValidations(options);
const validations = getTemplateValidations(state, id);
if (validations.length === 0) {
validations.push(new TemplateValidations()); // add empty validation for positive integer if it is missing one
}
Expand Down
32 changes: 18 additions & 14 deletions ...alidations/utils/templates-validations.ts → ...ts/create-vm-wizard/selectors/template.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,26 +1,19 @@
import { UpdateOptions } from '../../types';
import {
iGetVmSettingAttribute,
iGetVmSettingValue,
} from '../../../selectors/immutable/vm-settings';
import { iGetLoadedCommonData } from '../../../selectors/immutable/selectors';
import { VMSettingsField, VMWizardProps } from '../../../types';
import { iGetTemplateValidations } from '../../../../../selectors/immutable/template/selectors';
import { iGetTemplateValidations } from '../../../selectors/immutable/template/selectors';
import { TemplateValidations } from '../../../utils/validations/template/template-validations';
import {
iGetRelevantTemplate,
iGetRelevantTemplates,
} from '../../../../../selectors/immutable/template/combined';
import { TemplateValidations } from '../../../../../utils/validations/template/template-validations';
} from '../../../selectors/immutable/template/combined';
import { VMSettingsField, VMWizardProps } from '../types';
import { iGetLoadedCommonData } from './immutable/selectors';
import { iGetVmSettingAttribute, iGetVmSettingValue } from './immutable/vm-settings';

const getValidationsFromTemplates = (templates): TemplateValidations[] =>
templates.map(
(relevantTemplate) => new TemplateValidations(iGetTemplateValidations(relevantTemplate)),
);

export const getTemplateValidations = (options: UpdateOptions): TemplateValidations[] => {
const { getState, id } = options;
const state = getState();

export const getTemplateValidations = (state, id: string): TemplateValidations[] => {
const userTemplateName = iGetVmSettingValue(state, id, VMSettingsField.USER_TEMPLATE);
const os = iGetVmSettingAttribute(state, id, VMSettingsField.OPERATING_SYSTEM);
const flavor = iGetVmSettingAttribute(state, id, VMSettingsField.FLAVOR);
Expand Down Expand Up @@ -54,3 +47,14 @@ export const getTemplateValidations = (options: UpdateOptions): TemplateValidati

return getValidationsFromTemplates(relevantTemplates.toArray());
};

export const getTemplateValidation = (state, id: string): TemplateValidations => {
const templateValidations = getTemplateValidations(state, id);
if (templateValidations && templateValidations.length > 0) {
templateValidations.length > 1 &&
console.warn('WARNING: getTemplateValidation: multiple template validations detected!');
Copy link
Contributor

@benjaminapetersen benjaminapetersen Jan 23, 2020

Choose a reason for hiding this comment

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

@suomiy @irosenzw
If this is an error that should be handled, ensure there is something in place, otherwise, if its something that is sufficiently handled with the console statement, then please add:

// eslint-disable-next-line no-console
console.warn('....');

Otherwise the errors appear every time we build the app.
@jcaianirh

Copy link
Member

@atiratree atiratree Jan 29, 2020

Choose a reason for hiding this comment

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

fixed in #3949

return templateValidations[0];
}

return null;
};
11 changes: 11 additions & 0 deletions ...gin/src/components/create-vm-wizard/tabs/storage-tab/vm-wizard-storage-modal-enhanced.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@ import { DataVolumeWrapper } from '../../../../k8s/wrapper/vm/data-volume-wrappe
import { DiskModal } from '../../../modals/disk-modal';
import { VM_TEMPLATE_NAME_PARAMETER } from '../../../../constants/vm-templates';
import { PersistentVolumeClaimWrapper } from '../../../../k8s/wrapper/vm/persistent-volume-claim-wrapper';
import { TemplateValidations } from '../../../../utils/validations/template/template-validations';
import { DiskBus } from '../../../../constants/vm/storage/disk-bus';
import { getTemplateValidation } from '../../selectors/template';

const VMWizardStorageModal: React.FC<VMWizardStorageModalProps> = (props) => {
const {
Expand All @@ -34,6 +37,7 @@ const VMWizardStorageModal: React.FC<VMWizardStorageModalProps> = (props) => {
useProjects,
addUpdateStorage,
storages,
templateValidations,
...restProps
} = props;
const {
Expand Down Expand Up @@ -80,6 +84,10 @@ const VMWizardStorageModal: React.FC<VMWizardStorageModalProps> = (props) => {
},
];

const allowedBusses: Set<DiskBus> = (
templateValidations || new TemplateValidations()
).getAllowedBusses();

return (
<Firehose resources={resources}>
<DiskModal
Expand All @@ -90,6 +98,7 @@ const VMWizardStorageModal: React.FC<VMWizardStorageModalProps> = (props) => {
onNamespaceChanged={(n) => setNamespace(n)}
usedDiskNames={usedDiskNames}
usedPVCNames={usedPVCNames}
allowedBusses={allowedBusses}
disk={diskWrapper}
volume={volumeWrapper}
dataVolume={dataVolumeWrapper}
Expand Down Expand Up @@ -139,6 +148,7 @@ type VMWizardStorageModalProps = ModalComponentProps & {
isCreateTemplate: boolean;
storages: VMWizardStorageWithWrappers[];
addUpdateStorage: (storage: VMWizardStorage) => void;
templateValidations: TemplateValidations;
};

const stateToProps = (state, { wizardReduxID }) => {
Expand All @@ -148,6 +158,7 @@ const stateToProps = (state, { wizardReduxID }) => {
namespace: iGetCommonData(state, wizardReduxID, VMWizardProps.activeNamespace),
isCreateTemplate: iGetCommonData(state, wizardReduxID, VMWizardProps.isCreateTemplate),
storages: getStoragesWithWrappers(state, wizardReduxID),
templateValidations: getTemplateValidation(state, wizardReduxID),
};
};

Expand Down
1 change: 1 addition & 0 deletions ...ubevirt-plugin/src/components/create-vm-wizard/tabs/storage-tab/vm-wizard-storage-row.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ export const VmWizardStorageRow: React.FC<VMWizardNicRowProps> = ({
validation={{
name: validations.name || validations.url || validations.container || validations.pvc,
size: validations.size,
diskInterface: validations.diskInterface,
}}
columnClasses={columnClasses}
index={index}
Expand Down
29 changes: 26 additions & 3 deletions frontend/packages/kubevirt-plugin/src/components/modals/disk-modal/disk-modal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,11 +69,13 @@ export const DiskModal = withHandlePromise((props: DiskModalProps) => {
handlePromise,
close,
cancel,
allowedBusses = new Set(DiskBus.getAll()),
} = props;
const asId = prefixedID.bind(null, 'disk');
const disk = props.disk || DiskWrapper.EMPTY;
const volume = props.volume || VolumeWrapper.EMPTY;
const dataVolume = props.dataVolume || DataVolumeWrapper.EMPTY;
const validAllowedBusses = allowedBusses || new Set(DiskBus.getAll());

const combinedDisk = new CombinedDisk({
diskWrapper: disk,
Expand Down Expand Up @@ -102,7 +104,12 @@ export const DiskModal = withHandlePromise((props: DiskModalProps) => {
disk.getName() || getSequenceName('disk', usedDiskNames),
);
const [bus, setBus] = React.useState<DiskBus>(
disk.getDiskBus() || (isEditing ? null : DiskBus.VIRTIO),
disk.getDiskBus() ||
(isEditing
? null
: validAllowedBusses.has(DiskBus.VIRTIO)
? DiskBus.VIRTIO
: [...validAllowedBusses][0]),
);
const [storageClassName, setStorageClassName] = React.useState<string>(
combinedDisk.getStorageClassName(),
Expand Down Expand Up @@ -170,13 +177,15 @@ export const DiskModal = withHandlePromise((props: DiskModalProps) => {
size: sizeValidation,
container: containerValidation,
pvc: pvcValidation,
diskInterface: busValidation,
url: urlValidation,
},
isValid,
hasAllRequiredFilled,
} = validateDisk(resultDisk, resultVolume, resultDataVolume, resultPersistentVolumeClaim, {
usedDiskNames,
usedPVCNames,
allowedBusses,
});

const [showUIError, setShowUIError] = useShowErrorToggler(false, isValid, isValid);
Expand Down Expand Up @@ -370,17 +379,30 @@ export const DiskModal = withHandlePromise((props: DiskModalProps) => {
/>
</FormRow>
)}
<FormRow title="Interface" fieldId={asId('interface')} isRequired>
<FormRow
title="Interface"
fieldId={asId('interface')}
isRequired
validation={busValidation}
>
<FormSelect
onChange={React.useCallback((diskBus) => setBus(DiskBus.fromString(diskBus)), [
Copy link
Member

@atiratree atiratree Jan 22, 2020

Choose a reason for hiding this comment

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

please pass validation to FormRow as it is not visible

setBus,
])}
isValid={!isValidationError(busValidation)}
value={asFormSelectValue(bus)}
id={asId('interface')}
isDisabled={inProgress}
>
<FormSelectPlaceholderOption isDisabled placeholder="--- Select Interface ---" />
{DiskBus.getAll().map((b) => (
{!validAllowedBusses.has(bus) && (
<FormSelectOption
key={bus.getValue()}
value={bus.getValue()}
label={`${bus.toString()} --- Invalid ---`}
Copy link
Member

@atiratree atiratree Jan 23, 2020

Choose a reason for hiding this comment

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

Ok, I suppose the user should know it is not selectable

We could try to make the disabled work in the future

/>
)}
{[...validAllowedBusses].map((b) => (
<FormSelectOption key={b.getValue()} value={b.getValue()} label={b.toString()} />
))}
</FormSelect>
Expand Down Expand Up @@ -444,6 +466,7 @@ export type DiskModalProps = {
vmNamespace: string;
namespace: string;
onNamespaceChanged: (namespace: string) => void;
allowedBusses?: Set<DiskBus>;
usedDiskNames: Set<string>;
usedPVCNames: Set<string>;
} & ModalComponentProps &
Expand Down
14 changes: 14 additions & 0 deletions frontend/packages/kubevirt-plugin/src/utils/validations/common.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,11 @@ import {
validateEmptyValue,
ValidationErrorType,
ValidationObject,
joinGrammaticallyListOfItems,
} from '@console/shared';
import { parseURL } from '../url';
import { END_WHITESPACE_ERROR, START_WHITESPACE_ERROR, URL_INVALID_ERROR } from './strings';
import { DiskBus } from '../../constants/vm/storage/disk-bus';

export const isValidationError = (validationObject: ValidationObject) =>
!!validationObject && validationObject.type === ValidationErrorType.Error;
Expand Down Expand Up @@ -81,3 +83,15 @@ export const validateURL = (

return parseURL(value) ? null : asValidationObject(addMissingSubject(URL_INVALID_ERROR, subject));
};

export const validateBus = (value: DiskBus, allowedBusses: Set<DiskBus>): ValidationObject => {
if (allowedBusses && !allowedBusses.has(value)) {
return asValidationObject(
`Invalid interface type. Valid types are: ${joinGrammaticallyListOfItems(
[...allowedBusses].map((b) => b.toString()),
)}`,
ValidationErrorType.Error,
);
}
return null;
};
2 changes: 1 addition & 1 deletion ...end/packages/kubevirt-plugin/src/utils/validations/template/interval-validation-result.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ export class MemoryIntervalValidationResult extends IntervalValidationResult {
}`;
}
if (Number.isFinite(this.max)) {
return `Memory ${verb} be ${this.isMaxInclusive ? 'at most' : 'bellow'} ${
return `Memory ${verb} be ${this.isMaxInclusive ? 'at most' : 'below'} ${
humanizeBinaryBytes(this.max).string
}`;
}
Expand Down
25 changes: 24 additions & 1 deletion frontend/packages/kubevirt-plugin/src/utils/validations/template/template-validations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/* eslint-disable lines-between-class-members */
import * as _ from 'lodash';
import { ValidationErrorType } from '@console/shared/src';
import { ValueEnum } from '../../../constants';
import { ValueEnum, DiskBus } from '../../../constants';
import { CommonTemplatesValidation } from '../../../types/template';
import {
IntervalValidationResult,
Expand All @@ -14,6 +14,7 @@ export class ValidationJSONPath extends ValueEnum<string> {
static readonly MEMORY = new ValidationJSONPath(
'jsonpath::.spec.domain.resources.requests.memory',
);
static readonly BUS = new ValidationJSONPath('jsonpath::.spec.domain.devices.disks[*].disk.bus');
}

export class TemplateValidations {
Expand Down Expand Up @@ -110,6 +111,28 @@ export class TemplateValidations {
});
};

getAllowedBusses = (): Set<DiskBus> => {
const allowedBusses = this.getAllowedEnumValues(
ValidationJSONPath.BUS,
ValidationErrorType.Error,
).map(DiskBus.fromString);

return new Set(allowedBusses.length === 0 ? DiskBus.getAll() : allowedBusses);
};

// Empty array means all values are allowed
private getAllowedEnumValues = (
jsonPath: ValidationJSONPath,
type: ValidationErrorType,
): string[] => {
const relevantValidations = this.getRelevantValidations(jsonPath, type);

return relevantValidations.reduce(
(result: string[], validation: CommonTemplatesValidation) => result.concat(validation.values),
[],
);
};

private getRelevantValidations = (jsonPath: ValidationJSONPath, type: ValidationErrorType) => {
return this.validations.filter(
(validation: CommonTemplatesValidation) =>
Expand Down
15 changes: 13 additions & 2 deletions frontend/packages/kubevirt-plugin/src/utils/validations/vm/disk.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,16 @@ import {
ValidationErrorType,
ValidationObject,
} from '@console/shared';
import { validateTrim, validateURL } from '../common';
import { validateTrim, validateURL, validateBus } from '../common';
import { DiskWrapper } from '../../../k8s/wrapper/vm/disk-wrapper';
import { VolumeWrapper } from '../../../k8s/wrapper/vm/volume-wrapper';
import { DataVolumeWrapper } from '../../../k8s/wrapper/vm/data-volume-wrapper';
import { POSITIVE_SIZE_ERROR } from '../strings';
import { StorageUISource } from '../../../components/modals/disk-modal/storage-ui-source';
import { CombinedDisk } from '../../../k8s/wrapper/vm/combined-disk';
import { PersistentVolumeClaimWrapper } from '../../../k8s/wrapper/vm/persistent-volume-claim-wrapper';
import { DiskBus } from '../../../constants/vm/storage/disk-bus';
import { DiskType } from '../../../constants/vm/storage/disk-type';

const validateDiskName = (name: string, usedDiskNames: Set<string>): ValidationObject => {
let validation = validateDNS1123SubdomainValue(name);
Expand Down Expand Up @@ -51,16 +53,19 @@ export const validateDisk = (
{
usedDiskNames,
usedPVCNames,
allowedBusses,
}: {
usedDiskNames?: Set<string>;
usedPVCNames?: Set<string>;
allowedBusses: Set<DiskBus>;
},
): UIDiskValidation => {
const validations = {
name: validateDiskName(disk && disk.getName(), usedDiskNames),
size: null,
url: null,
container: null,
diskInterface: null,
pvc: null,
};
let hasAllRequiredFilled = disk && disk.getName();
Expand Down Expand Up @@ -129,8 +134,13 @@ export const validateDisk = (
addRequired(pvcName);
validations.pvc = validatePVCName(pvcName, usedPVCNames);
}
}

// TODO: implement CDROM disk bus validation
Copy link
Member

@atiratree atiratree Jan 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I meant, lets revert it to the same state it was before. Thumbs up for the TODO.

Suggested change
// TODO: implement CDROM disk bus validation
// TODO: implement CDROM disk bus validation
if (disk.getType() === DiskType.DISK) {
addRequired(allowedBusses.has(disk.getDiskBus()));
validations.diskInterface = validateBus(disk.getDiskBus(), allowedBusses);
}

if (disk.getType() === DiskType.DISK) {
Copy link
Member

@atiratree atiratree Jan 22, 2020

Choose a reason for hiding this comment

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

I think we can skip this if, as all disk types require bus

Copy link
Member

@atiratree atiratree Jan 22, 2020

Choose a reason for hiding this comment

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

we probably have to deffer this one to a followup if we cannot make CDs support into this PR on time.

addRequired(disk.getDiskBus());
validations.diskInterface = validateBus(disk.getDiskBus(), allowedBusses);
}
}
return {
validations,
hasAllRequiredFilled: !!hasAllRequiredFilled,
Expand All @@ -144,6 +154,7 @@ export type UIDiskValidation = {
size?: ValidationObject;
url?: ValidationObject;
container?: ValidationObject;
diskInterface?: ValidationObject;
pvc?: ValidationObject;
};
isValid: boolean;
Expand Down