Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
38 changes: 37 additions & 1 deletion api/v1/inferencepool_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ type InferencePoolSpec struct {
// +kubebuilder:validation:MinItems=1
// +kubebuilder:validation:MaxItems=8
// +kubebuilder:validation:XValidation:message="port number must be unique",rule="self.all(p1, self.exists_one(p2, p1.number==p2.number))"
// +kubebuilder:validation:XValidation:message="all ports must have the same AppProtocol",rule="self.all(p, (has(p.appProtocol) ? p.appProtocol : 'Unset') == (has(self[0].appProtocol) ? self[0].appProtocol : 'Unset'))"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why define it in TargetPort if all ports must share the same protocol? why not define it directly here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good call, listing the pros and cons below. I feel add a single appProtocol in InferenceSpec is better. So I changed accordingly. PTAL

Option 1. the original change: add appProtocol to Port and create a separate Port for EndPointPicker.

Pros:

  1. more aligned with the structure of servicePort putting Port and AppProtocol at the same level.
  2. give possibility for future mix gRPC and http model support

Cons:

  1. break the goClient backward compatibility
  2. poor user experience, user has to explicitly set all the appProtocol for every ports

Option 2. new solution(the comment here), add appProtocol in parallel with targetPorts.

Pros:

  1. simple change
  2. no breaking change even in go client.
  3. simple user experience

Cons:

  1. a little different from servicePort
  2. don't support mix protocol model servers. But this could be a very rare use cases. and we can always introduce new appProtocol field to Port(as option 1) to override it if necessary.

// +listType=atomic
// +required
TargetPorts []Port `json:"targetPorts,omitempty"`
Expand All @@ -94,8 +95,34 @@ type Port struct {
//
// +required
Number PortNumber `json:"number,omitempty"`

// AppProtocol describes the application protocol for this port.
//
// If unspecified, the protocol defaults to HTTP/1.1.
//
// Supported values include:
// * "http": HTTP/1.1. This is the default.
// * "kubernetes.io/h2c": HTTP/2 over cleartext.
//
// +kubebuilder:validation:Enum=http;"kubernetes.io/h2c"
// +optional
AppProtocol AppProtocol `json:"appProtocol,omitempty"`
}

// AppProtocol describes the application protocol for a port.
type AppProtocol string

const (
// AppProtocolHTTP represents the HTTP/1.1 protocol.
// This is the default protocol if AppProtocol is unspecified.
AppProtocolHTTP AppProtocol = "http"

// AppProtocolH2C represents HTTP/2 over cleartext (h2c).
// This protocol is typically used for gRPC workloads where TLS is terminated
// at the Gateway or not used within the cluster.
AppProtocolH2C AppProtocol = "kubernetes.io/h2c"
)

// EndpointPickerRef specifies a reference to an Endpoint Picker extension and its
// associated configuration.
// +kubebuilder:validation:XValidation:rule="self.kind != 'Service' || has(self.port)",message="port is required when kind is 'Service' or unspecified (defaults to 'Service')"
Expand Down Expand Up @@ -136,7 +163,7 @@ type EndpointPickerRef struct {
// resource or this field.
//
// +optional
Port *Port `json:"port,omitempty"`
Port *EndpointPickerPort `json:"port,omitempty"`

// FailureMode configures how the parent handles the case when the Endpoint Picker extension
// is non-responsive. When unspecified, defaults to "FailClose".
Expand All @@ -146,6 +173,15 @@ type EndpointPickerRef struct {
FailureMode EndpointPickerFailureMode `json:"failureMode,omitempty"`
}

// EndpointPickerPort defines the network port for the Endpoint Picker extension.
type EndpointPickerPort struct {
// Number defines the port number of the Endpoint Picker service.
// The number must be in the range 1 to 65535.
//
// +required
Number PortNumber `json:"number,omitempty"`
}

// EndpointPickerFailureMode defines the options for how the parent handles the case when the
// Endpoint Picker extension is non-responsive.
//
Expand Down
17 changes: 16 additions & 1 deletion api/v1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion apix/v1alpha2/inferencepool_conversion.go
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ func convertExtensionRefToV1(src *Extension) (v1.EndpointPickerRef, error) {
}
endpointPickerRef.Name = v1.ObjectName(src.Name)
if src.PortNumber != nil {
endpointPickerRef.Port = ptr.To(v1.Port{Number: v1.PortNumber(*src.PortNumber)})
endpointPickerRef.Port = ptr.To(v1.EndpointPickerPort{Number: v1.PortNumber(*src.PortNumber)})
}
if src.FailureMode != nil {
endpointPickerRef.FailureMode = v1.EndpointPickerFailureMode(*src.FailureMode)
Expand Down
2 changes: 1 addition & 1 deletion apix/v1alpha2/inferencepool_conversion_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ var (
v1Group = v1.Group("my-group")
v1Kind = v1.Kind("MyKind")
v1FailureMode = v1.EndpointPickerFailureMode("Deny")
v1Port = v1.Port{Number: 9000}
v1Port = v1.EndpointPickerPort{Number: 9000}
)

func TestInferencePoolConvertTo(t *testing.T) {
Expand Down
43 changes: 43 additions & 0 deletions client-go/applyconfiguration/api/v1/endpointpickerport.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 6 additions & 6 deletions client-go/applyconfiguration/api/v1/endpointpickerref.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 10 additions & 1 deletion client-go/applyconfiguration/api/v1/port.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions client-go/applyconfiguration/utils.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

18 changes: 17 additions & 1 deletion config/crd/bases/inference.networking.k8s.io_inferencepools.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ spec:
properties:
number:
description: |-
Number defines the port number to access the selected model server Pods.
Number defines the port number of the Endpoint Picker service.
The number must be in the range 1 to 65535.
format: int32
maximum: 65535
Expand Down Expand Up @@ -162,6 +162,19 @@ spec:
description: Port defines the network port that will be exposed
by this InferencePool.
properties:
appProtocol:
description: |-
AppProtocol describes the application protocol for this port.

If unspecified, the protocol defaults to HTTP/1.1.

Supported values include:
* "http": HTTP/1.1. This is the default.
* "kubernetes.io/h2c": HTTP/2 over cleartext.
enum:
- http
- kubernetes.io/h2c
type: string
number:
description: |-
Number defines the port number to access the selected model server Pods.
Expand All @@ -180,6 +193,9 @@ spec:
x-kubernetes-validations:
- message: port number must be unique
rule: self.all(p1, self.exists_one(p2, p1.number==p2.number))
- message: all ports must have the same AppProtocol
rule: 'self.all(p, (has(p.appProtocol) ? p.appProtocol : ''Unset'')
== (has(self[0].appProtocol) ? self[0].appProtocol : ''Unset''))'
required:
- endpointPickerRef
- selector
Expand Down
Loading
Loading