Skip to content

esti: test lakectl fs upload of files over 5GiB #10478

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
itaigilo merged 4 commits into from
Nov 7, 2026
Merged

esti: test lakectl fs upload of files over 5GiB #10478

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
360a33d
esti: test lakectl fs upload of files over 5GiB
highlyavailable Jul 12, 2026
92bda4e
esti: check size and clean storage in large upload test
highlyavailable Oct 31, 2026
8ee4609
esti: register large upload cleanup before the upload
highlyavailable Nov 2, 2026
221e697
ci: set AWS_REGION for lakectl compatibility tests
highlyavailable Nov 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions esti/golden/lakectl_fs_upload_large.golden
View file
Open in desktop

@itaigilo itaigilo Oct 26, 2026

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.

This golden serves no real purpose - you can just compare the size in the test itself.

@highlyavailable highlyavailable Oct 31, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

removed the golden file and the test now checks the size directly

Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
Path: ${FILE_PATH}
Modified Time: <DATE> <TIME> <TZ>
Size: 5368709121 bytes
Human Size: 5.4 GB
Physical Address: <OBJECT_KEY>
Checksum: <CHECKSUM>
Content-Type: application/octet-stream
33 changes: 33 additions & 0 deletions esti/lakectl_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,39 @@ func TestLakectlPreSignUpload(t *testing.T) {
})
}

func TestLakectlFsUploadLarge(t *testing.T) {
RequireBlockstoreType(t, block.BlockstoreTypeS3)

repoName := GenerateUniqueRepositoryName()
storage := GenerateUniqueStorageNamespace(repoName)
vars := map[string]string{
"REPO": repoName,
"STORAGE": storage,
"BRANCH": mainBranch,
}
RunCmdAndVerifySuccessWithFile(t, Lakectl()+" repo create lakefs://"+repoName+" "+storage, false, "lakectl_repo_create", vars)

// S3 rejects single-part uploads above 5GiB, so one byte more forces
// the multipart path (#9284). A sparse file keeps disk usage low.
const largeFileSize = 5<<30 + 1
f, err := os.CreateTemp("", "lakectl-large-upload-")
require.NoError(t, err)
t.Cleanup(func() {
require.NoError(t, os.Remove(f.Name()))
})
require.NoError(t, f.Truncate(largeFileSize))
require.NoError(t, f.Close())

t.Run("pre-sign", func(t *testing.T) {
vars["FILE_PATH"] = "large_file_presign"

@arielshaqed arielshaqed Oct 29, 2026

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.

What does vars["FILE_PATH"] do here?

@highlyavailable highlyavailable Oct 31, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

it was only fed the Path line in the golden file, which is gone now, so I removed it

RunCmdAndVerifySuccessWithFile(t, Lakectl()+" fs upload -s "+f.Name()+" lakefs://"+repoName+"/"+mainBranch+"/"+vars["FILE_PATH"]+" --pre-sign", false, "lakectl_fs_upload_large", vars)

@itaigilo itaigilo Oct 26, 2026

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.

This upload (and the following one) command uploads the large file to the storage.
While the repo is deleted as part of the ESTI tests cleanup, these files are not cleaned up from the storage itself. When running on a temp MinIO this is fine, but if it runs on S3, it will leave unused large files on the storage. Please make sure these are cleaned.

@highlyavailable highlyavailable Oct 31, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

each subtest now deletes its large object from S3 on cleanup, through a new Delete on the esti S3 adapter

@arielshaqed arielshaqed Oct 29, 2026

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.

Can we verify the contents, or the size? (ETag itself is hard, it is not really a function of contents because it also depends on the backend and on the split into parts.)

@highlyavailable highlyavailable Oct 31, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

addressed in 7e69d65, the test now verifies the size in lakectl's output and through the stat API. i think that's better than a full content check which would entails downloading 5db but I can add one if you'd recommend

})
t.Run("no pre-sign", func(t *testing.T) {
vars["FILE_PATH"] = "large_file"
RunCmdAndVerifySuccessWithFile(t, Lakectl()+" fs upload -s "+f.Name()+" lakefs://"+repoName+"/"+mainBranch+"/"+vars["FILE_PATH"], false, "lakectl_fs_upload_large", vars)
})
}

func TestLakectlCommit(t *testing.T) {
repoName := GenerateUniqueRepositoryName()
storage := GenerateUniqueStorageNamespace(repoName)
Expand Down
25 changes: 17 additions & 8 deletions esti/lakectl_util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,17 @@ var (
)

var (
reTimestamp = regexp.MustCompile(`timestamp: \d+`)
reTime = regexp.MustCompile(`\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2} [-+]\d{4} \w{1,4}`)
reCommitID = regexp.MustCompile(`[\d|a-f]{64}`)
reShortCommitID = regexp.MustCompile(`[\d|a-f]{16}`)
reChecksum = regexp.MustCompile(`([\d|a-f]{32})|(0x[0-9A-F]{15})`)
reEndpoint = regexp.MustCompile(`https?://\w+(:\d+)?/api/v\d+/`)
rePhysicalAddress = regexp.MustCompile(`/data/[0-9a-v]{20}/(?:[0-9a-v]{20}(?:,.+)?)?`)
reTimestamp = regexp.MustCompile(`timestamp: \d+`)
reTime = regexp.MustCompile(`\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2} [-+]\d{4} \w{1,4}`)
reCommitID = regexp.MustCompile(`[\d|a-f]{64}`)
reShortCommitID = regexp.MustCompile(`[\d|a-f]{16}`)
// multipart uploads report an etag with a part count suffix, e.g. <md5>-1025

@itaigilo itaigilo Oct 26, 2026

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.

Please keep the same code style and avoid these extra comments.
Same for the next vars here.

reChecksum = regexp.MustCompile(`([\d|a-f]{32}(-\d+)?)|(0x[0-9A-F]{15})`)
reEndpoint = regexp.MustCompile(`https?://\w+(:\d+)?/api/v\d+/`)
// physical addresses may be printed relative to the storage namespace (no
// leading slash), and multipart uploads append a part-count suffix, e.g.
// data/<prefix>/<key>,<suffix>
rePhysicalAddress = regexp.MustCompile(`/?data/[0-9a-v]{20}/(?:[0-9a-v]{20}(?:,.+)?)?`)
reVariable = regexp.MustCompile(`\$\{([^${}]+)}`)
rePreSignURL = regexp.MustCompile(`https?://\S+\?\S+`)
reSecretAccessKey = regexp.MustCompile(`secret_access_key: \S{16,128}`)
Expand Down Expand Up @@ -260,7 +264,12 @@ func normalizeProgramTimestamp(output string) string {
func normalizeRandomObjectKey(output string, objectPrefix string) string {
objectPrefix = strings.TrimPrefix(objectPrefix, "/")
for _, match := range rePhysicalAddress.FindAllString(output, -1) {
output = strings.Replace(output, objectPrefix+match, objectPrefix+"/<OBJECT_KEY>", 1)
if strings.Contains(output, objectPrefix+match) {

@itaigilo itaigilo Oct 26, 2026

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.

These changes can hide future bugs in golden files.
Instead, I suggest to use RunCmdAndVerifySuccess in your tests, avoid using a golden file for these tests, and keep this file untouched.

@highlyavailable highlyavailable Oct 31, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I reverted these so lakectl_util.go is untouched now... I used RunCmdAndVerifyContainsText on the size line instead of RunCmdAndVerifySuccess

output = strings.Replace(output, objectPrefix+match, objectPrefix+"/<OBJECT_KEY>", 1)
} else {
// relative physical address, not prefixed by the storage namespace
output = strings.Replace(output, match, "<OBJECT_KEY>", 1)
}
}
return output
}
Expand Down