Divide assigned limits with replicas #725
Conversation
|
|
|
This looks good so far! Thanks for your contribution! The reason CI is failing is because some unit tests regarding of how resources are calculated should be updated |
ok, will take a look |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #725 +/- ##
==========================================
+ Coverage 77.87% 77.88% +0.01%
==========================================
Files 67 67
Lines 5102 5106 +4
==========================================
+ Hits 3973 3977 +4
Misses 937 937
Partials 192 192
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -49,6 +49,9 @@ func Resources(tempo v1alpha1.TempoStack, component string) corev1.ResourceRequi | |||
| if ok { | |||
| totalCpuInt := totalCpu.MilliValue() | |||
| cpu := float32(totalCpuInt) * componentResources.cpu | |||
| if r := tempo.Spec.Template.Compactor.Replicas; r != nil && *r > 1 { | |||
There was a problem hiding this comment.
Resources() is called from every tempo component (compactor, distributor, ingester, etc). I'd suggest to add an additional function parameter replicas *int32 to Resources() and update all callers.
There was a problem hiding this comment.
Resources() is called from every tempo component (compactor, distributor, ingester, etc).
I see, but would there be any other possibilities for those components to have a replicas *int32 value other than tempo.Spec.Template.Compactor.Replicas?
because what I imagined is something like this
--- Resources: manifestutils.Resources(tempo, manifestutils.CompactorComponentName),
+++ Resources: manifestutils.Resources(tempo, manifestutils.CompactorComponentName, tempo.Spec.Template.Compactor.Replicas)
(file: compactor.go)
There was a problem hiding this comment.
Yes, each component can have individual replica settings - e.g. you can have 2 replicas of the compactor and 10 of the distributor.
etc.
There was a problem hiding this comment.
i see, thanks for the pointers
i will do the update
There was a problem hiding this comment.
updated, ready to review
Update Resources interface to accept replicas.
| @@ -231,7 +231,7 @@ func deployment(params manifestutils.Params) (*appsv1.Deployment, error) { | |||
| MountPath: manifestutils.TmpStoragePath, | |||
| }, | |||
| }, | |||
| Resources: manifestutils.Resources(tempo, manifestutils.QueryFrontendComponentName), | |||
| Resources: manifestutils.Resources(tempo, manifestutils.QueryFrontendComponentName, tempo.Spec.Template.QueryFrontend.Replicas), | |||
There was a problem hiding this comment.
@rubenvp8510 looks like we currently assign the query-frontend resources to both the query-frontend and tempo-query containers, so we're using the resources allocated to the query-frontend component twice 😮
There was a problem hiding this comment.
Looks good, thank you for your contributions @rafiramadhana! 🚀
|
it looks like the test was failing due to intermittent error @rubenvp8510 @andreasgerstmayr |
you're right, this was caused by a flaky test (fixed in #728) This PR is good to merge, thank you! |
Resolves #721