[server] Fix broken /ready endpoint and chaned the probe to be a StartupProbe (#20672)

geropl · web-flow · commit fad0801ca8ba · 2025-03-18T06:34:47.000-04:00
Tool: gitpod/catfood.gitpod.cloud
diff --git a/components/server/src/liveness/probes.ts b/components/server/src/liveness/probes.ts
@@ -4,22 +4,41 @@
  * See License.AGPL.txt in the project root for license information.
  */
 
+import * as http from "http";
 import express from "express";
 import { inject, injectable } from "inversify";
 import { LivenessController } from "./liveness-controller";
 import { ReadinessController } from "./readiness-controller";
+import { AddressInfo } from "net";
 
 @injectable()
 export class ProbesApp {
+    private app: express.Application;
+    private httpServer: http.Server | undefined = undefined;
+
     constructor(
         @inject(LivenessController) protected readonly livenessController: LivenessController,
         @inject(ReadinessController) protected readonly readinessController: ReadinessController,
-    ) {}
-
-    public create(): express.Application {
+    ) {
         const probesApp = express();
         probesApp.use("/live", this.livenessController.apiRouter);
         probesApp.use("/ready", this.readinessController.apiRouter);
-        return probesApp;
+        this.app = probesApp;
+    }
+
+    public async start(port: number): Promise<number> {
+        await this.readinessController.start();
+
+        return new Promise((resolve, reject) => {
+            const probeServer = this.app.listen(port, () => {
+                resolve((<AddressInfo>probeServer.address()).port);
+            });
+            this.httpServer = probeServer;
+        });
+    }
+
+    public async stop(): Promise<void> {
+        this.httpServer?.close();
+        await this.readinessController.stop();
     }
 }
diff --git a/components/server/src/liveness/readiness-controller.ts b/components/server/src/liveness/readiness-controller.ts
@@ -12,30 +12,36 @@ import { log } from "@gitpod/gitpod-protocol/lib/util/logging";
 import { v1 } from "@authzed/authzed-node";
 import { getExperimentsClientForBackend } from "@gitpod/gitpod-protocol/lib/experiments/configcat-server";
 import { Redis } from "ioredis";
+import { repeat } from "@gitpod/gitpod-protocol/lib/util/repeat";
+import { Disposable, DisposableCollection } from "@gitpod/gitpod-protocol";
 
 @injectable()
 export class ReadinessController {
     @inject(TypeORM) protected readonly typeOrm: TypeORM;
     @inject(SpiceDBClientProvider) protected readonly spiceDBClientProvider: SpiceDBClientProvider;
     @inject(Redis) protected readonly redis: Redis;
 
+    private readinessProbeEnabled: boolean = true;
+    private disposables: DisposableCollection = new DisposableCollection();
+
     get apiRouter(): express.Router {
         const router = express.Router();
         this.addReadinessHandler(router);
         return router;
     }
 
+    public async start() {
+        this.disposables.push(this.startPollingFeatureFlag());
+    }
+
+    public async stop() {
+        this.disposables.dispose();
+    }
+
     protected addReadinessHandler(router: express.Router) {
         router.get("/", async (_, res) => {
             try {
-                // Check feature flag first
-                const readinessProbeEnabled = await getExperimentsClientForBackend().getValueAsync(
-                    "server_readiness_probe",
-                    true, // Default to readiness probe, skip if false
-                    {},
-                );
-
-                if (!readinessProbeEnabled) {
+                if (!this.readinessProbeEnabled) {
                     log.debug("Readiness check skipped due to feature flag");
                     res.status(200);
                     return;
@@ -67,6 +73,7 @@ export class ReadinessController {
 
                 // All connections are good
                 res.status(200).send("Ready");
+                log.debug("Readiness check successful");
             } catch (error) {
                 log.error("Readiness check failed", error);
                 res.status(503).send("Readiness check failed");
@@ -79,6 +86,7 @@ export class ReadinessController {
             const connection = await this.typeOrm.getConnection();
             // Simple query to verify connection is working
             await connection.query("SELECT 1");
+            log.debug("Database connection check successful");
             return true;
         } catch (error) {
             log.error("Database connection check failed", error);
@@ -113,4 +121,21 @@ export class ReadinessController {
             return false;
         }
     }
+
+    private startPollingFeatureFlag(): Disposable {
+        return repeat(async () => {
+            // Check feature flag first
+            const readinessProbeEnabled = await getExperimentsClientForBackend().getValueAsync(
+                "server_readiness_probe",
+                true, // Default to readiness probe, skip if false
+                {},
+            );
+
+            log.debug("Feature flag server_readiness_probe updated", {
+                readinessProbeEnabled,
+                oldValue: this.readinessProbeEnabled,
+            });
+            this.readinessProbeEnabled = readinessProbeEnabled;
+        }, 10_000);
+    }
 }
diff --git a/components/server/src/server.ts b/components/server/src/server.ts
@@ -52,7 +52,7 @@ import {
 } from "./workspace/headless-log-service";
 import { runWithRequestContext } from "./util/request-context";
 import { AnalyticsController } from "./analytics-controller";
-import { ProbesApp as ProbesAppProvider } from "./liveness/probes";
+import { ProbesApp } from "./liveness/probes";
 
 const MONITORING_PORT = 9500;
 const IAM_SESSION_PORT = 9876;
@@ -69,8 +69,6 @@ export class Server {
     protected privateApiServer?: http.Server;
 
     protected readonly eventEmitter = new EventEmitter();
-    protected probesApp: express.Application;
-    protected probesServer?: http.Server;
     protected app?: express.Application;
     protected httpServer?: http.Server;
     protected monitoringApp?: express.Application;
@@ -105,7 +103,7 @@ export class Server {
         @inject(API) private readonly api: API,
         @inject(RedisSubscriber) private readonly redisSubscriber: RedisSubscriber,
         @inject(AnalyticsController) private readonly analyticsController: AnalyticsController,
-        @inject(ProbesAppProvider) private readonly probesAppProvider: ProbesAppProvider,
+        @inject(ProbesApp) private readonly probesApp: ProbesApp,
     ) {}
 
     public async init(app: express.Application) {
@@ -119,9 +117,6 @@ export class Server {
         await this.typeOrm.connect();
         log.info("connected to DB");
 
-        // probes
-        this.probesApp = this.probesAppProvider.create();
-
         // metrics
         app.use((req: express.Request, res: express.Response, next: express.NextFunction) => {
             const startTime = Date.now();
@@ -359,10 +354,9 @@ export class Server {
             throw new Error("server cannot start, not initialized");
         }
 
-        const probeServer = this.probesApp.listen(PROBES_PORT, () => {
-            log.info(`probes server listening on port: ${(<AddressInfo>probeServer.address()).port}`);
+        this.probesApp.start(PROBES_PORT).then((port) => {
+            log.info(`probes server listening on port: ${port}`);
         });
-        this.probesServer = probeServer;
 
         const httpServer = this.app.listen(port, () => {
             this.eventEmitter.emit(Server.EVENT_ON_START, httpServer);
@@ -419,7 +413,7 @@ export class Server {
             race(this.stopServer(this.httpServer), "stop httpserver"),
             race(this.stopServer(this.privateApiServer), "stop private api server"),
             race(this.stopServer(this.publicApiServer), "stop public api server"),
-            race(this.stopServer(this.probesServer), "stop probe server"),
+            race(this.probesApp.stop(), "stop probe server"),
             race((async () => this.disposables.dispose())(), "dispose disposables"),
         ]);
 
diff --git a/install/installer/pkg/components/server/deployment.go b/install/installer/pkg/components/server/deployment.go
@@ -371,7 +371,9 @@ func deployment(ctx *common.RenderContext) ([]runtime.Object, error) {
 								PeriodSeconds:       10,
 								FailureThreshold:    6,
 							},
-							ReadinessProbe: &corev1.Probe{
+							// StartupProbe, as we are only interested in controlling the startup of the server pod, and
+							// not interferring with the readiness afterwards.
+							StartupProbe: &corev1.Probe{
 								ProbeHandler: corev1.ProbeHandler{
 									HTTPGet: &corev1.HTTPGetAction{
 										Path: "/ready",
@@ -383,7 +385,7 @@ func deployment(ctx *common.RenderContext) ([]runtime.Object, error) {
 								},
 								InitialDelaySeconds: 5,
 								PeriodSeconds:       10,
-								FailureThreshold:    12, // try for 120 seconds
+								FailureThreshold:    18, // try for 180 seconds, then the Pod is restarted
 							},
 							SecurityContext: &corev1.SecurityContext{
 								Privileged:               pointer.Bool(false),
