Snapshot name collisions #4485

arinwt · 2020-11-30T11:21:56Z

First step towards fixing #3542

Adds a subtree at each layer of the stack (except DDS for now) to isolate dynamic tree keys, such as data store IDs and channel IDs.

This shifts all data store trees under a ".channels" tree of the root node.
- Protects against ".protocol", ".blobs" name collisions, and some driver/server ones as well
This shifts all channel trees under a ".channels" tree of each data store node.
- Does not add protection, because blobs are already isolated from trees
This does not shift all dds subtrees/blobs to a subtree of each channel node.
- Current design does not require this, since we only return ITreeEntrys from internal snapshot
This does not take care of isolating server/driver from runtime at the root

Old tree structure:

/
    [blob] .chunks
    [tree] .protocol/
    [tree] .blobs/
    [tree] <dataStoreIds>/
        [blob] .component
        [tree] <channelIds>/
            [blob] .attributes
            [tree] .../

New tree structure:

/
    [blob] .metadata **provides snapshot version
    [blob] .chunks
    [tree] .protocol/
    [tree] .blobs/
    [tree] .channels/
        [tree] <dataStoreIds>/
            [blob] .component
            [tree] .channels/
                [tree] <channelIds>/
                    [blob] .attributes
                    [tree] .../

With this change, ContainerRuntime only gives upper .channels tree to DataStores, restricting access.
Each DataStoreContext only gives the lower .channels tree to its DataStoreRuntime.

Changes in this PR are back/forwards compatible reading of new format only, it does not write the new snapshotFormatVersion. So after a few versions with this released, we can start writing in the new snapshotFormatVersion. This is because an old client runtime will not be able to understand the new format.

SummarizerNode handles this by calling parseSummaryForSubtrees which gives a tree to use for children nodes as well as the additional path part for the child nodes.

arinwt · 2020-11-30T11:24:49Z

packages/runtime/container-runtime-definitions/src/snapshot.ts

+    trees: {
+        [protocolTreeName]: ISnapshotTree;
+        [blobsTreeName]: ISnapshotTree;
+        ".dataStores": ISnapshotTree;


Open to suggestions for the names.

".channels" - refers to only data store trees for now, but we could store blobs, etc. that the DataStores class can access.

".channels" - refers to only the channel trees for now, but we could store blobs, etc. that the DataStoreRuntime can access. Was considering naming this something else like ".runtime" in case components wanted to store additional info here they could.

packages/runtime/container-runtime/src/dataStores.ts

arinwt · 2020-11-30T11:31:46Z

packages/runtime/container-runtime-definitions/src/snapshot.ts

+
+export const dataStoreAttributesBlobName = ".component";
+
+export interface IRuntimeSnapshot {


I was thinking to more strongly type the other layers as well (and actually wrote out the types as seen in the first commit), but it requires more work across the boundaries with not much gain right now. Will look more into it after fixing SummarizerNode.

I think id (at that level) should be always null., no?
Based on latest discussion with SPO, I think we should start asserting in our layers that we provide either value (content but id === null) or reference (id !== null, but no other fields are provided) for trees, similar how we do for blobs - we either reuse existing one, or write out new one.

My understanding was that storage always returns ID + all tree contents (i.e. this is the full skeleton). I think it could make sense for them to sometimes return partial trees, but I don't know exactly how that would be specified right now.

If there's a better way to type it that you know of, I can change it, but I know just from debugging that currently it will be id !== null and other fields are provided so that type would be misleading or at least limiting (maybe intentionally?).

Sorry, I though this interface is for writing snapshots (summaries), not for reading.
Runtime never reads "snapshots", it operates in individual (shallow) trees that always have content and ID.

BTW, we should be try to eliminate "snapshot" word from our code base. It is used as in "snapshot tree but I think naming is from the past. I think they are always shallow trees (i.e. only contain one level of data), right?

msfluid-bot · 2020-11-30T11:39:15Z

■ @fluidframework/base-host: No change

Metric Name	Baseline Size	Compare Size	Size Diff
main.js	164.76 KB	164.76 KB	■ No change
Total Size	164.76 KB	164.76 KB	■ No change

⯅ @fluid-example/bundle-size-tests: +894 Bytes

Metric Name	Baseline Size	Compare Size	Size Diff
container.js	190.29 KB	191.17 KB	⯅ +894 Bytes
map.js	45.84 KB	45.84 KB	■ No change
matrix.js	144.52 KB	144.52 KB	■ No change
odspDriver.js	193.22 KB	193.22 KB	■ No change
sharedString.js	158.46 KB	158.46 KB	■ No change
Total Size	732.33 KB	733.2 KB	⯅ +894 Bytes

Baseline commit: fa35947

Generated by 🚫 dangerJS against 06323cf

vladsud · 2020-12-01T06:07:14Z

This does not shift all dds subtrees/blobs to a subtree of each channel node.
Current design does not require this, since we only return ITreeEntrys from internal snapshot

I already violate it with mixinSummaryHandler() (and using it in Bohemia repo to write out /DataStoreId/_search/01 blobs

vladsud · 2020-12-01T06:10:03Z

[tree] .dataStores/

Can we please call it channels?
That's already what I use for routing (active PR, will try to merge this week).
And that's consistent with future we want to get to, where channels can be nested, and data store and DDS differ only in implementation, not in structure (i.e. it will be Ok for DDS to have DDS underneath it - formalizing what Daniel already did with 3 DDSs sitting on one channel)

vladsud · 2020-12-04T14:48:09Z

packages/runtime/container-runtime-definitions/src/snapshot.ts

+export type PropertyValues<T> = T[keyof T];
+
+export const containerSnapshotFormatVersions = {
+    missing: undefined,


can you please add some comments on what these fields mean?
Also should be defined interface properly (with all of the comments that go to documentation) then derive it from this value?

Added comments.

Not sure what exactly you mean by interface.. If I add an interface, I need to list the versions/types twice I think?
I have changed to more simple union type.

The alternative I could think (without listing all versions twice) was the same as it was before but change current to v1 and next to v2 etc. so that we don't have to update all references every time.

vladsud · 2020-12-04T14:54:02Z

packages/runtime/container-runtime/src/containerRuntime.ts

+            const blobId = context.baseSnapshot?.blobs[blobName];
+            if (context.baseSnapshot && blobId) {
+                return context.storage ?
+                    readAndParse<T>(context.storage, blobId) :


Curious, should it be reverse? I.e. if we have it in snapshot, why do we read it from storage?

Would be an unrelated change I think, but it makes sense to me. I don't know as much about what it means when the blobs are stored directly in the snapshot though, maybe we prioritize the ones in storage for some reason? i.e. out of date or something?

@jatgarg would know I think.

vladsud · 2020-12-04T14:58:05Z

packages/runtime/container-runtime/src/containerRuntime.ts

+        let dataStoresSnapshot = context.baseSnapshot;
+        let dataStoresSnapshotType: BaseSnapshotType = "legacy";
+
+        if (!!dataStoresSnapshot && metadata.snapshotFormatVersion !== containerSnapshotFormatVersions.missing) {


I'm a bit at a loss in terms of using "missing" & "next". What if we need to change format again?
Would approach of just saying that existing format is 1.0 (and that's default value if attributes are missing) and new format is 2.0 work? Then we can clearly articulate what is 1.0 format, what is 2.0 format, write documentation, and someone willing to write code on the side to parse our files can actually do it.
Maybe that's what I'm missing - formal description of format, i.e. what is the instruction to me as a side developer of how to read and write these formats, without looking in the code to understand formats and expectations.

I see - you essentially have 3 versions. I'd rather call them 1.0, 2.0 and 3.0 and make 1.0 default - a bit cleaner IMHO

This makes sense to me, I was trying to not diverge as far from the pre-existing "current" logic, but I agree it probably makes more sense not to use sliding version names when reading.

vladsud · 2020-12-04T15:11:44Z

packages/runtime/container-runtime-definitions/src/snapshot.ts

+
+export const containerSnapshotFormatVersions = {
+    missing: undefined,
+    next: "0.1",


I do not see containerSnapshotFormatVersions.next used anywhere in he code.

It isn't used, but we do check for undefined and treat that else scenario specially.

vladsud · 2020-12-04T15:14:03Z

packages/runtime/container-runtime/src/dataStores.ts

                // However the feature of loading a detached container from snapshot, is added when the
                // snapshotFormatVersion is "0.1", so we don't expect it to be anything else.
-                if (snapshotFormatVersion === currentSnapshotFormatVersion) {
+                if (snapshotFormatVersion === dataStoreSnapshotFormatVersions.current


I'm a bit confused here. Existing files will take the else branch, no?

That's intentional based on the comment:

// However the feature of loading a detached container from snapshot, is added when the
// snapshotFormatVersion is "0.1", so we don't expect it to be anything else.

vladsud · 2020-12-08T15:37:56Z

packages/runtime/container-runtime/src/dataStores.ts

- } from "./dataStoreContext";
+} from "./dataStoreContext";
+
+export type BaseSnapshotType = "legacy" | "next";


maybe put some comment in here describing what legacy and next mean in practice?
Should be given them more descriptive names? While this is only runtime data, I image that we might have "next next" format some day :)

vladsud · 2020-12-08T15:48:26Z

packages/runtime/runtime-utils/src/summarizerNode/summarizerNodeUtils.ts

+    /** Tree to use to find children subtrees */
+    childrenTree: ISnapshotTree,
+    /** Additional path part where children are isolated */
+    childrenPathPart: string | undefined,


I probably did not stair too long at the code to better understand it, but it is not clear to me from glancing what the actual meaning of childrenPathPart and how callers of API below use it. Maybe it's just me (and not having enough focus), but would it be helpful to have a bit more in comment on when it's undefined, and when it's not, what it means and how this data is expected to be used?

anthony-murphy · 2020-12-09T01:24:42Z

this needs tests, specifically for forward/back comapt

anthony-murphy · 2020-12-09T01:27:37Z

why do we need this hear? seems like it could be defined in container-runtime

Refers to: packages/runtime/runtime-definitions/src/summary.ts:184 in 597decc. [](commit_id = 597decc, deletion_comment = False)

anthony-murphy · 2020-12-09T01:29:43Z

packages/runtime/runtime-utils/src/summarizerNode/summarizerNodeUtils.ts

+ */
+export function parseSummaryForSubtrees(baseSummary: ISnapshotTree): ISubtreeInfo {
+    // New versions of snapshots have child nodes isolated in .channels subtree
+    const channelsSubtree = baseSummary.trees[channelsTreeName];


channelsTreeName [](start = 46, length = 16)

this seem coupled to our specific container runtime. i feel like this should live in container runtime

I agree, but will need to revisit later to extract this functionality.

packages/runtime/container-runtime-definitions/src/snapshot.ts

packages/runtime/container-runtime/src/containerRuntime.ts

packages/runtime/container-runtime/src/dataStores.ts

packages/runtime/container-runtime-definitions/src/snapshot.ts

anthony-murphy

I don't want to block this going in. I'd like BaseSnapshotType removed, but the file moves, and testing can come in a follow up

vladsud · 2020-12-27T17:22:13Z

@arinwt, it would be great to address this area sooner than later. Do you need any help with moving this PR forward?

arinwt · 2020-12-28T19:52:42Z

@vladsud no sorry, I just want to add support for DDS's and address a few of Tony's comments first (version as number if possible and move interface definition).

Arin added 2 commits November 30, 2020 05:44

First draft for container and datastores

d1f1021

Remove commented code

7b7ac94

arinwt changed the title ~~Name collisions~~ Snapshot name collisions Nov 30, 2020

github-actions bot requested review from vladsud and curtisman November 30, 2020 11:22

arinwt commented Nov 30, 2020

View reviewed changes

Correct snapshot format versions

102d7ab

arinwt commented Nov 30, 2020

View reviewed changes

packages/runtime/container-runtime/src/dataStores.ts Outdated Show resolved Hide resolved

arinwt commented Nov 30, 2020

View reviewed changes

Arin added 2 commits November 30, 2020 07:33

Small fixes

27d323b

Switch to plain objects for snapshotFormatVersions

4073b37

Rename .dataStores to .channels

7e3894c

vladsud reviewed Dec 4, 2020

View reviewed changes

Change to union type

32fe99c

github-actions bot requested a review from vladsud December 7, 2020 05:37

Arin added 4 commits December 7, 2020 02:04

Merge branch 'main' into name-collisions

287a7ad

Add support for .channels subtrees in SummarizerNode

d211939

Promote .channels and consolidate SummarizerNode logic

7225397

Fix broken build from imports

597decc

arinwt marked this pull request as ready for review December 7, 2020 18:53

arinwt mentioned this pull request Dec 8, 2020

Prevent Datastores Id's From Starting with Dot #4535

Closed

vladsud reviewed Dec 8, 2020

View reviewed changes

vladsud approved these changes Dec 8, 2020

View reviewed changes