From 4e83b9be9d730a2fe89c56bda06180924b98c77a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=A1clav=20Muzik=C3=A1=C5=99?= Date: Tue, 26 Jul 2022 18:37:20 +0200 Subject: [PATCH] `Recreate` upgrade strategy using the new Operator (#13326) Co-authored-by: Dominik Guhr --- .../controllers/KeycloakDeployment.java | 37 ++++++++++++++- .../integration/KeycloakDeploymentTest.java | 46 +++++++++++++++++++ 2 files changed, 81 insertions(+), 2 deletions(-) diff --git a/operator/src/main/java/org/keycloak/operator/controllers/KeycloakDeployment.java b/operator/src/main/java/org/keycloak/operator/controllers/KeycloakDeployment.java index 4ade5f600b..21cbaf5447 100644 --- a/operator/src/main/java/org/keycloak/operator/controllers/KeycloakDeployment.java +++ b/operator/src/main/java/org/keycloak/operator/controllers/KeycloakDeployment.java @@ -60,6 +60,8 @@ public class KeycloakDeployment extends OperatorManagedResource implements Statu private Set serverConfigSecretsNames; + private boolean migrationInProgress; + public KeycloakDeployment(KubernetesClient client, Config config, Keycloak keycloakCR, StatefulSet existingDeployment, String adminSecretName) { super(client, keycloakCR); this.config = config; @@ -101,6 +103,8 @@ public class KeycloakDeployment extends OperatorManagedResource implements Statu Optional.ofNullable(reconciledDeployment.getSpec().getTemplate().getMetadata()).map(m -> m.getAnnotations()).orElse(null), annotations -> reconciledDeployment.getSpec().getTemplate().getMetadata().setAnnotations(annotations)); } + + migrateDeployment(existingDeployment, reconciledDeployment); } return Optional.of(reconciledDeployment); @@ -402,7 +406,7 @@ public class KeycloakDeployment extends OperatorManagedResource implements Statu } var userRelativePath = readConfigurationValue(Constants.KEYCLOAK_HTTP_RELATIVE_PATH_KEY); - var kcRelativePath = (userRelativePath == null) ? "/" : userRelativePath; + var kcRelativePath = (userRelativePath == null) ? "" : userRelativePath; var protocol = (this.keycloakCR.getSpec().isHttp()) ? "http" : "https"; var kcPort = (this.keycloakCR.getSpec().isHttp()) ? Constants.KEYCLOAK_HTTP_PORT : Constants.KEYCLOAK_HTTPS_PORT; @@ -605,7 +609,9 @@ public class KeycloakDeployment extends OperatorManagedResource implements Statu status.addNotReadyMessage("Waiting for more replicas"); } - if (existingDeployment.getStatus() != null + if (migrationInProgress) { + status.addNotReadyMessage("Performing Keycloak upgrade, scaling down the deployment"); + } else if (existingDeployment.getStatus() != null && existingDeployment.getStatus().getCurrentRevision() != null && existingDeployment.getStatus().getUpdateRevision() != null && !existingDeployment.getStatus().getCurrentRevision().equals(existingDeployment.getStatus().getUpdateRevision())) { @@ -633,6 +639,33 @@ public class KeycloakDeployment extends OperatorManagedResource implements Statu .rolling().restart(); } + public void migrateDeployment(StatefulSet previousDeployment, StatefulSet reconciledDeployment) { + if (previousDeployment == null + || previousDeployment.getSpec() == null + || previousDeployment.getSpec().getTemplate() == null + || previousDeployment.getSpec().getTemplate().getSpec() == null + || previousDeployment.getSpec().getTemplate().getSpec().getContainers() == null + || previousDeployment.getSpec().getTemplate().getSpec().getContainers().get(0) == null) + { + return; + } + + var previousContainer = previousDeployment.getSpec().getTemplate().getSpec().getContainers().get(0); + var reconciledContainer = reconciledDeployment.getSpec().getTemplate().getSpec().getContainers().get(0); + + if (!previousContainer.getImage().equals(reconciledContainer.getImage()) + && previousDeployment.getStatus().getReplicas() > 1) { + // TODO Check if migration is really needed (e.g. based on actual KC version); https://github.com/keycloak/keycloak/issues/10441 + Log.info("Detected changed Keycloak image, assuming Keycloak upgrade. Scaling down the deployment to one instance to perform a safe database migration"); + Log.infof("original image: %s; new image: %s"); + + reconciledContainer.setImage(previousContainer.getImage()); + reconciledDeployment.getSpec().setReplicas(1); + + migrationInProgress = true; + } + } + public static String getEnvVarName(String kcConfigName) { // TODO make this use impl from Quarkus dist (Configuration.toEnvVarFormat) return "KC_" + replaceNonAlphanumericByUnderscores(kcConfigName).toUpperCase(); diff --git a/operator/src/test/java/org/keycloak/operator/testsuite/integration/KeycloakDeploymentTest.java b/operator/src/test/java/org/keycloak/operator/testsuite/integration/KeycloakDeploymentTest.java index a4d512b81d..fa0520ed8e 100644 --- a/operator/src/test/java/org/keycloak/operator/testsuite/integration/KeycloakDeploymentTest.java +++ b/operator/src/test/java/org/keycloak/operator/testsuite/integration/KeycloakDeploymentTest.java @@ -27,6 +27,7 @@ import org.awaitility.Awaitility; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.condition.EnabledIfSystemProperty; import org.keycloak.operator.Constants; +import org.keycloak.operator.crds.v2alpha1.deployment.KeycloakStatusCondition; import org.keycloak.operator.testsuite.utils.K8sUtils; import org.keycloak.operator.controllers.KeycloakAdminSecret; import org.keycloak.operator.controllers.KeycloakDeployment; @@ -48,6 +49,7 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.keycloak.operator.testsuite.utils.CRAssert.assertKeycloakStatusCondition; import static org.keycloak.operator.testsuite.utils.K8sUtils.deployKeycloak; import static org.keycloak.operator.testsuite.utils.K8sUtils.getDefaultKeycloakDeployment; import static org.keycloak.operator.testsuite.utils.K8sUtils.waitForKeycloakToBeReady; @@ -452,4 +454,48 @@ public class KeycloakDeploymentTest extends BaseOperatorTest { } } + @Test + public void testUpgradeRecreatesPods() { + try { + var kc = getDefaultKeycloakDeployment(); + kc.getSpec().setInstances(3); + deployKeycloak(k8sclient, kc, true); + + var stsGetter = k8sclient.apps().statefulSets().inNamespace(namespace).withName(kc.getMetadata().getName()); + final String origImage = stsGetter.get().getSpec().getTemplate().getSpec().getContainers().get(0).getImage(); + final String newImage = "quay.io/keycloak/non-existing-keycloak"; + + kc.getSpec().setImage(newImage); + deployKeycloak(k8sclient, kc, false); + + Awaitility.await() + .ignoreExceptions() + .pollInterval(Duration.ZERO) // make the test super fast not to miss the moment when Operator changes the STS + .untilAsserted(() -> { + var sts = stsGetter.get(); + assertEquals(1, sts.getStatus().getReplicas()); + assertEquals(origImage, sts.getSpec().getTemplate().getSpec().getContainers().get(0).getImage()); + + var currentKc = k8sclient.resources(Keycloak.class) + .inNamespace(namespace).withName(kc.getMetadata().getName()).get(); + assertKeycloakStatusCondition(currentKc, KeycloakStatusCondition.READY, false, "Performing Keycloak upgrade"); + }); + + Awaitility.await() + .ignoreExceptions() + .untilAsserted(() -> { + var sts = stsGetter.get(); + assertEquals(kc.getSpec().getInstances(), sts.getSpec().getReplicas()); // just checking specs as we're using a non-existing image + assertEquals(newImage, sts.getSpec().getTemplate().getSpec().getContainers().get(0).getImage()); + + var currentKc = k8sclient.resources(Keycloak.class) + .inNamespace(namespace).withName(kc.getMetadata().getName()).get(); + assertKeycloakStatusCondition(currentKc, KeycloakStatusCondition.READY, false, "Waiting for more replicas"); + }); + } catch (Exception e) { + savePodLogs(); + throw e; + } + } + }