Revert "Improve performance and reliability when deploying multiple 2…

…nd gen functions using single builds (firebase#6275)" (firebase#6296) This reverts commit eea3e22.
monogramdesign · Aug 25, 2023 · 5480a24 · 5480a24
1 parent d5d5290
commit 5480a24
Show file tree

Hide file tree

Showing 5 changed files with 35 additions and 67 deletions.
diff --git a/src/deploy/functions/release/fabricator.ts b/src/deploy/functions/release/fabricator.ts
@@ -172,7 +172,7 @@ export class Fabricator {
     if (endpoint.platform === "gcfv1") {
       await this.createV1Function(endpoint, scraper);
     } else if (endpoint.platform === "gcfv2") {
-      await this.createV2Function(endpoint, scraper);
+      await this.createV2Function(endpoint);
     } else {
       assertExhaustive(endpoint.platform);
     }
@@ -191,7 +191,7 @@ export class Fabricator {
     if (update.endpoint.platform === "gcfv1") {
       await this.updateV1Function(update.endpoint, scraper);
     } else if (update.endpoint.platform === "gcfv2") {
-      await this.updateV2Function(update.endpoint, scraper);
+      await this.updateV2Function(update.endpoint);
     } else {
       assertExhaustive(update.endpoint.platform);
     }
@@ -276,7 +276,7 @@ export class Fabricator {
     }
   }
 
-  async createV2Function(endpoint: backend.Endpoint, scraper: SourceTokenScraper): Promise<void> {
+  async createV2Function(endpoint: backend.Endpoint): Promise<void> {
     const storageSource = this.sources[endpoint.codebase!]?.storage;
     if (!storageSource) {
       logger.debug("Precondition failed. Cannot create a GCFv2 function without storage");
@@ -351,13 +351,11 @@ export class Fabricator {
     while (!resultFunction) {
       resultFunction = await this.functionExecutor
         .run(async () => {
-          apiFunction.buildConfig.sourceToken = await scraper.getToken();
           const op: { name: string } = await gcfV2.createFunction(apiFunction);
           return await poller.pollOperation<gcfV2.OutputCloudFunction>({
             ...gcfV2PollerOptions,
             pollerName: `create-${endpoint.codebase}-${endpoint.region}-${endpoint.id}`,
             operationResourceName: op.name,
-            onPoll: scraper.poller,
           });
         })
         .catch(async (err: any) => {
@@ -465,7 +463,7 @@ export class Fabricator {
     }
   }
 
-  async updateV2Function(endpoint: backend.Endpoint, scraper: SourceTokenScraper): Promise<void> {
+  async updateV2Function(endpoint: backend.Endpoint): Promise<void> {
     const storageSource = this.sources[endpoint.codebase!]?.storage;
     if (!storageSource) {
       logger.debug("Precondition failed. Cannot update a GCFv2 function without storage");
@@ -484,13 +482,11 @@ export class Fabricator {
     const resultFunction = await this.functionExecutor
       .run(
         async () => {
-          apiFunction.buildConfig.sourceToken = await scraper.getToken();
           const op: { name: string } = await gcfV2.updateFunction(apiFunction);
           return await poller.pollOperation<gcfV2.OutputCloudFunction>({
             ...gcfV2PollerOptions,
             pollerName: `update-${endpoint.codebase}-${endpoint.region}-${endpoint.id}`,
             operationResourceName: op.name,
-            onPoll: scraper.poller,
           });
         },
         { retryCodes: [...DEFAULT_RETRY_CODES, CLOUD_RUN_RESOURCE_EXHAUSTED_CODE] }

diff --git a/src/deploy/functions/release/sourceTokenScraper.ts b/src/deploy/functions/release/sourceTokenScraper.ts
@@ -11,15 +11,13 @@ type TokenFetchState = "NONE" | "FETCHING" | "VALID";
  */
 export class SourceTokenScraper {
   private tokenValidDurationMs;
-  private fetchTimeoutMs;
   private resolve!: (token?: string) => void;
   private promise: Promise<string | undefined>;
   private expiry: number | undefined;
   private fetchState: TokenFetchState;
 
-  constructor(validDurationMs = 1500000, fetchTimeoutMs = 180_000) {
+  constructor(validDurationMs = 1500000) {
     this.tokenValidDurationMs = validDurationMs;
-    this.fetchTimeoutMs = fetchTimeoutMs;
     this.promise = new Promise((resolve) => (this.resolve = resolve));
     this.fetchState = "NONE";
   }
@@ -29,15 +27,7 @@ export class SourceTokenScraper {
       this.fetchState = "FETCHING";
       return undefined;
     } else if (this.fetchState === "FETCHING") {
-      const timeout = new Promise<undefined>((resolve) => {
-        setTimeout(() => {
-          this.fetchState = "NONE";
-          resolve(undefined);
-        }, this.fetchTimeoutMs);
-      });
-      // wait until we get a source token, or the timeout occurs
-      // and we reset the fetch state and return an undefined token.
-      return Promise.race([this.promise, timeout]);
+      return this.promise; // wait until we get a source token
     } else if (this.fetchState === "VALID") {
       if (this.isTokenExpired()) {
         this.fetchState = "FETCHING";

diff --git a/src/gcp/cloudfunctionsv2.ts b/src/gcp/cloudfunctionsv2.ts
@@ -41,7 +41,6 @@ export interface BuildConfig {
   runtime: runtimes.Runtime;
   entryPoint: string;
   source: Source;
-  sourceToken?: string;
   environmentVariables: Record<string, string>;
 
   // Output only

diff --git a/src/test/deploy/functions/release/fabricator.spec.ts b/src/test/deploy/functions/release/fabricator.spec.ts
@@ -455,7 +455,7 @@ describe("Fabricator", () => {
         }
       );
 
-      await fab.createV2Function(ep, new scraper.SourceTokenScraper());
+      await fab.createV2Function(ep);
       expect(pubsub.createTopic).to.have.been.called;
       expect(gcfv2.createFunction).to.have.been.called;
     });
@@ -476,7 +476,7 @@ describe("Fabricator", () => {
         }
       );
 
-      await expect(fab.createV2Function(ep, new scraper.SourceTokenScraper())).to.be.rejectedWith(
+      await expect(fab.createV2Function(ep)).to.be.rejectedWith(
         reporter.DeploymentError,
         "create topic"
       );
@@ -500,7 +500,7 @@ describe("Fabricator", () => {
         }
       );
 
-      await fab.createV2Function(ep, new scraper.SourceTokenScraper());
+      await fab.createV2Function(ep);
       expect(eventarc.getChannel).to.have.been.called;
       expect(eventarc.createChannel).to.not.have.been.called;
       expect(gcfv2.createFunction).to.have.been.called;
@@ -530,7 +530,7 @@ describe("Fabricator", () => {
         }
       );
 
-      await fab.createV2Function(ep, new scraper.SourceTokenScraper());
+      await fab.createV2Function(ep);
       expect(eventarc.createChannel).to.have.been.called;
       expect(gcfv2.createFunction).to.have.been.called;
     });
@@ -568,7 +568,7 @@ describe("Fabricator", () => {
         }
       );
 
-      await fab.createV2Function(ep, new scraper.SourceTokenScraper());
+      await fab.createV2Function(ep);
       expect(eventarc.createChannel).to.have.been.calledOnceWith({ name: channelName });
       expect(poller.pollOperation).to.have.been.called;
     });
@@ -594,27 +594,22 @@ describe("Fabricator", () => {
         }
       );
 
-      await expect(
-        fab.createV2Function(ep, new scraper.SourceTokenScraper())
-      ).to.eventually.be.rejectedWith(reporter.DeploymentError, "upsert eventarc channel");
+      await expect(fab.createV2Function(ep)).to.eventually.be.rejectedWith(
+        reporter.DeploymentError,
+        "upsert eventarc channel"
+      );
     });
 
     it("throws on create function failure", async () => {
       gcfv2.createFunction.rejects(new Error("Server failure"));
 
       const ep = endpoint({ httpsTrigger: {} }, { platform: "gcfv2" });
-      await expect(fab.createV2Function(ep, new scraper.SourceTokenScraper())).to.be.rejectedWith(
-        reporter.DeploymentError,
-        "create"
-      );
+      await expect(fab.createV2Function(ep)).to.be.rejectedWith(reporter.DeploymentError, "create");
 
       gcfv2.createFunction.resolves({ name: "op", done: false });
       poller.pollOperation.rejects(new Error("Fail whale"));
 
-      await expect(fab.createV2Function(ep, new scraper.SourceTokenScraper())).to.be.rejectedWith(
-        reporter.DeploymentError,
-        "create"
-      );
+      await expect(fab.createV2Function(ep)).to.be.rejectedWith(reporter.DeploymentError, "create");
     });
 
     it("deletes broken function and retries on cloud run quota exhaustion", async () => {
@@ -625,7 +620,7 @@ describe("Fabricator", () => {
       poller.pollOperation.resolves({ name: "op" });
 
       const ep = endpoint({ httpsTrigger: {} }, { platform: "gcfv2" });
-      await fab.createV2Function(ep, new scraper.SourceTokenScraper(1500000, 0));
+      await fab.createV2Function(ep);
 
       expect(gcfv2.createFunction).to.have.been.calledTwice;
       expect(gcfv2.deleteFunction).to.have.been.called;
@@ -637,7 +632,7 @@ describe("Fabricator", () => {
       run.setInvokerCreate.rejects(new Error("Boom"));
 
       const ep = endpoint({ httpsTrigger: {} }, { platform: "gcfv2" });
-      await expect(fab.createV2Function(ep, new scraper.SourceTokenScraper())).to.be.rejectedWith(
+      await expect(fab.createV2Function(ep)).to.be.rejectedWith(
         reporter.DeploymentError,
         "set invoker"
       );
@@ -650,7 +645,7 @@ describe("Fabricator", () => {
         run.setInvokerCreate.resolves();
         const ep = endpoint({ httpsTrigger: {} }, { platform: "gcfv2" });
 
-        await fab.createV2Function(ep, new scraper.SourceTokenScraper());
+        await fab.createV2Function(ep);
         expect(run.setInvokerCreate).to.have.been.calledWith(ep.project, "service", ["public"]);
       });
 
@@ -667,7 +662,7 @@ describe("Fabricator", () => {
           { platform: "gcfv2" }
         );
 
-        await fab.createV2Function(ep, new scraper.SourceTokenScraper());
+        await fab.createV2Function(ep);
         expect(run.setInvokerCreate).to.have.been.calledWith(ep.project, "service", ["custom@"]);
       });
 
@@ -677,7 +672,7 @@ describe("Fabricator", () => {
         run.setInvokerCreate.resolves();
         const ep = endpoint({ httpsTrigger: { invoker: ["private"] } }, { platform: "gcfv2" });
 
-        await fab.createV2Function(ep, new scraper.SourceTokenScraper());
+        await fab.createV2Function(ep);
         expect(run.setInvokerCreate).to.not.have.been.called;
       });
     });
@@ -689,7 +684,7 @@ describe("Fabricator", () => {
         run.setInvokerCreate.resolves();
         const ep = endpoint({ callableTrigger: {} }, { platform: "gcfv2" });
 
-        await fab.createV2Function(ep, new scraper.SourceTokenScraper());
+        await fab.createV2Function(ep);
         expect(run.setInvokerCreate).to.have.been.calledWith(ep.project, "service", ["public"]);
       });
     });
@@ -701,7 +696,7 @@ describe("Fabricator", () => {
         run.setInvokerCreate.resolves();
         const ep = endpoint({ taskQueueTrigger: {} }, { platform: "gcfv2" });
 
-        await fab.createV2Function(ep, new scraper.SourceTokenScraper());
+        await fab.createV2Function(ep);
         expect(run.setInvokerCreate).to.not.have.been.called;
       });
 
@@ -717,7 +712,7 @@ describe("Fabricator", () => {
           },
           { platform: "gcfv2" }
         );
-        await fab.createV2Function(ep, new scraper.SourceTokenScraper());
+        await fab.createV2Function(ep);
         expect(run.setInvokerCreate).to.have.been.calledWith(ep.project, "service", ["custom@"]);
       });
     });
@@ -732,7 +727,7 @@ describe("Fabricator", () => {
           { platform: "gcfv2" }
         );
 
-        await fab.createV2Function(ep, new scraper.SourceTokenScraper());
+        await fab.createV2Function(ep);
         expect(run.setInvokerCreate).to.have.been.calledWith(ep.project, "service", ["public"]);
       });
     });
@@ -746,7 +741,7 @@ describe("Fabricator", () => {
         { platform: "gcfv2" }
       );
 
-      await fab.createV2Function(ep, new scraper.SourceTokenScraper());
+      await fab.createV2Function(ep);
       expect(run.setInvokerCreate).to.not.have.been.called;
     });
   });
@@ -756,17 +751,11 @@ describe("Fabricator", () => {
       gcfv2.updateFunction.rejects(new Error("Server failure"));
 
       const ep = endpoint({ httpsTrigger: {} }, { platform: "gcfv2" });
-      await expect(fab.updateV2Function(ep, new scraper.SourceTokenScraper())).to.be.rejectedWith(
-        reporter.DeploymentError,
-        "update"
-      );
+      await expect(fab.updateV2Function(ep)).to.be.rejectedWith(reporter.DeploymentError, "update");
 
       gcfv2.updateFunction.resolves({ name: "op", done: false });
       poller.pollOperation.rejects(new Error("Fail whale"));
-      await expect(fab.updateV2Function(ep, new scraper.SourceTokenScraper())).to.be.rejectedWith(
-        reporter.DeploymentError,
-        "update"
-      );
+      await expect(fab.updateV2Function(ep)).to.be.rejectedWith(reporter.DeploymentError, "update");
     });
 
     it("throws on set invoker failure", async () => {
@@ -775,7 +764,7 @@ describe("Fabricator", () => {
       run.setInvokerUpdate.rejects(new Error("Boom"));
 
       const ep = endpoint({ httpsTrigger: { invoker: ["private"] } }, { platform: "gcfv2" });
-      await expect(fab.updateV2Function(ep, new scraper.SourceTokenScraper())).to.be.rejectedWith(
+      await expect(fab.updateV2Function(ep)).to.be.rejectedWith(
         reporter.DeploymentError,
         "set invoker"
       );
@@ -794,7 +783,7 @@ describe("Fabricator", () => {
         { platform: "gcfv2" }
       );
 
-      await fab.updateV2Function(ep, new scraper.SourceTokenScraper());
+      await fab.updateV2Function(ep);
       expect(run.setInvokerUpdate).to.have.been.calledWith(ep.project, "service", ["custom@"]);
     });
 
@@ -811,7 +800,7 @@ describe("Fabricator", () => {
         { platform: "gcfv2" }
       );
 
-      await fab.updateV2Function(ep, new scraper.SourceTokenScraper());
+      await fab.updateV2Function(ep);
       expect(run.setInvokerUpdate).to.have.been.calledWith(ep.project, "service", ["custom@"]);
     });
 
@@ -828,7 +817,7 @@ describe("Fabricator", () => {
         { platform: "gcfv2" }
       );
 
-      await fab.updateV2Function(ep, new scraper.SourceTokenScraper());
+      await fab.updateV2Function(ep);
       expect(run.setInvokerUpdate).to.have.been.calledWith(ep.project, "service", ["public"]);
     });
 
@@ -838,7 +827,7 @@ describe("Fabricator", () => {
       run.setInvokerUpdate.resolves();
       const ep = endpoint({ httpsTrigger: {} }, { platform: "gcfv2" });
 
-      await fab.updateV2Function(ep, new scraper.SourceTokenScraper());
+      await fab.updateV2Function(ep);
       expect(run.setInvokerUpdate).to.not.have.been.called;
     });
 
@@ -851,7 +840,7 @@ describe("Fabricator", () => {
         { platform: "gcfv2" }
       );
 
-      await fab.updateV2Function(ep, new scraper.SourceTokenScraper());
+      await fab.updateV2Function(ep);
       expect(run.setInvokerUpdate).to.not.have.been.called;
     });
   });

diff --git a/src/test/deploy/functions/release/sourceTokenScraper.spec.ts b/src/test/deploy/functions/release/sourceTokenScraper.spec.ts
@@ -66,12 +66,6 @@ describe("SourceTokenScraper", () => {
     await expect(scraper.getToken()).to.eventually.equal("magic token #2");
   });
 
-  it("resets fetch state after timeout and returns undefined token", async () => {
-    const scraper = new SourceTokenScraper(100000, 10);
-    await expect(scraper.getToken()).to.eventually.be.undefined;
-    await expect(scraper.getToken()).to.eventually.be.undefined;
-  });
-
   it("concurrent requests for source token", async () => {
     const scraper = new SourceTokenScraper();