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

feat: Autoscaler #1077

Merged
merged 35 commits into from
May 31, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
c0630df
work in progress - broken
danieljbruce Apr 12, 2022
b1b898d
Add autoscaling options to interface
danieljbruce Apr 14, 2022
ea8860c
Work in progress on function for setting metadata
danieljbruce Apr 14, 2022
d62bb01
Fixed some tests that were broken due to the chang
danieljbruce Apr 19, 2022
8f58902
Cluster.ts in test file
danieljbruce Apr 20, 2022
96c71eb
Update cluster creation to accept new parameters
danieljbruce Apr 20, 2022
9eafb76
Fix for failing test
danieljbruce Apr 20, 2022
5ef31ef
Enhance system tests for cluster
danieljbruce Apr 20, 2022
ce608de
Add another test
danieljbruce Apr 21, 2022
13097f5
condense tests
danieljbruce Apr 21, 2022
3498c9f
Added more tests
danieljbruce Apr 21, 2022
bd780e5
Test improvements
danieljbruce Apr 21, 2022
4270a6a
Change update mask to get manual scaling right
danieljbruce Apr 21, 2022
67f6645
Added validation for cluster creation configs
danieljbruce Apr 21, 2022
370d41c
Refactor cluster object in tests
danieljbruce Apr 21, 2022
72d54ea
Another cluster id refactor
danieljbruce Apr 21, 2022
152d6e3
Remove TODO that no longer applies
danieljbruce Apr 21, 2022
059aa4e
Code reorganization for test cases
danieljbruce Apr 21, 2022
51ec882
Fix the test so that it breaks for good reason
danieljbruce Apr 21, 2022
bc32192
update mask push fix
danieljbruce Apr 22, 2022
822bd6d
Merge branch 'main' of https://github.com/googleapis/nodejs-bigtable …
danieljbruce Apr 22, 2022
4798915
Add nodes in all calls to update and create cluste
danieljbruce Apr 22, 2022
19ac452
Add license headers
danieljbruce Apr 22, 2022
931b25c
correct copyright year
danieljbruce Apr 22, 2022
9ea29bb
Add singlequote
danieljbruce Apr 22, 2022
61eb533
Add a validation
danieljbruce Apr 28, 2022
b4ad269
Update test descriptions
danieljbruce Apr 28, 2022
8ec8232
refactor to use one check metadata function
danieljbruce Apr 28, 2022
a95bf13
Fix tests as specifying nodes is required anyway
danieljbruce Apr 28, 2022
961697d
should added to test case descriptions
danieljbruce Apr 28, 2022
749ac86
PR updates
danieljbruce May 10, 2022
d759f43
Merge branch 'main' of https://github.com/googleapis/nodejs-bigtable …
danieljbruce May 27, 2022
c7cf8a2
validation error
danieljbruce May 30, 2022
b4025ed
Remove call to get metadata
danieljbruce May 30, 2022
232cc0d
PR updates
danieljbruce May 30, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 16 additions & 15 deletions src/cluster.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ const pumpify = require('pumpify');
import {google} from '../protos/protos';
import {Bigtable} from '.';
import {Instance} from './instance';
import {ClusterUtils} from './utils/cluster';

import {
Backup,
Expand Down Expand Up @@ -75,7 +76,10 @@ export type GetClustersCallback = (
apiResponse?: google.bigtable.admin.v2.IListClustersResponse
) => void;
export interface SetClusterMetadataOptions {
nodes: number;
nodes?: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

is this considered breaking if we change something from required to optional? I'm guessing it would be the other way around, but want to confirm :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here we are changing something from required to optional so that would make it not a breaking change since users can still pass just nodes in. This is where we make nodes optional because instead the user can provide autoscaling parameters. These parameters will be validated against a new validation function and an error will be thrown if an invalid combination of parameters are provided.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would not consider switching from required to optional for TypeScript breaking 👍

minServeNodes?: number;
maxServeNodes?: number;
cpuUtilizationPercent?: number;
}
export type SetClusterMetadataCallback = GenericOperationCallback<
Operation | null | undefined
Expand All @@ -84,8 +88,11 @@ export interface BasicClusterConfig {
encryption?: google.bigtable.admin.v2.Cluster.IEncryptionConfig;
key?: string;
location: string;
nodes: number;
nodes?: number;
storage?: string;
minServeNodes?: number;
maxServeNodes?: number;
cpuUtilizationPercent?: number;
}

export interface CreateBackupConfig extends ModifiableBackupFields {
Expand Down Expand Up @@ -690,29 +697,23 @@ Please use the format 'my-cluster' or '${instance.name}/clusters/my-cluster'.`);
gaxOptionsOrCallback?: CallOptions | SetClusterMetadataCallback,
cb?: SetClusterMetadataCallback
): void | Promise<SetClusterMetadataResponse> {
ClusterUtils.validateClusterMetadata(metadata);
const callback =
typeof gaxOptionsOrCallback === 'function' ? gaxOptionsOrCallback : cb!;
const gaxOptions =
typeof gaxOptionsOrCallback === 'object'
? gaxOptionsOrCallback
: ({} as CallOptions);

const reqOpts: ICluster = Object.assign(
{},
{
name: this.name,
serveNodes: metadata.nodes,
},
metadata
const reqOpts = ClusterUtils.getRequestFromMetadata(
metadata,
this?.metadata?.location,
this.name
);
// eslint-disable-next-line @typescript-eslint/no-explicit-any
delete (reqOpts as any).nodes;

this.bigtable.request<Operation>(
{
client: 'BigtableInstanceAdminClient',
method: 'updateCluster',
reqOpts,
method: 'partialUpdateCluster',
reqOpts: reqOpts,
gaxOpts: gaxOptions,
},
(err, resp) => {
Expand Down
14 changes: 9 additions & 5 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import {ServiceError} from 'google-gax';
import * as v2 from './v2';
import {PassThrough, Duplex} from 'stream';
import grpcGcpModule = require('grpc-gcp');
import {ClusterUtils} from './utils/cluster';

// eslint-disable-next-line @typescript-eslint/no-var-requires
const streamEvents = require('stream-events');
Expand Down Expand Up @@ -610,12 +611,15 @@ export class Bigtable {
'A cluster was provided with both `encryption` and `key` defined.'
);
}

clusters[cluster.id!] = {
location: Cluster.getLocation_(this.projectId, cluster.location!),
serveNodes: cluster.nodes,
ClusterUtils.validateClusterMetadata(cluster);
clusters[cluster.id!] = ClusterUtils.getClusterBaseConfig(
cluster,
Cluster.getLocation_(this.projectId, cluster.location!),
undefined
);
Object.assign(clusters[cluster.id!], {
defaultStorageType: Cluster.getStorageType_(cluster.storage!),
};
});

if (cluster.key) {
clusters[cluster.id!].encryptionConfig = {
Expand Down
22 changes: 9 additions & 13 deletions src/instance.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ import {ServiceError} from 'google-gax';
import {Bigtable} from '.';
import {google} from '../protos/protos';
import {Backup, RestoreTableCallback, RestoreTableResponse} from './backup';
import {ClusterUtils} from './utils/cluster';

export interface ClusterInfo extends BasicClusterConfig {
id: string;
Expand Down Expand Up @@ -391,9 +392,15 @@ Please use the format 'my-instance' or '${bigtable.projectName}/instances/my-ins
parent: this.name,
clusterId: id,
} as google.bigtable.admin.v2.CreateClusterRequest;

ClusterUtils.validateClusterMetadata(options);
if (!is.empty(options)) {
reqOpts.cluster = {};
reqOpts.cluster = ClusterUtils.getClusterBaseConfig(
options,
options.location
? Cluster.getLocation_(this.bigtable.projectId, options.location)
: undefined,
undefined
);
}

if (
Expand All @@ -415,17 +422,6 @@ Please use the format 'my-instance' or '${bigtable.projectName}/instances/my-ins
reqOpts.cluster!.encryptionConfig = options.encryption;
}

if (options.location) {
reqOpts.cluster!.location = Cluster.getLocation_(
this.bigtable.projectId,
options.location
);
}

if (options.nodes) {
kolea2 marked this conversation as resolved.
Show resolved Hide resolved
reqOpts.cluster!.serveNodes = options.nodes;
}

if (options.storage) {
const storageType = Cluster.getStorageType_(options.storage);
reqOpts.cluster!.defaultStorageType = storageType;
Expand Down
153 changes: 153 additions & 0 deletions src/utils/cluster.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,153 @@
// Copyright 2022 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// https://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

import * as protos from '../../protos/protos';
import {
BasicClusterConfig,
ICluster,
SetClusterMetadataOptions,
} from '../cluster';
import {google} from '../../protos/protos';

export class ClusterUtils {
static noConfigError =
'Must specify either serve_nodes or all of the autoscaling configurations (min_serve_nodes, max_serve_nodes, and cpu_utilization_percent).';
static allConfigError =
'Cannot specify both serve_nodes and autoscaling configurations (min_serve_nodes, max_serve_nodes, and cpu_utilization_percent).';
static incompleteConfigError =
'All of autoscaling configurations must be specified at the same time (min_serve_nodes, max_serve_nodes, and cpu_utilization_percent).';

static validateClusterMetadata(
metadata: SetClusterMetadataOptions | BasicClusterConfig
): void {
if (metadata.nodes) {
if (
metadata.minServeNodes ||
metadata.maxServeNodes ||
metadata.cpuUtilizationPercent
) {
throw new Error(this.allConfigError);
}
} else {
if (
metadata.minServeNodes ||
metadata.maxServeNodes ||
metadata.cpuUtilizationPercent
) {
if (
!(
metadata.minServeNodes &&
metadata.maxServeNodes &&
metadata.cpuUtilizationPercent
)
) {
throw new Error(this.incompleteConfigError);
}
} else {
throw new Error(this.noConfigError);
}
}
}
static getUpdateMask(metadata: SetClusterMetadataOptions): string[] {
const updateMask: string[] = [];
if (metadata.nodes) {
updateMask.push('serve_nodes');
if (
!(
metadata.minServeNodes ||
metadata.maxServeNodes ||
metadata.cpuUtilizationPercent
)
) {
updateMask.push('cluster_config.cluster_autoscaling_config');
}
}
if (metadata.minServeNodes) {
updateMask.push(
'cluster_config.cluster_autoscaling_config.autoscaling_limits.min_serve_nodes'
);
}
if (metadata.maxServeNodes) {
updateMask.push(
'cluster_config.cluster_autoscaling_config.autoscaling_limits.max_serve_nodes'
);
}
if (metadata.cpuUtilizationPercent) {
updateMask.push(
'cluster_config.cluster_autoscaling_config.autoscaling_targets.cpu_utilization_percent'
);
}
return updateMask;
}

static getClusterBaseConfig(
metadata: SetClusterMetadataOptions | BasicClusterConfig,
location: string | undefined | null,
name: string | undefined
): google.bigtable.admin.v2.ICluster {
let clusterConfig;
if (
metadata.cpuUtilizationPercent ||
metadata.minServeNodes ||
metadata.maxServeNodes
Copy link
Contributor

Choose a reason for hiding this comment

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

what happens if only one of these are set? Let's say metadata.minServeNodes = 4, but the rest are unset. Could that cause issues?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When we use this in instance.ts we validate beforehand so in this case an error would be thrown as required, but after investigating this further, I think I will add validation to createInstance as well to make sure an error gets thrown if properties are not set properly for each cluster.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When creating instances we now validate inputs.

) {
clusterConfig = {
clusterAutoscalingConfig: {
autoscalingTargets: {
cpuUtilizationPercent: metadata.cpuUtilizationPercent,
},
autoscalingLimits: {
minServeNodes: metadata.minServeNodes,
maxServeNodes: metadata.maxServeNodes,
},
},
};
}
return Object.assign(
{},
name ? {name} : null,
location ? {location} : null,
clusterConfig ? {clusterConfig} : null,
metadata.nodes ? {serveNodes: metadata.nodes} : null
);
}

static getClusterFromMetadata(
metadata: SetClusterMetadataOptions,
location: string | undefined | null,
name: string
): google.bigtable.admin.v2.ICluster {
const cluster: ICluster | SetClusterMetadataOptions = Object.assign(
{},
this.getClusterBaseConfig(metadata, location, name),
metadata
);
delete (cluster as SetClusterMetadataOptions).nodes;
delete (cluster as SetClusterMetadataOptions).minServeNodes;
delete (cluster as SetClusterMetadataOptions).maxServeNodes;
delete (cluster as SetClusterMetadataOptions).cpuUtilizationPercent;
return cluster as ICluster;
}

static getRequestFromMetadata(
metadata: SetClusterMetadataOptions,
location: string | undefined | null,
name: string
): protos.google.bigtable.admin.v2.IPartialUpdateClusterRequest {
return {
cluster: this.getClusterFromMetadata(metadata, location, name),
updateMask: {paths: this.getUpdateMask(metadata)},
};
}
}
Loading