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);