-
Notifications
You must be signed in to change notification settings - Fork 45
fix: respect Controller service clusterIP configuration for headless services #63
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.
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
base: main
Are you sure you want to change the base?
Conversation
This commit addresses an issue where the Slurm operator was ignoring the `clusterIP` configuration, preventing the creation of headless services necessary for StatefulSet pod DNS resolution. The `BuildControllerService()` function now checks the `ServiceSpec.ClusterIP` and sets `Headless` to true when `clusterIP: None` is specified. - Added a test case to verify headless service functionality - Ensured backward compatibility with existing ClusterIP services Signed-off-by: Krishnaswamy Subramanian <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
The other Slurm cluster components, which allow a service configuration, suffer from this same bug. If you would, please exptend the fix to all Slurm component services. Thanks.
|
||
// Test headless service configuration | ||
if tt.name == "headless service" { | ||
if got.Spec.ClusterIP != corev1.ClusterIPNone { | ||
t.Errorf("Expected headless service (ClusterIP=None), got ClusterIP=%v", got.Spec.ClusterIP) | ||
} | ||
if !got.Spec.PublishNotReadyAddresses { | ||
t.Errorf("Expected PublishNotReadyAddresses=true for headless service, got %v", got.Spec.PublishNotReadyAddresses) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a fragile methodology to express the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed the fragile tests
Perhaps bug is not right. Headless services are strictly defined by You should be able to do the following: controller:
service:
spec:
clusterIP: None
publishNotReadyAddresses: true Once a service is created, Kubernetes does not seem to allow the spec to be altered but it does not throw an error. That is an entirely different issue to handle. |
…d restapi Added support for headless services in the Builder for Accounting, Login, and RestAPI services. The ClusterIP is now set based on the ServiceSpec configuration, allowing for better service management in Kubernetes environments. Updated associated tests to validate new functionality.
@SkylerMalinowski kindly review the changes |
Description
This PR fixes #62 where the Slurm operator ignores the
controller.spec.service.spec.clusterIP
configuration, preventing the creation of headless services required for StatefulSet pod DNS resolution.Problem
clusterIP: None
Solution
BuildControllerService()
to checkServiceSpec.ClusterIP
and setHeadless: true
whenclusterIP: None
Changes Made
/internal/builder/controller_service.go
func (b *Builder) BuildControllerService(controller *slinkyv1alpha1.Controller) (*corev1.Service, error) { spec := controller.Spec.Service opts := ServiceOpts{ Key: controller.ServiceKey(), Metadata: controller.Spec.Template.PodMetadata, ServiceSpec: controller.Spec.Service.ServiceSpecWrapper.ServiceSpec, Selector: labels.NewBuilder(). WithControllerSelectorLabels(controller). Build(), + Headless: controller.Spec.Service.ServiceSpecWrapper.ServiceSpec.ClusterIP == corev1.ClusterIPNone, }
/internal/builder/controller_service_test.go
ClusterIP=None
andPublishNotReadyAddresses=true
Testing
clusterIP: None
is specifiedImpact
pod-name.service-name.namespace
)Helm Chart Deployment Issue
Installing the SlinkyProject Helm chart with headless service configuration fails due to this bug:
Configuration that was failing:
Symptoms observed:
After this fix: