Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix memory options for some functions #3924

Merged
merged 3 commits into from Nov 30, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
@@ -1,2 +1,3 @@
- Corrects a bug where containers in Artifact Registry would not be deleted if a function has an upper case character in its name
- Fixes issue where providing the `--project` flag during `init` would not be recognized with a default project already set. (#3870)
- Fixes issue with setting memory limits for some functions
52 changes: 49 additions & 3 deletions src/gcp/cloudfunctionsv2.ts
Expand Up @@ -88,7 +88,7 @@ export interface ServiceConfig {
uri?: string;

timeoutSeconds?: number;
availableMemoryMb?: number;
availableMemory?: string;
environmentVariables?: Record<string, string>;
maxInstanceCount?: number;
minInstanceCount?: number;
Expand Down Expand Up @@ -163,6 +163,40 @@ interface GenerateUploadUrlResponse {
storageSource: StorageSource;
}

// AvailableMemory suffixes and their byte count.
type MemoryUnit = "" | "k" | "M" | "G" | "Mi" | "Gi";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we skipping Ki b/c we don't think it's a popular choice? It looks like Cloud Run supports several more units, and our code will fail if the customer decides to use a unit not in this list.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the GCF proto, it looks like Ki is not a supported unit at this time.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can just extend this to support more than GCF supports pretty easily.

const BYTES_PER_UNIT: Record<MemoryUnit, number> = {
"": 1,
k: 1e3,
M: 1e6,
G: 1e9,
Mi: 1 << 20,
Gi: 1 << 30,
};

/**
* Returns the float-precision number of Mega(not Mebi)bytes in a
* Kubernetes-style quantity
* Must serve the same results as
* https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/api/resource/quantity.go
*/
export function megabytes(memory: string): number {
const re = /^([0-9]+(\.[0-9]*)?)(k|Mi|Gi|M|G|([eE]([0-9]+)))?$/;
const matches = re.exec(memory);
if (!matches) {
throw new Error(`Invalid memory quantity "${memory}""`);
}
const quantity = Number.parseFloat(matches[1]);
let bytes: number;
if (matches[5]) {
bytes = quantity * Math.pow(10, Number.parseFloat(matches[5]));
} else {
const suffix = matches[3] || "";
bytes = quantity * BYTES_PER_UNIT[suffix as MemoryUnit];
}
return bytes / 1e6;
}

/**
* Logs an error from a failed function deployment.
* @param funcName Name of the function that was unsuccessfully deployed.
Expand Down Expand Up @@ -356,13 +390,19 @@ export function functionFromEndpoint(endpoint: backend.Endpoint, source: Storage
proto.copyIfPresent(
gcfFunction.serviceConfig,
endpoint,
"availableMemoryMb",
"environmentVariables",
"vpcConnector",
"vpcConnectorEgressSettings",
"serviceAccountEmail",
"ingressSettings"
);
proto.renameIfPresent(
gcfFunction.serviceConfig,
endpoint,
"availableMemory",
"availableMemoryMb",
(mb: string) => `${mb}M`
);
proto.renameIfPresent(
gcfFunction.serviceConfig,
endpoint,
Expand Down Expand Up @@ -459,12 +499,18 @@ export function endpointFromFunction(gcfFunction: CloudFunction): backend.Endpoi
endpoint,
gcfFunction.serviceConfig,
"serviceAccountEmail",
"availableMemoryMb",
"vpcConnector",
"vpcConnectorEgressSettings",
"ingressSettings",
"environmentVariables"
);
proto.renameIfPresent(
endpoint,
gcfFunction.serviceConfig,
"availableMemoryMb",
"availableMemory",
megabytes
);
proto.renameIfPresent(
endpoint,
gcfFunction.serviceConfig,
Expand Down
23 changes: 20 additions & 3 deletions src/test/gcp/cloudfunctionsv2.spec.ts
Expand Up @@ -50,6 +50,22 @@ describe("cloudfunctionsv2", () => {
updateTime: new Date(),
};

describe("megabytes", () => {
it("Should handle decimal SI units", () => {
expect(cloudfunctionsv2.megabytes("1000k")).to.equal(1);
expect(cloudfunctionsv2.megabytes("1.5M")).to.equal(1.5);
expect(cloudfunctionsv2.megabytes("1G")).to.equal(1000);
});
it("Should handle binary SI units", () => {
expect(cloudfunctionsv2.megabytes("1Mi")).to.equal((1 << 20) / 1e6);
expect(cloudfunctionsv2.megabytes("1Gi")).to.equal((1 << 30) / 1e6);
});
it("Should handle no unit", () => {
expect(cloudfunctionsv2.megabytes("100000")).to.equal(0.1);
expect(cloudfunctionsv2.megabytes("1e9")).to.equal(1000);
expect(cloudfunctionsv2.megabytes("1.5E6")).to.equal(1.5);
});
});
describe("functionFromEndpoint", () => {
const UPLOAD_URL = "https://storage.googleapis.com/projects/-/buckets/sample/source.zip";
it("should guard against version mixing", () => {
Expand Down Expand Up @@ -130,7 +146,6 @@ describe("cloudfunctionsv2", () => {
...ENDPOINT,
httpsTrigger: {},
platform: "gcfv2",
availableMemoryMb: 128,
vpcConnector: "connector",
vpcConnectorEgressSettings: "ALL_TRAFFIC",
ingressSettings: "ALLOW_ALL",
Expand Down Expand Up @@ -159,7 +174,6 @@ describe("cloudfunctionsv2", () => {
vpcConnector: "connector",
vpcConnectorEgressSettings: "ALL_TRAFFIC",
ingressSettings: "ALLOW_ALL",
availableMemoryMb: 128,
serviceAccountEmail: "inlined@google.com",
},
};
Expand All @@ -183,6 +197,7 @@ describe("cloudfunctionsv2", () => {
maxInstances: 42,
minInstances: 1,
timeout: "15s",
availableMemoryMb: 128,
};

const complexGcfFunction: Omit<
Expand All @@ -199,6 +214,7 @@ describe("cloudfunctionsv2", () => {
maxInstanceCount: 42,
minInstanceCount: 1,
timeoutSeconds: 15,
availableMemory: "128M",
},
};

Expand Down Expand Up @@ -290,7 +306,6 @@ describe("cloudfunctionsv2", () => {

it("should copy optional fields", () => {
const extraFields: backend.ServiceConfiguration = {
availableMemoryMb: 128,
vpcConnector: "connector",
vpcConnectorEgressSettings: "ALL_TRAFFIC",
ingressSettings: "ALLOW_ALL",
Expand All @@ -305,6 +320,7 @@ describe("cloudfunctionsv2", () => {
serviceConfig: {
...HAVE_CLOUD_FUNCTION_V2.serviceConfig,
...extraFields,
availableMemory: "128M",
},
labels: {
foo: "bar",
Expand All @@ -316,6 +332,7 @@ describe("cloudfunctionsv2", () => {
httpsTrigger: {},
uri: RUN_URI,
...extraFields,
availableMemoryMb: 128,
labels: {
foo: "bar",
},
Expand Down