feat: add memory limiter to gateway collector #1086
Conversation
| // confgiures the processor spike_limit_mib. When memory usage exceeds the hard limit minus this amount, | ||
| // the processor will enter memory limited mode and will start refusing data. | ||
| // as recommended by the processor docs, this is set to 20% of the hard limit | ||
| memoryLimiterSpikeLimitMiB = 90 |
There was a problem hiding this comment.
Maybe worth considering defining these constants in a relative way like 0.2 * memoryLimiterLimitMib. This way we can define a single default memory size of requestMemory and calculate all the others based on that value.
That way we could add code that retries with a smaller requestMemory in case of failure to deploy (and all the other sizes will adjust)
There was a problem hiding this comment.
Thanks @RonFed ,
I refactored the code so now it's configurable and relative. Can you please take a look again?
There was a problem hiding this comment.
Do I understand correctly that the defaults are as percentages calculated, but when user wants to specify memory limits, they can only use absolute values?
The user cannot set the actual request limit on the gateway, because that will be overwritten by a value on OdigosConfiguration?
|
|
||
| memoryLimiterSpikeLimitMiB := memoryLimiterLimitMib * defaultMemoryLimiterSpikePercentage / 100.0 | ||
| if odigosConfig.Spec.CollectorGatewayMemoryLimiterSpikeLimitMiB != 0 { | ||
| memoryLimiterSpikeLimitMiB = odigosConfig.Spec.CollectorGatewayMemoryLimiterSpikeLimitMiB |
There was a problem hiding this comment.
Maybe worth adding a check here that memoryLimiterSpikeLimitMiB < memoryLimiterLimitMib?
Why are the configuration options used as absolute values and our defaults relative? I think we should be consistent with the relative approach
| logger := log.FromContext(ctx) | ||
| desiredDeployment, err := getDesiredDeployment(dests, configData, gateway, scheme, imagePullSecrets, odigosVersion) | ||
| desiredDeployment, err := getDesiredDeployment(dests, configData, gateway, scheme, imagePullSecrets, odigosVersion, memConfig) |
There was a problem hiding this comment.
I'm not sure how often it might happen, but if our default is to request 500MB, and there is some resource limit preventing us from doing so, we should probably have some handling for it.
The relative defaults allow a user to set just one value (the memory request) and have all the other values automatically derived from it. If a user wants to set specific values, I thought it would be more straight forward to set the values as they would be written into the k8s manifest, instead of specifying relative amounts which requires more mental effort to calculate and understand. but I am open for change if you think it will be easier for users to use relative values @rauno56 @RonFed |
|
I get where you are coming from, but I was also a bit surprised with the difference there. Both of you have piles more experience in this specific case so I'll let you to thumb wrestle this out, but as a unsuspecting user-developer I'd expect defaults to have the same "power" as user-set values. It'll likely also help with the maintainability to have the terms, concepts and the logic match. |
No description provided.