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 1870114: Updated template PVC name and namespace to use parameters #6428

Merged
merged 4 commits into from
Aug 26, 2020
Merged

Bug 1870114: Updated template PVC name and namespace to use parameters #6428

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
adb17c3
updated template PVC name and namespace to use parameters
yaacov Aug 25, 2020
a5f557c
rename getParameter getParamterValue
yaacov Aug 26, 2020
7db54ae
move devel logic into the compare method
yaacov Aug 26, 2020
f285fee
change os disk image on flavor/wl change
yaacov Aug 26, 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
3 changes: 2 additions & 1 deletion frontend/packages/kubevirt-plugin/src/components/create-vm-wizard/create-vm-wizard.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
TEMPLATE_TYPE_VM,
VMWizardMode,
VMWizardView,
TEMPLATE_VM_GOLDEN_OS_NAMESPACE,
} from '../../constants/vm';
import { getResource } from '../../utils';
import { IDReferences, makeIDReferences } from '../../utils/redux/id-reference';
Expand Down Expand Up @@ -386,7 +387,7 @@ export const CreateVMWizardPageComponent: React.FC<CreateVMWizardPageComponentPr
matchLabels: { [TEMPLATE_TYPE_LABEL]: TEMPLATE_TYPE_BASE },
}),
getResource(PersistentVolumeClaimModel, {
namespace: 'openshift-cnv-base-images',
namespace: TEMPLATE_VM_GOLDEN_OS_NAMESPACE,
prop: VMWizardProps.openshiftCNVBaseImages,
}),
);
Expand Down
15 changes: 8 additions & 7 deletions ...t-plugin/src/components/create-vm-wizard/redux/initial-state/storage-tab-initial-state.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,15 +25,19 @@ import {
} from '../../../../selectors/config-map/sc-defaults';
import { toShallowJS, iGetIn } from '../../../../utils/immutable';
import { generateDataVolumeName } from '../../../../utils';
import { DUMMY_VM_NAME, TEMPLATE_DATAVOLUME_ANNOTATION } from '../../../../constants/vm';
import {
DUMMY_VM_NAME,
TEMPLATE_DATAVOLUME_NAME_PARAMETER,
TEMPLATE_DATAVOLUME_NAMESPACE_PARAMETER,
} from '../../../../constants/vm';
import {
iGetVmSettingValue,
iGetProvisionSource,
iGetRelevantTemplateSelectors,
} from '../../selectors/immutable/vm-settings';
import { iGetLoadedCommonData, iGetName } from '../../selectors/immutable/selectors';
import { iGetRelevantTemplate } from '../../../../selectors/immutable/template/combined';
import { iGetAnnotation } from '../../../../selectors/immutable/common';
import { iGetPrameterValue } from '../../../../selectors/immutable/common';

const ROOT_DISK_NAME = 'rootdisk';
const WINTOOLS_DISK_NAME = 'windows-guest-tools';
Expand Down Expand Up @@ -186,11 +190,8 @@ export const getNewProvisionSourceStorage = (state: any, id: string): VMWizardSt
const iCommonTemplates = iGetLoadedCommonData(state, id, VMWizardProps.commonTemplates);
const iTemplate =
iCommonTemplates && iGetRelevantTemplate(null, iCommonTemplates, relevantOptions);
const pvcName = iGetAnnotation(
iTemplate,
`${TEMPLATE_DATAVOLUME_ANNOTATION}/${relevantOptions.os}`,
);
const pvcNamespace = iGetAnnotation(iTemplate, `${TEMPLATE_DATAVOLUME_ANNOTATION}/namespace`);
const pvcName = iGetPrameterValue(iTemplate, TEMPLATE_DATAVOLUME_NAME_PARAMETER);
const pvcNamespace = iGetPrameterValue(iTemplate, TEMPLATE_DATAVOLUME_NAMESPACE_PARAMETER);

const iBaseImage = iGetLoadedCommonData(state, id, VMWizardProps.openshiftCNVBaseImages)
.valueSeq()
Expand Down
21 changes: 14 additions & 7 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 @@ -15,12 +15,17 @@ import {
iGetCommonData,
iGetLoadedCommonData,
iGetName,
iGetNamespace,
} from '../../selectors/immutable/selectors';
import { iGetRelevantTemplate } from '../../../../selectors/immutable/template/combined';
import { CUSTOM_FLAVOR, TEMPLATE_DATAVOLUME_ANNOTATION } from '../../../../constants/vm';
import {
CUSTOM_FLAVOR,
TEMPLATE_DATAVOLUME_NAME_PARAMETER,
TEMPLATE_DATAVOLUME_NAMESPACE_PARAMETER,
} from '../../../../constants/vm';
import { ProvisionSource } from '../../../../constants/vm/provision-source';
import { prefillVmTemplateUpdater } from './prefill-vm-template-state-update';
import { iGetAnnotation } from '../../../../selectors/immutable/common';
import { iGetPrameterValue } from '../../../../selectors/immutable/common';

const selectUserTemplateOnLoadedUpdater = (options: UpdateOptions) => {
const { id, dispatch, getState } = options;
Expand Down Expand Up @@ -129,14 +134,16 @@ const baseImageUpdater = ({ id, prevState, dispatch, getState }: UpdateOptions)
const iCommonTemplates = iGetLoadedCommonData(state, id, VMWizardProps.commonTemplates);
const iTemplate =
iCommonTemplates && iGetRelevantTemplate(null, iCommonTemplates, relevantOptions);
Copy link
Member

Choose a reason for hiding this comment

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

can you please include flavor and workloadProfile in hasVMSettingsValueChanged so we always get relevant template here?

Copy link
Member Author

Choose a reason for hiding this comment

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

ok

p.s.
the spec says that os disk image should not change on cpu/mem or orher none os related changes,
but since we have this problem all over the code, so we can fix it in a folow-up.

Copy link
Member

Choose a reason for hiding this comment

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

it is not an problem as this not about the spec. I'd be glad if our code worked also on a broken/customized env with different templates of different versions with different base images for different oses, if it doesn't cost us that much. Just using these selectors properly with all the relevant info.

const pvcName = iGetAnnotation(
iTemplate,
`${TEMPLATE_DATAVOLUME_ANNOTATION}/${relevantOptions?.os}`,
);
const pvcName = iGetPrameterValue(iTemplate, TEMPLATE_DATAVOLUME_NAME_PARAMETER);
const pvcNamespace = iGetPrameterValue(iTemplate, TEMPLATE_DATAVOLUME_NAMESPACE_PARAMETER);

const iBaseImages = iGetLoadedCommonData(state, id, VMWizardProps.openshiftCNVBaseImages);
iBaseImage =
pvcName && iBaseImages && iBaseImages.valueSeq().find((iPVC) => iGetName(iPVC) === pvcName);
pvcName &&
iBaseImages &&
iBaseImages
.valueSeq()
.find((iPVC) => iGetName(iPVC) === pvcName && iGetNamespace(iPVC) === pvcNamespace);
}

dispatch(
Expand Down
7 changes: 5 additions & 2 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 @@ -32,7 +32,7 @@ import { getPlaceholder, getFieldId } from '../../utils/renderable-field-utils';
import { nullOnEmptyChange } from '../../utils/utils';
import { operatingSystemsNative } from '../../../../constants/vm-templates/os';
import { OperatingSystemRecord } from '../../../../types';
import { iGetName } from '../../selectors/immutable/selectors';
import { iGetName, iGetNamespace } from '../../selectors/immutable/selectors';
import {
BASE_IMAGE_AND_PVC_SHORT,
BASE_IMAGE_AND_PVC_MESSAGE,
Expand Down Expand Up @@ -120,7 +120,10 @@ export const OSFlavor: React.FC<OSFlavorProps> = React.memo(
const operatingSystemBaseImages = operatingSystems.map(
(operatingSystem: OperatingSystemRecord) => {
const pvcName = operatingSystem?.dataVolumeName;
const baseImageFoundInCluster = loadedBaseImages?.find((pvc) => iGetName(pvc) === pvcName);
const pvcNamespace = operatingSystem?.dataVolumeNamespace;
const baseImageFoundInCluster = loadedBaseImages?.find(
(pvc) => iGetName(pvc) === pvcName && iGetNamespace(pvc) === pvcNamespace,
);
const osField = {
id: operatingSystem.id,
name: operatingSystem.name,
Expand Down
5 changes: 3 additions & 2 deletions frontend/packages/kubevirt-plugin/src/constants/vm/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,11 @@ export const TEMPLATE_TYPE_BASE = 'base';
export const TEMPLATE_WORKLOAD_LABEL = 'workload.template.kubevirt.io';
export const TEMPLATE_VM_NAME_LABEL = 'vm.kubevirt.io/name';
export const TEMPLATE_OS_NAME_ANNOTATION = 'name.os.template.kubevirt.io';
export const TEMPLATE_DATAVOLUME_ANNOTATION = 'datavolume.template.kubevirt.io';
export const TEMPLATE_DATAVOLUME_NAME_PARAMETER = 'SRC_PVC_NAME';
export const TEMPLATE_DATAVOLUME_NAMESPACE_PARAMETER = 'SRC_PVC_NAMESPACE';
export const TEMPLATE_VM_DOMAIN_LABEL = 'kubevirt.io/domain';
export const TEMPLATE_VM_SIZE_LABEL = 'kubevirt.io/size';
export const TEMPLATE_VM_GOLDEN_OS_NAMESPACE = 'openshift-cnv-base-images';
export const TEMPLATE_VM_GOLDEN_OS_NAMESPACE = 'openshift-kubevirt-base-images';
export const TEMPLATE_VM_COMMON_NAMESPACE = 'openshift';

export const LABEL_USED_TEMPLATE_NAME = 'vm.kubevirt.io/template';
Expand Down
9 changes: 7 additions & 2 deletions frontend/packages/kubevirt-plugin/src/selectors/immutable/common.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,10 @@ import { ILabels } from '../../types/template';
export const iGetLabels = (obj): ILabels => iGetIn(obj, ['metadata', 'labels']);
export const iGetCreationTimestamp = (obj): string =>
iGetIn(obj, ['metadata', 'creationTimestamp']);
export const iGetAnnotation = (obj, key: string, defaultValue = undefined): string =>
iGetIn(obj, ['metadata', 'annotations', key], defaultValue);
export const iGetPrameterValue = (obj, name: string, defaultValue = null): any => {
const parameter = iGetIn(obj, ['parameters'])
?.valueSeq()
.find((p) => iGetIn(p, ['name']) === name);

return iGetIn(parameter, ['value'], defaultValue);
};
7 changes: 7 additions & 0 deletions frontend/packages/kubevirt-plugin/src/selectors/immutable/template/combined.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,13 @@ export const iGetRelevantTemplates = (
const versionCMP = compareVersions(splitVersion(aVersion), splitVersion(bVersion)) * -1; // descending

if (versionCMP !== 0) {
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 move this logic to compareVersions and also add it to the unit test?

Copy link
Member Author

Choose a reason for hiding this comment

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

done 👍

// 'devel' version if exist is always the highst version.
if (aVersion === 'devel') {
return -1;
}
if (bVersion === 'devel') {
return 1;
}
return versionCMP;
}

Expand Down
5 changes: 4 additions & 1 deletion frontend/packages/kubevirt-plugin/src/selectors/selectors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import * as _ from 'lodash';
import { K8sResourceKind } from '@console/internal/module/k8s';
import { K8sResourceKind, TemplateParameter } from '@console/internal/module/k8s';
import { VMGenericLikeEntityKind } from '../types/vmLike';

export const getKind = (value) => _.get(value, 'kind') as K8sResourceKind['kind'];
Expand Down Expand Up @@ -36,6 +36,9 @@ export const getAnnotation = (
defaultValue?: string,
): string => _.get(entity, ['metadata', 'annotations', annotationName], defaultValue);

export const getParameter = (obj, name: string, defaultValue = {}): TemplateParameter =>
_.get(obj, ['parameters'], []).find((parameter) => parameter.name === name) || defaultValue;

export const getAnnotationKeySuffix = (
entity: K8sResourceKind,
annotationPrefix: string,
Expand Down
15 changes: 7 additions & 8 deletions frontend/packages/kubevirt-plugin/src/selectors/vm-template/advanced.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,18 @@ import {
CloudInitDataHelper,
CloudInitDataFormKeys,
} from '../../k8s/wrapper/vm/cloud-init-data-helper';
import { getAnnotation, getAnnotations, getLabels } from '../selectors';
import { getAnnotation, getAnnotations, getLabels, getParameter } from '../selectors';
import {
TEMPLATE_FLAVOR_LABEL,
TEMPLATE_OS_LABEL,
TEMPLATE_OS_NAME_ANNOTATION,
TEMPLATE_TYPE_LABEL,
TEMPLATE_TYPE_VM,
TEMPLATE_WORKLOAD_LABEL,
TEMPLATE_DATAVOLUME_NAME_PARAMETER,
TEMPLATE_DATAVOLUME_NAMESPACE_PARAMETER,
} from '../../constants/vm';
import {
getCloudInitVolume,
getOperatingSystemDataVolumeAnnotation,
getOperatingSystemDataVolumeNamespaceAnnotation,
} from '../vm/selectors';
import { getCloudInitVolume } from '../vm/selectors';
import { VolumeWrapper } from '../../k8s/wrapper/vm/volume-wrapper';
import { selectVM } from './basic';
import { removeOSDups } from '../../utils/sort';
Expand Down Expand Up @@ -83,11 +81,12 @@ export const getTemplateOperatingSystems = (templates: TemplateKind[]) => {
(t) =>
!!Object.keys(getAnnotations(t, {})).find((annotation) => annotation === nameAnnotation),
);

return {
id: osId,
name: getAnnotation(template, nameAnnotation),
dataVolumeName: getOperatingSystemDataVolumeAnnotation(template, osId),
dataVolumeNamespace: getOperatingSystemDataVolumeNamespaceAnnotation(template),
dataVolumeName: getParameter(template, TEMPLATE_DATAVOLUME_NAME_PARAMETER)?.value,
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 have similar selectors and change it to getPrameterValue?

Copy link
Member Author

Choose a reason for hiding this comment

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

done :-)

dataVolumeNamespace: getParameter(template, TEMPLATE_DATAVOLUME_NAMESPACE_PARAMETER)?.value,
};
}),
);
Expand Down
13 changes: 0 additions & 13 deletions frontend/packages/kubevirt-plugin/src/selectors/vm/selectors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import * as _ from 'lodash';
import { getName } from '@console/shared/src/selectors/common';
import { createBasicLookup } from '@console/shared/src/utils/utils';
import {
TEMPLATE_DATAVOLUME_ANNOTATION,
TEMPLATE_FLAVOR_LABEL,
TEMPLATE_OS_LABEL,
TEMPLATE_OS_NAME_ANNOTATION,
Expand Down Expand Up @@ -182,15 +181,3 @@ export const getIsGraphicsConsoleAttached = (vm: VMKind) =>

export const getIsSerialConsoleAttached = (vm: VMKind) =>
vm?.spec?.template?.spec?.domain?.devices?.autoattachSerialConsole;

export const getOperatingSystemDataVolumeAnnotation = (
vmLike: VMGenericLikeEntityKind,
osId?: string,
) =>
getValueByPrefix(
getAnnotations(vmLike),
`${TEMPLATE_DATAVOLUME_ANNOTATION}/${osId || getOperatingSystem(vmLike)}`,
);

export const getOperatingSystemDataVolumeNamespaceAnnotation = (vmLike: VMGenericLikeEntityKind) =>
getValueByPrefix(getAnnotations(vmLike), `${TEMPLATE_DATAVOLUME_ANNOTATION}/namespace`);