Skip to content

[Filestore] [1/N] Document the filestore resize flow in legacy and throttled modes - #6376

Open
Maximus5 wants to merge 1 commit into
mainfrom
users/maximus5/filestore-resize-persist-state
Open

Maximus5 wants to merge 1 commit into
mainfrom
users/maximus5/filestore-resize-persist-state

Conversation

@Maximus5

@Maximus5 Maximus5 commented Jun 29, 2026 •

Copy link
Copy Markdown
Collaborator

@Maximus5
Maximus5 requested a review from qkrorlqr June 29, 2026 23:37
@github-actions

github-actions Bot commented Jun 29, 2026 •

Copy link
Copy Markdown
Contributor

Note

This is an automated comment that will be appended during run.

Note

All workloads for linux-x86_64-relwithdebinfo have completed.

Tip

Planned checks for linux-x86_64-relwithdebinfo.

🟢 linux-x86_64-relwithdebinfo target: cloud/tasks/,cloud/storage/ (test time: 259s): all tests PASSED for commit 946bfb7.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
964 964 0 0 0 0 0

🟢 linux-x86_64-relwithdebinfo target: cloud/disk_manager/ (test time: 248s): all tests PASSED for commit 946bfb7.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
1184 1184 0 0 0 0 0

🔴 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 1241s): some tests FAILED for commit 946bfb7.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
6543 6532 0 10 0 1 0

🟢 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 18s): all tests PASSED for commit 946bfb7.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
89 89 0 0 0 0 0

🟢 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 4462s): all tests PASSED for commit 946bfb7.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
3857 3857 0 0 0 0 0

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 946bfb74c0

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@qkrorlqr qkrorlqr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you planning to implement this logic separately for CreateFileSystem requests?

.Set(shardIndex);

auto request =
std::make_unique<TEvIndexTablet::TEvUnsafeChangeTabletStateRequest>();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we probably need some kind of a ChangeStateCookie in this request to avoid ChangeState-races for concurrent requests from multiple filestore-servers

e.g. use ChangeStateCookie = hash(PrevState) to be able to check in the tablet code that the caller actually knows the most recent state

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With ResizeState.Version and updated Merge/Verify/Retry logic in TAlterFileStoreActor::HandleResizeStateResponse protect us from races.

  1. Actors A and B both read ResizeState.Version = 1.
  2. A creates shard X and sends bitmap {X} with Version = 1.
  3. B creates shard Y and sends bitmap {Y} with Version = 1.
  4. The tablet handles e.g. A first, stores bitmap {X}, and bumps Version to 2.
  5. The tablet handles B next. B's Version = 1 is stale, so the tablet does not
    apply bitmap {Y}. The response contains the current persisted state:
    Version = 2, bitmap {X}.
  6. B merges current persisted state with its own local state, and if this merged state differs from persisted state, does retry to store bitmap {Y'}

@Maximus5 Maximus5 Sep 10, 2026 •

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway. When two concurrent resize requests were started, both actors have the same initial topology, and they both tries to create shard{curSize+1}. Where only one actor wins, another got StatusAlreadyExists error and stop processing.

CreateShard(ctx, NextShardToCreate);
++NextShardToCreate;
++requests;
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it would be less error-prone to keep this code here as well:

if (ShardsToCreate == 0) {
    ConfigureShards(ctx);
    return;
}

The caller code could also be simplified this way.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would keep single responsibility for ContinueCreateShards method.

We want to run ConfigureShards only if no create calls were initiated by this actor. I.e. if there are in-flight requests to update bitmap, we want first to wait until it's stored durably.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add early Resize/Alter retries to our medium tests?

For example, we can add them to this test, which we use for feature adoption: https://github.com/ydb-platform/nbs/blob/main/cloud/filestore/tests/scheme_cache/test.py

It would also be good to run two filestore-server instances and send Resize retries to them in a round-robin manner.

@github-actions

github-actions Bot commented Sep 7, 2026 •

Copy link
Copy Markdown
Contributor

Note

This is an automated comment that will be appended during run.

Note

All workloads for linux-x86_64-relwithdebinfo have completed.

Tip

Planned checks for linux-x86_64-relwithdebinfo.

🟢 linux-x86_64-relwithdebinfo target: cloud/tasks/,cloud/storage/ (test time: 267s): all tests PASSED for commit fa93d9a.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
1046 1046 0 0 0 0 0

🟢 linux-x86_64-relwithdebinfo target: cloud/disk_manager/ (test time: 260s): all tests PASSED for commit fa93d9a.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
1562 1562 0 0 0 0 0

🟢 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 1426s): all tests PASSED for commit fa93d9a.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
6620 6619 0 0 0 1 0

🔴 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 4628s): some tests FAILED for commit fa93d9a.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
3627 3626 0 0 1 0 0

🔴 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 4237s): some tests FAILED for commit fa93d9a.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
3627 3626 0 0 1 0 0

🔴 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 4212s): some tests FAILED for commit fa93d9a.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
3627 3626 0 0 1 0 0

@Maximus5
Maximus5 force-pushed the users/maximus5/filestore-resize-persist-state branch from fa93d9a to 3107f78 Compare September 9, 2026 17:32
@github-actions

github-actions Bot commented Sep 9, 2026 •

Copy link
Copy Markdown
Contributor

Note

This is an automated comment that will be appended during run.

Note

All workloads for linux-x86_64-relwithdebinfo have completed.

Tip

Planned checks for linux-x86_64-relwithdebinfo.

🟢 linux-x86_64-relwithdebinfo target: cloud/tasks/,cloud/storage/ (test time: 274s): all tests PASSED for commit 3107f78.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
1449 1449 0 0 0 0 0

🟢 linux-x86_64-relwithdebinfo target: cloud/disk_manager/ (test time: 333s): all tests PASSED for commit 3107f78.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
1696 1696 0 0 0 0 0

🔴 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 1605s): some tests FAILED for commit 3107f78.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
7275 7273 0 1 0 1 0

🟢 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 46s): all tests PASSED for commit 3107f78.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
2 2 0 0 0 0 0

🟢 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 4872s): all tests PASSED for commit 3107f78.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
4813 4812 0 0 0 1 0

@Maximus5
Maximus5 force-pushed the users/maximus5/filestore-resize-persist-state branch from 3107f78 to 749d57f Compare September 10, 2026 11:12
@github-actions

github-actions Bot commented Sep 10, 2026 •

Copy link
Copy Markdown
Contributor

Note

This is an automated comment that will be appended during run.

Note

All workloads for linux-x86_64-relwithdebinfo have completed.

Tip

Planned checks for linux-x86_64-relwithdebinfo.

🟢 linux-x86_64-relwithdebinfo target: cloud/tasks/,cloud/storage/ (test time: 275s): all tests PASSED for commit 749d57f.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
1463 1463 0 0 0 0 0

🟢 linux-x86_64-relwithdebinfo target: cloud/disk_manager/ (test time: 324s): all tests PASSED for commit 749d57f.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
1696 1696 0 0 0 0 0

🔴 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 1689s): some tests FAILED for commit 749d57f.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
7300 7286 0 8 0 6 0

🟢 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 28s): all tests PASSED for commit 749d57f.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
18 18 0 0 0 0 0

🔴 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 5348s): some tests FAILED for commit 749d57f.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
4835 4829 0 5 0 1 0

🔴 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 643s): some tests FAILED for commit 749d57f.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
8 3 0 5 0 0 0

🔴 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 639s): some tests FAILED for commit 749d57f.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
8 3 0 5 0 0 0

@Maximus5

Copy link
Copy Markdown
Collaborator Author

are you planning to implement this logic separately for CreateFileSystem requests?

I want to keep this PR explicitly for ResizeFileStore request. And explore possibilities for Create next.

@github-actions

github-actions Bot commented Sep 10, 2026 •

Copy link
Copy Markdown
Contributor

Note

This is an automated comment that will be appended during run.

Note

All workloads for linux-x86_64-relwithdebinfo have completed.

Tip

Planned checks for linux-x86_64-relwithdebinfo.

🟢 linux-x86_64-relwithdebinfo target: cloud/tasks/,cloud/storage/ (test time: 273s): all tests PASSED for commit 2a3c7c2.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
1463 1463 0 0 0 0 0

🟢 linux-x86_64-relwithdebinfo target: cloud/disk_manager/ (test time: 315s): all tests PASSED for commit 2a3c7c2.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
1696 1696 0 0 0 0 0

🟢 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 1482s): all tests PASSED for commit 2a3c7c2.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
7300 7299 0 0 0 1 0

🟢 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 4923s): all tests PASSED for commit 2a3c7c2.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
4835 4834 0 0 0 1 0

@Maximus5
Maximus5 force-pushed the users/maximus5/filestore-resize-persist-state branch from aee64bb to 7175627 Compare September 11, 2026 09:43
@github-actions

github-actions Bot commented Sep 11, 2026 •

Copy link
Copy Markdown
Contributor

Note

This is an automated comment that will be appended during run.

Note

All workloads for linux-x86_64-relwithdebinfo have completed.

Tip

Planned checks for linux-x86_64-relwithdebinfo.

  • 🚫 tasks + storage (cancelled or timed out before reporting completion)
  • 🚫 blockstore (cancelled or timed out before reporting completion)
  • 🚫 disk + manager (cancelled or timed out before reporting completion)
  • 🚫 filestore (cancelled or timed out before reporting completion)

@github-actions

github-actions Bot commented Sep 11, 2026 •

Copy link
Copy Markdown
Contributor

Note

This is an automated comment that will be appended during run.

Note

All workloads for linux-x86_64-relwithdebinfo have completed.

Tip

Planned checks for linux-x86_64-relwithdebinfo.

  • 🚫 tasks + storage (cancelled or timed out before reporting completion)
  • 🚫 blockstore (cancelled or timed out before reporting completion)
  • 🚫 disk + manager (cancelled or timed out before reporting completion)
  • 🚫 filestore (cancelled or timed out before reporting completion)

@github-actions

github-actions Bot commented Sep 11, 2026 •

Copy link
Copy Markdown
Contributor

Note

This is an automated comment that will be appended during run.

Note

All workloads for linux-x86_64-relwithdebinfo have completed.

Tip

Planned checks for linux-x86_64-relwithdebinfo.

  • 🚫 tasks + storage (cancelled or timed out before reporting completion)
  • 🚫 blockstore (cancelled or timed out before reporting completion)
  • 🚫 disk + manager (cancelled or timed out before reporting completion)
  • 🚫 filestore (cancelled or timed out before reporting completion)

@github-actions

github-actions Bot commented Sep 11, 2026 •

Copy link
Copy Markdown
Contributor

Note

This is an automated comment that will be appended during run.

Note

All workloads for linux-x86_64-relwithdebinfo have completed.

Tip

Planned checks for linux-x86_64-relwithdebinfo.

🟢 linux-x86_64-relwithdebinfo target: cloud/tasks/,cloud/storage/ (test time: 276s): all tests PASSED for commit d0efd0f.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
1519 1519 0 0 0 0 0

🟢 linux-x86_64-relwithdebinfo target: cloud/disk_manager/ (test time: 318s): all tests PASSED for commit d0efd0f.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
1696 1696 0 0 0 0 0

🟢 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 1535s): all tests PASSED for commit d0efd0f.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
7371 7370 0 0 0 1 0

🟢 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 4919s): all tests PASSED for commit d0efd0f.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
4911 4910 0 0 0 1 0

TEvIndexTablet::TEvConfigureShardsResponse,
HandleConfigureMainFileStoreResponse);
HFunc(
TEvIndexTablet::TEvUnsafeChangeTabletStateResponse,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally the Unsafe* set of operations was intended for manual interventions into the tablet state. Routing ordinary requests through it is a bit unusual

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decided to use Usafe after a talk with @qkrorlqr. Reasoning - reuse already existing API to avoid reserving new operation IDs just to get/put state of the tablet.
Probably, we may rename this operation to just ChangeTabletState.

I'm not sure if this could cause any incompatibilities during rolling updates?

}
}

bool TAlterFileStoreActor::HasUnpersistedCreatedShards() const

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is identical code across two actors. Probably worth unifying into a common method


message TCompressedBitmapChunk
{
uint32 ChunkIdx = 1;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be missing something, but what is the value of chunking here as opposed to just single bytes or repeated bool?

"[%s] UnsafeChangeTabletState failed: %s",
LogTag.c_str(),
FormatError(msg->GetError()).Quote().c_str());
return;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. We can get a retriable error in case of an ongoing tablet restart. It is probably worth retrying retriable errors (see GetErrorKind function)

  2. If we reach this place, won't this actor just leak even without replying to the caller?

  3. It is both reasonable to eeither propagate non-retiriable error to the caller, or raise a critical event if this is supposed to be an Impossible event

}

if (shardCreationState.GetVersion() < ShardCreationStateVersion) {
return;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this line also lead to an actor leak?

@github-actions

github-actions Bot commented Sep 18, 2026 •

Copy link
Copy Markdown
Contributor

Note

This is an automated comment that will be appended during run.

Note

All workloads for linux-x86_64-relwithdebinfo have completed.

Tip

Planned checks for linux-x86_64-relwithdebinfo.

🟢 linux-x86_64-relwithdebinfo target: cloud/tasks/,cloud/storage/ (test time: 273s): all tests PASSED for commit ee413d7.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
1519 1519 0 0 0 0 0

🟢 linux-x86_64-relwithdebinfo target: cloud/disk_manager/ (test time: 356s): all tests PASSED for commit ee413d7.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
1696 1696 0 0 0 0 0

🟢 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 1579s): all tests PASSED for commit ee413d7.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
7371 7370 0 0 0 1 0

🟢 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 5013s): all tests PASSED for commit ee413d7.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
4925 4924 0 0 0 1 0

@github-actions

github-actions Bot commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

Note

This is an automated comment that will be appended during run.

Note

All workloads for linux-x86_64-relwithdebinfo have completed.

Tip

Planned checks for linux-x86_64-relwithdebinfo.

🟢 linux-x86_64-relwithdebinfo target: cloud/tasks/,cloud/storage/ (test time: 272s): all tests PASSED for commit 35411cf.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
1519 1519 0 0 0 0 0

🟢 linux-x86_64-relwithdebinfo target: cloud/disk_manager/ (test time: 308s): all tests PASSED for commit 35411cf.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
1696 1696 0 0 0 0 0

🟢 linux-x86_64-relwithdebinfo target: cloud/blockstore/ (test time: 1519s): all tests PASSED for commit 35411cf.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
7371 7370 0 0 0 1 0

🟢 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 4837s): all tests PASSED for commit 35411cf.

TESTS PASSED ERRORS FAILED FAILED BUILD SKIPPED MUTED?
4931 4930 0 0 0 1 0

@Maximus5
Maximus5 force-pushed the users/maximus5/filestore-resize-persist-state branch 2 times, most recently from 35411cf to 7f35614 Compare September 21, 2026 13:48
@Maximus5 Maximus5 changed the title [Filestore] Persist ResizeState on failures [Filestore] Persist ResizeState on failures [1/N] Sep 21, 2026
@Maximus5 Maximus5 changed the title [Filestore] Persist ResizeState on failures [1/N] [Filestore] [1/N] Document the filestore resize flow in legacy and throttled modes Sep 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants