Skip to content

[release-4.18] OCPBUGS-56757: HorizontalPodAutoscalers via console require CPU resource limit even though HPAs use requests for scaling #15093

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

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 1 addition & 6 deletions frontend/packages/dev-console/locales/en/devconsole.json
Original file line number Diff line number Diff line change
Expand Up @@ -342,13 +342,9 @@
"Path": "Path",
"argument": "argument",
"The command to run inside the Container.": "The command to run inside the Container.",
"CPU and memory resource limits must be set if you want to use CPU and memory utilization. The HorizontalPodAutoscaler will not have CPU or memory metrics until resource limits are set.": "CPU and memory resource limits must be set if you want to use CPU and memory utilization. The HorizontalPodAutoscaler will not have CPU or memory metrics until resource limits are set.",
"CPU and memory resource requests must be set if you want to use CPU and memory utilization. The HorizontalPodAutoscaler will not have CPU or memory metrics until resource requests are set.": "CPU and memory resource requests must be set if you want to use CPU and memory utilization. The HorizontalPodAutoscaler will not have CPU or memory metrics until resource requests are set.",
"CPU resource limits must be set if you want to use CPU utilization. The HorizontalPodAutoscaler will not have CPU metrics until resource limits are set.": "CPU resource limits must be set if you want to use CPU utilization. The HorizontalPodAutoscaler will not have CPU metrics until resource limits are set.",
"Memory resource limits must be set if you want to use memory utilization. The HorizontalPodAutoscaler will not have memory metrics until resource limits are set.": "Memory resource limits must be set if you want to use memory utilization. The HorizontalPodAutoscaler will not have memory metrics until resource limits are set.",
"Cannot create a HorizontalPodAutoscaler with no valid metrics.": "Cannot create a HorizontalPodAutoscaler with no valid metrics.",
"CPU and memory utilization cannot be used currently.": "CPU and memory utilization cannot be used currently.",
"CPU utilization cannot be used currently.": "CPU utilization cannot be used currently.",
"Memory utilization cannot be used currently.": "Memory utilization cannot be used currently.",
"Note: Some fields may not be represented in this form view. Please select \"YAML view\" for full control.": "Note: Some fields may not be represented in this form view. Please select \"YAML view\" for full control.",
"Minimum Pods": "Minimum Pods",
"Maximum Pods": "Maximum Pods",
Expand All @@ -368,7 +364,6 @@
"Maximum Pods must be an integer.": "Maximum Pods must be an integer.",
"Maximum Pods should be greater than or equal to Minimum Pods.": "Maximum Pods should be greater than or equal to Minimum Pods.",
"Max Pods must be defined.": "Max Pods must be defined.",
"Average utilization must be a positive number.": "Average utilization must be a positive number.",
"Health checks": "Health checks",
"Resource limits": "Resource limits",
"Labels": "Labels",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ const HPADetailsForm: React.FC = () => {
const {
setFieldValue,
values: {
disabledFields: { name: nameDisabled, cpuUtilization, memoryUtilization },
disabledFields: { name: nameDisabled },
showCanUseYAMLMessage,
},
} = useFormikContext<HPAFormValues>();
Expand Down Expand Up @@ -84,14 +84,12 @@ const HPADetailsForm: React.FC = () => {
setOutputAsIntegerFlag
/>
<HPAUtilizationField
disabled={cpuUtilization}
hpa={field.value}
label={t('devconsole~CPU')}
onUpdate={updateField('cpu')}
type="cpu"
/>
<HPAUtilizationField
disabled={memoryUtilization}
hpa={field.value}
label={t('devconsole~Memory')}
onUpdate={updateField('memory')}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import { EditorType } from '@console/shared/src/components/synced-editor/editor-
import { safeYAMLToJS } from '@console/shared/src/utils/yaml';
import {
getFormData,
getInvalidUsageError,
getYAMLData,
hasCustomMetrics,
isCpuUtilizationPossible,
Expand Down Expand Up @@ -50,12 +49,6 @@ const HPAFormikForm: React.FC<HPAFormikFormProps> = ({ existingHPA, targetResour
values.editorType === EditorType.YAML ? safeYAMLToJS(values.yamlData) : values.formData,
);

const invalidUsageError = getInvalidUsageError(hpa, values);
if (invalidUsageError) {
helpers.setStatus({ submitError: invalidUsageError });
return Promise.resolve();
}

const method: (
kind: K8sKind,
data: HorizontalPodAutoscalerKind,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import { cloneDeep } from 'lodash';
import { DeploymentKind, HorizontalPodAutoscalerKind } from '@console/internal/module/k8s';
import {
getFormData,
getInvalidUsageError,
getLimitWarning,
getMetricByType,
getYAMLData,
Expand All @@ -12,7 +11,6 @@ import {
sanitizeHPAToForm,
sanityForSubmit,
} from '../hpa-utils';
import { HPAFormValues } from '../types';
import { deploymentConfigExamples, deploymentExamples, hpaExamples } from './hpa-utils-data';

describe('isCpuUtilizationPossible provides accurate checks', () => {
Expand Down Expand Up @@ -144,30 +142,28 @@ describe('getYAMLData gets back an hpa structured editor string', () => {

describe('getMetricByType returns an appropriate metric and the index it is at', () => {
it('expect no metrics to return a default metric as a new metric on the end', () => {
const { metric, index } = getMetricByType(hpaExamples.noMetrics, 'memory');
const { metric } = getMetricByType(hpaExamples.noMetrics, 'memory');
expect(metric).toBeTruthy();
expect(metric.resource.name).toBe('memory');
expect(metric.resource.target.averageUtilization).toBe(0);
expect(index).toBe(0);
expect(metric.resource.target.averageUtilization).toBe(50);
});

it('expect to get back a default memory metric when only cpu metric is available', () => {
const { metric, index } = getMetricByType(hpaExamples.cpuScaled, 'memory');
expect(metric).toBeTruthy();
expect(metric.resource.name).toBe('memory');
expect(metric.resource.target.averageUtilization).toBe(0);
expect(metric.resource.target.averageUtilization).toBe(50);
expect(index).toBe(1);
});

it('expect to get back the cpu metric when it is available', () => {
const hpaResource: HorizontalPodAutoscalerKind = hpaExamples.cpuScaled;
const { metric, index } = getMetricByType(hpaResource, 'cpu');
const { metric } = getMetricByType(hpaResource, 'cpu');
expect(metric).toBeTruthy();
expect(metric.resource.name).toBe('cpu');
expect(metric.resource.target.averageUtilization).toBe(
hpaResource.spec.metrics[0].resource.target.averageUtilization,
);
expect(index).toBe(0);
});
});

Expand Down Expand Up @@ -234,8 +230,8 @@ describe('sanityForSubmit covers some basic field locking and trimming', () => {

it('expect not to have empty cpu or memory metrics', () => {
const overriddenResource: HorizontalPodAutoscalerKind = cloneDeep(hpaResource);
overriddenResource.spec.metrics[0].resource.target.averageUtilization = 0;
expect(sanityForSubmit(deploymentResource, overriddenResource).spec.metrics.length).toBe(0);
overriddenResource.spec.metrics[0].resource.target.averageUtilization = 50;
expect(sanityForSubmit(deploymentResource, overriddenResource).spec.metrics.length).toBe(1);
});

it('expect not to trim custom resource metrics', () => {
Expand All @@ -246,40 +242,6 @@ describe('sanityForSubmit covers some basic field locking and trimming', () => {
});
});

describe('getInvalidUsageError returns an error string when limits are not set', () => {
const formValues: HPAFormValues = {
showCanUseYAMLMessage: false,
disabledFields: {
name: false,
cpuUtilization: true,
memoryUtilization: true,
},
editorType: null,
formData: null,
yamlData: null,
};
const hpaResource: HorizontalPodAutoscalerKind = hpaExamples.cpuScaled;

it('expect no metrics to be an error', () => {
const noMetricsHPA: HorizontalPodAutoscalerKind = hpaExamples.noMetrics;
expect(typeof getInvalidUsageError(noMetricsHPA, formValues)).toBe('string');

const emptyMetricsHPA: HorizontalPodAutoscalerKind = cloneDeep(noMetricsHPA);
emptyMetricsHPA.spec.metrics = [];
expect(typeof getInvalidUsageError(emptyMetricsHPA, formValues)).toBe('string');
});

it('expect cpu metric not to be allowed while disabled', () => {
expect(typeof getInvalidUsageError(hpaResource, formValues)).toBe('string');
});

it('expect memory metric to not be allowed while disabled', () => {
const memoryHPA = cloneDeep(hpaResource);
memoryHPA.spec.metrics[0].resource.name = 'memory';
expect(typeof getInvalidUsageError(memoryHPA, formValues)).toBe('string');
});
});

describe('hasCustomMetrics accurately determines if an hpa contains non-default metrics', () => {
it('expect no metrics to mean no custom metrics', () => {
expect(hasCustomMetrics(null)).toBe(false);
Expand Down
42 changes: 3 additions & 39 deletions frontend/packages/dev-console/src/components/hpa/hpa-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {
} from '@console/internal/module/k8s';
import { LimitsData } from '@console/shared/src';
import { safeJSToYAML, safeYAMLToJS } from '@console/shared/src/utils/yaml';
import { HPAFormValues, SupportedMetricTypes } from './types';
import { SupportedMetricTypes } from './types';

export const VALID_HPA_TARGET_KINDS = ['Deployment', 'DeploymentConfig'];

Expand All @@ -31,7 +31,7 @@ export const getLimitWarning = (resource: K8sResourceKind): string => {

if (!limits) {
return i18next.t(
'devconsole~CPU and memory resource limits must be set if you want to use CPU and memory utilization. The HorizontalPodAutoscaler will not have CPU or memory metrics until resource limits are set.',
'devconsole~CPU and memory resource requests must be set if you want to use CPU and memory utilization. The HorizontalPodAutoscaler will not have CPU or memory metrics until resource requests are set.',
);
}

Expand All @@ -58,7 +58,7 @@ const getDefaultMetric = (type: SupportedMetricTypes): HPAMetric => ({
resource: {
name: type,
target: {
averageUtilization: 0,
averageUtilization: 50,
type: 'Utilization',
},
},
Expand Down Expand Up @@ -128,45 +128,9 @@ export const sanityForSubmit = (
{ spec: { scaleTargetRef: createScaleTargetRef(targetResource) } },
);

// Remove empty metrics
validHPA.spec.metrics = validHPA.spec.metrics?.filter(
(metric: HPAMetric) =>
!['cpu', 'memory'].includes(metric?.resource?.name?.toLowerCase()) ||
(metric.resource.target.type === 'Utilization' &&
metric.resource.target.averageUtilization > 0),
);

return validHPA;
};

export const getInvalidUsageError = (
hpa: HorizontalPodAutoscalerKind,
formValues: HPAFormValues,
): string => {
const lackCPULimits = formValues.disabledFields.cpuUtilization;
const lackMemoryLimits = formValues.disabledFields.memoryUtilization;
const metricNames = (hpa.spec.metrics || []).map((metric) =>
metric.resource?.name?.toLowerCase(),
);
const invalidCPU = lackCPULimits && metricNames.includes('cpu');
const invalidMemory = lackMemoryLimits && metricNames.includes('memory');

if (metricNames.length === 0) {
return i18next.t('devconsole~Cannot create a HorizontalPodAutoscaler with no valid metrics.');
}
if (invalidCPU && invalidMemory) {
return i18next.t('devconsole~CPU and memory utilization cannot be used currently.');
}
if (invalidCPU) {
return i18next.t('devconsole~CPU utilization cannot be used currently.');
}
if (invalidMemory) {
return i18next.t('devconsole~Memory utilization cannot be used currently.');
}

return null;
};

export const hasCustomMetrics = (hpa?: HorizontalPodAutoscalerKind): boolean => {
const metrics = hpa?.spec?.metrics;
if (!metrics) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,24 +51,6 @@ export const hpaValidationSchema = (t: TFunction) =>
},
)
.required(t('devconsole~Max Pods must be defined.')),
metrics: yup.array(
yup.object({
resource: yup.object({
target: yup.object({
averageUtilization: yup
.mixed()
.test(
'test-for-valid-utilization',
t('devconsole~Average utilization must be a positive number.'),
function (avgUtilization) {
if (!avgUtilization) return true;
return /^\d+$/.test(String(avgUtilization));
},
),
}),
}),
}),
),
}),
}),
}),
Expand Down