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
Add memory validations in the VM wizard #3839
Add memory validations in the VM wizard #3839
Conversation
}, | ||
[VMSettingsField.CPU]: { | ||
detectValueChanges: [VMSettingsField.CPU], | ||
validator: asVMSettingsFieldValidator(validatePositiveInteger), | ||
validators: [asVMSettingsFieldValidator(validatePositiveInteger)], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need a cpu validation ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
both validations for CPU and memory that check for a positive integer value seems unnecessary, as you cannot type anything that isn't a positive integer.
But I'm not sure what happens when you use a template that has an invalid value.
haven't tried it though, but I think we can address this question in another PR.
@yaacov What do you think ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the template has a vlidation:
{
"name": "core-limits",
"path": "jsonpath::.spec.domain.cpu.cores",
"message": "cpu cores must be limited",
"rule": "integer",
"min": 100,
"max": 800
}
and user enter 5 cpus, who checks that 5 is not valid value, what am I missing here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@irosenzw we let the user enter any cpu numer they want, I think we should check for cpu outside the range ... is it problematic ?
For example: a "big-cpu-template" have a rule that min cpu is 18, and user entered 6, this is an invalid cpu number, do we check it ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all, the PR is for memory validation only at this point. In the future we will cover all the fields in the VM wizard.
Regarding your question, if this is the only validation, it will fail.
but it there is another template that has a validation of :
{
"name": "core-limits",
"path": "jsonpath::.spec.domain.cpu.cores",
"message": "cpu cores must be limited",
"rule": "integer",
"min": 2
}
it will pass.
let relevantTemplates; | ||
// Get templates based on OS and WorkloadProfile | ||
if (os && workloadProfile) { | ||
relevantTemplates = (commonTemplates || []).filter( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you try to make "getRelevantTemplates" method and use it here instead to doing it inline, i think it will look nicer ?
const relevantTemplates = (os && workloadProfile) ?
getRelevantTemplates(templates, os, profile, flavor) | []
); | ||
|
||
// Get the exact template when a flavor is chosen | ||
if (flavor && flavor !== 'Custom') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you try to add this check to the filter in line 40 ?
... &&
( (flavor === 'Custom' || iGetIn(t, ['metadata', 'labels', `${TEMPLATE_FLAVOR_LABEL}/${flavor}`]) ),
...
|
||
// Get all relevant validations for the specific fieldId | ||
const relevantFieldValidations: CommonTemplatesValidation[] = []; | ||
myValidations.forEach((validationArray: CommonTemplatesValidation[]) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you make a "getValidation" method ( similar to comment about getRelevantTemplates ) ?
may even be in the selector file and not here ?
relevantTemplates = (commonTemplates || []).filter( | ||
(template) => | ||
iGetIn(template, ['metadata', 'labels', TEMPLATE_TYPE_LABEL]) === 'base' && | ||
iGetIn(template, ['metadata', 'labels', `${TEMPLATE_OS_LABEL}/${os}`]) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ca you try this pattern in the tests:
( !os || iGetIn(template, ['metadata', 'labels', `${TEMPLATE_OS_LABEL}/${os}`]) )
it makes this metyhod more general, and will handle cases where we want to search only by flavor or onlt by os ...
...evirt-plugin/src/components/create-vm-wizard/redux/validations/vm-settings-tab-validation.ts
Outdated
Show resolved
Hide resolved
const memValueBytes = memValueGB * 1024 ** 3; | ||
|
||
// filter all the 'min' validation rules | ||
const minValidations = validations.filter((v) => 'min' in v); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like better running each check individually and looking for first success, then finding the min of all validations and then checking once. because it create a pattern of checking rules we can apply for all checks, e.g. on regex:
validations.once( ... // do one min validation at a time )
validations.once( ... // do one regex validation at a time )
iGetIn(t, ['metadata', 'labels', `${TEMPLATE_FLAVOR_LABEL}/${flavor}`]), | ||
); | ||
} | ||
const myValidations: CommonTemplatesValidation[][] = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my
prefix :-)
6a7ffc9
to
6397059
Compare
const commonTemplates = iGetLoadedCommonData(state, id, VMWizardProps.commonTemplates); | ||
|
||
// Get templates based on OS and WorkloadProfile | ||
if (os && workloadProfile) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have this check in getRelevantTemplates
, it will return an empty array [], and then if getFieldValidations
return empty array, you will not need this "if" and not the if in line 50
6397059
to
e634b24
Compare
case IntegerValidtionType.max: | ||
return validateMaxRule(ivo, memValueBytes); | ||
default: | ||
return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true, are you sure, i'm missing somthing ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. It should be false.
); | ||
}; | ||
|
||
export const getValidationsFromTemplates = (templates): CommonTemplatesValidation[][] => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'm missing something here - why we do not flatten the rules ?
for example, if you have two templates:
template1:
[
{
"name": "core-limits",
"path": "jsonpath::.spec.domain.cpu.cores",
"message": "cpu cores must be limited",
"min": 1,
"max": 8
}
]
template2:
[
{
"name": "validation-rule-01",
"valid": "jsonpath::.some.json.path",
"path”: "jsonpath::.some.json.path[*].leaf",
"rule”: "integer",
"message”: ".some.json.path[*].leaf must exists",
"min”: 1,
"justWarning": true
},
{
"name": "validation-rule-02",
"valid": "jsonpath::.another.json.path",
"path": "jsonpath::.another.json.path.item",
"rule": "integer",
"message": "jsonpath::./another.json.path.item must be below a threshold",
"max": "jsonpath::.yet.another.json.path.defines.the.limit"
}
]
we should end up with a flat 3 rules and not with 2 sets one having 1 rule and the other 2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Multiple templates can occur when for example the workload profile is not selected right? Only one template will be selected at the end.
I think we should pick the most relaxed validation (meaning allowing everything if one template is missing the validation. Or combining min max of the other ones to get the largest interval span).
We should also probably use the validation from the user template if one is selected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all, the user must pick the OS
and the workload profile
in order to get the relevant templates from common templates. Only the flavor
can be selected as Custom
.
Second, when a user pick a flavor, we don't do any validations to the memory - since it is hard-coded in the template. when the user chooses Custom
we are getting multiple templates, hence multiple validations from each template.
The idea here is to PASS if at least one of the validations had passed, and FAIL if none had.
user template
isn't in the scope of this feature and will/should be addressed in another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
user template isn't in the scope of this feature and will/should be addressed in another PR.
are you sure ? IFAIU it's the simplest test, where you have one template and do not need to look for any other ...
getRelevantTemplates = customTemplate ? [ customTemplate ] | ....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've Added support for user template validations
} | ||
if (validationAttrs.includes('min')) { | ||
if (integerValidationObj.type === IntegerValidtionType.max) { | ||
// change the type to 'between' because we have two seperate validations of min and max |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, when calling this method we should have only one rule - see comment https://github.com/openshift/console/pull/3839/files#r362532470
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not necessarily.
Most of the times we will have just one, it will be one of the three:
- {min:1},{cpu:2},....
- {max:10},{cpu:2},...
- {min:1,max:10},{cpu:2},....
but we have here an edge case when abetween
validation can be written as a 2 separated validations in a single template: - {min:1},{max:10},{cpu:2},....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this method should be called with one rule at a time, you should check why it is called with an array ...
// eslint-disable-next-line consistent-return | ||
validations.forEach((validation) => { | ||
const validationAttrs = getValidationAttributes(validation); | ||
if (validationAttrs.includes('min') && validationAttrs.includes('max')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we check the object directly without getting the validationAttrs
?
e.g. is their a way to ask if validation
itself has an attribute without first getting all the attributes into a list ... ?
getValidationAttributes
looks like something that we should get for free from the language ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. I'll remove the method
return getFieldValidations(validations, IDToJsonPath[fieldId]); | ||
}; | ||
|
||
export const getIntegerValidationType = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this method should get one CommonTemplatesValidation
and return an enum, we are doing something wrong here ...
I think it's because we didn't flatten the rules in https://github.com/openshift/console/pull/3839/files#r362532470
errorMsg = ivo.message; | ||
switch (ivo.type) { | ||
case IntegerValidtionType.between: | ||
return validateBetweenRule(ivo, memValueBytes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sinse we know that the current rule vArr ( should be one not an array ) is a "between" rule, then vArr must have a min and max properties, we should be able to write: memValueBytes >= vArr.min && vArr.max <= memValueBytes
, what am I missing ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
commented above. it should be an array.
@@ -119,24 +178,28 @@ const validationConfig: VMSettingsValidationConfig = { | |||
] | |||
: [VMWizardProps.activeNamespace, VMWizardProps.virtualMachines]; | |||
}, | |||
validator: validateVm, | |||
validators: [validateVm], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's not complicate it with an array of validators. Semantically one fields needs only one validation call. The validation can branch inside the call depending on the use case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to have multiple validators like this. It is clearer that a field has multiple validators rather than a validator that may or may not branch into multiple validators inside it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it better?
It is unclear what the execution of the validators should be e.g. the order and the validation message.
IMO the interface should be expressive enough for future changes (possible cross validation).
updateAcc[validationFieldKey] = { validation }; | ||
} | ||
|
||
// break when one validation is invalid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can keep the old code here if we use one validator and decide the validation priority inside the validator.
return { type: IntegerValidtionType.between, value, message }; | ||
}; | ||
|
||
export const validateMinRule = (ivo: IntegerValidationObj, value: number): boolean => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need all of these wrappers.
I think we could rather do one loop and a set two variables according to the min/max/between
let min = 0;
let max = Number.POSITIVE_INFINITY;
loop over attributes and set accordingly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey, my 2 cents are that we do not need this set of methods at all, instead of calling them, we shoud do:
rule.min >= value
all this checks are already valid after we check getType(rule)
becase if type is minType
rule must have a rule.min
attribute ...
IntegerBetween, | ||
} from './validation-types'; | ||
|
||
export const getRelevantTemplates = (commonTemplates, os, workloadProfile, flavor) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we move this to vm-template selectors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can. no problem.
); | ||
}; | ||
|
||
export const getValidationsFromTemplates = (templates): CommonTemplatesValidation[][] => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Multiple templates can occur when for example the workload profile is not selected right? Only one template will be selected at the end.
I think we should pick the most relaxed validation (meaning allowing everything if one template is missing the validation. Or combining min max of the other ones to get the largest interval span).
We should also probably use the validation from the user template if one is selected.
// eslint-disable-next-line consistent-return | ||
validations.forEach((validation) => { | ||
const validationAttrs = getValidationAttributes(validation); | ||
if (validationAttrs.includes('min') && validationAttrs.includes('max')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
e634b24
to
41051ac
Compare
}; | ||
|
||
export const runValidation = (validation: CommonTemplatesValidation, value: number): boolean => { | ||
if ('integerBetween' in validation && validation.integerBetween === true) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we do
if (('min' in validation) && ('max' in validation))
instead ?
import { CommonTemplatesValidation } from './validation-types'; | ||
|
||
export const getValidationsFromTemplates = (templates): CommonTemplatesValidation[][] => { | ||
return templates.map((relevantTemplate) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we do ?
return templates.map((relevantTemplate) =>
JSON.parse(iGetIn(relevantTemplate, ['metadata', 'annotations', 'validations'])),
).flat();
message: string; // User-friendly string message describing the failure, should the rule not be satisfied | ||
min?: number; // For 'integer' rule | ||
max?: number; // For 'integer' rule | ||
integerBetween?: boolean; // For 'integer' rule |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
integerBetween?: boolean; // For 'integer' rule | ||
minLength?: number; // For 'string' rule | ||
maxLength?: number; // For 'string' rule | ||
stringBetween?: boolean; // For 'string' rule |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
): CommonTemplatesValidation[] => { | ||
const allTemplateValidations: CommonTemplatesValidation[] = []; | ||
validations.forEach((validationArray: CommonTemplatesValidation[]) => { | ||
validationArray.forEach((validation: CommonTemplatesValidation) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this hould be one level: https://github.com/openshift/console/pull/3839/files#r363103746
}; | ||
|
||
export const getFieldValidations = ( | ||
validations: CommonTemplatesValidation[][], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
validations should be flat ... validations: CommonTemplatesValidation[]
return fieldValidations; | ||
}; | ||
|
||
export const runValidation = (validation: CommonTemplatesValidation, value: number): boolean => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we do ?
export const runValidations = (validations: CommonTemplatesValidation[], value: number): (valid: boolean, mesage: string) => {
let message = null
const valid = validations.some((validation) => {
message = validation.message
...
if ('max' in validation) {
return value <= validation.max;
}
return false
}
...
return { valid, messge}
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry the signiture should be like in:
see: https://github.com/openshift/console/pull/3839/files#r362222830
const runVlidations = (validations, value: any) => ...
because this method can accept any type of falue number of string and run the appropriate validation rules.
There is no value that would satisfy both validations. | ||
*/ | ||
let errorMsg; | ||
const isValid = validations.some((validation) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we do ?
const ( isValid, message } = runValidations(validations, memValueBytes);
see https://github.com/openshift/console/pull/3839/files#r363104852
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or we could create a Validation class which would encapsulate all of it - validation data (on initialization) and the logic
41051ac
to
01326b7
Compare
Fixes: CNV-2913 - Add common-templates validation type - Consume memory validations from the user template if chosen or from common templates based on OS, workload Profile and flavor fields and use them in the VM wizard. Signed-off-by: Ido Rosenzwig <irosenzw@redhat.com>
01326b7
to
9d7481f
Compare
workloadProfile: string, | ||
flavor: string, | ||
) => { | ||
if (!commonTemplates || commonTemplates.length === 0 || !os || !workloadProfile) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: I think it's nice to allow the logic of "if os is not defined => allow all possible os"
so in my view we should do:
if (!commonTemplates || commonTemplates.length === 0) {
and let the checks in L150 and L151 implement the above logic.
.map((relevantTemplate) => | ||
JSON.parse(iGetIn(relevantTemplate, ['metadata', 'annotations', 'validations'])), | ||
) | ||
.valueSeq() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: what does .valueSeq().toArray()
adds here ? does it work if we remove them ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it gets the validations as an array from the immutable object. Thus cannot be removed.
// Get userTemplate if it was chosen | ||
const userTemplateName = iGetVmSettingValue(state, id, VMSettingsField.USER_TEMPLATE); | ||
const iUserTemplates = iGetLoadedCommonData(state, id, VMWizardProps.userTemplates); | ||
const iUserTemplate = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: if we have iUserTemplate
we do not need to fetch os
, flavor
, workloadProfile
and commonTemplates
so if we move lines 41...46 above line 32, we can do:
if (iUserTemplate) {
validations = getValidationsFromTemplates([iUserTemplate]);
return getFieldValidations(validations, IDToJsonPath[fieldId]);
}
// Get OS, workload-profile and flavor attributes
const os = iGetVmSettingAttribute(s...
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: irosenzw, yaacov The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
/retest Please review the full test history for this PR and help us cut down flakes. |
Nitpicks from #3839
Fixes: CNV-2913
and use them in the VM wizard
can have more than one validator function for a single key
Signed-off-by: Ido Rosenzwig irosenzw@redhat.com