Skip to content

Commit

Permalink
Put Memory Blob Storage Under a Flag (microsoft#21612)
Browse files Browse the repository at this point in the history
We recently made changes to how detached blob storage works, and enabled
a default in-memory blob storage such that blobs would be supported by
default in detached containers.

However, when blobs are in a detached container, it changes the
container attachment flow, specifically, an empty file is created, the
blobs are uploaded, and then the summary is uploaded. Due to this
differing flow some drivers, like odsp, create files with a `.tmp`
extension appended to the file name, this is meant to signify that that
file could be incomplete. For example, the summary could fail to upload
after blobs have been uploaded, and this will not be a valid fluid file.
it is expected that the application will rename the file after
attachment completes if the `.tmp` extension is not desired. If an
application does not expect the `.tmp` extension this can create issues,
so for now, we will put this support under a flag, that must be
explicitly enabled `"Fluid.Container.MemoryBlobStorageEnabled" = true`
so that apps must opt into this behavior.

related to microsoft#21144

Co-authored-by: Tony Murphy <anthonm@microsoft.com>
  • Loading branch information
anthony-murphy and anthony-murphy committed Jun 25, 2024
1 parent e1884ab commit 06c2c5d
Show file tree
Hide file tree
Showing 4 changed files with 59 additions and 31 deletions.
16 changes: 11 additions & 5 deletions packages/loader/container-loader/src/container.ts
Original file line number Diff line number Diff line change
Expand Up @@ -480,7 +480,7 @@ export class Container
private readonly scope: FluidObject;
private readonly subLogger: ITelemetryLoggerExt;
// eslint-disable-next-line import/no-deprecated
private readonly detachedBlobStorage: IDetachedBlobStorage;
private readonly detachedBlobStorage: IDetachedBlobStorage | undefined;
private readonly protocolHandlerBuilder: ProtocolHandlerBuilder;
private readonly client: IClient;

Expand Down Expand Up @@ -798,7 +798,6 @@ export class Container
// Tracking alternative ways to handle this in AB#4129.
this.options = { ...options };
this.scope = scope;
this.detachedBlobStorage = detachedBlobStorage ?? createMemoryDetachedBlobStorage();
this.protocolHandlerBuilder =
protocolHandlerBuilder ??
((
Expand Down Expand Up @@ -975,6 +974,12 @@ export class Container
this.mc.config.getBoolean("Fluid.Container.summarizeProtocolTree2") ??
options.summarizeProtocolTree;

this.detachedBlobStorage =
detachedBlobStorage ??
(this.mc.config.getBoolean("Fluid.Container.MemoryBlobStorageEnabled") === true
? createMemoryDetachedBlobStorage()
: undefined);

this.storageAdapter = new ContainerStorageAdapter(
this.detachedBlobStorage,
this.mc.logger,
Expand Down Expand Up @@ -1236,7 +1241,8 @@ export class Container
baseSnapshot,
snapshotBlobs,
pendingRuntimeState,
hasAttachmentBlobs: this.detachedBlobStorage.size > 0,
hasAttachmentBlobs:
this.detachedBlobStorage !== undefined && this.detachedBlobStorage.size > 0,
attachmentBlobs: serializeMemoryDetachedBlobStorage(this.detachedBlobStorage),
};
return JSON.stringify(detachedContainerState);
Expand Down Expand Up @@ -1353,7 +1359,7 @@ export class Container
const snapshotWithBlobs = await attachP;
this.serializedStateManager.setInitialSnapshot(snapshotWithBlobs);
if (!this.closed) {
this.detachedBlobStorage.dispose?.();
this.detachedBlobStorage?.dispose?.();
this.handleDeltaConnectionArg(attachProps?.deltaConnection, {
fetchOpsFromStorage: false,
reason: { text: "createDetached" },
Expand Down Expand Up @@ -1791,7 +1797,7 @@ export class Container
tryInitializeMemoryDetachedBlobStorage(this.detachedBlobStorage, attachmentBlobs);
}
assert(
this.detachedBlobStorage.size > 0,
this.detachedBlobStorage !== undefined && this.detachedBlobStorage.size > 0,
0x250 /* "serialized container with attachment blobs must be rehydrated with detached blob storage" */,
);
}
Expand Down
12 changes: 8 additions & 4 deletions packages/loader/container-loader/src/memoryBlobStorage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ interface MemoryDetachedBlobStorage extends IDetachedBlobStorage {

function isMemoryDetachedBlobStorage(
// eslint-disable-next-line import/no-deprecated
detachedStorage: IDetachedBlobStorage,
detachedStorage: IDetachedBlobStorage | undefined,
): detachedStorage is MemoryDetachedBlobStorage {
return (
isObject(detachedStorage) &&
Expand All @@ -33,16 +33,20 @@ function isMemoryDetachedBlobStorage(

export function serializeMemoryDetachedBlobStorage(
// eslint-disable-next-line import/no-deprecated
detachedStorage: IDetachedBlobStorage,
detachedStorage: IDetachedBlobStorage | undefined,
): string | undefined {
if (detachedStorage.size > 0 && isMemoryDetachedBlobStorage(detachedStorage)) {
if (
detachedStorage !== undefined &&
detachedStorage.size > 0 &&
isMemoryDetachedBlobStorage(detachedStorage)
) {
return detachedStorage.serialize();
}
}

export function tryInitializeMemoryDetachedBlobStorage(
// eslint-disable-next-line import/no-deprecated
detachedStorage: IDetachedBlobStorage,
detachedStorage: IDetachedBlobStorage | undefined,
attachmentBlobs: string,
) {
if (!isMemoryDetachedBlobStorage(detachedStorage)) {
Expand Down
56 changes: 36 additions & 20 deletions packages/test/test-end-to-end-tests/src/test/blobs.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,7 @@ import {
ContainerMessageType,
DefaultSummaryConfiguration,
} from "@fluidframework/container-runtime/internal";
import {
ConfigTypes,
IConfigProviderBase,
IErrorBase,
IFluidHandle,
} from "@fluidframework/core-interfaces";
import { IErrorBase, IFluidHandle } from "@fluidframework/core-interfaces";
import { Deferred } from "@fluidframework/core-utils/internal";
import { IDocumentServiceFactory } from "@fluidframework/driver-definitions/internal";
import { ReferenceType } from "@fluidframework/merge-tree/internal";
Expand All @@ -33,6 +28,7 @@ import {
ChannelFactoryRegistry,
ITestContainerConfig,
ITestObjectProvider,
createTestConfigProvider,
getContainerEntryPointBackCompat,
waitForContainerConnection,
} from "@fluidframework/test-utils/internal";
Expand All @@ -46,10 +42,6 @@ import {
getUrlFromDetachedBlobStorage,
} from "./mockDetachedBlobStorage.js";

const configProvider = (settings: Record<string, ConfigTypes>): IConfigProviderBase => ({
getRawConfig: (name: string): ConfigTypes => settings[name],
});

function makeTestContainerConfig(registry: ChannelFactoryRegistry): ITestContainerConfig {
return {
runtimeOptions: {
Expand Down Expand Up @@ -447,6 +439,9 @@ function serializationTests({
loaderProps: {
detachedBlobStorage,
options: { summarizeProtocolTree },
configProvider: createTestConfigProvider({
"Fluid.Container.MemoryBlobStorageEnabled": true,
}),
},
});
const container = await loader.createDetachedContainer(
Expand Down Expand Up @@ -499,7 +494,12 @@ function serializationTests({
it("serialize/rehydrate container with blobs", async function () {
const loader = provider.makeTestLoader({
...testContainerConfig,
loaderProps: { detachedBlobStorage },
loaderProps: {
detachedBlobStorage,
configProvider: createTestConfigProvider({
"Fluid.Container.MemoryBlobStorageEnabled": true,
}),
},
});
const serializeContainer = await loader.createDetachedContainer(
provider.defaultCodeDetails,
Expand Down Expand Up @@ -548,10 +548,10 @@ function serializationTests({
loaderProps: {
detachedBlobStorage,
documentServiceFactory,
configProvider: {
getRawConfig: (name) =>
name === "Fluid.Container.RetryOnAttachFailure" ? true : undefined,
},
configProvider: createTestConfigProvider({
"Fluid.Container.MemoryBlobStorageEnabled": true,
"Fluid.Container.RetryOnAttachFailure": true,
}),
},
});
const serializeContainer = await loader.createDetachedContainer(
Expand Down Expand Up @@ -602,13 +602,19 @@ function serializationTests({
ContainerCloseUsageError,
async function () {
// test with and without offline load enabled
const offlineCfg = configProvider({
const offlineCfg = {
"Fluid.Container.enableOfflineLoad": true,
});
};
for (const cfg of [undefined, offlineCfg]) {
const loader = provider.makeTestLoader({
...testContainerConfig,
loaderProps: { detachedBlobStorage, configProvider: cfg },
loaderProps: {
detachedBlobStorage,
configProvider: createTestConfigProvider({
"Fluid.Container.MemoryBlobStorageEnabled": true,
...offlineCfg,
}),
},
});
const detachedContainer = await loader.createDetachedContainer(
provider.defaultCodeDetails,
Expand Down Expand Up @@ -673,7 +679,12 @@ function serializationTests({
async function () {
const loader = provider.makeTestLoader({
...testContainerConfig,
loaderProps: { detachedBlobStorage },
loaderProps: {
detachedBlobStorage,
configProvider: createTestConfigProvider({
"Fluid.Container.MemoryBlobStorageEnabled": true,
}),
},
});
const serializeContainer = await loader.createDetachedContainer(
provider.defaultCodeDetails,
Expand Down Expand Up @@ -722,7 +733,12 @@ function serializationTests({
async function () {
const loader = provider.makeTestLoader({
...testContainerConfig,
loaderProps: { detachedBlobStorage },
loaderProps: {
detachedBlobStorage,
configProvider: createTestConfigProvider({
"Fluid.Container.MemoryBlobStorageEnabled": true,
}),
},
});
let container = await loader.createDetachedContainer(provider.defaultCodeDetails);

Expand Down
6 changes: 4 additions & 2 deletions packages/test/test-utils/src/TestConfigs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,10 @@ export interface ITestConfigProvider extends IConfigProviderBase {
* Creates a test config provider with the ability to set configs values and clear all config values.
* @internal
*/
export const createTestConfigProvider = (): ITestConfigProvider => {
const settings: Record<string, ConfigTypes> = {};
export const createTestConfigProvider = (
defaults: Record<string, ConfigTypes> = {},
): ITestConfigProvider => {
const settings: Record<string, ConfigTypes> = { ...defaults };
return {
getRawConfig: (name: string): ConfigTypes => settings[name],
set: (key: string, value: ConfigTypes) => {
Expand Down

0 comments on commit 06c2c5d

Please sign in to comment.