Add support to configure multiple service ports #316
Conversation
shivamerla
commented
Feb 7, 2025
shivamerla
commented
- Default named port of "api" will be added with legacy .spec.port attribute
- Ability to setup multiple named ports "api", "metrics" etc.
- auto detection of metrics and api ports for probes and metrics collection
shivamerla
requested review from
ArangoGutierrez,
slu2011 and
visheshtanksale
as code owners
shivamerla
requested review from
shengnuo and
varunrsekar
and removed request for
ArangoGutierrez
shivamerla
force-pushed
the
support_multiple_service_ports
branch
from
11f0e22 to
4d4944e
Compare
* Default named port of "api" will be added with legacy .spec.port attribute * Ability to setup multiple named ports "api", "metrics" etc. * auto detection of metrics and api ports for probes and metrics collection Signed-off-by: Shiva Krishna, Merla <[email protected]>
Signed-off-by: Shiva Krishna, Merla <[email protected]>
Signed-off-by: Shiva Krishna, Merla <[email protected]>
shivamerla
force-pushed
the
support_multiple_service_ports
branch
from
4d4944e to
41b8ae6
Compare
Signed-off-by: Shiva Krishna, Merla <[email protected]>
| // Port is the main api serving port | ||
| Port int32 `json:"port"` | ||
| // MetricsPort is the port to be used for the metrics collection (optional) | ||
| MetricsPort int32 `json:"metricsPort,omitempty"` |
There was a problem hiding this comment.
The helm charts for customizer allow for its internal Port to be customizable. Are we not going to support it here?
There was a problem hiding this comment.
| // Port is the main api serving port | |
| Port int32 `json:"port"` | |
| // MetricsPort is the port to be used for the metrics collection (optional) | |
| MetricsPort int32 `json:"metricsPort,omitempty"` | |
| // Port is the main api serving port | |
| Port *int32 `json:"port"` | |
| // MetricsPort is the port to be used for the metrics collection (optional) | |
| MetricsPort *int32 `json:"metricsPort,omitempty"` |
Also, these ports needs to be a pointer. Not setting port and setting port: 0 have different meanings.
There was a problem hiding this comment.
No, internal port need not be configurable, this is for the jobs to report back status to the customer service and all auto-configured by the operator. Also, the port setting was intentional. We don't want users to be able to set to 0. In that case we use the default port 8000.
There was a problem hiding this comment.
We don't want users to be able to set to 0
To address this, we would add a validation for that value to be between1and65535.
k8s APIs explicitly reject that port number to be set to be 0. We should atleast be consistent with that. But for the per-service defaults, this still needs to be a ptr.
| @@ -435,18 +441,15 @@ func (n *NemoCustomizer) GetStartupProbe() *corev1.Probe { | |||
| // GetDefaultStartupProbe returns the default startup probe for the NemoCustomizer container | |||
| func (n *NemoCustomizer) GetDefaultStartupProbe() *corev1.Probe { | |||
There was a problem hiding this comment.
Can we extract this to a common util? The only difference for each CR seems to be the Path
There was a problem hiding this comment.
This is true for all defaults being applied, need to refactor in a separate MR.
| }, | ||
| } | ||
|
|
||
| if n.GetMetricsPort() != 0 { |
There was a problem hiding this comment.
For nimservice, defauling MetricsPort to 9009 would make sense since they always expose it. But for nemo microservices, they dont yet have metrics exposed in a separate port. How do we differentiate between the 2 cases?
Do we still need to default the metrics port here?
There was a problem hiding this comment.
NIMs don't expose 9009, all are reported through default API port. Same with NeMo services too.
There was a problem hiding this comment.
Was metrics port introduced as a placeholder? Trying to understand which CR uses a separate port for metrics. If no one is using it, lets remove it now and reintroduce it later?
Signed-off-by: Shiva Krishna, Merla <[email protected]>
