US-392813 Make HPA targets independently optional in the HPA template #217
Conversation
US-392813 Small fix US-392813 Small fix US-392813 Add automatic test US-392813 Update README.md
pmaslak92
force-pushed
the
hpa_template_moreflexible
branch
from
b428e89 to
339e0fc
Compare
charts/pega/README.md
Outdated
| @@ -403,6 +403,8 @@ Parameter | Description | Default value | |||
| `hpa.maxReplicas` | Maximum number of replicas that HPA can scale-up | `5` | |||
| `hpa.targetAverageCPUUtilization` | Threshold value for scaling based on initial CPU request utilization (The default value is `70` which corresponds to 70% of 2) | `70` | |||
| `hpa.targetAverageMemoryUtilization` | Threshold value for scaling based on initial memory utilization (The default value is `85` which corresponds to 85% of 6Gi ) | `85` | |||
| `hpa.disableCPUTarget` | Set to true if you want to remove scaling based on CPU utilization and only rely on memory utilization | false | |||
There was a problem hiding this comment.
Rather than have the flag be to disable the target, can we have it be true=on and false=off and avoid the negative parameter? We can still default to true if not specified.
There was a problem hiding this comment.
@dcasavant I am not an expert in helm templates, but from what I see it doesn't distinguish between a property being absent or it being equal to false. We wanted to make this backwards-compatible, so that if someone has an existing values file without the new properties, it works as before. We tried doing it like you suggest, but in the template when we used "(default true .hpa.enableCpuTarget)" it would always evaluate to true, even if we set this value explicitly to false, so it looks like this operator is just an OR.
Is there a different way to provide a default value that will work properly with booleans?
There was a problem hiding this comment.
@pmaslak92 I agree with @dcasavant - let's not use negatives. I believe the user story asks for .hpa.cpu and .hpa.memory.
You use default function in a reversed order, it should be "(default .hpa.enableCpuTarget true)"
There was a problem hiding this comment.
@andreiadamian I have tried in this order too. You can see it here - pmaslak92@b579675 - the test is failing, because helm templates don't distinguish between a property being absent or being equal to false.
As discussed with @andreiadamian, I changed the property to be a string "on" or "off". @dcasavant Can you take a look?
There was a problem hiding this comment.
I believe the this wording must be updated...here's my guess at an appropriate alternative to line 406: "Set to on to enable scaling based on CPU utilization; set to off to enable scaling based on memory utilization | on"
There was a problem hiding this comment.
@taz-pega-work it was "on" and "off" at some point, but it was requested that this is a boolean, so now it's 'true' and 'false' everywhere. The current wording is correct.
pmaslak92
force-pushed
the
hpa_template_moreflexible
branch
from
a0f9d1c to
ae41b93
Compare
pmaslak92
force-pushed
the
hpa_template_moreflexible
branch
from
daee0bb to
6dff458
Compare
US-392813 Small fix
US-392813 Small fix
US-392813 Add automatic test