introduce service configuration at configmap level #3672
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: spolti The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| type ServiceConfig struct { | ||
| // ServiceClusterIPNone is a flag to indicate if the service should have a clusterIP set to None. | ||
| // If the DeploymentMode is Raw, the default value for ServiceClusterIPNone is true when the value is absent. | ||
| ServiceClusterIPNone *bool `json:"serviceClusterIPNone,omitempty"` | ||
| } | ||
|
|
There was a problem hiding this comment.
@yuzisun @sivanantha321 any thoughts on this?
spolti
force-pushed
the
CipNone
branch
5 times, most recently
from
07d380b
to
1d8c407
Compare
|
/rerun-all |
|
/hold |
spolti
force-pushed
the
CipNone
branch
7 times, most recently
from
2fb48f6
to
2f43284
Compare
|
test-graph is failing. Maybe push empty commits to see if it's flaky. |
chore: This PR introduces a new configuration in the inferenceservice-config configMap to enable user to set the service `ClusterIP` to `None` to rely on headless services. With this configuration, users are enabled to rely on third party balancers having total control over it. The reasoning behind opening a new configuration support is to allow it easier to customize any other important flag at the service creation in the future Signed-off-by: Spolti <fspolti@redhat.com>
|
/unhold |
|
|
||
| # ====================================== SERVICE CONFIGURATION ====================================== | ||
| # Example | ||
| service: |- |
There was a problem hiding this comment.
I wonder if it isn't better to reuse the deploy key, rather than creating a new one just for the Service?
There was a problem hiding this comment.
We had defined a new option in the config to not mix the configuration purpose, this affects only the service created, for now only Raw.
But in the future, more fields can be easily added here if needed..
| service.Spec.ClusterIP = corev1.ClusterIPNone | ||
| } | ||
| } else if raw { | ||
| service.Spec.ClusterIP = corev1.ClusterIPNone |
There was a problem hiding this comment.
I'm not sure this is a safe default. Clients would need to know how to handle the multiple DNS entries. Also, KServe defaults would diverge from Kubernetes defaults...
Most importantly, I see this as a backwards incompatible change.
There was a problem hiding this comment.
It is the default only when raw deployments and the flag is not explicitly set to false. However, it is just a proposal atm, we need to define first the way we will be handling this configuration
| # ServiceClusterIPNone is a flag to indicate if the service should have a clusterIP set to None. | ||
| # This is particularly useful for headless services to allow users to use out of the box balancers. | ||
| # If the DeploymentMode is Raw, the default value for ServiceClusterIPNone is true |
There was a problem hiding this comment.
| # ServiceClusterIPNone is a flag to indicate if the service should have a clusterIP set to None. | |
| # This is particularly useful for headless services to allow users to use out of the box balancers. | |
| # If the DeploymentMode is Raw, the default value for ServiceClusterIPNone is true | |
| # ServiceClusterIPNone indicates if KServe should create headless services (clusterIP=None). | |
| # If the DeploymentMode is Raw, the default value for ServiceClusterIPNone is true |
External LBs is probably just one use case for a headless service. I'd omit such details.
chore: This PR introduces a new configuration in the inferenceservice-config
configMap to enable user to set the service
ClusterIPtoNoneto rely onheadless services.
With this configuration, users are enabled to rely on third party balancers
having total control over it.
The reasoning behind opening a new configuration support is to allow it easier
to customize any other important flag at the service creation in the future
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Type of changes
Please delete options that are not relevant.
Feature/Issue validation/testing:
Please describe the tests that you ran to verify your changes and relevant result summary. Provide instructions so it can be reproduced.
Please also list any relevant details for your test configuration.
Test A
Test B
Logs
Special notes for your reviewer:
Checklist:
Release note:
Re-running failed tests
/rerun-all- rerun all failed workflows./rerun-workflow <workflow name>- rerun a specific failed workflow. Only one workflow name can be specified. Multiple /rerun-workflow commands are allowed per comment.