From 455b922a4aff687e872a1588d91da5402cdb6cf3 Mon Sep 17 00:00:00 2001 From: Matthias Linhuber Date: Fri, 25 Sep 2026 16:14:10 +0200 Subject: [PATCH] fix(operator): reconcile the warm pool on ADDED, not just MODIFIED A released image never reached the warm pool. Mannheim served a pull request's image, pr-170, to students for four weeks after 1.3.0 shipped, and Bonn served 1.2.0 from nine of its ten instances. The reconcile logic was already correct - it compares each instance's appdefinition-generation label against the AppDefinition and recreates the stale ones, skipping any owned by a live Session. It simply never ran. appDefinitionAdded called ensureCapacity, which creates missing ids and never compares generations, and ADDED is the path an image change actually arrives on: the operator replays every existing AppDefinition as ADDED at startup, and a helm upgrade restarts the operator in the same release that rewrites the AppDefinition. Ten instances existed, none were missing, so nothing happened. Confirmed on the live cluster. The operator logged "[init] Ensuring pool capacity: 10 for thm-java-25-latest" at 11:24:24, thirty seconds after the new AppDefinition landed, and left every instance on the old image. Annotating the AppDefinition to force one MODIFIED event recreated eight of ten within four seconds; the two it skipped were the two bound to live sessions, which is the behaviour we want. restoreEmailConfigsOfClaimedInstances runs in reconcile as well as in ensureCapacity, so claimed instances keep their oauth2-proxy allow-list. ensureCapacity now has no caller; it is left in place because there is uncommitted work in that file on another branch. Co-Authored-By: Claude Opus 5 (1M context) --- AGENTS.md | 18 +++++ .../EagerStartAppDefinitionAddedHandler.java | 15 +++-- .../pool/PrewarmedResourcePoolTests.java | 65 +++++++++++++++++++ 3 files changed, 93 insertions(+), 5 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 6070dc10..93c2d264 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -122,6 +122,24 @@ sidecar Deployment and Service are created **before** the Theia deployment so DNS resolves at pod startup, and releasing an eager session restarts the sidecar pods rather than deleting them. +**Every operator start replays existing AppDefinitions as ADDED, so ADDED is not +"new".** `BasicTheiaCloudOperator` lists the AppDefinitions at startup and feeds +each one through `handleAppDefnitionEvent(ADDED, ...)` before the watch begins; +there is no periodic resync, and the in-memory cache dies with the process. A +helm upgrade restarts the operator in the same release that rewrites the +AppDefinition, so an image change arrives on the ADDED path, never MODIFIED. +Both paths must therefore reconcile. They used to differ - ADDED called +`ensureCapacity`, which creates missing ids and never compares generations - and +production served a pull request's image from its warm pool for weeks because +ten instances existed and none were missing. + +**Pool membership is a name count, not a state check.** +`TheiaCloudHandlerUtil.computeIdsOfMissing*` decides an instance is present if a +resource whose name yields id *N* exists. Image, generation and readiness are +not consulted, and `reserveInstance` hands out the lowest free instance without +looking either. Anything that should replace an instance has to say so through +`isOutdated`; nothing else will notice. + **The pool's email ConfigMaps carry live session state.** `instance-N-email-…` holds the `authenticated-emails-list` oauth2-proxy admits; it is written when a session reserves the instance and cleared when the instance is released. That diff --git a/java/operator/org.eclipse.theia.cloud.operator/src/main/java/org/eclipse/theia/cloud/operator/handler/appdef/EagerStartAppDefinitionAddedHandler.java b/java/operator/org.eclipse.theia.cloud.operator/src/main/java/org/eclipse/theia/cloud/operator/handler/appdef/EagerStartAppDefinitionAddedHandler.java index c57728ff..d232f5fb 100644 --- a/java/operator/org.eclipse.theia.cloud.operator/src/main/java/org/eclipse/theia/cloud/operator/handler/appdef/EagerStartAppDefinitionAddedHandler.java +++ b/java/operator/org.eclipse.theia.cloud.operator/src/main/java/org/eclipse/theia/cloud/operator/handler/appdef/EagerStartAppDefinitionAddedHandler.java @@ -83,13 +83,18 @@ public boolean appDefinitionAdded(AppDefinition appDefinition, String correlatio LOGGER.trace(formatLogMessage(correlationId, "HTTPRoute available")); - // Ensure pool has minimum capacity - ISpan poolSpan = Tracing.childSpan(tx, "appdef.ensure_capacity", "Ensure pool capacity"); - poolSpan.setData("min_instances", minInstances); + // Reconcile rather than only filling gaps. Every operator start replays the + // existing AppDefinitions as ADDED, and a helm upgrade restarts the operator + // in the same release that rewrites the AppDefinition - so ADDED is the path + // an image change actually arrives on. Counting instances and finding none + // missing left the pool on the previous image until something else happened + // to touch the AppDefinition. + ISpan poolSpan = Tracing.childSpan(tx, "appdef.reconcile_pool", "Reconcile pool to target"); + poolSpan.setData("target_instances", minInstances); - boolean success = pool.ensureCapacity(appDefinition, minInstances, correlationId); + boolean success = pool.reconcile(appDefinition, minInstances, correlationId); - poolSpan.setTag("pool.operation", "ensure_capacity"); + poolSpan.setTag("pool.operation", "reconcile"); poolSpan.setTag("outcome", success ? "success" : "failure"); Tracing.finish(poolSpan, success ? SpanStatus.OK : SpanStatus.INTERNAL_ERROR); diff --git a/java/operator/org.eclipse.theia.cloud.operator/src/test/java/org/eclipse/theia/cloud/operator/pool/PrewarmedResourcePoolTests.java b/java/operator/org.eclipse.theia.cloud.operator/src/test/java/org/eclipse/theia/cloud/operator/pool/PrewarmedResourcePoolTests.java index 8e1bee75..bd77311b 100644 --- a/java/operator/org.eclipse.theia.cloud.operator/src/test/java/org/eclipse/theia/cloud/operator/pool/PrewarmedResourcePoolTests.java +++ b/java/operator/org.eclipse.theia.cloud.operator/src/test/java/org/eclipse/theia/cloud/operator/pool/PrewarmedResourcePoolTests.java @@ -10,6 +10,7 @@ package org.eclipse.theia.cloud.operator.pool; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; @@ -18,6 +19,7 @@ import static org.mockito.Mockito.when; import java.lang.reflect.Field; +import java.lang.reflect.Method; import java.util.List; import java.util.Map; import java.util.Optional; @@ -38,6 +40,7 @@ import org.mockito.Mockito; import io.fabric8.kubernetes.api.model.ConfigMap; +import io.fabric8.kubernetes.api.model.HasMetadata; import io.fabric8.kubernetes.api.model.ConfigMapBuilder; import io.fabric8.kubernetes.api.model.ConfigMapList; import io.fabric8.kubernetes.api.model.ObjectMetaBuilder; @@ -397,6 +400,68 @@ private Session createSession(String appDefinition, String user, String instance return session; } + // ========== isOutdated ========== + // + // The generation label is what tells a pool instance apart from the AppDefinition + // it was built from. appDefinitionAdded reconciles rather than only filling gaps, + // and reconcile decides what to recreate entirely on this comparison, so these are + // the cases that decide whether a released image actually reaches the warm pool. + + @Test + void isOutdated_generationMatches() { + assertFalse(isOutdated(deploymentWithLabels(Map.of( + PrewarmedResourcePool.APPDEFINITION_GENERATION_LABEL, "6")), 6L)); + } + + @Test + void isOutdated_generationIsOlder() { + assertTrue(isOutdated(deploymentWithLabels(Map.of( + PrewarmedResourcePool.APPDEFINITION_GENERATION_LABEL, "5")), 6L)); + } + + @Test + void isOutdated_labelMissing() { + // Instances created before the label existed. Production had ten of them still + // serving a pull request's image weeks after a release. + assertTrue(isOutdated(deploymentWithLabels(Map.of("app", APP_DEFINITION)), 6L)); + } + + @Test + void isOutdated_noLabelsAtAll() { + assertTrue(isOutdated(deploymentWithLabels(null), 6L)); + } + + @Test + void isOutdated_labelIsNotANumber() { + assertTrue(isOutdated(deploymentWithLabels(Map.of( + PrewarmedResourcePool.APPDEFINITION_GENERATION_LABEL, "not-a-generation")), 6L)); + } + + private Deployment deploymentWithLabels(Map labels) { + ObjectMetaBuilder metadata = new ObjectMetaBuilder().withName("instance-1-" + APP_DEFINITION); + if (labels != null) { + metadata.withLabels(labels); + } + Deployment deployment = new DeploymentBuilder().build(); + deployment.setMetadata(metadata.build()); + return deployment; + } + + private boolean isOutdated(Deployment deployment, long currentGeneration) { + try { + Method method = PrewarmedResourcePool.class.getDeclaredMethod("isOutdated", HasMetadata.class, + long.class); + method.setAccessible(true); + return (boolean) method.invoke(pool(), deployment, currentGeneration); + } catch (Exception e) { + throw new IllegalStateException(e); + } + } + + private PrewarmedResourcePool pool() { + return Mockito.mock(PrewarmedResourcePool.class, Mockito.CALLS_REAL_METHODS); + } + private void setField(Object target, String fieldName, Object value) throws Exception { Field field = target.getClass().getDeclaredField(fieldName); field.setAccessible(true);