-
Notifications
You must be signed in to change notification settings - Fork 344
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
Added auto-scale to the collector #856
Conversation
@kevinearls not sure we want to add a new e2e test for this, but perhaps you might have a good idea that wouldn't be too fragile? |
I just ran a longer test this morning, showing that it scales up and down. First, I deployed the
And here's a set of screenshots from OpenShift (older events are at the bottom): |
A Horizontal Pod Autoscaler (HPA) was added in this PR, along with a new MinReplicas and MaxReplicas. With that, the collector should now automatically scale up and down based on the CPU and/or memory consumption. When none of the new properties are specified, the minimum amount of replicas is 1, while the maximum number of replicas is 100. The HPA configuration is added only when the deployment strategy is either production or streaming. Signed-off-by: Juraci Paixão Kröhling <[email protected]>
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.
Looks good. Just some minor comments.
spec: | ||
containers: | ||
- name: tracegen | ||
image: jaegertracing/jaeger-tracegen:latest |
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.
Shouldn't tracegen image be versioned inline with the other jaeger components?
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.
Yes, opened an issue (#866) to track this. The first image will probably be 1.17 (next release).
@@ -21,7 +21,7 @@ func init() { | |||
|
|||
func TestNegativeReplicas(t *testing.T) { | |||
size := int32(-1) | |||
jaeger := v1.NewJaeger(types.NamespacedName{Name: "TestNegativeReplicas"}) | |||
jaeger := v1.NewJaeger(types.NamespacedName{Name: "my-instance"}) |
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.
What happened to the convention of naming instances after the test?
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 started naming them all "my-instance" some time ago, as individual names don't bring much value and we had a few copy/paste mistakes.
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
@jpkrohling Approval subject to tests passing :) |
Local run has shown that a new permission is missing. I'm testing it locally and will update the PR once I confirm the tests are passing. |
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
A Horizontal Pod Autoscaler (HPA) was added in this PR, along with a new MinReplicas and MaxReplicas. With that, the collector should now automatically scale up and down based on the CPU and/or memory consumption. When none of the new properties are specified, the minimum amount of replicas is 1, while the maximum number of replicas is 100. The HPA configuration is added only when the deployment strategy is either production or streaming.
Closes #848, even though the scaling of the storage isn't implemented by this one.
Signed-off-by: Juraci Paixão Kröhling [email protected]