From 02403d6db72cc9a7e094260d240d2ce02ed6384f Mon Sep 17 00:00:00 2001 From: Jackie Zhang Date: Mon, 11 Nov 2024 12:59:50 -0800 Subject: [PATCH 1/5] explict create blob container --- .../gitrest-base/src/routes/summaries.ts | 18 ++++++++++++++++++ .../src/utils/gitWholeSummaryManager.ts | 9 +++++++++ .../utils/wholeSummary/writeWholeSummary.ts | 5 +++++ 3 files changed, 32 insertions(+) diff --git a/server/gitrest/packages/gitrest-base/src/routes/summaries.ts b/server/gitrest/packages/gitrest-base/src/routes/summaries.ts index 728fb057d10d..02a6adf33047 100644 --- a/server/gitrest/packages/gitrest-base/src/routes/summaries.ts +++ b/server/gitrest/packages/gitrest-base/src/routes/summaries.ts @@ -138,6 +138,7 @@ async function createSummary( persistLatestFullEphemeralSummary = false, enableLowIoWrite: "initial" | boolean = false, optimizeForInitialSummary: boolean = false, + enableContainerPerDocTimeStamp: number = 0, ): Promise { const lumberjackProperties = { ...getLumberjackBasePropertiesFromRepoManagerParams(repoManagerParams), @@ -152,12 +153,14 @@ async function createSummary( { enableLowIoWrite, optimizeForInitialSummary, + enableContainerPerDocTimeStamp, }, ); Lumberjack.info("Creating summary", lumberjackProperties); const { isNew, writeSummaryResponse } = await wholeSummaryManager.writeSummary( + fileSystemManager, payload, isInitialSummary, ); @@ -256,6 +259,19 @@ export function create( const repoPerDocEnabled: boolean = store.get("git:repoPerDocEnabled") ?? false; const enforceStrictPersistedFullSummaryReads: boolean = store.get("git:enforceStrictPersistedFullSummaryReads") ?? false; + // Get the containerPerDocEnabling timestamp, the 1st element in the config array. + const containerPerDocMigrationCutoffTS: string = store.get( + "azureBlobFs:containerPerDocMigrationCutoffTS", + ) ?? undefined; + let enableContainerPerDocTimeStamp: number = 0; + if (containerPerDocMigrationCutoffTS) { + const migrationCutoffTimeStamp = containerPerDocMigrationCutoffTS + .split(",") + .shift(); + if (migrationCutoffTimeStamp) { + enableContainerPerDocTimeStamp = new Date(migrationCutoffTimeStamp.trim()).getTime(); + } + } /** * Retrieves a summary. @@ -315,6 +331,7 @@ export function create( // eslint-disable-next-line @typescript-eslint/no-misused-promises router.post("/repos/:owner/:repo/git/summaries", async (request, response) => { const repoManagerParams = getRepoManagerParamsFromRequest(request); + // Todo: Add cmk encryption scope here. const tenantId = repoManagerParams.storageRoutingId?.tenantId; const documentId = repoManagerParams.storageRoutingId?.documentId; // request.query type is { [string]: string } but it's actually { [string]: any } @@ -384,6 +401,7 @@ export function create( persistLatestFullEphemeralSummary, enableLowIoWrite, optimizeForInitialSummary, + enableContainerPerDocTimeStamp, ); })().catch((error) => logAndThrowApiError(error, request, repoManagerParams)); handleResponse(resultP, response, undefined, undefined, 201); diff --git a/server/gitrest/packages/gitrest-base/src/utils/gitWholeSummaryManager.ts b/server/gitrest/packages/gitrest-base/src/utils/gitWholeSummaryManager.ts index cc6363394a37..4ed428925e9b 100644 --- a/server/gitrest/packages/gitrest-base/src/utils/gitWholeSummaryManager.ts +++ b/server/gitrest/packages/gitrest-base/src/utils/gitWholeSummaryManager.ts @@ -27,6 +27,7 @@ import { getSoftDeletedMarkerPath } from "./helpers"; const DefaultSummaryWriteFeatureFlags: ISummaryWriteFeatureFlags = { enableLowIoWrite: false, optimizeForInitialSummary: false, + enableContainerPerDocTimeStamp: 0, }; export { isChannelSummary, isContainerSummary } from "./wholeSummary"; @@ -92,6 +93,7 @@ export class GitWholeSummaryManager { * If the summary is a "channel" summary, the tree sha will be returned so that it can be referenced by a future "container" summary. */ public async writeSummary( + fileSystemManager: IFileSystemManager, payload: IWholeSummaryPayload, isInitial?: boolean, ): Promise { @@ -99,6 +101,7 @@ export class GitWholeSummaryManager { ...this.lumberjackProperties, enableLowIoWrite: this.summaryWriteFeatureFlags.enableLowIoWrite, optimizeForInitialSummary: this.summaryWriteFeatureFlags.optimizeForInitialSummary, + enableContainerPerDocTS: this.summaryWriteFeatureFlags.enableContainerPerDocTimeStamp, isInitial, }; const writeSummaryMetric = Lumberjack.newLumberMetric( @@ -106,6 +109,12 @@ export class GitWholeSummaryManager { lumberjackProperties, ); try { + // Create blob container if initial summary and blobContainerPerDoc is enabled. + if (isInitial && Date.now() > this.summaryWriteFeatureFlags.enableContainerPerDocTimeStamp) { + const summaryFolderPath = this.repoManager.path; + await fileSystemManager.promises.mkdir(summaryFolderPath, "create-blob-container"); + Lumberjack.warning(`[Azfs-debug] Created blob container for initial summary`, lumberjackProperties); + } if (isChannelSummary(payload)) { lumberjackProperties.summaryType = "channel"; writeSummaryMetric.setProperty("summaryType", "channel"); diff --git a/server/gitrest/packages/gitrest-base/src/utils/wholeSummary/writeWholeSummary.ts b/server/gitrest/packages/gitrest-base/src/utils/wholeSummary/writeWholeSummary.ts index 5751d77101ca..c878f6f1cc8d 100644 --- a/server/gitrest/packages/gitrest-base/src/utils/wholeSummary/writeWholeSummary.ts +++ b/server/gitrest/packages/gitrest-base/src/utils/wholeSummary/writeWholeSummary.ts @@ -50,6 +50,11 @@ export interface ISummaryWriteFeatureFlags { * to avoid unnecessary storage operations. This can improve performance when creating a new document. */ optimizeForInitialSummary: boolean; + /** + * If the runtime time stamp is greater than enableContainerPerDocTimeStamp, meaning blobContainerPerDoc is enabled. + * The azure blob container for the document will be explictily created before initial summary upload. + */ + enableContainerPerDocTimeStamp: number; } /** From ddcc441d78474f69e325d06dee42714079d24a44 Mon Sep 17 00:00:00 2001 From: Jackie Zhang Date: Mon, 11 Nov 2024 17:52:18 -0800 Subject: [PATCH 2/5] fix UTs and format --- .../gitrest-base/src/routes/summaries.ts | 9 ++-- .../gitrest-base/src/test/summaries.spec.ts | 44 +++++++++++++++---- .../src/utils/gitWholeSummaryManager.ts | 10 ++++- 3 files changed, 46 insertions(+), 17 deletions(-) diff --git a/server/gitrest/packages/gitrest-base/src/routes/summaries.ts b/server/gitrest/packages/gitrest-base/src/routes/summaries.ts index 02a6adf33047..b5991d01f65c 100644 --- a/server/gitrest/packages/gitrest-base/src/routes/summaries.ts +++ b/server/gitrest/packages/gitrest-base/src/routes/summaries.ts @@ -260,14 +260,11 @@ export function create( const enforceStrictPersistedFullSummaryReads: boolean = store.get("git:enforceStrictPersistedFullSummaryReads") ?? false; // Get the containerPerDocEnabling timestamp, the 1st element in the config array. - const containerPerDocMigrationCutoffTS: string = store.get( - "azureBlobFs:containerPerDocMigrationCutoffTS", - ) ?? undefined; + const containerPerDocMigrationCutoffTS: string = + store.get("azureBlobFs:containerPerDocMigrationCutoffTS") ?? undefined; let enableContainerPerDocTimeStamp: number = 0; if (containerPerDocMigrationCutoffTS) { - const migrationCutoffTimeStamp = containerPerDocMigrationCutoffTS - .split(",") - .shift(); + const migrationCutoffTimeStamp = containerPerDocMigrationCutoffTS.split(",").shift(); if (migrationCutoffTimeStamp) { enableContainerPerDocTimeStamp = new Date(migrationCutoffTimeStamp.trim()).getTime(); } diff --git a/server/gitrest/packages/gitrest-base/src/test/summaries.spec.ts b/server/gitrest/packages/gitrest-base/src/test/summaries.spec.ts index 2102a91e997f..f64955cc7b26 100644 --- a/server/gitrest/packages/gitrest-base/src/test/summaries.spec.ts +++ b/server/gitrest/packages/gitrest-base/src/test/summaries.spec.ts @@ -261,7 +261,11 @@ testFileSystems.forEach((fileSystem) => { // Test standard summary flow and storage access frequency. it("Can create and read an initial summary and a subsequent incremental summary", async () => { + const fsManager = fsManagerFactory.create({ + rootDir: repoManager.path, + }); const initialWriteResponse = await getWholeSummaryManager().writeSummary( + fsManager, sampleInitialSummaryUpload, true, ); @@ -292,6 +296,7 @@ testFileSystems.forEach((fileSystem) => { ); const channelWriteResponse = await getWholeSummaryManager().writeSummary( + fsManager, sampleChannelSummaryUpload, false, ); @@ -311,6 +316,7 @@ testFileSystems.forEach((fileSystem) => { ); const containerWriteResponse = await getWholeSummaryManager().writeSummary( + fsManager, // Replace the referenced channel summary with the one we just wrote. // This matters when low-io write is enabled, because it alters how the tree is stored. replaceTestShas(sampleContainerSummaryUpload, [ @@ -379,7 +385,11 @@ testFileSystems.forEach((fileSystem) => { * 6. Wait until Client Summary is written */ it("Can create and read multiple summaries", async () => { + const fsManager = fsManagerFactory.create({ + rootDir: repoManager.path, + }); const initialWriteResponse = await getWholeSummaryManager().writeSummary( + fsManager, ElaborateInitialPayload, true, ); @@ -403,6 +413,7 @@ testFileSystems.forEach((fileSystem) => { ); const firstChannelWriteResponse = await getWholeSummaryManager().writeSummary( + fsManager, ElaborateFirstChannelPayload, false, ); @@ -422,6 +433,7 @@ testFileSystems.forEach((fileSystem) => { ); const firstContainerWriteResponse = await getWholeSummaryManager().writeSummary( + fsManager, // Replace the referenced channel summary with the one we just wrote. // This matters when low-io write is enabled, because it alters how the tree is stored. replaceTestShas(ElaborateFirstContainerPayload, [ @@ -463,6 +475,7 @@ testFileSystems.forEach((fileSystem) => { const firstServiceContainerWriteResponse = await getWholeSummaryManager().writeSummary( + fsManager, // Replace the referenced channel summary with the one we just wrote. // This matters when low-io write is enabled, because it alters how the tree is stored. replaceTestShas(ElaborateFirstServiceContainerPayload, [ @@ -492,6 +505,7 @@ testFileSystems.forEach((fileSystem) => { ); const secondChannelWriteResponse = await getWholeSummaryManager().writeSummary( + fsManager, // Replace the referenced container summary with the one we just wrote. replaceTestShas(ElaborateSecondChannelPayload, [ { @@ -517,6 +531,7 @@ testFileSystems.forEach((fileSystem) => { ); const secondContainerWriteResponse = await getWholeSummaryManager().writeSummary( + fsManager, // Replace the referenced channel summary with the one we just wrote. // This matters when low-io write is enabled, because it alters how the tree is stored. replaceTestShas(ElaborateSecondContainerPayload, [ @@ -557,8 +572,12 @@ testFileSystems.forEach((fileSystem) => { * Validates that after deletion we cannot read and subsequent delete attempts are no-ops, not errors. */ it("Can hard-delete a document's summary data", async () => { + const fsManager = fsManagerFactory.create({ + rootDir: repoManager.path, + }); // Write and validate initial summary. const initialWriteResponse = await getWholeSummaryManager().writeSummary( + fsManager, ElaborateInitialPayload, true, ); @@ -581,9 +600,6 @@ testFileSystems.forEach((fileSystem) => { ); // Delete document. - const fsManager = fsManagerFactory.create({ - rootDir: repoManager.path, - }); await getWholeSummaryManager().deleteSummary(fsManager, false /* softDelete */); // Validate that we cannot read the summary. await assert.rejects( @@ -607,8 +623,12 @@ testFileSystems.forEach((fileSystem) => { * Validates that after deletion we cannot read and subsequent delete attempts are no-ops, not errors. */ it("Can soft-delete a document's summary data", async () => { + const fsManager = fsManagerFactory.create({ + rootDir: repoManager.path, + }); // Write and validate initial summary. const initialWriteResponse = await getWholeSummaryManager().writeSummary( + fsManager, ElaborateInitialPayload, true, ); @@ -631,9 +651,6 @@ testFileSystems.forEach((fileSystem) => { ); // Delete document. - const fsManager = fsManagerFactory.create({ - rootDir: repoManager.path, - }); await getWholeSummaryManager().deleteSummary(fsManager, true /* softDelete */); // Validate that soft-deletion flag is detected. assert.rejects( @@ -670,9 +687,12 @@ testFileSystems.forEach((fileSystem) => { it(`Can read from and write to an initial summary stored ${ enableLowIoWrite ? "with" : "without" } low-io write`, async () => { + const fsManager = fsManagerFactory.create({ + rootDir: repoManager.path, + }); await getWholeSummaryManager({ enableLowIoWrite, - }).writeSummary(sampleInitialSummaryUpload, true); + }).writeSummary(fsManager, sampleInitialSummaryUpload, true); const initialReadResponse = await getWholeSummaryManager().readSummary(LatestSummaryId); @@ -682,10 +702,12 @@ testFileSystems.forEach((fileSystem) => { "Initial summary read response should match expected response.", ); const channelWriteResponse = await getWholeSummaryManager().writeSummary( + fsManager, sampleChannelSummaryUpload, false, ); const containerWriteResponse = await getWholeSummaryManager().writeSummary( + fsManager, // Replace the referenced channel summary with the one we just wrote. // This matters when low-io write is enabled, because it alters how the tree is stored. JSON.parse( @@ -711,15 +733,19 @@ testFileSystems.forEach((fileSystem) => { it(`Can read an incremental summary stored ${ enableLowIoWrite ? "with" : "without" } low-io write`, async () => { + const fsManager = fsManagerFactory.create({ + rootDir: repoManager.path, + }); await getWholeSummaryManager({ enableLowIoWrite, - }).writeSummary(sampleInitialSummaryUpload, true); + }).writeSummary(fsManager, sampleInitialSummaryUpload, true); const channelWriteResponse = await getWholeSummaryManager({ enableLowIoWrite, - }).writeSummary(sampleChannelSummaryUpload, false); + }).writeSummary(fsManager, sampleChannelSummaryUpload, false); const containerWriteResponse = await getWholeSummaryManager({ enableLowIoWrite, }).writeSummary( + fsManager, // Replace the referenced channel summary with the one we just wrote. // This matters when low-io write is enabled, because it alters how the tree is stored. JSON.parse( diff --git a/server/gitrest/packages/gitrest-base/src/utils/gitWholeSummaryManager.ts b/server/gitrest/packages/gitrest-base/src/utils/gitWholeSummaryManager.ts index 4ed428925e9b..0d049f9c587c 100644 --- a/server/gitrest/packages/gitrest-base/src/utils/gitWholeSummaryManager.ts +++ b/server/gitrest/packages/gitrest-base/src/utils/gitWholeSummaryManager.ts @@ -110,10 +110,16 @@ export class GitWholeSummaryManager { ); try { // Create blob container if initial summary and blobContainerPerDoc is enabled. - if (isInitial && Date.now() > this.summaryWriteFeatureFlags.enableContainerPerDocTimeStamp) { + if ( + isInitial && + Date.now() > this.summaryWriteFeatureFlags.enableContainerPerDocTimeStamp + ) { const summaryFolderPath = this.repoManager.path; await fileSystemManager.promises.mkdir(summaryFolderPath, "create-blob-container"); - Lumberjack.warning(`[Azfs-debug] Created blob container for initial summary`, lumberjackProperties); + Lumberjack.warning( + `[Azfs-debug] Created blob container for initial summary`, + lumberjackProperties, + ); } if (isChannelSummary(payload)) { lumberjackProperties.summaryType = "channel"; From 813c3601c861abfa0d9b5ecdbb05922925acb215 Mon Sep 17 00:00:00 2001 From: Jackie Zhang Date: Mon, 11 Nov 2024 21:17:30 -0800 Subject: [PATCH 3/5] add config enableContainerPerDocTimeStamp truthy check --- .../packages/gitrest-base/src/utils/gitWholeSummaryManager.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/server/gitrest/packages/gitrest-base/src/utils/gitWholeSummaryManager.ts b/server/gitrest/packages/gitrest-base/src/utils/gitWholeSummaryManager.ts index 0d049f9c587c..d91de6f3eb7e 100644 --- a/server/gitrest/packages/gitrest-base/src/utils/gitWholeSummaryManager.ts +++ b/server/gitrest/packages/gitrest-base/src/utils/gitWholeSummaryManager.ts @@ -112,6 +112,7 @@ export class GitWholeSummaryManager { // Create blob container if initial summary and blobContainerPerDoc is enabled. if ( isInitial && + this.summaryWriteFeatureFlags.enableContainerPerDocTimeStamp && Date.now() > this.summaryWriteFeatureFlags.enableContainerPerDocTimeStamp ) { const summaryFolderPath = this.repoManager.path; From f5ab3686a0fc038e1843b53084d60d74412d9f0d Mon Sep 17 00:00:00 2001 From: Jackie Zhang Date: Wed, 13 Nov 2024 16:23:27 -0800 Subject: [PATCH 4/5] customize mkdir options --- server/gitrest/packages/gitrest-base/src/index.ts | 1 + .../packages/gitrest-base/src/utils/definitions.ts | 9 +++++++++ .../gitrest-base/src/utils/gitWholeSummaryManager.ts | 12 ++++++++++-- .../gitrest/packages/gitrest-base/src/utils/index.ts | 1 + 4 files changed, 21 insertions(+), 2 deletions(-) diff --git a/server/gitrest/packages/gitrest-base/src/index.ts b/server/gitrest/packages/gitrest-base/src/index.ts index 582437f7e684..65d2c3686f8e 100644 --- a/server/gitrest/packages/gitrest-base/src/index.ts +++ b/server/gitrest/packages/gitrest-base/src/index.ts @@ -29,6 +29,7 @@ export { IFileSystemManagerFactory, IFileSystemManagerParams, IFileSystemPromises, + IFRSMakeDirectoryOptions, IRepoManagerParams, IRepositoryManager, IRepositoryManagerFactory, diff --git a/server/gitrest/packages/gitrest-base/src/utils/definitions.ts b/server/gitrest/packages/gitrest-base/src/utils/definitions.ts index 004bacb353ba..e022962a5949 100644 --- a/server/gitrest/packages/gitrest-base/src/utils/definitions.ts +++ b/server/gitrest/packages/gitrest-base/src/utils/definitions.ts @@ -5,6 +5,7 @@ import fsPromises from "fs/promises"; import * as git from "@fluidframework/gitresources"; +import type { MakeDirectoryOptions } from "fs"; export enum Constants { StorageRoutingIdHeader = "Storage-Routing-Id", @@ -113,6 +114,14 @@ export interface IRepoManagerParams { fileSystemManagerParams?: IFileSystemManagerParams; optimizeForInitialSummary?: boolean; isEphemeralContainer?: boolean; + // This maybe the place to hookup cmk scope. +} + +export interface IFRSMakeDirectoryOptions extends MakeDirectoryOptions { + /** + * FRS specific cmk encryption scope. + */ + scope: string; } export interface IRepositoryManagerFactory { diff --git a/server/gitrest/packages/gitrest-base/src/utils/gitWholeSummaryManager.ts b/server/gitrest/packages/gitrest-base/src/utils/gitWholeSummaryManager.ts index d91de6f3eb7e..e8020a8ffcdf 100644 --- a/server/gitrest/packages/gitrest-base/src/utils/gitWholeSummaryManager.ts +++ b/server/gitrest/packages/gitrest-base/src/utils/gitWholeSummaryManager.ts @@ -10,7 +10,11 @@ import { isNetworkError, } from "@fluidframework/server-services-client"; import { Lumberjack } from "@fluidframework/server-services-telemetry"; -import { IRepositoryManager, type IFileSystemManager } from "./definitions"; +import { + IRepositoryManager, + type IFileSystemManager, + type IFRSMakeDirectoryOptions, +} from "./definitions"; import { GitRestLumberEventName } from "./gitrestTelemetryDefinitions"; import { Constants, @@ -115,8 +119,12 @@ export class GitWholeSummaryManager { this.summaryWriteFeatureFlags.enableContainerPerDocTimeStamp && Date.now() > this.summaryWriteFeatureFlags.enableContainerPerDocTimeStamp ) { + const frsOptions: IFRSMakeDirectoryOptions = { + recursive: false, + scope: "cmk-scope", + }; const summaryFolderPath = this.repoManager.path; - await fileSystemManager.promises.mkdir(summaryFolderPath, "create-blob-container"); + await fileSystemManager.promises.mkdir(summaryFolderPath, frsOptions); Lumberjack.warning( `[Azfs-debug] Created blob container for initial summary`, lumberjackProperties, diff --git a/server/gitrest/packages/gitrest-base/src/utils/index.ts b/server/gitrest/packages/gitrest-base/src/utils/index.ts index 6260b38babbc..9c5ce046430c 100644 --- a/server/gitrest/packages/gitrest-base/src/utils/index.ts +++ b/server/gitrest/packages/gitrest-base/src/utils/index.ts @@ -12,6 +12,7 @@ export { IFileSystemManagerFactory, IFileSystemManagerParams, IFileSystemPromises, + IFRSMakeDirectoryOptions, IRepoManagerParams, IRepositoryManager, IRepositoryManagerFactory, From 324ef1ac70d75abb91819ecc76cb0aa80e6e1248 Mon Sep 17 00:00:00 2001 From: Jackie Zhang Date: Thu, 14 Nov 2024 20:59:01 -0800 Subject: [PATCH 5/5] refactor debug log --- .../packages/gitrest-base/src/utils/gitWholeSummaryManager.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/gitrest/packages/gitrest-base/src/utils/gitWholeSummaryManager.ts b/server/gitrest/packages/gitrest-base/src/utils/gitWholeSummaryManager.ts index e8020a8ffcdf..84a78aca7b53 100644 --- a/server/gitrest/packages/gitrest-base/src/utils/gitWholeSummaryManager.ts +++ b/server/gitrest/packages/gitrest-base/src/utils/gitWholeSummaryManager.ts @@ -124,11 +124,11 @@ export class GitWholeSummaryManager { scope: "cmk-scope", }; const summaryFolderPath = this.repoManager.path; - await fileSystemManager.promises.mkdir(summaryFolderPath, frsOptions); Lumberjack.warning( - `[Azfs-debug] Created blob container for initial summary`, + `[Azfs-debug] Force created blob container, path: ${summaryFolderPath}`, lumberjackProperties, ); + await fileSystemManager.promises.mkdir(summaryFolderPath, frsOptions); } if (isChannelSummary(payload)) { lumberjackProperties.summaryType = "channel";