Split function deployments by region AND memory (#4253)

Today, Function deployments are made in batches of functions grouped by region. Combined w/ source token to re-used built containers for deployment, we get meaningful decrease in deploy time and in Cloud Build resource usage.

Unfortunately, this setup has a peculiar bug; Google Cloud Functions bakes in the flag to expand heap size for appropriate for the memory configuration of a function (`--max_old_space_size`) on the container itself. That means that if you batch two functions with differing memory configuration (e.g. 256MB vs 4 GB), it's guaranteed that one function will have wrongly configured `--max_old_space_size` flag value (Follow #3716 (comment) for how we discovered this issue).

This PR proposes batching function by region AND `availalbeMemoryMB` to fix this bug. We do this by refactoring `calculateRegionalChanges` to generate a collection of `Changeset` (previously known as `RegionalChanges`) that sub-divides functions in a region by their memory. In fact, we generalize the approach by allowing arbitrary subdivision by `keyFn` (e.g. `keyFn: (endpoint) => endpoint.availableMemoryMb`) as I anticipate that we will revisit this section of the code once I start working on "codebases".

Fixes #3716

Author: taeold

CHANGELOG.md

@@ -0,0 +1 @@
+- Fixes bug where functions' memory configurations weren't preserved in batched function deploys (#4253).

src/deploy/functions/release/fabricator.ts

@@ -79,12 +79,12 @@ export class Fabricator {
       totalTime: 0,
       results: [],
     };
-    const deployRegions = Object.values(plan).map(async (changes): Promise<void> => {
-      const results = await this.applyRegionalChanges(changes);
+    const deployChangesets = Object.values(plan).map(async (changes): Promise<void> => {
+      const results = await this.applyChangeset(changes);
       summary.results.push(...results);
       return;
     });
-    const promiseResults = await utils.allSettled(deployRegions);
+    const promiseResults = await utils.allSettled(deployChangesets);
 
     const errs = promiseResults
       .filter((r) => r.status === "rejected")
@@ -100,9 +100,7 @@ export class Fabricator {
     return summary;
   }
 
-  async applyRegionalChanges(
-    changes: planner.RegionalChanges
-  ): Promise<Array<reporter.DeployResult>> {
+  async applyChangeset(changes: planner.Changeset): Promise<Array<reporter.DeployResult>> {
     const deployResults: reporter.DeployResult[] = [];
     const handle = async (
       op: reporter.OperationType,

src/deploy/functions/release/planner.ts

@@ -10,39 +10,63 @@ export interface EndpointUpdate {
   deleteAndRecreate?: backend.Endpoint;
 }
 
-export interface RegionalChanges {
+export interface Changeset {
   endpointsToCreate: backend.Endpoint[];
   endpointsToUpdate: EndpointUpdate[];
   endpointsToDelete: backend.Endpoint[];
 }
 
-export type DeploymentPlan = Record<string, RegionalChanges>;
+export type DeploymentPlan = Record<string, Changeset>;
 
 export interface Options {
   filters?: string[][];
   // If set to false, will delete only functions that are managed by firebase
   deleteAll?: boolean;
 }
 
-/** Calculate the changes needed for a given region. */
-export function calculateRegionalChanges(
+/** Calculate the changesets of given endpoints by grouping endpoints with keyFn. */
+export function calculateChangesets(
   want: Record<string, backend.Endpoint>,
   have: Record<string, backend.Endpoint>,
+  keyFn: (e: backend.Endpoint) => string,
   options: Options
-): RegionalChanges {
-  const endpointsToCreate = Object.keys(want)
-    .filter((id) => !have[id])
-    .map((id) => want[id]);
-
-  const endpointsToDelete = Object.keys(have)
-    .filter((id) => !want[id])
-    .filter((id) => options.deleteAll || isFirebaseManaged(have[id].labels || {}))
-    .map((id) => have[id]);
-
-  const endpointsToUpdate = Object.keys(want)
-    .filter((id) => have[id])
-    .map((id) => calculateUpdate(want[id], have[id]));
-  return { endpointsToCreate, endpointsToUpdate, endpointsToDelete };
+): Record<string, Changeset> {
+  const toCreate = utils.groupBy(
+    Object.keys(want)
+      .filter((id) => !have[id])
+      .map((id) => want[id]),
+    keyFn
+  );
+
+  const toDelete = utils.groupBy(
+    Object.keys(have)
+      .filter((id) => !want[id])
+      .filter((id) => options.deleteAll || isFirebaseManaged(have[id].labels || {}))
+      .map((id) => have[id]),
+    keyFn
+  );
+
+  const toUpdate = utils.groupBy(
+    Object.keys(want)
+      .filter((id) => have[id])
+      .map((id) => calculateUpdate(want[id], have[id])),
+    (eu: EndpointUpdate) => keyFn(eu.endpoint)
+  );
+
+  const result: Record<string, Changeset> = {};
+  const keys = new Set([
+    ...Object.keys(toCreate),
+    ...Object.keys(toDelete),
+    ...Object.keys(toUpdate),
+  ]);
+  for (const key of keys) {
+    result[key] = {
+      endpointsToCreate: toCreate[key] || [],
+      endpointsToUpdate: toUpdate[key] || [],
+      endpointsToDelete: toDelete[key] || [],
+    };
+  }
+  return result;
 }
 
 /**
@@ -78,7 +102,7 @@ export function createDeploymentPlan(
   have: backend.Backend,
   options: Options = {}
 ): DeploymentPlan {
-  const deployment: DeploymentPlan = {};
+  let deployment: DeploymentPlan = {};
   want = backend.matchingBackend(want, (endpoint) => {
     return functionMatchesAnyGroup(endpoint, options.filters || []);
   });
@@ -88,11 +112,13 @@ export function createDeploymentPlan(
 
   const regions = new Set([...Object.keys(want.endpoints), ...Object.keys(have.endpoints)]);
   for (const region of regions) {
-    deployment[region] = calculateRegionalChanges(
+    const changesets = calculateChangesets(
       want.endpoints[region] || {},
       have.endpoints[region] || {},
+      (e) => `${e.region}-${e.availableMemoryMb || "default"}`,
       options
     );
+    deployment = { ...deployment, ...changesets };
   }
 
   if (upgradedToGCFv2WithoutSettingConcurrency(want, have)) {

src/test/deploy/functions/release/fabricator.spec.ts

@@ -988,7 +988,7 @@ describe("Fabricator", () => {
       const ep1 = endpoint({ httpsTrigger: {} }, { id: "A" });
       const ep2 = endpoint({ httpsTrigger: {} }, { id: "B" });
       const ep3 = endpoint({ httpsTrigger: {} }, { id: "C" });
-      const changes: planner.RegionalChanges = {
+      const changes: planner.Changeset = {
         endpointsToCreate: [ep1, ep2],
         endpointsToUpdate: [{ endpoint: ep3 }],
         endpointsToDelete: [],
@@ -1014,19 +1014,19 @@ describe("Fabricator", () => {
       const updateEndpoint = sinon.stub(fab, "updateEndpoint");
       updateEndpoint.callsFake(fakeUpsert);
 
-      await fab.applyRegionalChanges(changes);
+      await fab.applyChangeset(changes);
     });
 
     it("handles errors and wraps them in results", async () => {
       // when it hits a real API it will fail.
       const ep = endpoint();
-      const changes: planner.RegionalChanges = {
+      const changes: planner.Changeset = {
         endpointsToCreate: [ep],
         endpointsToUpdate: [],
         endpointsToDelete: [],
       };
 
-      const results = await fab.applyRegionalChanges(changes);
+      const results = await fab.applyChangeset(changes);
       expect(results[0].error).to.be.instanceOf(reporter.DeploymentError);
       expect(results[0].error?.message).to.match(/create function/);
     });
@@ -1036,13 +1036,13 @@ describe("Fabricator", () => {
     // when it hits a real API it will fail.
     const createEP = endpoint({ httpsTrigger: {} }, { id: "A" });
     const deleteEP = endpoint({ httpsTrigger: {} }, { id: "B" });
-    const changes: planner.RegionalChanges = {
+    const changes: planner.Changeset = {
       endpointsToCreate: [createEP],
       endpointsToUpdate: [],
       endpointsToDelete: [deleteEP],
     };
 
-    const results = await fab.applyRegionalChanges(changes);
+    const results = await fab.applyChangeset(changes);
     const result = results.find((r) => r.endpoint.id === deleteEP.id);
     expect(result?.error).to.be.instanceOf(reporter.AbortedDeploymentError);
     expect(result?.durationMs).to.equal(0);
@@ -1053,7 +1053,7 @@ describe("Fabricator", () => {
     const updateEP = endpoint({ httpsTrigger: {} }, { id: "B" });
     const deleteEP = endpoint({ httpsTrigger: {} }, { id: "C" });
     const update: planner.EndpointUpdate = { endpoint: updateEP };
-    const changes: planner.RegionalChanges = {
+    const changes: planner.Changeset = {
       endpointsToCreate: [createEP],
       endpointsToUpdate: [update],
       endpointsToDelete: [deleteEP],
@@ -1066,7 +1066,7 @@ describe("Fabricator", () => {
     const deleteEndpoint = sinon.stub(fab, "deleteEndpoint");
     deleteEndpoint.resolves();
 
-    const results = await fab.applyRegionalChanges(changes);
+    const results = await fab.applyChangeset(changes);
     expect(createEndpoint).to.have.been.calledWithMatch(createEP);
     expect(updateEndpoint).to.have.been.calledWithMatch(update);
     expect(deleteEndpoint).to.have.been.calledWith(deleteEP);

src/test/deploy/functions/release/planner.spec.ts

@@ -150,14 +150,16 @@ describe("planner", () => {
       const have = { updated, deleted, pantheon };
 
       // note: pantheon is not updated in any way
-      expect(planner.calculateRegionalChanges(want, have, {})).to.deep.equal({
-        endpointsToCreate: [created],
-        endpointsToUpdate: [
-          {
-            endpoint: updated,
-          },
-        ],
-        endpointsToDelete: [deleted],
+      expect(planner.calculateChangesets(want, have, (e) => e.region, {})).to.deep.equal({
+        region: {
+          endpointsToCreate: [created],
+          endpointsToUpdate: [
+            {
+              endpoint: updated,
+            },
+          ],
+          endpointsToDelete: [deleted],
+        },
       });
     });
 
@@ -172,19 +174,69 @@ describe("planner", () => {
       const have = { updated, deleted, pantheon };
 
       // note: pantheon is deleted because we have deleteAll: true
-      expect(planner.calculateRegionalChanges(want, have, { deleteAll: true })).to.deep.equal({
-        endpointsToCreate: [created],
-        endpointsToUpdate: [
-          {
-            endpoint: updated,
-          },
-        ],
-        endpointsToDelete: [deleted, pantheon],
+      expect(
+        planner.calculateChangesets(want, have, (e) => e.region, { deleteAll: true })
+      ).to.deep.equal({
+        region: {
+          endpointsToCreate: [created],
+          endpointsToUpdate: [
+            {
+              endpoint: updated,
+            },
+          ],
+          endpointsToDelete: [deleted, pantheon],
+        },
       });
     });
   });
 
   describe("createDeploymentPlan", () => {
+    it("groups deployment by region and memory", () => {
+      const region1mem1Created: backend.Endpoint = func("id1", "region1");
+      const region1mem1Updated: backend.Endpoint = func("id2", "region1");
+
+      const region2mem1Created: backend.Endpoint = func("id3", "region2");
+      const region2mem2Updated: backend.Endpoint = func("id4", "region2");
+      region2mem2Updated.availableMemoryMb = 512;
+      const region2mem2Deleted: backend.Endpoint = func("id5", "region2");
+      region2mem2Deleted.availableMemoryMb = 512;
+      region2mem2Deleted.labels = deploymentTool.labels();
+
+      const have = backend.of(region1mem1Updated, region2mem2Updated, region2mem2Deleted);
+      const want = backend.of(
+        region1mem1Created,
+        region1mem1Updated,
+        region2mem1Created,
+        region2mem2Updated
+      );
+
+      expect(planner.createDeploymentPlan(want, have, {})).to.deep.equal({
+        "region1-default": {
+          endpointsToCreate: [region1mem1Created],
+          endpointsToUpdate: [
+            {
+              endpoint: region1mem1Updated,
+            },
+          ],
+          endpointsToDelete: [],
+        },
+        "region2-default": {
+          endpointsToCreate: [region2mem1Created],
+          endpointsToUpdate: [],
+          endpointsToDelete: [],
+        },
+        "region2-512": {
+          endpointsToCreate: [],
+          endpointsToUpdate: [
+            {
+              endpoint: region2mem2Updated,
+            },
+          ],
+          endpointsToDelete: [region2mem2Deleted],
+        },
+      });
+    });
+
     it("applies filters", () => {
       const group1Created = func("g1-created", "region");
       const group1Updated = func("g1-updated", "region");
@@ -201,7 +253,7 @@ describe("planner", () => {
       const have = backend.of(group1Updated, group1Deleted, group2Updated, group2Deleted);
 
       expect(planner.createDeploymentPlan(want, have, { filters: [["g1"]] })).to.deep.equal({
-        region: {
+        "region-default": {
           endpointsToCreate: [group1Created],
           endpointsToUpdate: [
             {

src/test/utils.spec.ts

@@ -297,4 +297,37 @@ describe("utils", () => {
       ]);
     });
   });
+
+  describe("groupBy", () => {
+    it("should transform simple array by fn", () => {
+      const arr = [3.4, 1.2, 7.7, 2, 3.9];
+      expect(utils.groupBy(arr, Math.floor)).to.deep.equal({
+        1: [1.2],
+        2: [2],
+        3: [3.4, 3.9],
+        7: [7.7],
+      });
+    });
+
+    it("should transform array of objects by fn", () => {
+      const arr = [
+        { id: 1, location: "us" },
+        { id: 2, location: "us" },
+        { id: 3, location: "asia" },
+        { id: 4, location: "europe" },
+        { id: 5, location: "asia" },
+      ];
+      expect(utils.groupBy(arr, (obj) => obj.location)).to.deep.equal({
+        us: [
+          { id: 1, location: "us" },
+          { id: 2, location: "us" },
+        ],
+        asia: [
+          { id: 3, location: "asia" },
+          { id: 5, location: "asia" },
+        ],
+        europe: [{ id: 4, location: "europe" }],
+      });
+    });
+  });
 });

src/utils.ts

@@ -620,3 +620,21 @@ export function assertIsStringOrUndefined(
     });
   }
 }
+
+/**
+ * Polyfill for groupBy.
+ */
+export function groupBy<T, K extends string | number | symbol>(
+  arr: T[],
+  f: (item: T) => K
+): Record<K, T[]> {
+  return arr.reduce((result, item) => {
+    const key = f(item);
+    if (result[key]) {
+      result[key].push(item);
+    } else {
+      result[key] = [item];
+    }
+    return result;
+  }, {} as Record<K, T[]>);
+}