fix: keeping the image consistent for the recreate strategy #39714
Conversation
closes: keycloak#39710 Signed-off-by: Steve Hawkins <shawkins@redhat.com>
| desired.getMetadata().getAnnotations().put(Constants.KEYCLOAK_MIGRATING_ANNOTATION, Boolean.TRUE.toString()); | ||
| desired.getSpec().setReplicas(0); | ||
| var currentImage = RecreateOnImageChangeUpdateLogic.extractImage(actual); | ||
| kcContainer.setImage(currentImage); |
There was a problem hiding this comment.
Instead of modifying the replicas in the desired, shouldn't we modify the replicas in the actual?
Edit: I'm asking because I'm concerned that the same incorrect behavior may happen if the user changes the configuration (i.e. the EnvVar)
There was a problem hiding this comment.
Instead of modifying the replicas in the desired, shouldn't we modify the replicas in the actual?
desired is what is actually being returned to the operator sdk. We don't want to use the actual as the desired. What we're effectively relying upon the controller behavior that honors the replica count first, then rolls out any changes in the statefulset spec.
Edit: I'm asking because I'm concerned that the same incorrect behavior may happen if the user changes the configuration (i.e. the EnvVar)
I don't follow that concern. This pr reinstates the recreate behavior we've always had. Are you thinking that there is some scenario where a change in the env will somehow be rolled out prior to the replica count being honored?
Alternative behavior would be working against the operator sdk / SSA - that is rather than honoring a desired state we'd have to directly isssue a patch against the actual.
There was a problem hiding this comment.
Are you thinking that there is some scenario where a change in the env will somehow be rolled out prior to the replica count being honored?
Yes exactly. Is that possible?
There was a problem hiding this comment.
No, the controller will respect the replica reduction first - why restart the pods if you are just going to scale them down anyway.
There was a problem hiding this comment.
So, I don't understand why it does not respect the replica count when the image is different. 🤔
There was a problem hiding this comment.
The problem is with our logic if we don't keep the old image - the recreate stategy needs to see that difference. If it sees the new image on the actual, it will let the desired logic immediately set the replica count back to what is in the keycloak cr.
You could alternatively try to account for this in the recreate stategy logic - if the images are the same, but the actual statefulset spec has 0 replicas, and the status replicas are greater than 0, then keep using the recreate stategy. It seems simpler to just keep the desired image as the old image.
There was a problem hiding this comment.
Ah! Gotcha! Sorry for being slow 😅 It makes sense now. Thank you.
closes: #39710