Skip to content

feat(storage): implement move command#814

Open
nhedger wants to merge 3 commits into
exoscale:masterfrom
nhedger:feat/storage-move
Open

feat(storage): implement move command#814
nhedger wants to merge 3 commits into
exoscale:masterfrom
nhedger:feat/storage-move

Conversation

@nhedger
Copy link
Copy Markdown
Contributor

@nhedger nhedger commented Mar 24, 2026

Note

Disclosure:

  • AI assistance was used to implement this feature
  • Manual review and testing was done

Description

Closes #604

This PR adds exo storage move (mv) to move objects within a bucket or across buckets without downloading them locally.

The implementation uses server-side copy + delete for regular objects, and multipart server-side copy for large objects. It also preserves object metadata, headers, and ACLs across the move.

A few notes to help review:

  • cmd/storage/storage_move.go adds the new CLI command, argument parsing, dry-run / verbose behavior, recursive prefix support, and a --concurrency flag for multipart copy workers
  • pkg/storage/sos/move.go adds the storage-layer move implementation for both single-object and recursive moves, including multipart copy + abort-on-failure handling
  • pkg/storage/sos/acl.go refactors ACL translation so the same ACL preservation logic can be reused for both CopyObject and CreateMultipartUpload
  • pkg/storage/sos/object.go extracts multipart part-size calculation into a shared helper so upload and move use the same dynamic sizing logic and stay under the 10,000-part limit
  • pkg/storage/sos/s3api.go, pkg/storage/sos/s3api_mock_test.go, and pkg/storage/sos/move_test.go extend the S3 abstraction and test coverage for HeadObject, multipart copy, abort handling, and configurable concurrency

Open questions

  • Should --concurrency remain a user-facing flag for multipart moves? While it can improve large-object move times, higher values may put additional strain on the object storage backend and increase request pressure on the infrastructure.

Checklist

(For exoscale contributors)

  • Changelog updated (under Unreleased block, and add the Pull Request #number for each bit you add to the CHANGELOG.md)
  • Testing

Testing

  • go test ./...
  • verified dry-run behavior for single-object moves
  • verified same-bucket object moves
  • verified cross-bucket object moves
  • verified recursive cross-bucket prefix moves
  • verified live >5 GiB cross-bucket move path
  • verified configurable multipart copy concurrency

@imiric imiric requested a review from a team April 2, 2026 08:10
@natalie-o-perret
Copy link
Copy Markdown
Contributor

Thanks for this PR!

The implementation is solid overall, clean separation of concerns, and proper abort handling for multipart uploads.

A couple of things that might be worth addressing:

  1. Documenting the non-atomic copy+delete behaviour

    • CopyObject + DeleteObjects is not atomic. If the delete fails after a successful copy, the object will exist in both the source and destination with no automatic rollback.
    • This is a "generic" S3 limitation, but it might be worth warning users about it.
    • Could be a small note in the Long description in cmd/storage/storage_move.go L23, e.g.,

      Warning: move is implemented as server-side copy followed by delete.

      If the delete step fails, the object will remain in both locations.

      There is no automatic rollback.

  2. --concurrency vs --multipart-concurrency

    • The flag controls the number of workers for multipart copy parts within a single large object, not how many objects are moved in parallel, the outer loop in MoveObjects L62 is serial.
    • --multipart-concurrency might reflect that more accurately and avoid confusion for users with large buckets of small objects.
    • Could also be worth mentioning in the Long description that multi-object prefix moves are processed serially.
  3. -r vs trailing /

    • A trailing / on the source already triggers prefix mode, -r only controls whether ForEachObject recurses into subdirectories.
    • exo storage move sos://bucket/prefix/ sos://other/ (without -r) would do a non-recursive prefix move, which might not be obvious.
    • A small clarification in the Long description or flag usage could help.
  4. Tests for the multipart copy path

  5. Minor: comment in deleteMovedObject

    • deleteMovedObject L398 only inspects res.Errors[0], which is fine since it always deletes exactly one object, a short // always one object comment might just make that obvious at a glance.

@natalie-o-perret natalie-o-perret self-requested a review April 21, 2026 09:38
@natalie-o-perret natalie-o-perret added the enhancement New feature or request label Apr 21, 2026
@natalie-o-perret natalie-o-perret requested review from a team and kobajagi and removed request for a team May 7, 2026 10:27
@pierre-emmanuelJ pierre-emmanuelJ self-requested a review May 7, 2026 12:12
Comment on lines +142 to +153
if len(moved) == 0 {
if !globalstate.Quiet {
fmt.Printf("no objects exist at %q\n", source.Key)
}
return nil
}

if dryRun {
return nil
}

return nil
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
if len(moved) == 0 {
if !globalstate.Quiet {
fmt.Printf("no objects exist at %q\n", source.Key)
}
return nil
}
if dryRun {
return nil
}
return nil
if len(moved) == 0 && !globalstate.Quiet {
fmt.Printf("no objects exist at %q\n", source.Key)
}
return nil

@pierre-emmanuelJ pierre-emmanuelJ added the HOLD ✋ Hold, don't merge label Jun 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request HOLD ✋ Hold, don't merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature Request]: implement a exo storage move

4 participants