8000 making the update reason and recreate annotations stable by shawkins · Pull Request #38844 · keycloak/keycloak · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

making the update reason and recreate annotations stable #38844

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

Merged
merged 2 commits into from
Apr 14, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
import io.fabric8.kubernetes.api.model.EnvVarBuilder;
import io.fabric8.kubernetes.api.model.EnvVarSource;
import io.fabric8.kubernetes.api.model.EnvVarSourceBuilder;
import io.fabric8.kubernetes.api.model.ObjectMetaFluent;
import io.fabric8.kubernetes.api.model.PodSpecFluent;
import io.fabric8.kubernetes.api.model.PodTemplateSpec;
import io.fabric8.kubernetes.api.model.Secret;
Expand Down Expand Up @@ -130,7 +129,7 @@ public void setUseServiceCaCrt(boolean useServiceCaCrt) {
public StatefulSet desired(Keycloak primary, Context<Keycloak> context) {
Config operatorConfig = ContextUtils.getOperatorConfig(context);
WatchedResources watchedResources = ContextUtils.getWatchedResources(context);

StatefulSet baseDeployment = createBaseDeployment(primary, context, operatorConfig);
TreeSet<String> allSecrets = new TreeSet<>();
if (isTlsConfigured(primary)) {
Expand All @@ -147,23 +146,33 @@ public StatefulSet desired(Keycloak primary, Context<Keycloak> context) {
watchedResources.annotateDeployment(new ArrayList<>(allSecrets), Secret.class, baseDeployment, context.getClient());
}

// add revision from CR if set
// default to the new revision - will be overriden to the old one if needed
UpdateSpec.getRevision(primary).ifPresent(rev -> addUpdateRevisionAnnotation(rev, baseDeployment));

var existingDeployment = ContextUtils.getCurrentStatefulSet(context).orElse(null);

if (existingDeployment != null) {
// copy the existing annotations to keep the status consistent
CRDUtils.findUpdateReason(existingDeployment).ifPresent(r -> baseDeployment.getMetadata().getAnnotations()
.put(Constants.KEYCLOAK_UPDATE_REASON_ANNOTATION, r));
CRDUtils.fetchIsRecreateUpdate(existingDeployment).ifPresent(b -> baseDeployment.getMetadata()
.getAnnotations().put(Constants.KEYCLOAK_RECREATE_UPDATE_ANNOTATION, b.toString()));
}

var updateType = ContextUtils.getUpdateType(context);
// empty means no existing stateful set.
if (updateType.isEmpty()) {

if (existingDeployment == null || updateType.isEmpty()) {
return baseDeployment;
}

var existingDeployment = ContextUtils.getCurrentStatefulSet(context).orElseThrow();

// version 22 changed the match labels, account for older versions
if (!existingDeployment.isMarkedForDeletion() && !hasExpectedMatchLabels(existingDeployment, primary)) {
context.getClient().resource(existingDeployment).lockResourceVersion().delete();
Log.info("Existing Deployment found with old label selector, it will be recreated");
}

baseDeployment.getMetadata().getAnnotations().put(Constants.KEYCLOAK_UPDATE_REASON_ANNOTATION, ContextUtils.getUpdateReason(context));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is updating the metadata safe without using the "builder" classes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is. We don't need another copy because the desired is a working copy.

We also want to avoid using the actual state as a whole:

The operator sdk was allowing this to happen - but should not have been (we already removed similar logic from fabric8). When using server side apply there could other stuff contributed by other controllers. Applying all of the actual state will make our operator assume ownership of everything.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the explanation.


return switch (updateType.get()) {
case ROLLING -> handleRollingUpdate(baseDeployment, context, primary);
case RECREATE -> handleRecreateUpdate(existingDeployment, baseDeployment, context);
Expand Down Expand Up @@ -551,43 +560,31 @@ protected Optional<String> readConfigurationValue(String key, Keycloak keycloakC
private static StatefulSet handleRollingUpdate(StatefulSet desired, Context<Keycloak> context, Keycloak primary) {
// return the desired stateful set since Kubernetes does a rolling in-place update by default.
Log.debug("Performing a rolling update");
var builder = desired.toBuilder()
.editMetadata()
.addToAnnotations(Constants.KEYCLOAK_RECREATE_UPDATE_ANNOTATION, "false")
.addToAnnotations(Constants.KEYCLOAK_UPDATE_REASON_ANNOTATION, ContextUtils.getUpdateReason(context));
return builder.endMetadata().build();
desired.getMetadata().getAnnotations().put(Constants.KEYCLOAK_RECREATE_UPDATE_ANNOTATION, Boolean.FALSE.toString());
return desired;
}

private static StatefulSet handleRecreateUpdate(StatefulSet actual, StatefulSet desired, Context<Keycloak> context) {
if (actual.getStatus().getReplicas() == 0) {
private static StatefulSet handleRecreateUpdate(StatefulSet actual, StatefulSet desired,
Context<Keycloak> context) {
desired.getMetadata().getAnnotations().put(Constants.KEYCLOAK_RECREATE_UPDATE_ANNOTATION, Boolean.TRUE.toString());

if (Optional.ofNullable(actual.getStatus().getReplicas()).orElse(0) == 0) {
Log.debug("Performing a recreate update - scaling up the stateful set");
return desired;

// desired state correct as is
} else {
Log.debug("Performing a recreate update - scaling down the stateful set");

// keep the old revision, mark as migrating, and scale down
CRDUtils.getRevision(actual).ifPresent(rev -> addUpdateRevisionAnnotation(rev, desired));
desired.getMetadata().getAnnotations().put(Constants.KEYCLOAK_MIGRATING_ANNOTATION, Boolean.TRUE.toString());
desired.getSpec().setReplicas(0);
}
Log.debug("Performing a recreate update - scaling down the stateful set");
// return the existing stateful set, but set replicas to zero
var builder = actual.toBuilder();
builder.editSpec()
.withReplicas(0)
.endSpec();
// update metadata from the new stateful set, it is safe to do so.
var metadataBuilder = desired.getMetadata().edit()
.addToAnnotations(Constants.KEYCLOAK_MIGRATING_ANNOTATION, Boolean.TRUE.toString())
.addToAnnotations(Constants.KEYCLOAK_RECREATE_UPDATE_ANNOTATION, Boolean.TRUE.toString())
.addToAnnotations(Constants.KEYCLOAK_UPDATE_REASON_ANNOTATION, ContextUtils.getUpdateReason(context));
// copy revision number from the previous stateful set.
CRDUtils.getRevision(actual).ifPresent(rev -> addUpdateRevisionAnnotation(rev, metadataBuilder));
return builder
.withMetadata(metadataBuilder.build())
.build();
return desired;
}

private static void addUpdateRevisionAnnotation(String revision, StatefulSet toUpdate) {
var metadataBuilder = toUpdate.getMetadata().edit();
addUpdateRevisionAnnotation(revision, metadataBuilder);
toUpdate.setMetadata(metadataBuilder.build());
toUpdate.getMetadata().getAnnotations().put(Constants.KEYCLOAK_UPDATE_REVISION_ANNOTATION, revision);
}

private static void addUpdateRevisionAnnotation(String revision, ObjectMetaFluent<?> builder) {
builder.addToAnnotations(Constants.KEYCLOAK_UPDATE_REVISION_ANNOTATION, revision);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,7 @@ public void testNoJobReuse() throws InterruptedException {
@Test
public void testExplicitStrategy() throws InterruptedException {
var kc = createInitialDeployment(UpdateStrategy.EXPLICIT);
disableProbes(kc);

var updateCondition = assertUnknownUpdateTypeStatus(kc);
deployKeycloak(k8sclient, kc, true);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,8 @@ public static CompletableFuture<Void> eventuallyRollingUpdateStatus(KubernetesCl
public static CompletableFuture<Void> eventuallyRecreateUpdateStatus(KubernetesClient client, Keycloak keycloak, String reason) {
var cf1 = client.resource(keycloak).informOnCondition(kcs -> {
try {
assertKeycloakStatusCondition(kcs.get(0), KeycloakStatusCondition.READY, false, "Performing Keycloak update");
// could be not ready "Performing Keycloak update", or "Waiting for more replicas"
assertKeycloakStatusCondition(kcs.get(0), KeycloakStatusCondition.READY, false, null);
return true;
} catch (AssertionError e) {
return false;
Expand Down
0