esti: test lakectl fs upload of files over 5GiB by highlyavailable · Pull Request #10478 · treeverse/lakeFS

highlyavailable · 2026-07-12T12:57:58Z

Adds esti coverage for lakectl fs upload of a file over 5GiB, with and without --pre-sign, which is the multipart path fixed in #9283.

uses a sparse 5GiB+1 temp file so disk usage stays low
only runs on the S3 blockstore, where the 5GiB single-part limit applies
checks the uploaded size in lakectl's output and through the stat API
removes each uploaded object from the backing store on cleanup, since deleting the repo leaves its data behind (adds a Delete to the esti S3 adapter for this)

Covers the multipart path fixed in treeverse#9283 for both pre-signed and regular uploads. Extends the output sanitizers to handle multipart etags (md5-N) and relative physical addresses with part suffixes. Closes treeverse#9284

highlyavailable · 2026-10-22T20:53:54Z

hi @itaiad200, just checking in on this one

itaiad200 · 2026-10-25T07:56:36Z

hi @itaiad200, just checking in on this one

I think you meant to tag @itaigilo

itaigilo

Thanks @highlyavailable for your contribution!

These tests indeed add to the robustness of lakeFS, which is great.

However some changes are required, both to avoid unnecessary changes to the codebase, but mainly to make sure these large files are properly cleaned.

itaigilo · 2026-10-26T11:20:23Z

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

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

itaigilo · 2026-10-26T11:21:20Z

 	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) {


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.

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

itaigilo · 2026-10-26T11:26:36Z

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


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.

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

itaigilo · 2026-10-26T11:29:18Z

+	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


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

arielshaqed

Thanks for this contribution, seems great!

arielshaqed · 2026-10-29T08:47:18Z

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


What does vars["FILE_PATH"] do here?

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

arielshaqed · 2026-10-29T09:12:52Z

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


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.)

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

Drop the golden file and the sanitizer changes it needed. The test now checks the uploaded size through lakectl output and the stat API, and deletes each large object from the backing store once the subtest ends, since deleting the repository leaves its data in place.

itaigilo

Thanks for the updates, @highlyavailable -
This looks much cleaner now.

Still a small fix is required,
plus a nit - the PR's description is stale now.

itaigilo · 2026-11-02T10:51:43Z

+		// deleting the repository does not remove its data, so remove the large object from the storage
+		physicalAddress, err := url.Parse(statResp.JSON200.PhysicalAddress)
+		require.NoError(t, err)
+		t.Cleanup(func() {


That's a solid improvement.
However, the cleanup should be registered before the upload, in case one of the previous calls are failing.
It should be registered earlier.

Look up the object's physical address in the cleanup itself, so the large object is removed from storage even if the upload or a later check fails.

itaigilo · 2026-11-03T15:03:59Z

Great stuff @highlyavailable - almost there!

I ran the CI on a separate branch, and it seems that the only thing missing is adding AWS_REGION: us-east-1 to .github/workflows/lakectl-compatibility-tests.yaml, so the test knows from which region to delete the files:

That's the only thing blocking the CI, other that that, LGTM.

The large upload test deletes its objects straight from S3, so the default AWS config needs a region.

itaigilo

LGTM -
Thanks @highlyavailable for your contribution!

esti: test lakectl fs upload of files over 5GiB

360a33d

Covers the multipart path fixed in treeverse#9283 for both pre-signed and regular uploads. Extends the output sanitizers to handle multipart etags (md5-N) and relative physical addresses with part suffixes. Closes treeverse#9284

github-actions Bot added the area/testing Improvements or additions to tests label Jul 12, 2026

itaigilo requested changes Oct 26, 2026

View reviewed changes

itaigilo requested review from N-o-Z and arielshaqed October 26, 2026 11:29

arielshaqed reviewed Oct 29, 2026

View reviewed changes

highlyavailable force-pushed the esti-lakectl-large-upload branch from 7e69d65 to 92bda4e Compare October 31, 2026 04:43

itaigilo reviewed Nov 2, 2026

View reviewed changes

esti: register large upload cleanup before the upload

8ee4609

Look up the object's physical address in the cleanup itself, so the large object is removed from storage even if the upload or a later check fails.

itaigilo added exclude-changelog PR description should not be included in next release changelog minor-change Used for PRs that don't require issue attached labels Nov 3, 2026

ci: set AWS_REGION for lakectl compatibility tests

221e697

The large upload test deletes its objects straight from S3, so the default AWS config needs a region.

github-actions Bot added the area/ci label Nov 7, 2026

itaigilo approved these changes Nov 7, 2026

View reviewed changes

itaigilo mentioned this pull request Nov 7, 2026

CI run for #10478 (do not merge) #10599

Closed

itaigilo merged commit 99ea350 into treeverse:master Nov 7, 2026
98 of 100 checks passed

Conversation

highlyavailable commented Jul 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

highlyavailable commented Oct 22, 2026

Uh oh!

itaiad200 commented Oct 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

itaigilo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arielshaqed left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

itaigilo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

itaigilo commented Nov 3, 2026

Uh oh!

itaigilo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

highlyavailable commented Jul 12, 2026 •

edited

Loading

itaiad200 commented Oct 25, 2026 •

edited

Loading