fix(signoz): decouple container ports from service ports - #899
Open
Mohith1612 wants to merge 1 commit into
Open
Conversation
The SigNoz statefulset derived its containerPorts from signoz.service.*Port, but the process always listens on 8080, 8085 and 4320 inside the container. Setting signoz.service.port to anything else therefore renamed the port the container declares without moving the listener. Both probes default to the named port `http`, so they ended up polling a port nothing was bound to and the liveness failure restarted the pod in a loop. Service.targetPort resolves by name too, so traffic went nowhere either. Pin the container ports to the ports the application binds and leave signoz.service.*Port to describe the Service alone. The OpAMP endpoint in the collector config now follows signoz.service.opampPort as well, since it dials the Service and previously hardcoded 4320. Closes SigNoz#762
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A basic install that sets
signoz.service.portto anything other than the default putssignoz-0intoCrashLoopBackOff, even though the container logs show the process starting cleanly. Reported in #762 withsignoz.service.port: 80for a LoadBalancer service.The statefulset derived its container ports from the service port values:
but SigNoz always listens on 8080, 8085 and 4320 inside the container, and the chart has no way to change that. So
signoz.service.port: 80only renamed the port the container declares, it did not move the listener:signoz.livenessProbe.portandreadinessProbe.portdefault to the named porthttp, which now resolved to 80. Nothing was bound there, so the liveness probe failed and restarted the container in a loop.Service.targetPort: httpresolves by name too, so even with probes disabled the Service routed to a dead port.The chart already assumes the fixed in-container ports in two other places, which is what made this inconsistent rather than intentional:
signoz_alertmanager_signoz_external__url: http://localhost:8080invalues.yaml, and the hardcoded:4320in the collector's OpAMP config.otelCollector.portsalready models this properly with separatecontainerPortandservicePortper entry.Changes
templates/signoz/statefulset.yaml: pin the container ports to 8080 / 8085 / 4320, sosignoz.service.*Portnow describes the Service only. Named-port resolution then does the right thing for both the probes andtargetPort.templates/otel-collector/configmap.yaml: the OpAMPserver_endpointdials the SigNoz Service, so it should followsignoz.service.opampPortinstead of hardcoding 4320. Without this, changingopampPortmoves the Service port and leaves the collector dialling 4320. Happy to drop this one if you would rather keep it separate.values.yaml: note on each of the threesignoz.service.*Portkeys that the container port is fixed.No new values keys: since the chart cannot change what the process binds to, a container-port knob would only be misleading. Let me know if you would prefer one anyway.
Testing
tests/signoz-service_port_test.yaml. The three regression cases fail onmainand pass here;helm unittest charts/signozis 4 suites / 18 tests green.helm template charts/signozon default values is byte-identical tomainapart from the version label and the config checksum that label feeds into.helm template charts/signoz --set signoz.service.port=80now renderscontainerPort: 8080with Serviceport: 80andtargetPort: http.helm template charts/signoz --set signoz.service.opampPort=4321rendersws://<release>-signoz:4321/v1/opamp.helm lint --strict charts/signozpasses, andmake chart-docs CHARTS=charts/signoz,charts/k8s-infrais clean after the commit.Closes #762