feat: add `WithMount` method to support cross repository blob mounting by ktarplee · Pull Request #632 · oras-project/oras-go

ktarplee · 2023-10-31T12:37:56Z

This includes everything from #631 but adds more (debatable) changes.

Closes #580

I realize I still need to add tests but I wanted to see if this approach is acceptable first.

codecov-commenter · 2023-10-31T22:31:39Z

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

Comparison is base (faaa1dd) 75.41% compared to head (4becd16) 75.36%.

Files	Patch %	Lines
copy.go	64.86%	9 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #632      +/-   ##
==========================================
- Coverage   75.41%   75.36%   -0.06%     
==========================================
  Files          59       59              
  Lines        5606     5647      +41     
==========================================
+ Hits         4228     4256      +28     
- Misses       1015     1024       +9     
- Partials      363      367       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ktarplee · 2023-11-03T09:42:44Z

@Wwwsylvia Any feedback on this approach?

Wwwsylvia · 2023-11-03T10:21:15Z

@ktarplee I'm busy with other stuffs these days, might take a closer look into this a bit later. Sorry for the inconvenience. 😟

Wwwsylvia

I think this approach works and looks concise. @shizhMSFT Do you have any concerns?

ktarplee · 2023-11-17T17:07:21Z

Why does registry.Repository not include registry.Mounter?

Wwwsylvia · 2023-11-23T03:45:49Z

Why does registry.Repository not include registry.Mounter?

registry.Mounter was introduced in v2.2.0 where breaking changes were not allowed.

oras-go/registry/repository.go

Lines 113 to 124 in f296072

    
           // Mounter allows cross-repository blob mounts. 
        
           // For backward compatibility reasons, this is not implemented by 
        
           // BlobStore: use a type assertion to check availability. 
        
           type Mounter interface { 
        
           	// Mount makes the blob with the given descriptor in fromRepo 
        
           	// available in the repository signified by the receiver. 
        
           	Mount(ctx context.Context, 
        
           		desc ocispec.Descriptor, 
        
           		fromRepo string, 
        
           		getContent func() (io.ReadCloser, error), 
        
           	) error 
        
           }

Wwwsylvia · 2023-11-23T03:46:45Z

I need some more time to review this and do some integration tests.

Wwwsylvia

I've tested this against ACR, zot and distribution.
The blobs can be mounted as expected in ACR and distribution, while they get skipped in zot. Looks like zot deduplicates blobs across repositories.

LGTM with a few nit suggestions.
BTW can we have the PR title start with "feat:"?

Wwwsylvia · 2023-11-27T09:45:02Z

@qweeah Can you also help to review this and check if it meets the needs of ORAS CLI?

Wwwsylvia

LGTM

Wwwsylvia · 2023-11-27T13:23:31Z

@shizhMSFT Please take a look when you have a chance.

shizhMSFT · 2023-11-27T13:26:09Z

@shizhMSFT Please take a look when you have a chance.

Please expect delayed review since I’m currently OOF.

qweeah · 2023-11-30T05:00:13Z

LGTM although IANAM

(Tried to implement an index creation command based on d5cc4c2 and it looks good, see )

shizhMSFT

@ktarplee Sorry for the late review as I was OOF.

shizhMSFT · 2023-12-07T12:28:06Z

+
+	opts := oras.CopyOptions{}
+	// Enable cross-repository blob mounting
+	opts.WithMount("source", dst.(registry.Mounter), nil)


What will happen if other target instead of dst is passed to WithMount?

Let's say the true destination target is dst but instead they pass in dst2. The blobs would be mounted or copied to dst2 but the manifest would be copied to dst and fail due to missing blobs.

Can we avoid that situation in the first place with a better API design?

shizhMSFT · 2023-12-07T12:30:09Z

+
+	opts := oras.CopyOptions{}
+	// Enable cross-repository blob mounting
+	opts.WithMount("source", dst.(registry.Mounter), nil)


What will happen if someone call WithMount twice?

This is a fun one. I think it would work and not produce an error (but I have not tried it). Let's call the preCopy functions preCopy1, preCopy2, preCopy3. The last one is the one created by the last call to WithMount. The first thing that would happen is the call to preCopy3 which would attempt a mount. Let's assume the mount failed, then getContent would get called (defined by preCopy3) and it would call preCopy2. preCopy2 was created by the first call to WithMount so it will try to mount the blob with that source repo. Let's assume it also fails, then getContent would call preCopy1. Then getContent (from preCopy2) would return errdef.ErrUnsupported (to be renamed) and the blob would be copied. Then getContent (from preCopy3) would return errdef.ErrUnsupported (to be renamed) and the blob would be copied again. So it seems like the blob would be copied twice with this design. Not fatal but not ideal either.

We could add a field to CopyOptions, something like tryingMount bool. In WithMount() we can check that tryingMount is false (else panic) and then set it to true. Then if the programmer makes the error to call WithMount twice they get a reasonable error.

shizhMSFT · 2023-12-07T12:33:05Z

+
+	tagName := "latest"
+
+	opts := oras.CopyOptions{}


What if someone reuse oras.CopyOptions?

The CopyOptions after a WithMount would only be usable when the same source and destination are not changed. That is not ideal. A shallow copy of the oras.CopyOptions before calling WithMount() solves this problem. That is how I use this in my code.

It would be ideal if we would reuse the CopyOptions. Currently, only PackOptions is not re-usable.

We could change the signature of the WithMount() to return a modified copy of the receiver. It would not modify the receiver so opts below could be reused. It would then be used like so:

desc, err := oras.Copy(ctx, src, tagName, dst, tagName, opts.WithMount("source", dst.(registry.Mounter), nil))

I think that would reduce the misuse and make it more obvious what it does.

Implements a WithMount method on CopyGraphOptions Also allows for getContent to return ErrUnsupported to fall back to default behavior. Signed-off-by: Kyle M. Tarplee <kmtarplee@ieee.org>

Signed-off-by: Kyle M. Tarplee <kmtarplee@ieee.org>

ktarplee · 2024-01-02T18:06:17Z

I have an idea to improve the API design of this and allow for trying to mount from multiple source locations. I will work on that very soon. The worst part about this design is that it is too limiting. For example, imagine someone is copying from reg1.example.com/repo/a to reg2.example.com/repo/b. In this case the blobs cannot be mounted from reg1.example.com since it is a different registry. However, maybe blob1 can be mounted from reg2.example.com/repo/c and blob2 can be mounted from reg2.example.com/repo/d. The WithMount(src, ...) approach is too limiting in the sense that it only allows for the common case of a single source repository for all blobs in the copy. It does not allow for the flexibility of having

blobs come from different source repos (common in the database case)
blobs coming from one of any number of repos (common in the database case)

I think some of the design issues highlighted above with the WithMount approach would go away if we went with an approach similar to this.

ktarplee · 2024-01-05T11:04:12Z

Closing this in favor of #665

Adds MountFrom and OnMounted to CopyGraphOptions. Allows for trying to mount from multiple repositories. Closes #580 I think this is a better approach than my other PR #632 Signed-off-by: Kyle M. Tarplee <kmtarplee@ieee.org>

ktarplee force-pushed the 580-mount-copy branch from 46b08cb to c1b4ce3 Compare October 31, 2023 22:29

ktarplee force-pushed the 580-mount-copy branch from c1b4ce3 to 59f6b8f Compare November 5, 2023 16:13

This was referenced Nov 7, 2023

Support cross-repository mounting in Copy #580

Closed

Rename ErrSkipDesc to SkipNode #634

Closed

Wwwsylvia reviewed Nov 17, 2023

View reviewed changes

Comment thread copy.go Outdated

ktarplee force-pushed the 580-mount-copy branch from 345e9d5 to 024ea12 Compare November 17, 2023 16:23

ktarplee commented Nov 17, 2023

View reviewed changes

Comment thread copy.go Outdated

ktarplee force-pushed the 580-mount-copy branch from 024ea12 to d26ffea Compare November 17, 2023 18:13

ktarplee marked this pull request as ready for review November 17, 2023 18:14

ktarplee requested review from SteveLasker, sajayantony and shizhMSFT as code owners November 17, 2023 18:14

ktarplee force-pushed the 580-mount-copy branch 2 times, most recently from 3a4bc30 to 45a1cb8 Compare November 22, 2023 11:21

ktarplee force-pushed the 580-mount-copy branch from 45a1cb8 to 583be41 Compare November 25, 2023 16:04

Wwwsylvia reviewed Nov 27, 2023

View reviewed changes

Comment thread copy_test.go Outdated

Comment thread registry/remote/repository_test.go Outdated

ktarplee force-pushed the 580-mount-copy branch from 583be41 to d5cc4c2 Compare November 27, 2023 12:56

ktarplee changed the title ~~Added a helper function to support cross repo blob mounting~~ feat: Added a helper function to support cross repo blob mounting Nov 27, 2023

ktarplee changed the title ~~feat: Added a helper function to support cross repo blob mounting~~ feat: Add WithMount method to support cross repository blob mounting Nov 27, 2023

Wwwsylvia approved these changes Nov 27, 2023

View reviewed changes

shizhMSFT changed the title ~~feat: Add WithMount method to support cross repository blob mounting~~ feat: add WithMount method to support cross repository blob mounting Dec 7, 2023

shizhMSFT reviewed Dec 7, 2023

View reviewed changes

ktarplee added 2 commits December 29, 2023 06:48

feat: added blob mounting support for oras Copy functions

6f0c1f0

Implements a WithMount method on CopyGraphOptions Also allows for getContent to return ErrUnsupported to fall back to default behavior. Signed-off-by: Kyle M. Tarplee <kmtarplee@ieee.org>

fix: added a custom error type for getContent's use

4becd16

Signed-off-by: Kyle M. Tarplee <kmtarplee@ieee.org>

ktarplee force-pushed the 580-mount-copy branch from bc912ca to 4becd16 Compare December 29, 2023 11:49

ktarplee mentioned this pull request Jan 3, 2024

feat: add MountFrom and OnMounted to CopyGraphOptions #665

Merged

ktarplee closed this Jan 5, 2024

Conversation

ktarplee commented Oct 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Oct 31, 2023 • edited by codecov Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ktarplee commented Nov 3, 2023

Uh oh!

Wwwsylvia commented Nov 3, 2023

Uh oh!

Wwwsylvia left a comment • 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!

Uh oh!

Uh oh!

ktarplee commented Nov 17, 2023

Uh oh!

Wwwsylvia commented Nov 23, 2023

Uh oh!

Wwwsylvia commented Nov 23, 2023

Uh oh!

Wwwsylvia left a comment • 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!

Uh oh!

Uh oh!

Wwwsylvia commented Nov 27, 2023

Uh oh!

Wwwsylvia left a comment

Choose a reason for hiding this comment

Uh oh!

Wwwsylvia commented Nov 27, 2023

Uh oh!

shizhMSFT commented Nov 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qweeah commented Nov 30, 2023

Uh oh!

shizhMSFT left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktarplee Dec 29, 2023 • 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!

ktarplee commented Jan 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ktarplee commented Jan 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

ktarplee commented Oct 31, 2023 •

edited

Loading

codecov-commenter commented Oct 31, 2023 •

edited by codecov Bot

Loading

Wwwsylvia left a comment •

edited

Loading

Wwwsylvia left a comment •

edited

Loading

shizhMSFT commented Nov 27, 2023 •

edited

Loading

ktarplee Dec 29, 2023 •

edited

Loading

ktarplee commented Jan 2, 2024 •

edited

Loading