adds an instance label to support multiple instances - #20906
Conversation
|
I can see there is also a multiple Keycloak test in testSingleSecretMultipleKeycloaks, if desired I can factor out common logic for creating 2 (or n) keycloaks. |
|
Hi @shawkins some naive questions from me inline.
I'm not sure what others from the team think, but today I cannot think about any use-case or scenario to support more than 63 characters. From my limited knowledge, it seems like a good practice to keep the name of the resources short.
As far as I'm aware today, Keycloak does not provide zero-downtime upgrades. If the brief downtime is a one-time event, and future upgrades won't face another downtime, honestly, I don't see a problem. Considering the implementation complexity required to prevent a downtime from the current version to the next. @mabartos @stianst do you see any issues?
@shawkins considering that keycloak.org will be used. It makes sense to have operator.keycloak.org. @stianst @mabartos any preference about this?
|
Great that will keep things nice and easy.
I think so too. If it seems sufficient that the user can check their CR to see an error condition indicating a failed dependent resource create, then there's nothing more that needs done with the pr we'll just implicitly rely on the kubernetes label limitation.
I'm fine with operator.keycloak.org as the prefix. For this pr it's currently using the standard app.kubernetes.io/instance, which is consistent with the other common labels we're adding (app, and app.kubernetes.io/managed-by). If anyone feels strongly that we should use our own label, then let's use operator.keycloak.org/instance instead. A related follow on will be to change the watched secrets label to use operator.keycloak.org. Opening that issue Label propogation is captured as #15395 |
I'd say it's sufficient. I'd rely on the Kubernetes limitation as was stated without the need to change these limits.
I'd use the default standard prefix with
Agree with @abstractj and provided additional complexity. It'd be nice to avoid downtime at all, but if there's no better approach, I'm ok with the brief downtime. The downtime is basically equal to the restart of the stateful set (more or less), right? @Pepo48 Do you have any input on this? |
@mabartos yes it will be similar to downtime of rolling in a new image. @abstractj added a doc entry into the migration to cover this. |
Those have been resolved. |
There was a problem hiding this comment.
This should be probably in the migration guide instead.
There was a problem hiding this comment.
Ok do you want to leave anything in the release notes about the feature, or just move the whole thing to the migration guide?
There was a problem hiding this comment.
I'd probably move the whole thing to the migration guide. It's not so big change for it to be in the release notes (it'll get there in any case as part of the list of fixed GH Issues).
There was a problem hiding this comment.
I think the one off downtime is fine. It'll happen only during migration from KC <=21 to >=22. For existing deployments it'll happen only once, for new deployments it doesn't happen at all. In any case, downtime happens during any KC image change, so it's inevitable.
But I have some other concerns.
- The reconciliation loop finishes in a desired state. That means that status fields will indicate KC as ready whereas in reality the deployment is being terminated. Although this is useful as a side effect to retrigger the reconciliation loop (that's necessary to re-create the StS again), I believe we should not indicate it's ready. Or perhaps that's ok since the deletion itself immediately triggers another reconciliation loop.
- Do we really not trust the cached version of existing StS? I'm not sure it's really desired to explicitly fetch the StS in every reconciliation loop. Are there any serious concerns that the cached version is incorrect? If so, I'd say it's an JOSDK issue.
There was a problem hiding this comment.
The reconciliation loop finishes in a desired state.
Ideally we won't have a blocking delete in the reconciliation loop. In this case the deletion of the statefulset should be basically immediate - the pods shouldn't block it - but I was still hesitent to add that in. I think kubectl scales statefulsets to 0 first probably for better behavior in a resource constrained environment, rather than relying on gc to clean up the pods. But that didn't seem necessary either.
That means that status fields will indicate KC as ready whereas in reality the deployment is being terminated. Although this is useful as a side effect to retrigger the reconciliation loop (that's necessary to re-create the StS again), I believe we should not indicate it's ready. Or perhaps that's ok since the deletion itself immediately triggers another reconciliation loop.
Correct, based upon the current state the status for that loop may indicate it's ready - however that will be ephemal. The event of marking the deletion timestamp on the statefulset, its eventual deletion, or any other event will trigger another reconciliation, that will update the status to whatever is observed at that point on the statefulset.
The alternative, which I'll change the pr to, is to just let the reconciliation logic proceed as normal after the delete call rather than returning an empty optional - if the statefulset still exists, we'll get an ephemeral error, if not then things will proceed.
Do we really not trust the cached version of existing StS? I'm not sure it's really desired to explicitly fetch the StS in every reconciliation loop.
It won't be fetched everytime. It's only for a final check prior to deletion.
Are there any serious concerns that the cached version is incorrect? If so, I'd say it's an JOSDK issue.
This isn't really a JOSDK issue. The JOSDK mitigates this in a couple of ways - for one all updates / deletes are locked with respect to the resourceVersion. So it's quite possible for the api server to throw a conflict error when there's been a change that has not yet been picked up by the informer watch - that is part of the motivation for refining the status handling so that the likely very ephemeral error doesn't seem worse than it is. We could do that check here instead - if the resourceVersion is set on the existing (which is should be except for some of the tests) then issue a delete locked to that version. I guess I thought that it would be clearer doing another fetch, but I'll change it do a locked operation instead.
Another mechanism in the JOSDK is that your own changes to the resource are tracked in another cache beside the informer cache, so as long as it's you making the changes you don't have to react to your own events. In the case of a statefulset it can of course have other modifications from the statefulset controller.
There was a problem hiding this comment.
A consequence of these changes is that it becomes hard to test the migration behavior with the current integration test logic. We need to create a keycloak instance and modify the associated statefulset and replace it with one that has the old match labels. More than likely this would require an additional setup step where the keycloak and statefulset could be created priort to operator running, or the introduction of a pause reconciliation annotation so that we could modify the statefulset later, then remove the annotation and let the operator migrate it.
There was a problem hiding this comment.
Thanks for the elaborate reply.
To me, a locked delete feels more correct than a fetch every time the reconciliation loop runs. The delete will be performed only once, if ever.
I agree testing it would be a bit harder, but I think it's the way to go.
There was a problem hiding this comment.
To me, a locked delete feels more correct than a fetch every time the reconciliation loop runs. The delete will be performed only once, if ever.
To clarify it was only doing the fetch if the existing appeared to be in the old state, so that would only occur a limited number of times during migration.
I agree testing it would be a bit harder, but I think it's the way to go.
Are you in favor of putting in a pause mechanism as part of this pr?
There was a problem hiding this comment.
To clarify it was only doing the fetch if the existing appeared to be in the old state, so that would only occur a limited number of times during migration.
Correct, sorry for the confusion from my side. :) Still, locked version feels more correct IMHO.
Are you in favor of putting in a pause mechanism as part of this pr?
A silly question. Can't we simply manually create the StS in test (prior creating the Keycloak CR) and create the CR after that for Operator to migrate the StS?
There was a problem hiding this comment.
A silly question. Can't we simply manually create the StS in test (prior creating the Keycloak CR) and create the CR after that for Operator to migrate the StS?
With a few other test changes. If we create the sts first, then we need to not wait for the keycloak to become ready first (it will keep failing as the sts is in the way). Then update the sts to have the owner reference of the newly created sts, at that point it would be picked up by the informer. Let me do it this way and save the pause annotation for another day.
|
Updated the docs and the test - other than some logging of failures it does exercise this path. |
Updates the dependent label and label selectors to include the kubernetes recommended instance label. This renders moot the concern of #20854 in that any keycloak created with a name length over 63 characters will effectively be invalid as the label values for the instance name will then be invalid. If supporting keycloak resource names greater than 63 characters is desirable or if we want the controller to validate the name length, I can address that with this pr.
The biggest issue that needs more discussion is https://github.com/keycloak/keycloak/compare/main...shawkins:iss10562?expand=1#diff-a4e5365416b1195232c06a11372ec4f5b48f0527e2a0de2ee356ec0b92697506R96 that is we aren't allowed to change the statefulet labelselectors because they are immutable. This pr adds a simple check for this and deletes the existing statefulset, but it will cause a brief downtime for anyone who upgrades to this operator version. Other options include:
Also to the point of it of #10562 you could opt for a keycloak specific instance label instead (for a namespace keycloak.org is being used, operator.keycloak.org was suggested on the issue) - if this is done late, or anything that affects the statefulset selectors, we'll hit the same issue again. So it's best to make sure we're good with what's being done here.
Closes #10562 #14220 - but does not address the concern of propogating other labels - for example general keycloak.metadata.labels or other cr locations. I think this is only happening now via the unsupported pod template. Since this labels should not be part of the selectors, I agree with #10562 that is should be a separate enhancement.