fix: adding the discovery servicename to the statefulset #39419
Conversation
closes: keycloak#38757 Signed-off-by: Steve Hawkins <shawkins@redhat.com>
|
Double checked locally that it doesn't matter what order the statefulset and service are created in - kube / minikube will create the statefulset just fine without the service in existence, and it won't cause a restart to create it after the fact. |
| @@ -287,6 +287,7 @@ private StatefulSet createBaseDeployment(Keycloak keycloakCR, Context<Keycloak> | |||
| .editOrNewSpec().withImagePullSecrets(keycloakCR.getSpec().getImagePullSecrets()).endSpec() | |||
| .endTemplate() | |||
| .withReplicas(keycloakCR.getSpec().getInstances()) | |||
| .withServiceName(KeycloakDiscoveryServiceDependentResource.getName(keycloakCR)) | |||
There was a problem hiding this comment.
Should it rather use the main service?
| .withServiceName(KeycloakDiscoveryServiceDependentResource.getName(keycloakCR)) | |
| .withServiceName(KeycloakServiceDependentResource.getServiceName(keycloakCR)) |
There was a problem hiding this comment.
No, it's supposed to be a headless service.
There was a problem hiding this comment.
From the docs it does not seem so?
Additionally it says:
This service must exist before the StatefulSet
Despite it now works with StS created before the Service, can we really rely on it as that behaviour might change to align with the spec?
There was a problem hiding this comment.
From the docs it does not seem so?
I'm referencing https://kubernetes.io/docs/concepts/workloads/controllers/statefulset/#stable-network-id
I suppose the language there does say "can", not "must" be headless.
Despite it now works with StS created before the Service, can we really rely on 8000 it as that behaviour might change to align with the spec?
This behavior / validation has been poorly defined for quite some time. kubernetes/kubernetes#69608 was logged years ago.
Even if it doesn't "work" at some point in the furture, the worst case behavior would be the statefulset not scaling up until the service is created - since these are peer dependent resources, that would be a narrow window.
I'm leary of adding any kind of reconciliation condition for this kind of behavior - instead if you are worried about not eventually seeing some kind of ephemeral error, then I'd opt instead for Dependent.dependsOn
There was a problem hiding this comment.
I'm referencing https://kubernetes.io/docs/concepts/workloads/controllers/statefulset/#stable-network-id
I suppose the language there does say "can", not "must" be headless.
I see. Well, it says headless service so probably there's no going around it. However, it feels a bit odd to me that the KC Pod's DNS records would be derived from the Discovery Service. Moreover, what if we switch to the JDBC ping in the future, so that Service is not needed anymore?
But it's not a blocker for me, I don't have a better idea. We might create a dedicated headlessService just for this purpose but that seems like an overkill at this point.
There was a problem hiding this comment.
Even if it doesn't "work" at some point in the furture, the worst case behavior would be the statefulset not scaling up until the service is created - since these are peer dependent resources, that would be a narrow window.
Makes sense 👍
There was a problem hiding this comment.
We might create a dedicated headlessService just for this purpose but that seems like an overkill at this point.
I think we'd just leave the discovery service in place. There's not much harm / risk and users may have already adopted using it for their own purposes as well.
) closes: keycloak#38757 Signed-off-by: Steve Hawkins <shawkins@redhat.com>
) closes: keycloak#38757 Signed-off-by: Steve Hawkins <shawkins@redhat.com>
closes: #38757