Add resumable downloads for Azure & HTTP datastores #50

jsfakian · 2025-10-17T15:37:22Z

This PR introduces resumable, range-based downloads for HTTP datastores and fixes the process for Azure datastores:

Azure Blob datastore: Skips already-completed chunks instead of always starting from the first chunk.
HTTP datastore:
- Resume from existing local file: if local file exists, and the HTTP server has Range capability.
- Identify copiedSize from the doneParts.
- Update doneParts periodically for each downloaded chunk.

deitch

lint is failing because your regex is not used, which means it probably doesn't work as expected.

The azure part makes sense. As long as you can find a sane way to test it, that is good.

The http stuff also looks good in principle. I have a few questions:

does it handle http servers that don't support range requests?
Can we create a test for this? It shouldn't be too hard, we should be able to spin up a test http server right inside a go test.
Perhaps most importantly, is there any off-the-shelf library that supports all of this (the http part) and we can just get rid of our own code?

deitch · 2025-10-19T07:32:24Z

zedUpload/azureutil/azure.go


 const (
 	// SingleMB contains chunk size
-	SingleMB    int64 = 4 * 1024 * 1024


We actually had SingleMB defined for 4MB? 🤦‍♂️

jsfakian · 2025-10-21T13:14:34Z

lint is failing because your regex is not used, which means it probably doesn't work as expected.

The azure part makes sense. As long as you can find a sane way to test it, that is good.

The http stuff also looks good in principle. I have a few questions:

does it handle http servers that don't support range requests?

Can we create a test for this? It shouldn't be too hard, we should be able to spin up a test http server right inside a go test.

Perhaps most importantly, is there any off-the-shelf library that supports all of this (the http part) and we can just get rid of our own code?

For point 1, I updated the code to also work for servers that do not support range requests.
I am working on creating a test for it.
About point 3, we could use https://github.com/cavaliergopher/grab. It does not expose the inactivity timeout that we currently have in our code, but I will figure out how to use it with the library. I am not sure about switching to this lib.

jsfakian · 2025-10-21T15:10:07Z

I have added unit tests for resuming downloads in HTTP datastores.

jsfakian · 2025-10-21T15:14:15Z

The tests fail due to the race addressed in the other PR.

Signed-off-by: Ioannis Sfakianakis <[email protected]>

christoph-zededa · 2025-10-24T11:13:04Z

zedUpload/datastore_http_test.go

+	}
+	if err := os.MkdirAll(nettraceFolder, 0755); err != nil {
+		t.Fatalf("unable to make nettrace log directory: %v", err)
+	}


Suggested change

}

}

defer os.RemoveAll(nettraceFolder)

just to be sure that if you run the test a second time, it does not interfere with leftovers from the first run

Perhaps the same for httpDownloadDir, but it seems you already remove all created files in there.

christoph-zededa · 2025-10-24T11:16:42Z

zedUpload/azureutil/azure.go

+	for _, p := range doneParts.Parts {
+		idx := int(p.Ind)
+		if 0 <= idx && idx < totalChunks && !doneChunks[idx] {
+			doneChunks[idx] = true


I don't understand why you need an array. It makes sense to have an array if it is possible to have holes, but I don't see that (perhaps it is outside of this PR?).

Instead you can just store idx in a doneIdx and later do:

if chunkIndex <= doneIdx { // already downloaded in a previous run continue }

It is safer that way. We download 16 chunks concurrently so it might be the case that we have holes.

Okay, that makes sense to me.

christoph-zededa · 2025-10-24T11:25:53Z

zedUpload/datastore_http_test.go

+		})
+		ts = httptest.NewServer(r)
+
+		cleanup = func() {


I see you like lambdas, so your lambda returns a lambda :D

christoph-zededa · 2025-10-24T11:32:40Z

zedUpload/httputil/http.go

+		f, ferr := os.OpenFile(localFile, os.O_RDWR, 0644)
+		if ferr != nil {
+			stats.Error = ferr
+			return stats, Resp{}
+		}
+		local = f


Suggested change

f, ferr := os.OpenFile(localFile, os.O_RDWR, 0644)

if ferr != nil {

stats.Error = ferr

return stats, Resp{}

}

local = f

local, stats.Error = os.OpenFile(localFile, os.O_RDWR, 0644)

if stats.Error != nil {

return stats, Resp{}

}

christoph-zededa · 2025-10-24T11:41:07Z

zedUpload/httputil/http.go

+	if _, err := os.Stat(localFile); err == nil {
+		local, stats.Error = os.OpenFile(localFile, os.O_RDWR, 0644)
+		if stats.Error != nil {
+			return stats, Resp{}
+		}
+		if _, err = local.Seek(copiedSize, io.SeekStart); err != nil {
+			local.Close()
+			stats.Error = err
+			return stats, Resp{}
+		}
+	} else if os.IsNotExist(err) {
+		f, ferr := os.Create(localFile)
+		if ferr != nil {
+			stats.Error = ferr
+			return stats, Resp{}
+		}
+		local = f
+	} else {
+		stats.Error = err
 		return stats, Resp{}
 	}


Instead of first checking if the file exists and depending on that creating the file or open it, can't you just open it with os.O_CREATE (but without os.O_TRUNC)?

Suggested change

if _, err := os.Stat(localFile); err == nil {

local, stats.Error = os.OpenFile(localFile, os.O_RDWR, 0644)

if stats.Error != nil {

return stats, Resp{}

}

if _, err = local.Seek(copiedSize, io.SeekStart); err != nil {

local.Close()

stats.Error = err

return stats, Resp{}

}

} else if os.IsNotExist(err) {

f, ferr := os.Create(localFile)

if ferr != nil {

stats.Error = ferr

return stats, Resp{}

}

local = f

} else {

stats.Error = err

return stats, Resp{}

}

local, stats.Error = os.OpenFile(localFile, os.O_RDWR | os.O_CREATE, 0644)

if stats.Error != nil {

return stats, Resp{}

}

if _, err = local.Seek(copiedSize, io.SeekStart); err != nil {

local.Close()

stats.Error = err

return stats, Resp{}

}

christoph-zededa · 2025-10-24T11:42:32Z

zedUpload/datastore_http_test.go

 	}
 }

+func TestHTTPDatastoreResume(t *testing.T) {


Thank you for adding a test :-)

Signed-off-by: Ioannis Sfakianakis <[email protected]>

jsfakian · 2025-11-04T22:20:34Z

Should we merge it?

deitch · 2025-11-05T07:20:25Z

Should we merge it?

Definitely. I thought you would merge it after some approvals. I will do it now.

jsfakian requested review from deitch and rouming as code owners October 17, 2025 15:37

deitch requested changes Oct 19, 2025

View reviewed changes

jsfakian force-pushed the Support-for-http-datastores branch from 07c3322 to c0d4945 Compare October 21, 2025 13:08

jsfakian force-pushed the Support-for-http-datastores branch 2 times, most recently from af8ddeb to 325d4a9 Compare October 21, 2025 15:08

jsfakian force-pushed the Support-for-http-datastores branch from 325d4a9 to 624ec0f Compare October 24, 2025 11:03

jsfakian requested a review from milan-zededa as a code owner October 24, 2025 11:03

jsfakian force-pushed the Support-for-http-datastores branch from 624ec0f to c8666bd Compare October 24, 2025 11:05

Fixed bug in resumability for Azure datastores

5942ad2

Signed-off-by: Ioannis Sfakianakis <[email protected]>

christoph-zededa reviewed Oct 24, 2025

View reviewed changes

jsfakian force-pushed the Support-for-http-datastores branch from c8666bd to 94b2ddc Compare October 24, 2025 11:13

christoph-zededa reviewed Oct 24, 2025

View reviewed changes

jsfakian force-pushed the Support-for-http-datastores branch from 94b2ddc to 6b297fc Compare October 24, 2025 11:40

christoph-zededa reviewed Oct 24, 2025

View reviewed changes

zedUpload/datastore_http_test.go

}

}

func TestHTTPDatastoreResume(t *testing.T) {

Copy link

Contributor

christoph-zededa Oct 24, 2025

Choose a reason for hiding this comment

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

Thank you for adding a test :-)

Added resumability for http datastores

921ca13

Signed-off-by: Ioannis Sfakianakis <[email protected]>

jsfakian force-pushed the Support-for-http-datastores branch from 6b297fc to 921ca13 Compare October 24, 2025 12:11

jsfakian requested a review from deitch October 31, 2025 19:32

deitch approved these changes Nov 1, 2025

View reviewed changes

deitch merged commit 1c066d5 into lf-edge:main Nov 5, 2025
5 checks passed

Add resumable downloads for Azure & HTTP datastores #50

Add resumable downloads for Azure & HTTP datastores #50

Uh oh!

Conversation

jsfakian commented Oct 17, 2025

Uh oh!

deitch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsfakian commented Oct 21, 2025

Uh oh!

jsfakian commented Oct 21, 2025

Uh oh!

jsfakian commented Oct 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsfakian Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

jsfakian commented Nov 4, 2025

Uh oh!

deitch commented Nov 5, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jsfakian Oct 24, 2025 •

edited

Loading