Add context to storagedriver.(Filewriter).Commit() #4109

milosgajdos · 2023-10-18T09:39:24Z

This PR changes storagedriver.Filewriter interface by adding context.Context as an argument to the Commit interface func.

We pass the context appropriately where need be throughout the distribution codebase to all the writers and tests.

In the case of the S3 storage driver we pass the context down to the driver writer and update all the AWS SDK calls in the writer with their WithContext variant. See this PR for more context (pun intended): #4066

PTAL @corhere, would love to get your thoughts/ideas on this.

Jamstah · 2023-10-18T11:58:48Z

registry/storage/driver/s3-aws/s3.go

@@ -1338,6 +1338,7 @@ func (b *buffer) Clear() {
 // cleanly resumed in the future. This is violated if Close is called after less
 // than a full chunk is written.
 type writer struct {
+	ctx       context.Context


https://go.dev/blog/context-and-structs

Yes, I know this is sad. Please read the whole thread in the discussion linked in the PR description. It's also why this PR is marked as a Draft. @corhere and I wanted to chat some things through and broken code is the best place to discuss them 😄

That doesn't really explain it for me. Why have the context in the object instead of adding a ctx param to every method of writer that makes an s3 call?

Because writer is created in S3 driver dynamically in the (driver).Writer() call

distribution/registry/storage/driver/s3-aws/s3.go

Line 699 in d242437

return d.newWriter(key, *resp.UploadId, nil), nil

distribution/registry/storage/driver/s3-aws/s3.go

Line 746 in d242437

return d.newWriter(key, *multi.UploadId, allParts), nil

Now, it ten calls a bunch of AWS SDK funcs (that fire API reqs to AWS) that need to have context passed down to them from the upstream consumer so they can be called etc,.. The biggest problem is the Write method that must implement io.Writer:

distribution/registry/storage/driver/s3-aws/s3.go

Line 1406 in d242437

if _, aErr := w.driver.S3.AbortMultipartUpload(&s3.AbortMultipartUploadInput{

And then there is also flush which also needs the upstream context because it makes upload finalising API calls and is called from the above-mentioned Write and from Close (which is there to implement io.Closer IIRC).

How else do imagine this should work?

I don't have a better solution, but I will need to read a little more code before I understand it enough to review confidently :)

SGTM. I hope you don't get too horrified by all that code 😁 No rush on this one

Had a look, apologies for just pointing at generic doc instead of reading properly the first time.

I guess this is similar to solutions like https://pkg.go.dev/github.com/dolmen-go/contextio

One thing that could be a nice addition is a test that starts a writer and then cancels it to make sure we get back a context cancellation error, but that's not blocking for me.

registry/storage/driver/s3-aws/s3.go

corhere

Since v3.0.0 has not been tagged, we currently have a rare opportunity to make breaking API changes. While there is nothing we can do about the standard library io.WriteCloser interface, the driver.FileWriter interface is under our control. Now's our chance to add a context argument to (FileWriter).Commit()!

distribution/registry/storage/driver/storagedriver.go

Lines 123 to 126 in 915ad2d

    
           // Commit flushes all content written to this FileWriter and makes it 
        
           // available for future calls to StorageDriver.GetContent and 
        
           // StorageDriver.Reader. 
        
           Commit() error

I have spotted other candidate interfaces to be made more context-aware. But to avoid derailing this PR or expanding its scope too much, I have opened #4110 to host that discussion.

registry/storage/driver/s3-aws/s3.go

corhere · 2023-10-18T15:31:53Z

registry/storage/driver/s3-aws/s3.go

@@ -1536,7 +1537,7 @@ func (w *writer) Cancel(ctx context.Context) error {
 		return fmt.Errorf("already committed")
 	}
 	w.cancelled = true


Aside: does this (and the other flags) need to be be changed to atomics—in its own PR, of course—so Cancel() is safe to be called concurrently with Write() and friends?

registry/storage/driver/s3-aws/s3.go

milosgajdos · 2023-10-18T17:28:34Z

This PR has got a bit chunkier. Thanks @corhere 🤣

I should probably rephrase the PR message now that we've taken a bit of a turn.

corhere

LGTM!

corhere · 2023-10-18T18:19:06Z

registry/storage/driver/gcs/gcs.go

 	if err := w.checkClosed(); err != nil {
 		return err
 	}
 	w.closed = true
-	ctx := context.TODO()


It's always nice to see TODOs melt away.

Jamstah · 2023-10-19T08:47:20Z

registry/storage/driver/s3-aws/s3.go

@@ -1338,6 +1338,7 @@ func (b *buffer) Clear() {
 // cleanly resumed in the future. This is violated if Close is called after less
 // than a full chunk is written.
 type writer struct {
+	ctx       context.Context


Had a look, apologies for just pointing at generic doc instead of reading properly the first time.

I guess this is similar to solutions like https://pkg.go.dev/github.com/dolmen-go/contextio

One thing that could be a nice addition is a test that starts a writer and then cancels it to make sure we get back a context cancellation error, but that's not blocking for me.

milosgajdos · 2023-10-19T09:16:45Z

I will squash the commits before merging if that's ok with you @Jamstah

There are some "broken" commits in this PR so would be good to squash them to a single one.

Jamstah · 2023-10-19T09:38:56Z

Yes of course, squash away. And use a conventional commit ;)

This commit changes storagedriver.Filewriter interface by adding context.Context as an argument to its Commit func. We pass the context appropriately where need be throughout the distribution codebase to all the writers and tests. S3 driver writer unfortunately must maintain the context passed down to it from upstream so it contnues to implement io.Writer and io.Closer interfaces which do not allow accepting the context in any of their funcs. Co-authored-by: Cory Snider <corhere@gmail.com> Signed-off-by: Milos Gajdos <milosthegajdos@gmail.com>

milosgajdos added the area/storage/s3 label Oct 18, 2023

milosgajdos requested a review from corhere October 18, 2023 09:39

Jamstah reviewed Oct 18, 2023

View reviewed changes

registry/storage/driver/s3-aws/s3.go Show resolved Hide resolved

milosgajdos changed the title ~~Pass the S3 storage driver context its writer~~ Pass the S3 storage driver context to its writer Oct 18, 2023

corhere mentioned this pull request Oct 18, 2023

Make more interfaces context-aware #4110

Closed

4 tasks

corhere requested changes Oct 18, 2023

View reviewed changes

milosgajdos requested review from corhere and Jamstah October 18, 2023 17:32

milosgajdos changed the title ~~Pass the S3 storage driver context to its writer~~ Add context to storagedriver.(Filewriter).Commit() Oct 18, 2023

milosgajdos added refactor area/storage and removed area/storage/s3 labels Oct 18, 2023

milosgajdos marked this pull request as ready for review October 18, 2023 18:07

milosgajdos requested a review from thaJeztah October 18, 2023 18:08

corhere approved these changes Oct 18, 2023

View reviewed changes

Jamstah approved these changes Oct 19, 2023

View reviewed changes

milosgajdos force-pushed the writer-context branch from d925665 to 3d30611 Compare October 19, 2023 10:26

milosgajdos force-pushed the writer-context branch from 3d30611 to cb0d083 Compare October 19, 2023 10:28

milosgajdos merged commit 5aee8e1 into distribution:main Oct 19, 2023
12 checks passed

milosgajdos deleted the writer-context branch October 19, 2023 10:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add context to storagedriver.(Filewriter).Commit() #4109

Add context to storagedriver.(Filewriter).Commit() #4109

milosgajdos commented Oct 18, 2023 •

edited

Jamstah Oct 18, 2023

milosgajdos Oct 18, 2023 •

edited

Jamstah Oct 18, 2023

milosgajdos Oct 18, 2023 •

edited

Jamstah Oct 18, 2023

milosgajdos Oct 18, 2023

Jamstah Oct 19, 2023

corhere left a comment

corhere Oct 18, 2023

milosgajdos commented Oct 18, 2023 •

edited

corhere left a comment

corhere Oct 18, 2023

Jamstah Oct 19, 2023

milosgajdos commented Oct 19, 2023

Jamstah commented Oct 19, 2023

	// Commit flushes all content written to this FileWriter and makes it
	// available for future calls to StorageDriver.GetContent and
	// StorageDriver.Reader.
	Commit() error

Add context to storagedriver.(Filewriter).Commit() #4109

Add context to storagedriver.(Filewriter).Commit() #4109

Conversation

milosgajdos commented Oct 18, 2023 • edited

Choose a reason for hiding this comment

milosgajdos Oct 18, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

milosgajdos Oct 18, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corhere left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

milosgajdos commented Oct 18, 2023 • edited

corhere left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

milosgajdos commented Oct 19, 2023

Jamstah commented Oct 19, 2023

milosgajdos commented Oct 18, 2023 •

edited

milosgajdos Oct 18, 2023 •

edited

milosgajdos Oct 18, 2023 •

edited

milosgajdos commented Oct 18, 2023 •

edited