Skip to content
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

Fix panic if maxreplicas is set but autoscale is not defined in the CR #1201

Merged
merged 3 commits into from
Oct 26, 2022

Conversation

kevinearls
Copy link
Member

Signed-off-by: Kevin Earls kearls@redhat.com

Fixes #1197

Signed-off-by: Kevin Earls <kearls@redhat.com>
@kevinearls kevinearls requested a review from a team October 24, 2022 08:48
@kevinearls kevinearls changed the title Fix panic if maxreplicas is set but autoscale is not defined in the PR Fix panic if maxreplicas is set but autoscale is not defined in the CR Oct 24, 2022
@jsirianni
Copy link
Member

Thanks for handling this so fast. I built the image and tested it against a development instance. It does seem to resolve the panic, but I do not see an HPA anymore. In the past, a horizontal pod autoscaler would be deployed.

It is unclear to me if the HPA issue is related, or a separate issue.

@kevinearls
Copy link
Member Author

@jsirianni what does your CR look like? I used the one below and that worked for me. You may need to first make sure you've deleted any previous collector instances. Use kubectl get otelcols --all-namespaces=true to find them.

apiVersion: opentelemetry.io/v1alpha1
kind: OpenTelemetryCollector
metadata:
  name: simplest
spec:
  minReplicas: 1
  maxReplicas: 2
  config: |
    receivers:
      jaeger:
        protocols:
          grpc:
      otlp:
        protocols:
          grpc:
          http:
    processors:

    exporters:
      logging:

    service:
      pipelines:
        traces:
          receivers: [jaeger,otlp]
          processors: []
          exporters: [logging]

@jsirianni
Copy link
Member

@kevinearls you were right, looks good on my end now 👍

Thanks again

Copy link
Member

@frzifus frzifus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it covered by an e2e test?

@kevinearls
Copy link
Member Author

kevinearls commented Oct 25, 2022

@frzifus This should be covered by the unit test. We just need to be sure that the AutoScalerSpec is not null when the CR has maxreplicas set but does not include autoscale.

@pavolloffay pavolloffay merged commit c59c882 into open-telemetry:main Oct 26, 2022
@kevinearls kevinearls deleted the fix-autoscale-panic branch October 26, 2022 12:48
ItielOlenick pushed a commit to ItielOlenick/opentelemetry-operator that referenced this pull request May 1, 2024
open-telemetry#1201)

* Fix panic if maxreplicas is set but autosclae is not defined in the PR

Signed-off-by: Kevin Earls <kearls@redhat.com>

* Add a unit test

Signed-off-by: Kevin Earls <kearls@redhat.com>

Signed-off-by: Kevin Earls <kearls@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setting maxReplicas causes runtime panic
4 participants