From 7303f7a3484e356aa36e4985e991e1aa87b91b01 Mon Sep 17 00:00:00 2001 From: Morgan McCauley Date: Fri, 21 Jun 2024 21:08:30 +1200 Subject: [PATCH] fix: Allow re-provisioning of recently de-provisioned resources (#825) Provisioning a recently de-provisioned Data Layer would silently fail. This is due to the fact that de/provisioning tasks are stored in-memory, keyed by a hash of the config. So if a provisioning task was recently completed, attempting to re-provision would return that same task. This PR keys by random UUIDs, instead of hashes, so we can trigger multiple provisioning jobs for a given Indexer/Data Layer, allowing for the Provision > De-provision > Provision flow. To protect against re-provisioning an _existing_ Data Layer, we only start the task after verifying it doesn't already exist. Also removed cache from `Provisioner` to ensure we are getting accurate results. --- runner/src/provisioner/provisioner.test.ts | 10 +-- runner/src/provisioner/provisioner.ts | 24 +---- .../data-layer/data-layer-service.test.ts | 36 +++----- .../services/data-layer/data-layer-service.ts | 89 +++++++++---------- 4 files changed, 57 insertions(+), 102 deletions(-) diff --git a/runner/src/provisioner/provisioner.test.ts b/runner/src/provisioner/provisioner.test.ts index c27016832..90d932182 100644 --- a/runner/src/provisioner/provisioner.test.ts +++ b/runner/src/provisioner/provisioner.test.ts @@ -173,24 +173,21 @@ describe('Provisioner', () => { it('returns false if datasource doesnt exists', async () => { hasuraClient.doesSourceExist = jest.fn().mockReturnValueOnce(false); - await expect(provisioner.fetchUserApiProvisioningStatus(indexerConfig)).resolves.toBe(false); - expect(provisioner.isUserApiProvisioned(indexerConfig.accountId, indexerConfig.functionName)).toBe(false); + await expect(provisioner.isProvisioned(indexerConfig)).resolves.toBe(false); }); it('returns false if datasource and schema dont exists', async () => { hasuraClient.doesSourceExist = jest.fn().mockReturnValueOnce(false); hasuraClient.doesSchemaExist = jest.fn().mockReturnValueOnce(false); - await expect(provisioner.fetchUserApiProvisioningStatus(indexerConfig)).resolves.toBe(false); - expect(provisioner.isUserApiProvisioned(indexerConfig.accountId, indexerConfig.functionName)).toBe(false); + await expect(provisioner.isProvisioned(indexerConfig)).resolves.toBe(false); }); it('returns true if datasource and schema exists', async () => { hasuraClient.doesSourceExist = jest.fn().mockReturnValueOnce(true); hasuraClient.doesSchemaExist = jest.fn().mockReturnValueOnce(true); - await expect(provisioner.fetchUserApiProvisioningStatus(indexerConfig)).resolves.toBe(true); - expect(provisioner.isUserApiProvisioned(indexerConfig.accountId, indexerConfig.functionName)).toBe(true); + await expect(provisioner.isProvisioned(indexerConfig)).resolves.toBe(true); }); }); @@ -233,7 +230,6 @@ describe('Provisioner', () => { 'delete' ] ); - expect(provisioner.isUserApiProvisioned(accountId, functionName)).toBe(true); }); it('skips provisioning the datasource if it already exists', async () => { diff --git a/runner/src/provisioner/provisioner.ts b/runner/src/provisioner/provisioner.ts index f23d8ca97..5533c677e 100644 --- a/runner/src/provisioner/provisioner.ts +++ b/runner/src/provisioner/provisioner.ts @@ -49,7 +49,6 @@ const defaultConfig: Config = { export default class Provisioner { tracer: Tracer = trace.getTracer('queryapi-runner-provisioner'); - #hasBeenProvisioned: Record> = {}; constructor ( private readonly hasuraClient: HasuraClient = new HasuraClient(), @@ -70,17 +69,6 @@ export default class Provisioner { .replace(/\//g, '0'); } - isUserApiProvisioned (accountId: string, functionName: string): boolean { - const accountIndexers = this.#hasBeenProvisioned[accountId]; - if (!accountIndexers) { return false; } - return accountIndexers[functionName]; - } - - private setProvisioned (accountId: string, functionName: string): void { - this.#hasBeenProvisioned[accountId] ??= {}; - this.#hasBeenProvisioned[accountId][functionName] = true; - } - async createDatabase (name: string): Promise { await this.adminDefaultPgClient.query(this.pgFormat('CREATE DATABASE %I', name)); } @@ -156,12 +144,8 @@ export default class Provisioner { ); } - async fetchUserApiProvisioningStatus (indexerConfig: ProvisioningConfig): Promise { + async isProvisioned (indexerConfig: ProvisioningConfig): Promise { const checkProvisioningSpan = this.tracer.startSpan('Check if indexer is provisioned'); - if (this.isUserApiProvisioned(indexerConfig.accountId, indexerConfig.functionName)) { - checkProvisioningSpan.end(); - return true; - } const databaseName = indexerConfig.databaseName(); const schemaName = indexerConfig.schemaName(); @@ -172,10 +156,9 @@ export default class Provisioner { } const schemaExists = await this.hasuraClient.doesSchemaExist(databaseName, schemaName); - if (schemaExists) { - this.setProvisioned(indexerConfig.accountId, indexerConfig.functionName); - } + checkProvisioningSpan.end(); + return schemaExists; } @@ -356,7 +339,6 @@ export default class Provisioner { await this.trackForeignKeyRelationships(schemaName, databaseName); await this.addPermissionsToTables(indexerConfig, updatedTableNames, ['select', 'insert', 'update', 'delete']); - this.setProvisioned(indexerConfig.accountId, indexerConfig.functionName); }, 'Failed to provision endpoint' ); diff --git a/runner/src/server/services/data-layer/data-layer-service.test.ts b/runner/src/server/services/data-layer/data-layer-service.test.ts index bc4904514..88541de69 100644 --- a/runner/src/server/services/data-layer/data-layer-service.test.ts +++ b/runner/src/server/services/data-layer/data-layer-service.test.ts @@ -67,25 +67,27 @@ describe('DataLayerService', () => { }); describe('StartProvisioningTask', () => { - it('should return the current task if it exists', (done) => { - const tasks: Record = { - '8291150845651941809f8f3db28eeb7fd8acdfeb422cb07c10178020070836b8': { pending: false, completed: true, failed: false } as unknown as AsyncTask - }; + it('returns FAILED_PRECONDITION if already provisioned', (done) => { + const provisioner = { + isProvisioned: jest.fn().mockResolvedValue(true) + } as unknown as Provisioner; + const tasks = {}; const call = { - request: { accountId: 'testAccount', functionName: 'testFunction', schema: 'schema' } + request: { accountId: 'testAccount', functionName: 'testFunction', schema: 'testSchema' } } as unknown as ServerUnaryCall; - const callback = (_error: any, response: any): void => { - expect(tasks[response.taskId]).toBeDefined(); - expect(tasks[response.taskId].completed).toBe(true); + const callback = (error: any): void => { + expect(error.code).toBe(status.FAILED_PRECONDITION); + expect(error.details).toBe('Data Layer is already provisioned'); done(); }; - createDataLayerService(undefined, tasks).StartProvisioningTask(call, callback); + createDataLayerService(provisioner, tasks).StartProvisioningTask(call, callback); }); it('should start a new provisioning task', (done) => { const tasks: Record = {}; const provisioner = { + isProvisioned: jest.fn().mockResolvedValue(false), provisionUserApi: jest.fn().mockResolvedValue(null) } as unknown as Provisioner; const call = { @@ -102,22 +104,6 @@ describe('DataLayerService', () => { }); describe('StartDeprovisioningTask', () => { - it('should return ALREADY_EXISTS if the task exists', (done) => { - const tasks = { - f92a9f97d2609849e6837b483d8210c7b308c6f615a691449087ec00db1eef06: { pending: true, completed: false, failed: false } as unknown as AsyncTask - }; - const call = { - request: { accountId: 'testAccount', functionName: 'testFunction', schema: 'schema' } - } as unknown as ServerUnaryCall; - const callback = (error: any): void => { - expect(error.code).toBe(status.ALREADY_EXISTS); - expect(error.details).toBe('Deprovisioning task already exists'); - done(); - }; - - createDataLayerService(undefined, tasks).StartDeprovisioningTask(call, callback); - }); - it('should start a new deprovisioning task', (done) => { const tasks: Record = {}; const provisioner = { diff --git a/runner/src/server/services/data-layer/data-layer-service.ts b/runner/src/server/services/data-layer/data-layer-service.ts index f82cbc1eb..5f60c18dd 100644 --- a/runner/src/server/services/data-layer/data-layer-service.ts +++ b/runner/src/server/services/data-layer/data-layer-service.ts @@ -39,17 +39,6 @@ export class AsyncTask { type AsyncTasks = Record; -enum TaskType { - PROVISION = 'PROVISION', - DEPROVISION = 'DEPROVISION' -} - -const hash = (...args: string[]): string => { - const hash = crypto.createHash('sha256'); - hash.update(args.join(':')); - return hash.digest('hex'); -}; - const createLogger = (config: ProvisioningConfig): typeof parentLogger => { const logger = parentLogger.child({ accountId: config.accountId, @@ -98,31 +87,45 @@ export function createDataLayerService ( const logger = createLogger(provisioningConfig); - const taskId = hash(accountId, functionName, schema, TaskType.PROVISION); - - const task = tasks[taskId]; - - if (task) { - callback(null, { taskId }); - - return; - }; - - logger.info(`Starting provisioning task: ${taskId}`); - - tasks[taskId] = new AsyncTask( - provisioner - .provisionUserApi(provisioningConfig) - .then(() => { - logger.info('Successfully provisioned Data Layer'); - }) - .catch((err) => { - logger.error('Failed to provision Data Layer', err); - throw err; - }) - ); - - callback(null, { taskId }); + provisioner + .isProvisioned(provisioningConfig) + .then((isProvisioned) => { + if (isProvisioned) { + const failedPrecondition = new StatusBuilder() + .withCode(status.FAILED_PRECONDITION) + .withDetails('Data Layer is already provisioned') + .build(); + + callback(failedPrecondition); + + return; + } + + const taskId = crypto.randomUUID(); + + tasks[taskId] = new AsyncTask( + provisioner + .provisionUserApi(provisioningConfig) + .then(() => { + logger.info('Successfully deprovisioned Data Layer'); + }) + .catch((err) => { + logger.error('Failed to deprovision Data Layer', err); + throw err; + }) + ); + + callback(null, { taskId }); + }) + .catch((err) => { + logger.error('Failed to check if Data Layer is provisioned', err); + + const internal = new StatusBuilder() + .withCode(status.INTERNAL) + .withDetails('Failed to check Data Layer provisioned status') + .build(); + callback(internal); + }); }, StartDeprovisioningTask (call: ServerUnaryCall, callback: sendUnaryData): void { @@ -132,19 +135,7 @@ export function createDataLayerService ( const logger = createLogger(provisioningConfig); - const taskId = hash(accountId, functionName, TaskType.DEPROVISION); - - const task = tasks[taskId]; - - if (task) { - const exists = new StatusBuilder() - .withCode(status.ALREADY_EXISTS) - .withDetails('Deprovisioning task already exists') - .build(); - callback(exists); - - return; - }; + const taskId = crypto.randomUUID(); logger.info(`Starting deprovisioning task: ${taskId}`);