Nicchambers/daemonsetvaluechanges#1622
Nicchambers/daemonsetvaluechanges#1622NicAtMS wants to merge 10 commits intolongw/addon-to-extension-merge-chartsfrom
Conversation
| {{- else if eq $suffix "" -}} | ||
| {{- if contains "." $number -}} | ||
| {{/* Decimal number without suffix - assume CPU cores, convert to millicores */}} | ||
| {{- $result = add (mul $intPart 1000) (div (mul $fracPart 1000) $fracDivisor) -}} |
There was a problem hiding this comment.
will this treat cpu 1 as 1 core or 1m? cpu and memory should be considered differently
There was a problem hiding this comment.
this will treat it as 1 core. This behavior mimics what is done for addons right now. From one of the snapshots where we set the value to 3 via the variable set (essentially toggles) we get:
resources:
limits:
cpu: 3
memory: 3Gi
requests:
cpu: 229m
memory: 1058Mi
| {{- $number := "" -}} | ||
| {{- $suffix := "" -}} | ||
| {{- $i := 0 -}} | ||
| {{- range $char := (split "" $quantity) -}} |
There was a problem hiding this comment.
split "" is not a robust/portable way to iterate characters in Helm/Sprig across versions; it can behave unexpectedly consider using use a safer parsing approach with regex (regexFind, regexReplaceAll
| kind: DaemonSet | ||
| metadata: | ||
| name: ama-logs | ||
| name: ama-logs{{- if gt $index 0 }}-{{ $size.name }}{{- end }} |
There was a problem hiding this comment.
could you share how can we check the nodeAffinity logic is mutually exclusive per tier
There was a problem hiding this comment.
the way the node affinity is constructed forces mutual exclusivity. we are essentially doing:
maxCPU: { valueOfCurrentTshirtSize }
MinCPU: { valueOfPreviousTshirtSize }
This means that, for example, the large size daemonset will have bounds of:
maxCPU { largeCPUSize }
minCPU { mediumCPUSize + 1}
this ensures that even if the large size is misconfigured for some reason, as in, it's set to 5 and the medium size is also set to 5, the node afinity for large would be anything less than 5 and larger than 6, which is not valid, meaning it would essentially be omitted. There is no way for these values to conflict.
| {{- $containerResources := index $size.containers "ama-logs-prometheus" }} | ||
| cpu: {{ $containerResources.cpuLimit }} | ||
| memory: {{ $containerResources.memoryLimit }} | ||
| cpu: {{ include "maxResourceValue" (list $.Values.OmsAgent.omsAgentPrometheusSidecarCPULimit $containerResources.cpuLimit) }} |
There was a problem hiding this comment.
for test we would want to make sure that both cpu and memory be covered with different set of values; such as cpu with 750m, 1, 6 memory with 750mi, 1Gi 6Gi
There was a problem hiding this comment.
in my testing with fixtures, I tested the following values:
CPU: 500m, 2, 3
Memory: 1Gi, 2Gi, 3Gi
Please let me know if these values are acceptable tests
2 participants
Updated the logs helm chart to support daemonset t-shirt sizing and introduced the new cluster kind which will be replacing isAutomaticSKU. These changes have been tested with fixtures and produce the expected results when t-shirt sizing is enabled and disabled