Skip to content

Commit 7c2ac9b

Browse files
markfieldsmarkfields
authored
GC: Use compatibilityModeRuntimeOptions for disabling GC for Azure Client 2.0 when needed (#21537)
Rather than using config overrides (aka feature gates) to disable GC at first for 2.0, we use ContainerRuntime options, specified in the `compatibilityModeRuntimeOptions` record according to applicable compatibility constraints. One change in behavior: If Sweep is not enabled, we won't run Tombstone Autorecovery path. This gives us one single knob for disabling code that emits the GC op.
1 parent 93c7b46 commit 7c2ac9b

File tree

14 files changed

+85
-165
lines changed

14 files changed

+85
-165
lines changed

‎packages/framework/fluid-static/src/compatibilityConfiguration.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,10 @@ export const compatibilityModeRuntimeOptions: Record<
3232
// Grouped batching is on by default but introduces a new type of op which is not compatible with 1.x clients.
3333
enableGroupedBatching: false,
3434
// TODO: Include explicit disables for things that are currently off-by-default?
35+
36+
// Explicitly disable running Sweep and enforcing Tombstone
37+
// This ensures we don't send the new GC op which is not compatible with 1.x clients
38+
gcOptions: { enableGCSweep: undefined, gcDisableThrowOnTombstoneLoad: true },
3539
},
3640
"2": {
3741
// Explicit schema control explicitly makes the container incompatible with 1.x clients, to force their
@@ -40,5 +44,7 @@ export const compatibilityModeRuntimeOptions: Record<
4044
// The runtime ID compressor is a prerequisite to use SharedTree but is off by default and must be explicitly enabled.
4145
// It introduces a new type of op which is not compatible with 1.x clients.
4246
enableRuntimeIdCompressor: "on",
47+
// GC is not yet enabled by default, although it could be enabled here from a compat standpoint
48+
gcOptions: { enableGCSweep: undefined, gcDisableThrowOnTombstoneLoad: true },
4349
},
4450
};

‎packages/runtime/container-runtime/src/gc/garbageCollection.ts

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -910,7 +910,10 @@ export class GarbageCollector implements IGarbageCollector {
910910
break;
911911
}
912912
case GarbageCollectionMessageType.TombstoneLoaded: {
913-
if (this.mc.config.getBoolean(disableAutoRecoveryKey) === true) {
913+
if (
914+
!this.configs.tombstoneAutorecoveryEnabled ||
915+
this.mc.config.getBoolean(disableAutoRecoveryKey) === true
916+
) {
914917
break;
915918
}
916919

@@ -1083,7 +1086,10 @@ export class GarbageCollector implements IGarbageCollector {
10831086
* before runnint GC next.
10841087
*/
10851088
private triggerAutoRecovery(nodePath: string) {
1086-
if (this.mc.config.getBoolean(disableAutoRecoveryKey) === true) {
1089+
if (
1090+
!this.configs.tombstoneAutorecoveryEnabled ||
1091+
this.mc.config.getBoolean(disableAutoRecoveryKey) === true
1092+
) {
10871093
return;
10881094
}
10891095

‎packages/runtime/container-runtime/src/gc/gcConfigs.ts

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@ import {
2929
maxSnapshotCacheExpiryMs,
3030
oneDayMs,
3131
runSessionExpiryKey,
32-
runSweepKey,
3332
throwOnTombstoneLoadOverrideKey,
3433
throwOnTombstoneUsageKey,
3534
} from "./gcDefinitions.js";
@@ -121,8 +120,7 @@ export function generateGCConfigs(
121120
const sweepEnabled: boolean =
122121
!gcEnabled || tombstoneTimeoutMs === undefined
123122
? false
124-
: mc.config.getBoolean(runSweepKey) ??
125-
(sweepAllowed && createParams.gcOptions.enableGCSweep === true);
123+
: sweepAllowed && createParams.gcOptions.enableGCSweep === true;
126124
const disableDatastoreSweep =
127125
mc.config.getBoolean(disableDatastoreSweepKey) === true ||
128126
createParams.gcOptions[gcDisableDataStoreSweepOptionName] === true;
@@ -132,6 +130,10 @@ export function generateGCConfigs(
132130
: "YES"
133131
: "NO";
134132

133+
// If we aren't running sweep, also disable AutoRecovery which also emits the GC op.
134+
// This gives a simple control surface for compability concerns around introducing the new op.
135+
const tombstoneAutorecoveryEnabled = shouldRunSweep !== "NO";
136+
135137
// Override inactive timeout if test config or gc options to override it is set.
136138
const inactiveTimeoutMs =
137139
mc.config.getNumber("Fluid.GarbageCollection.TestOverride.InactiveTimeoutMs") ??
@@ -172,6 +174,7 @@ export function generateGCConfigs(
172174
return {
173175
gcEnabled, // For this document
174176
sweepEnabled: sweepAllowed, // For this document (based on current GC Generation option)
177+
tombstoneAutorecoveryEnabled,
175178
shouldRunSweep, // For this session
176179
runFullGC,
177180
testMode,

‎packages/runtime/container-runtime/src/gc/gcDefinitions.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -60,8 +60,6 @@ export const gcDisableDataStoreSweepOptionName = "disableDataStoreSweep";
6060
*/
6161
export const gcGenerationOptionName = "gcGeneration";
6262

63-
/** Config key to turn GC sweep on / off. */
64-
export const runSweepKey = "Fluid.GarbageCollection.RunSweep";
6563
/** Config key to turn GC test mode on / off. */
6664
export const gcTestModeKey = "Fluid.GarbageCollection.GCTestMode";
6765
/** Config key to expire a session after a set period of time. Defaults to true. */
@@ -476,6 +474,8 @@ export interface IGarbageCollectorConfigs {
476474
* throughout its lifetime.
477475
*/
478476
readonly sweepEnabled: boolean;
477+
/** Is Tombstone AutoRecovery enabled? Useful for preventing the GC "TombstoneLoaded" op, for compatibility reasons */
478+
readonly tombstoneAutorecoveryEnabled: boolean;
479479
/**
480480
* Tracks if sweep phase should run or not, or if it should run only for attachment blobs.
481481
* Even if the sweep phase is allowed for a document (see sweepEnabled), it may be disabled or partially enabled

‎packages/runtime/container-runtime/src/gc/gcTelemetry.ts

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@ import {
2222
IGarbageCollectorConfigs,
2323
UnreferencedState,
2424
disableTombstoneKey,
25-
runSweepKey,
2625
throwOnTombstoneLoadOverrideKey,
2726
throwOnTombstoneUsageKey,
2827
} from "./gcDefinitions.js";
@@ -289,9 +288,6 @@ export class GCTelemetryTracker {
289288
ThrowOnTombstoneUsage: this.mc.config.getBoolean(throwOnTombstoneUsageKey),
290289
ThrowOnTombstoneLoad: this.mc.config.getBoolean(throwOnTombstoneLoadOverrideKey),
291290
},
292-
sweepFlags: {
293-
EnableSweepFlag: this.mc.config.getBoolean(runSweepKey),
294-
},
295291
};
296292

297293
if (
@@ -431,9 +427,6 @@ export function sendGCUnexpectedUsageEvent(
431427
ThrowOnTombstoneUsage: mc.config.getBoolean(throwOnTombstoneUsageKey),
432428
ThrowOnTombstoneLoad: mc.config.getBoolean(throwOnTombstoneLoadOverrideKey),
433429
});
434-
event.sweepFlags = JSON.stringify({
435-
EnableSweepFlag: mc.config.getBoolean(runSweepKey),
436-
});
437430
event.gcVersion = getGCVersionInEffect(mc.config);
438431

439432
mc.logger.sendTelemetryEvent(event, error);

‎packages/runtime/container-runtime/src/gc/index.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@ export {
3131
IGCStats,
3232
oneDayMs,
3333
runSessionExpiryKey,
34-
runSweepKey,
3534
stableGCVersion,
3635
disableAutoRecoveryKey,
3736
disableDatastoreSweepKey,

‎packages/runtime/container-runtime/src/test/gc/garbageCollection.spec.ts

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,7 @@ type GcWithPrivates = IGarbageCollector & {
8989
readonly deletedNodes: Set<string>;
9090
readonly unreferencedNodesState: Map<string, UnreferencedStateTracker>;
9191
readonly submitMessage: (message: ContainerRuntimeGCMessage) => void;
92+
readonly triggerAutoRecovery: (nodePath: string) => void;
9293
runGC: (fullGC: boolean) => Promise<IGCStats>;
9394
};
9495

@@ -442,6 +443,7 @@ describe("Garbage Collection Tests", () => {
442443

443444
// getGCData set up to sometimes return the corrupted data
444445
gc = createGarbageCollector({
446+
createParams: { gcOptions: { enableGCSweep: true } }, // Required to run AutoRecovery
445447
getGCData: async (fullGC?: boolean) => {
446448
return fullGC ? defaultGCData : corruptedGCData;
447449
},
@@ -546,6 +548,23 @@ describe("Garbage Collection Tests", () => {
546548
"node 0 should not be unreferenced after repairing GC Data",
547549
);
548550
});
551+
552+
it("Autorecovery disabled if enableGCSweep not set", async () => {
553+
gc = createGarbageCollector({
554+
createParams: { gcOptions: { enableGCSweep: undefined } },
555+
});
556+
const spies = {
557+
gc: {
558+
submitMessage: spy(gc, "submitMessage"),
559+
},
560+
};
561+
562+
gc.triggerAutoRecovery(""); // nodePath is irrelevant
563+
assert(
564+
spies.gc.submitMessage.notCalled,
565+
"triggerAutoRecovery should no-op if gcOp is not supported in schema",
566+
);
567+
});
549568
});
550569

551570
describe("errors when unreferenced objects are used after they are inactive / deleted", () => {

‎packages/runtime/container-runtime/src/test/gc/gcConfigs.spec.ts

Lines changed: 10 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,6 @@ import {
4343
nextGCVersion,
4444
oneDayMs,
4545
runSessionExpiryKey,
46-
runSweepKey,
4746
stableGCVersion,
4847
throwOnTombstoneLoadOverrideKey,
4948
} from "../../gc/index.js";
@@ -694,15 +693,13 @@ describe("Garbage Collection configurations", () => {
694693
sweepEnabled_doc: boolean;
695694
sweepEnabled_session: boolean;
696695
disableDataStoreSweep?: "viaGCOption" | "viaConfigProvider";
697-
runSweep_config?: boolean;
698696
expectedShouldRunSweep: IGarbageCollectorConfigs["shouldRunSweep"];
699697
}[] = [
700698
{
701699
gcEnabled_doc: false, // Veto
702700
sweepEnabled_doc: true,
703701
sweepEnabled_session: true,
704702
disableDataStoreSweep: "viaGCOption",
705-
runSweep_config: true,
706703
expectedShouldRunSweep: "NO",
707704
},
708705
{
@@ -719,21 +716,6 @@ describe("Garbage Collection configurations", () => {
719716
disableDataStoreSweep: "viaGCOption",
720717
expectedShouldRunSweep: "NO",
721718
},
722-
{
723-
gcEnabled_doc: true,
724-
sweepEnabled_doc: true,
725-
sweepEnabled_session: true,
726-
runSweep_config: false, // Veto
727-
disableDataStoreSweep: "viaGCOption",
728-
expectedShouldRunSweep: "NO",
729-
},
730-
{
731-
gcEnabled_doc: true,
732-
sweepEnabled_doc: true,
733-
sweepEnabled_session: false, // Overriden by runSweep_config
734-
runSweep_config: true,
735-
expectedShouldRunSweep: "YES",
736-
},
737719
{
738720
gcEnabled_doc: true,
739721
sweepEnabled_doc: true,
@@ -758,14 +740,12 @@ describe("Garbage Collection configurations", () => {
758740
gcEnabled_doc: true,
759741
sweepEnabled_doc: true,
760742
sweepEnabled_session: true,
761-
runSweep_config: true,
762-
disableDataStoreSweep: "viaGCOption", // Applies after runSweep_config
743+
disableDataStoreSweep: "viaGCOption",
763744
expectedShouldRunSweep: "ONLY_BLOBS",
764745
},
765746
];
766747
testCases.forEach((testCase, index) => {
767748
it(`Test Case ${JSON.stringify(testCase)}`, () => {
768-
configProvider.set(runSweepKey, testCase.runSweep_config);
769749
configProvider.set(
770750
disableDatastoreSweepKey,
771751
testCase.disableDataStoreSweep === "viaConfigProvider",
@@ -914,14 +894,22 @@ describe("Garbage Collection configurations", () => {
914894
beforeEach(() => {
915895
configProvider.set(testOverrideSessionExpiryMsKey, defaultSessionExpiryDurationMs); // Required for sweep to be enabled
916896
});
917-
it("gcDisableThrowOnTombstoneLoad true", () => {
897+
it("gcDisableThrowOnTombstoneLoad true (w/ Sweep)", () => {
918898
gc = createGcWithPrivateMembers(
919899
undefined /* metadata */,
920900
{ enableGCSweep: true, [gcDisableThrowOnTombstoneLoadOptionName]: true },
921901
false /* isSummarizerClient */,
922902
);
923903
assert.equal(gc.configs.throwOnTombstoneLoad, false, "throwOnTombstoneLoad incorrect");
924904
});
905+
it("gcDisableThrowOnTombstoneLoad true (w/o Sweep)", () => {
906+
gc = createGcWithPrivateMembers(
907+
undefined /* metadata */,
908+
{ enableGCSweep: undefined, [gcDisableThrowOnTombstoneLoadOptionName]: true },
909+
false /* isSummarizerClient */,
910+
);
911+
assert.equal(gc.configs.throwOnTombstoneLoad, false, "throwOnTombstoneLoad incorrect");
912+
});
925913
it("gcDisableThrowOnTombstoneLoad false", () => {
926914
gc = createGcWithPrivateMembers(
927915
undefined /* metadata */,
@@ -941,15 +929,6 @@ describe("Garbage Collection configurations", () => {
941929
);
942930
assert.equal(gc.configs.throwOnTombstoneLoad, true, "throwOnTombstoneLoad incorrect");
943931
});
944-
it("gcDisableThrowOnTombstoneLoad undefined - Sweep Disabled does not interfere", () => {
945-
configProvider.set(runSweepKey, false); // Disable Sweep
946-
gc = createGcWithPrivateMembers(
947-
undefined /* metadata */,
948-
{ [gcDisableThrowOnTombstoneLoadOptionName]: undefined },
949-
false /* isSummarizerClient */,
950-
);
951-
assert.equal(gc.configs.throwOnTombstoneLoad, true, "throwOnTombstoneLoad incorrect");
952-
});
953932
it("Old 'enable' option false (ignored)", () => {
954933
const gcThrowOnTombstoneLoadOptionName_old = "gcThrowOnTombstoneLoad";
955934
gc = createGcWithPrivateMembers(

‎packages/runtime/container-runtime/src/test/gc/gcTelemetry.spec.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,7 @@ describe("GC Telemetry Tracker", () => {
7575
gcEnabled: true,
7676
sweepEnabled: false,
7777
shouldRunSweep: "NO",
78+
tombstoneAutorecoveryEnabled: false,
7879
runFullGC: false,
7980
testMode: false,
8081
tombstoneMode: false,

‎packages/service-clients/azure-client/src/AzureClient.ts

Lines changed: 1 addition & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -64,27 +64,13 @@ const azureClientFeatureGates = {
6464
"Fluid.Container.ForceWriteConnection": true,
6565
};
6666

67-
/**
68-
* Feature gates required to support runtime compatibility when V1 and V2 clients are collaborating
69-
*/
70-
const azureClientV1CompatFeatureGates = {
71-
// Disable Garbage Collection
72-
"Fluid.GarbageCollection.RunSweep": false, // To prevent the GC op
73-
"Fluid.GarbageCollection.DisableAutoRecovery": true, // To prevent the GC op
74-
"Fluid.GarbageCollection.ThrowOnTombstoneLoadOverride": false, // For a consistent story of "GC is disabled"
75-
};
76-
7767
/**
7868
* Wrap the config provider to fall back on the appropriate defaults for Azure Client.
7969
* @param baseConfigProvider - The base config provider to wrap
8070
* @returns A new config provider with the appropriate defaults applied underneath the given provider
8171
*/
8272
function wrapConfigProvider(baseConfigProvider?: IConfigProviderBase): IConfigProviderBase {
83-
const defaults = {
84-
...azureClientFeatureGates,
85-
...azureClientV1CompatFeatureGates,
86-
};
87-
return wrapConfigProviderWithDefaults(baseConfigProvider, defaults);
73+
return wrapConfigProviderWithDefaults(baseConfigProvider, azureClientFeatureGates);
8874
}
8975

9076
/**

‎packages/service-clients/azure-client/src/test/AzureClient.spec.ts

Lines changed: 16 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@ import type { ConnectionMode } from "@fluidframework/driver-definitions";
1212
import { ScopeType } from "@fluidframework/driver-definitions/internal";
1313
import type { ContainerSchema, IFluidContainer } from "@fluidframework/fluid-static";
1414
import { SharedMap } from "@fluidframework/map/internal";
15-
import type { MonitoringContext } from "@fluidframework/telemetry-utils/internal";
1615
import { InsecureTokenProvider } from "@fluidframework/test-runtime-utils/internal";
1716
import { timeoutPromise } from "@fluidframework/test-utils/internal";
1817
import { SchemaFactory, TreeViewConfiguration } from "@fluidframework/tree";
@@ -287,34 +286,26 @@ for (const compatibilityMode of ["1", "2"] as const) {
287286
);
288287
});
289288

290-
it("GC is disabled by default, but can be enabled", async function () {
289+
it("GC is disabled for both compat modes", async function () {
291290
const { container: container_defaultConfig } = await client.createContainer(
292291
schema,
293292
compatibilityMode,
294293
);
295-
assert.strictEqual(
296-
(
297-
container_defaultConfig as unknown as { container: { mc: MonitoringContext } }
298-
).container.mc.config.getBoolean("Fluid.GarbageCollection.RunSweep"),
299-
false,
300-
"Expected GC to be disabled per configs set in constructor",
301-
);
302-
303-
const client_gcEnabled = createAzureClient({
304-
configProvider: {
305-
getRawConfig: (name: string) => ({ "Fluid.GarbageCollection.RunSweep": true })[name],
306-
},
307-
});
308-
const { container: container_gcEnabled } = await client_gcEnabled.createContainer(
309-
schema,
310-
compatibilityMode,
311-
);
312-
assert.strictEqual(
313-
(
314-
container_gcEnabled as unknown as { container: { mc: MonitoringContext } }
315-
).container.mc.config.getBoolean("Fluid.GarbageCollection.RunSweep"),
316-
true,
317-
"Expected GC to be able to enable GC via config provider",
294+
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
295+
const { shouldRunSweep, tombstoneAutorecoveryEnabled, throwOnTombstoneLoad } =
296+
// eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-member-access
297+
(container_defaultConfig as any).container._runtime.garbageCollector.configs;
298+
299+
const expectedConfigs = {
300+
shouldRunSweep: "NO",
301+
tombstoneAutorecoveryEnabled: false,
302+
throwOnTombstoneLoad: false,
303+
};
304+
assert.deepStrictEqual(
305+
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
306+
{ shouldRunSweep, tombstoneAutorecoveryEnabled, throwOnTombstoneLoad },
307+
expectedConfigs,
308+
"Expected GC to be disabled per compatibilityModeRuntimeOptions",
318309
);
319310
});
320311

0 commit comments

Comments
 (0)