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.

Sign up for GitHub

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

Add support to configure multiple service ports #316

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add support to configure multiple service ports #316

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
db0979d
Add support to configure multiple service ports
shivamerla Feb 7, 2025
ce452e2
Set targetports to defaults for each microservice when they are not c…
shivamerla Feb 12, 2025
bc7188a
Further cleanup
shivamerla Feb 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 57 additions & 0 deletions api/apps/v1alpha1/common_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
autoscalingv2 "k8s.io/api/autoscaling/v2"
corev1 "k8s.io/api/core/v1"
networkingv1 "k8s.io/api/networking/v1"
"k8s.io/apimachinery/pkg/util/intstr"
)

// Expose defines attributes to expose the service
Expand Down Expand Up @@ -115,3 +116,59 @@ type CertConfig struct {
// MountPath is the path where the certificates should be mounted in the container.
MountPath string `json:"mountPath"`
}

// selectNamedPort returns the first occurrence of a given named port, or an empty string if not found.
func selectNamedPort(serviceSpec Service, portNames ...string) string {
for _, name := range portNames {
for _, port := range serviceSpec.Ports {
if port.Name == name {
return name
}
}
}
return ""
}

// getProbePort determines the appropriate port for probes based on the service spec.
func getProbePort(serviceSpec Service) intstr.IntOrString {
Copy link
Collaborator

Choose a reason for hiding this comment

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

we can probably use rendertypes.ServiceParams as input here and simplify this to always prefer "api" namedPort from the ServiceParams.Ports list

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Each resource is rendered independently during reconciliation. That will add a dependency that we always have to render serviceParams before setting up deployment or servicemonitor.

switch len(serviceSpec.Ports) {
case 1:
port := serviceSpec.Ports[0]
if port.Name != "" {
return intstr.FromString(port.Name)
}
return intstr.FromInt(int(port.Port))
case 0:
// Default to "api" as the operator always adds a default named port with 8000
return intstr.FromString("api")
default:
// Multiple ports: Prefer "api"
if portName := selectNamedPort(serviceSpec, "api"); portName != "" {
shivamerla marked this conversation as resolved.
Show resolved Hide resolved
return intstr.FromString(portName)
}
// Default when multiple ports exist
return intstr.FromString("api")
}
}
Copy link
Collaborator

@varunrsekar varunrsekar Feb 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

case where length is 1 and default case where length > 1 should have similar behavior:

  • Prefer "api"
  • Otherwise, pick first port
    Isn't this the case? Defaulting to "api" when other options exist doesnt seem right to me.
Suggested change
switch len(serviceSpec.Ports) {
case 1:
port := serviceSpec.Ports[0]
if port.Name != "" {
return intstr.FromString(port.Name)
}
return intstr.FromInt(int(port.Port))
case 0:
// Default to "api" as the operator always adds a default named port with 8000
return intstr.FromString("api")
default:
// Multiple ports: Prefer "api"
if portName := selectNamedPort(serviceSpec, "api"); portName != "" {
return intstr.FromString(portName)
}
// Default when multiple ports exist
return intstr.FromString("api")
}
}
switch len(serviceSpec.Ports) {
case 0:
// Default to "api" as the operator always adds a default named port with 8000
return intstr.FromString("api")
default:
// Multiple ports: Prefer "api"
if portName := selectNamedPort(serviceSpec, "api"); portName != "" {
return intstr.FromString(portName)
}
// Default when multiple ports exist
port := serviceSpec.Ports[0]
if port.Name != "" {
return intstr.FromString(port.Name)
}
return intstr.FromInt(int(port.Port))
}
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We just can't assume to use the first port when multiple ports are given. For e.g. in case of Customizer, if "api" and "internal" ports are used with different names, we can't assume "internal" ports for liveness/startup probes etc. Hence i was always enforcing that a named port of "api" should be specified in that case. Probably better things is to validate this during reconciliation and error out when required named ports are not provided.


// getMetricsPort determines the appropriate port for metrics based on the service spec.
func getMetricsPort(serviceSpec Service) intstr.IntOrString {
switch len(serviceSpec.Ports) {
case 1:
port := serviceSpec.Ports[0]
if port.Name != "" {
return intstr.FromString(port.Name)
}
return intstr.FromInt(int(port.Port))
case 0:
// Default to "api" as the operator always adds a default named port with 8000
return intstr.FromString("api")
default:
// Multiple ports: Prefer "metrics", fallback to "api"
if portName := selectNamedPort(serviceSpec, "metrics", "api"); portName != "" {
return intstr.FromString(portName)
}
// Default when multiple ports exist
return intstr.FromString("metrics")
Copy link
Collaborator

Choose a reason for hiding this comment

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

similarly here: Shouldn't we be getting the first port from the list if "metrics" and "api" dont exist?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

same logic as above, in documentation we have to specify that named ports for "api" or "metrics" should be provided when they are served differently by the underlying service. Randomly assigning would not work.

}
}
72 changes: 72 additions & 0 deletions api/apps/v1alpha1/common_types_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
/*
Copyright 2025.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package v1alpha1

import (
"testing"

corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/util/intstr"
)

// assertEqual checks if the actual result matches the expected result.
func assertEqual(t *testing.T, actual, expected intstr.IntOrString) {
if actual != expected {
t.Errorf("Got %v, expected %v", actual, expected)
}
}

func TestGetProbePort(t *testing.T) {
tests := []struct {
name string
serviceSpec Service
expected intstr.IntOrString
}{
{"Single named port", Service{Ports: []corev1.ServicePort{{Name: "api", Port: 8080}}}, intstr.FromString("api")},
{"Single unnamed port", Service{Ports: []corev1.ServicePort{{Port: 9090}}}, intstr.FromInt(9090)},
{"Multiple ports - prefers 'api'", Service{Ports: []corev1.ServicePort{{Name: "metrics", Port: 9090}, {Name: "api", Port: 8080}}}, intstr.FromString("api")},
{"No ports - uses legacy Port field", Service{Port: 7070}, intstr.FromString("api")},
{"No ports at all - defaults to 'api'", Service{}, intstr.FromString("api")},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
assertEqual(t, getProbePort(tt.serviceSpec), tt.expected)
})
}
}

func TestGetMetricsPort(t *testing.T) {
tests := []struct {
name string
serviceSpec Service
expected intstr.IntOrString
}{
{"Single named port", Service{Ports: []corev1.ServicePort{{Name: "metrics", Port: 8081}}}, intstr.FromString("metrics")},
{"Single unnamed port", Service{Ports: []corev1.ServicePort{{Port: 8181}}}, intstr.FromInt(8181)},
Copy link
Collaborator

Choose a reason for hiding this comment

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

trying to understand the logic.. in this case, why it shouldn't produce api like the test case in L63?

Copy link
Collaborator Author

@shivamerla shivamerla Feb 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

when say user provides input as below:

  expose:
    service:
      type: ClusterIP
      ports:
        - port: 8000
           protocol: TCP

then, in the deployment spec, we don't add any named ports like "api". Hence we use the port number that is given to us. we will add a default named port ("api: 8000" ) when no ports are given in the config at all.

{"Multiple ports - prefers 'metrics'", Service{Ports: []corev1.ServicePort{{Name: "api", Port: 8080}, {Name: "metrics", Port: 9090}}}, intstr.FromString("metrics")},
{"Multiple ports - no 'metrics', uses 'api'", Service{Ports: []corev1.ServicePort{{Name: "grpc", Port: 5050}, {Name: "api", Port: 8080}}}, intstr.FromString("api")},
{"No ports - uses legacy Port field", Service{Port: 6060}, intstr.FromString("api")},
{"No ports at all - defaults to 'metrics'", Service{}, intstr.FromString("api")},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
assertEqual(t, getMetricsPort(tt.serviceSpec), tt.expected)
})
}
}
87 changes: 44 additions & 43 deletions api/apps/v1alpha1/nemo_customizer_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,13 @@ import (
networkingv1 "k8s.io/api/networking/v1"
rbacv1 "k8s.io/api/rbac/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/intstr"
)

const (
// CustomizerAPIPort is the default port that customizer serves on
CustomizerAPIPort = 8000
// CustomizerInternalPort is the default port used for syncing training progress
CustomizerInternalPort = 9009
// NemoCustomizerConditionReady indicates that the NEMO CustomizerService is ready.
NemoCustomizerConditionReady = "Ready"
// NemoCustomizerConditionFailed indicates that the NEMO CustomizerService has failed.
Expand Down Expand Up @@ -436,10 +439,7 @@ func (n *NemoCustomizer) GetDefaultStartupProbe() *corev1.Probe {
ProbeHandler: corev1.ProbeHandler{
HTTPGet: &corev1.HTTPGetAction{
Path: "/v1/health/ready",
Port: intstr.IntOrString{
Type: intstr.Type(1),
StrVal: "api",
},
Port: getProbePort(n.Spec.Expose.Service),
},
},
}
Expand All @@ -466,11 +466,7 @@ func (n *NemoCustomizer) GetDefaultLivenessProbe() *corev1.Probe {
ProbeHandler: corev1.ProbeHandler{
HTTPGet: &corev1.HTTPGetAction{
Path: "/v1/health/live",
Port: intstr.IntOrString{
Type: intstr.Type(1),
StrVal: "api",
},
Scheme: "HTTP",
Port: getProbePort(n.Spec.Expose.Service),
},
},
}
Expand All @@ -496,10 +492,7 @@ func (n *NemoCustomizer) GetDefaultReadinessProbe() *corev1.Probe {
ProbeHandler: corev1.ProbeHandler{
HTTPGet: &corev1.HTTPGetAction{
Path: "/v1/health/ready",
Port: intstr.IntOrString{
Type: intstr.Type(1),
StrVal: "api",
},
Port: getProbePort(n.Spec.Expose.Service),
},
},
}
Expand Down Expand Up @@ -569,19 +562,14 @@ func (n *NemoCustomizer) GetInternalServicePort() int32 {
}

// Default to 9009 if no internal port is found
return 9009
return CustomizerInternalPort
}

// GetServicePorts returns the service ports for the NemoCustomizer deployment
func (n *NemoCustomizer) GetServicePorts() []corev1.ServicePort {
return n.Spec.Expose.Service.Ports
}

// GetServicePort returns the service port for the NemoCustomizer deployment
func (n *NemoCustomizer) GetServicePort() int32 {
return n.Spec.Expose.Service.Port
}

// GetServiceType returns the service type for the NemoCustomizer deployment
func (n *NemoCustomizer) GetServiceType() string {
return string(n.Spec.Expose.Service.Type)
Expand Down Expand Up @@ -661,24 +649,22 @@ func (n *NemoCustomizer) GetDeploymentParams() *rendertypes.DeploymentParams {
// Set runtime class
params.RuntimeClassName = n.GetRuntimeClass()

// Extract ports from spec and update rendering params
if len(n.GetServicePorts()) > 0 {
var containerPorts []corev1.ContainerPort
for _, svcPort := range n.GetServicePorts() {
containerPorts = append(containerPorts, corev1.ContainerPort{
Name: svcPort.Name,
Protocol: svcPort.Protocol,
ContainerPort: svcPort.Port,
})
}
params.Ports = containerPorts
} else {
params.Ports = []corev1.ContainerPort{{
// Setup container ports for customizer
// TODO: set these to use defined values in the service spec
// once that is allowed by the customizer through env variables
containerPorts := []corev1.ContainerPort{
{
Name: "api",
Protocol: corev1.ProtocolTCP,
ContainerPort: n.GetServicePort(),
}}
ContainerPort: CustomizerAPIPort,
},
{
Name: "internal",
Protocol: corev1.ProtocolTCP,
ContainerPort: CustomizerInternalPort,
},
}
params.Ports = containerPorts

return params
}
Expand Down Expand Up @@ -749,13 +735,19 @@ func (n *NemoCustomizer) GetServiceParams() *rendertypes.ServiceParams {
if len(servicePorts) != 0 {
params.Ports = servicePorts
} else {
// Use corev1.ServicePort instead of deprecated params.Port
params.Ports = []corev1.ServicePort{{
Name: "api",
Port: 8000,
TargetPort: intstr.FromInt(8000),
Protocol: corev1.ProtocolTCP,
}}
// Set default ports
params.Ports = []corev1.ServicePort{
{
Name: "api",
Port: CustomizerAPIPort,
Protocol: corev1.ProtocolTCP,
},
{
Name: "internal",
Port: CustomizerInternalPort,
Protocol: corev1.ProtocolTCP,
},
}
}

return params
Expand Down Expand Up @@ -892,11 +884,20 @@ func (n *NemoCustomizer) GetServiceMonitorParams() *rendertypes.ServiceMonitorPa
params.Labels = svcLabels
params.Annotations = n.GetServiceMonitorAnnotations()

// Determine the appropriate port for monitoring
metricsPort := getMetricsPort(n.Spec.Expose.Service)

// Set Service Monitor spec
smSpec := monitoringv1.ServiceMonitorSpec{
NamespaceSelector: monitoringv1.NamespaceSelector{MatchNames: []string{n.Namespace}},
Selector: metav1.LabelSelector{MatchLabels: n.GetServiceLabels()},
Endpoints: []monitoringv1.Endpoint{{Port: "service-port", ScrapeTimeout: serviceMonitor.ScrapeTimeout, Interval: serviceMonitor.Interval}},
Endpoints: []monitoringv1.Endpoint{
{
Port: metricsPort.StrVal,
ScrapeTimeout: serviceMonitor.ScrapeTimeout,
Interval: serviceMonitor.Interval,
},
},
}
params.SMSpec = smSpec
return params
Expand Down
Loading