Pilorama migration #960

Merged
fyrchik merged 1 commit from dstepanov-yadro/frostfs-node:feat/pilorama_migrate into master 2024-09-04 19:51:06 +00:00

Extended frostfs-cli control shards evacuation start to support pilorama evacuation: added option scope to specify evacuation scope: all, objects or trees.
Trees will be evacuated to other shards or to other container node.

Relates #947

Extended `frostfs-cli control shards evacuation start` to support pilorama evacuation: added option `scope` to specify evacuation scope: `all`, `objects` or `trees`. Trees will be evacuated to other shards or to other container node. Relates #947
dstepanov-yadro force-pushed feat/pilorama_migrate from 38d6200ee8 to 7307525f62 2024-02-05 14:52:42 +00:00 Compare
dstepanov-yadro force-pushed feat/pilorama_migrate from 7307525f62 to 863cb6ca8b 2024-02-05 14:58:04 +00:00 Compare
dstepanov-yadro force-pushed feat/pilorama_migrate from fc161ebd0e to da99e6d8df 2024-02-06 12:23:11 +00:00 Compare
dstepanov-yadro force-pushed feat/pilorama_migrate from 51db28a666 to b64596e1d7 2024-02-07 07:06:08 +00:00 Compare
dstepanov-yadro force-pushed feat/pilorama_migrate from b64596e1d7 to 6cfb4da032 2024-02-07 10:26:37 +00:00 Compare
dstepanov-yadro force-pushed feat/pilorama_migrate from 6cfb4da032 to d36a41e61f 2024-02-07 10:27:45 +00:00 Compare
dstepanov-yadro force-pushed feat/pilorama_migrate from d36a41e61f to 8adaba09ad 2024-02-07 11:11:48 +00:00 Compare
dstepanov-yadro force-pushed feat/pilorama_migrate from 8adaba09ad to 66834db5fe 2024-02-07 11:30:54 +00:00 Compare
dstepanov-yadro force-pushed feat/pilorama_migrate from 66834db5fe to 5ab641f211 2024-02-07 16:31:39 +00:00 Compare
dstepanov-yadro force-pushed feat/pilorama_migrate from 5ab641f211 to 89b1c4f69e 2024-02-07 16:50:29 +00:00 Compare
dstepanov-yadro force-pushed feat/pilorama_migrate from 89b1c4f69e to 576ee32bb1 2024-02-07 16:51:04 +00:00 Compare
dstepanov-yadro changed title from WIP: Pilorama migration to Pilorama migration 2024-02-07 16:58:24 +00:00
dstepanov-yadro force-pushed feat/pilorama_migrate from 576ee32bb1 to 043088f016 2024-02-08 08:46:43 +00:00 Compare
aarifullin approved these changes 2024-02-08 08:58:33 +00:00
aarifullin left a comment
Member

Nice!

Nice!
dstepanov-yadro force-pushed feat/pilorama_migrate from 043088f016 to 0f55610555 2024-02-08 09:05:49 +00:00 Compare
dstepanov-yadro force-pushed feat/pilorama_migrate from 0f55610555 to 72d03ba6e8 2024-02-08 10:01:54 +00:00 Compare
dstepanov-yadro requested review from storage-core-committers 2024-02-08 14:44:29 +00:00
dstepanov-yadro requested review from storage-core-developers 2024-02-08 14:44:29 +00:00
fyrchik requested changes 2024-02-08 17:25:04 +00:00
@ -290,3 +312,1 @@
resp.GetBody().GetTotal(),
resp.GetBody().GetFailed(),
resp.GetBody().GetSkipped()))
sb.WriteString(fmt.Sprintf(" Evacuated %d objects out of %d, failed to evacuate: %d, skipped: %d; evacuated %d trees out of %d, failed to evacuate: %d.",
Owner

What's the benefit in outputting both objects in trees if only 1 evacuation is being used (the numbers for other will be 0, I guess)

What's the benefit in outputting both objects in trees if only 1 evacuation is being used (the numbers for other will be 0, I guess)
Author
Member

To have unified output: it could be parsed by tests for example.

To have unified output: it could be parsed by tests for example.
fyrchik marked this conversation as resolved
@ -301,0 +444,4 @@
}
if success {
e.log.Debug(logs.EngineShardsEvacuationTreeEvacuatedLocal,
zap.String("cid", contTree.CID.EncodeToString()), zap.String("treeID", contTree.TreeID),
Owner

We use shard_id in labels usually, why is it shardID here?

We use `shard_id` in labels usually, why is it `shardID` here?
Author
Member

fixed

fixed
fyrchik marked this conversation as resolved
@ -301,0 +480,4 @@
prm EvacuateShardPrm, shards []pooledShard, weights []float64, shardsToEvacuate map[string]*shard.Shard,
) (bool, string, error) {
target, found, err := e.findShardToEvacuateTree(ctx, tree, shards, weights, shardsToEvacuate)
if err != nil && !prm.IgnoreErrors {
Owner

I believe !prm.IgnoreErrors is not needed here: we ignore errors of evacuation, but if we haven't found a suitable shard, there is nothing we can do below, right?

I believe `!prm.IgnoreErrors` is not needed here: we ignore errors of evacuation, but if we haven't found a suitable shard, there is nothing we can do below, right?
Author
Member

right, fixed

right, fixed
fyrchik marked this conversation as resolved
@ -396,0 +434,4 @@
require.Nil(t, res, "async evacuation must return nil")
require.NoError(t, err, "first evacuation failed")
require.Eventually(t, func() bool {
Owner

Eventually will eventually fail on CI.
Though, it is sometimes inevitable, may we could provide a notify channel in parameters, which we close upon finish?

`Eventually` will eventually fail on CI. Though, it is sometimes inevitable, may we could provide a notify channel in parameters, which we `close` upon finish?
Author
Member

Changed test to sync version.

Changed test to sync version.
fyrchik marked this conversation as resolved
@ -1137,0 +1210,4 @@
err := metaerr.Wrap(t.db.View(func(tx *bbolt.Tx) error {
c := tx.Cursor()
for k, _ := c.Seek(prm.NextPageToken); k != nil; k, _ = c.Next() {
if bytes.Equal(k, dataBucket) || bytes.Equal(k, logBucket) || bytes.Equal(k, prm.NextPageToken) {
Owner

buckets can be filtered with a simple v != nil value (v is get from both Seek() and Next()))
prm.NextPageToken could be checked only on the first iteration

buckets can be filtered with a simple `v != nil` value (v is get from both `Seek()` and `Next()`)) `prm.NextPageToken` could be checked only on the first iteration
Author
Member

buckets can be filtered with a simple v != nil value

Cursor opened on tx, so it iterates only buckets.

prm.NextPageToken could be checked only on the first iteration

One more variable and one more if? Ok, fixed.

> buckets can be filtered with a simple `v != nil` value Cursor opened on `tx`, so it iterates only buckets. > `prm.NextPageToken` could be checked only on the first iteration One more variable and one more `if`? Ok, fixed.
Owner

One more variable and one more if? Ok, fixed.

I've thought about this if outside the loop because iteration is ordered and equality can happen only once.

>One more variable and one more if? Ok, fixed. I've thought about this if outside the loop because iteration is ordered and equality can happen only once.
@ -1191,1 +1191,4 @@
}
func TestForest_ListTrees(t *testing.T) {
prev := TreeListTreesBatchSize
Owner

Please, no
We provide parameter struct in TreeListTrees, why not include batch size there?

Please, no We provide parameter _struct_ in `TreeListTrees`, why not include batch size there?
Author
Member

Done

Done
fyrchik marked this conversation as resolved
@ -66,0 +66,4 @@
// TreeListTrees returns all pairs "containerID:treeID".
TreeListTrees(ctx context.Context, prm TreeListTreesPrm) (*TreeListTreesResult, error)
TreeApplyStream(ctx context.Context, cnr cidSDK.ID, treeID string, source <-chan *Move) error
Owner

We should mention in comments that TreeApplyStream has specific use and can block other write tx for a pretty long time

We should mention in comments that `TreeApplyStream` has specific use and can block other write tx for a pretty long time
Author
Member

Done for boltdb implementation.

Done for boltdb implementation.
fyrchik marked this conversation as resolved
@ -101,0 +127,4 @@
},
}
err = tree.SignMessage(req, s.key)
Owner

Why do se sign here and not in the tree service?

Why do se sign here and not in the tree service?
Author
Member

Just because we form request here too. So tree service gets completed request.

Just because we form request here too. So tree service gets completed request.
fyrchik marked this conversation as resolved
dstepanov-yadro force-pushed feat/pilorama_migrate from 72d03ba6e8 to b2e94e35e3 2024-02-09 07:09:57 +00:00 Compare
dstepanov-yadro force-pushed feat/pilorama_migrate from b2e94e35e3 to 6a3513745c 2024-02-09 07:11:54 +00:00 Compare
dstepanov-yadro force-pushed feat/pilorama_migrate from 6a3513745c to f2256cac2b 2024-02-09 07:17:55 +00:00 Compare
dstepanov-yadro force-pushed feat/pilorama_migrate from f2256cac2b to 3298a78c7a 2024-02-09 07:34:25 +00:00 Compare
dstepanov-yadro force-pushed feat/pilorama_migrate from 3298a78c7a to 4e6fb83407 2024-02-09 07:47:50 +00:00 Compare
dstepanov-yadro force-pushed feat/pilorama_migrate from 4e6fb83407 to 72e77e02a9 2024-02-09 07:51:21 +00:00 Compare
fyrchik approved these changes 2024-02-09 08:04:07 +00:00
dstepanov-yadro force-pushed feat/pilorama_migrate from 72e77e02a9 to 03f6051c0f 2024-02-09 08:21:08 +00:00 Compare
dstepanov-yadro force-pushed feat/pilorama_migrate from 03f6051c0f to 60527abb65 2024-02-09 08:33:27 +00:00 Compare
acid-ant approved these changes 2024-02-09 14:02:25 +00:00
fyrchik merged commit 60527abb65 into master 2024-02-09 14:03:26 +00:00
Sign in to join this conversation.
No reviewers
TrueCloudLab/storage-core-developers
No milestone
No project
No assignees
4 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-node#960
No description provided.