GC: Persist SweepTimeout to summary and support using a shorter duration for testing by markfields · Pull Request #12331 · microsoft/FluidFramework

markfields · 2022-10-07T14:10:51Z

Important update

This PR seems to have a fatal problem: The initial summary (created while detached) cannot include the driver's snapshot cache policy. So we have to persist some placeholder value and depend on a later summary to fill it in. But this means that there is no authoritative value, as different sessions could have different policies in effect.

Discussing this with Navin, we'll see what the outcome is. One possibility is to close this in favor of #12419

Description

This does a few key things:

Plumbs the Snapshot Cache Expiry value in from the driver
Persists that value, along with the value used for the buffer, so all 3 Sweep Timeout constituents are persisted
Ensures Snapshot Cache policy is consistent with other config state

It does not change the logic around overriding Session Expiry, and uses a very simple scheme for overriding buffer, tying it to whether SnapshotCache is 0 or not.

We can iterate on strategy for setting these values for new containers over time - the important part is to get the values persisted and always use those values for existing containers.

See previous PR #12044

Reviewer Guidance

Still need to update unit tests
Still need to write some e2e tests to try out shrinking the timeout (now we can test sweep in e2e tests!)
We may consider porting part of this back to internal.1.4 so that release uses the persisted values if present. I think this is optional though. Or maybe we port the whole change. 🤷‍♂️

Does this introduce a breaking change?

If this introduces a breaking change, please describe the impact and migration path for existing applications below.
See Breaking-vs-Non-breaking-Changes for details.
If there are no breaking changes, delete this section.

Any relevant logs or outputs

Use this section to provide either screenshots or output of logs as code snippets

Other information or known dependencies

Any other information or known dependencies that is important to this PR.

Links to internal bug/work tracker items.

TODO that are to be done after this PR.

markfields · 2022-10-07T14:12:45Z

        const pendingRuntimeState = context.pendingLocalState as IPendingRuntimeState | undefined;
        const baseSnapshot: ISnapshotTree | undefined = pendingRuntimeState?.baseSnapshot ?? context.baseSnapshot;

+        //* Test to prove this doesn't get persisted as undefined?


This is a todo, might or might not do it, it would be an e2e test. When this is undefined due to storage not existing yet, it shouldn't be persisted because only Summarizer persists these values.

tyler-cai-microsoft · 2022-10-10T22:54:14Z

+            // If we're using TestOverrides to achieve a short Sweep Timeout, lock the buffer to 1ms. Otherwise 1 day.
+            this.sweepTimeoutBufferMs =
+                (this.maxSnapshotCacheDurationMs === 0)
+                    ? 1


Why is this 1?

Just felt like it should be non-zero 🤷‍♂️

tyler-cai-microsoft · 2022-10-10T23:01:05Z

+    /** If this is present, the session for this container will expire after this time and the container will close */
+    readonly sessionExpiryTimeoutMs?: number;
+    /** Maximum duration a snapshot may be cached and then loaded from for this container */
+    readonly maxSnapshotCacheDurationMs?: number | "none";


Why are there two different types?

There are 3 actually:

undefined - old files created when the value was hardcoded to 5 days (or 2 days for even older files)

none - files created at a hypothetical future time where the driver didn't implement the policy so GC shouldn't run

number - files created by this code where the driver does implement the policy and passed in the policy's value

markfields · 2022-10-12T13:56:49Z

 * Validates this scenario: When a GC node (data store or attachment blob) becomes inactive, i.e, it has been
 * unreferenced for a certain amount of time, using the node results in an error telemetry.
 */
-describeNoCompat("GC inactive nodes tests", (getTestObjectProvider) => {


This file is WIP - I will not be replacing the existing tests, but rather copying them first or updating them to run in 2 modes.

markfields · 2022-10-14T23:27:41Z

Replaced by #12419

## Background As-is, the Sweep Timeout will always be at least 6 days due to hardcoded Snapshot Cache limit (5 days) and Buffer (1 day). This means it's impossible to test the Sweep Timer and the code that runs after something is SweepReady! There have been multiple attempts on my part to address this: #11084, #12044, #12331 Much of the difficulty has arisen from the challenge of aligning driver policy with GC config. I'm finally punting on that and tracking it as a separate item ([AB#2238](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/2238)). ## This change It's pretty simple: * Persist SweepTimeoutMs in the summary * For new containers, that can be overridden via the `"Fluid.GarbageCollection.TestOverride.SweepTimeoutMs"` config setting * Default behavior for new containers (and for backfilling existing containers with no value persisted) is SweepTimeout + 6 days (same as it has been)

## Background As-is, the Sweep Timeout will always be at least 6 days due to hardcoded Snapshot Cache limit (5 days) and Buffer (1 day). This means it's impossible to test the Sweep Timer and the code that runs after something is SweepReady! There have been multiple attempts on my part to address this: microsoft#11084, microsoft#12044, microsoft#12331 Much of the difficulty has arisen from the challenge of aligning driver policy with GC config. I'm finally punting on that and tracking it as a separate item ([AB#2238](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/2238)). ## This change It's pretty simple: * Persist SweepTimeoutMs in the summary * For new containers, that can be overridden via the `"Fluid.GarbageCollection.TestOverride.SweepTimeoutMs"` config setting * Default behavior for new containers (and for backfilling existing containers with no value persisted) is SweepTimeout + 6 days (same as it has been)

_back-port of #12419 to internal.2.0 release_ ## Background As-is, the Sweep Timeout will always be at least 6 days due to hardcoded Snapshot Cache limit (5 days) and Buffer (1 day). This means it's impossible to test the Sweep Timer and the code that runs after something is SweepReady! There have been multiple attempts on my part to address this: #11084, #12044, #12331 Much of the difficulty has arisen from the challenge of aligning driver policy with GC config. I'm finally punting on that and tracking it as a separate item ([AB#2238](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/2238)). ## This change It's pretty simple: * Persist SweepTimeoutMs in the summary * For new containers, that can be overridden via the `"Fluid.GarbageCollection.TestOverride.SweepTimeoutMs"` config setting * Default behavior for new containers (and for backfilling existing containers with no value persisted) is SweepTimeout + 6 days (same as it has been)

markfields added 2 commits October 7, 2022 03:16

Plumb maximumCacheDurationMs policy from driver to GC

7423f75

Persist all parts of Sweep Timeout

ef0ebf3

markfields requested review from a team as code owners October 7, 2022 14:10

github-actions Bot added area: loader Loader related issues area: runtime Runtime related issues area: tests Tests to add, test infrastructure improvements, etc labels Oct 7, 2022

markfields requested review from agarwal-navin and tyler-cai-microsoft October 7, 2022 14:11

github-actions Bot added the base: main PRs targeted against main branch label Oct 7, 2022

markfields commented Oct 7, 2022

View reviewed changes

Comment thread packages/runtime/container-runtime/src/garbageCollection.ts Outdated

markfields mentioned this pull request Oct 7, 2022

Add TestOverride setting for SweepTimeout #12044

Closed

markfields added 2 commits October 10, 2022 14:06

Add qt option

5dc9c57

Fix bugs and unit tests

4b35acf

markfields requested a review from msfluid-bot as a code owner October 10, 2022 18:01

tyler-cai-microsoft reviewed Oct 10, 2022

View reviewed changes

Hit the dead end, thanks to e2e tests

3b85a48

markfields requested review from a team as code owners October 12, 2022 13:50

github-actions Bot added the area: driver Driver related issues label Oct 12, 2022

markfields marked this pull request as draft October 12, 2022 13:51

markfields commented Oct 12, 2022

View reviewed changes

markfields closed this Oct 14, 2022

markfields mentioned this pull request Oct 14, 2022

Add TestOverride.SweepTimeout #12419

Merged

markfields mentioned this pull request Oct 17, 2022

[v2int/2.0] Persist sweepTimeoutMs to summary #12517

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GC: Persist SweepTimeout to summary and support using a shorter duration for testing#12331

GC: Persist SweepTimeout to summary and support using a shorter duration for testing#12331
markfields wants to merge 5 commits into
microsoft:mainfrom
markfields:gc-sweep-timeout-testoverrides

markfields commented Oct 7, 2022 •

edited

Loading

Uh oh!

markfields Oct 7, 2022

Uh oh!

Uh oh!

tyler-cai-microsoft Oct 10, 2022

Uh oh!

markfields Oct 11, 2022

Uh oh!

tyler-cai-microsoft Oct 10, 2022

Uh oh!

markfields Oct 11, 2022 •

edited

Loading

Uh oh!

markfields Oct 12, 2022

Uh oh!

markfields commented Oct 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

markfields commented Oct 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Important update

Description

Reviewer Guidance

Does this introduce a breaking change?

Any relevant logs or outputs

Other information or known dependencies

Uh oh!

markfields Oct 7, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tyler-cai-microsoft Oct 10, 2022

Choose a reason for hiding this comment

Uh oh!

markfields Oct 11, 2022

Choose a reason for hiding this comment

Uh oh!

tyler-cai-microsoft Oct 10, 2022

Choose a reason for hiding this comment

Uh oh!

markfields Oct 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markfields Oct 12, 2022

Choose a reason for hiding this comment

Uh oh!

markfields commented Oct 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

markfields commented Oct 7, 2022 •

edited

Loading

markfields Oct 11, 2022 •

edited

Loading