Skip to content
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.

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

[WIP] Datatx initiate transfer #1759

Closed
wants to merge 4 commits into from

Conversation

redblom
Copy link
Contributor

@redblom redblom commented Jun 3, 2021

No description provided.

@redblom redblom requested a review from labkode as a code owner June 3, 2021 10:10
@update-docs
Copy link

update-docs bot commented Jun 3, 2021

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@lgtm-com
Copy link

lgtm-com bot commented Jun 3, 2021

This pull request introduces 1 alert when merging 3fff444 into ede4219 - view on LGTM.com

new alerts:

  • 1 for Useless assignment to local variable

@redblom redblom force-pushed the datatx-initiate-transfer branch from 3fff444 to dc0cdff Compare June 3, 2021 10:50
@lgtm-com
Copy link

lgtm-com bot commented Jun 3, 2021

This pull request introduces 1 alert when merging dc0cdff into ede4219 - view on LGTM.com

new alerts:

  • 1 for Useless assignment to local variable

@redblom redblom force-pushed the datatx-initiate-transfer branch from dc0cdff to 50f1719 Compare June 3, 2021 13:22
@redblom redblom changed the title [WIP] Create transfer share [WIP] Datatx initiate transfer Jun 3, 2021
@redblom redblom force-pushed the datatx-initiate-transfer branch from 50f1719 to 53593c4 Compare June 3, 2021 15:14
@redblom
Copy link
Contributor Author

redblom commented Jun 4, 2021

@ishank011 : Some issues:

  1. in case the src path is a file, gateway.ocmshareprovider.createOCMReference creates a ref with a mount path like /home/Data-Transfers/some-file.ext. But statting this file returns RESOURCE_TYPE_CONTAINER (it's always set to container). We would like to know the type so we can decide what rclone copy action to use (file or folder copy).
    In theory we could do a src path rclone stat in the driver, but I think we want to rely on the created dest reference. And then we also can do some path checking if this reference is available.
  2. I put the datatx driver call inside ocmshareprovider.ocmshareprovider.UpdateReceivedOCMShare but at that point the dest reference has not been created. So it looks to me this has to be rearranged a bit in gateway.ocmshareprovider.UpdateReceivedOCMShare. What do you think?

Copy link
Contributor

@ishank011 ishank011 left a comment

Choose a reason for hiding this comment

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

  1. in case the src path is a file, gateway.ocmshareprovider.createOCMReference creates a ref with a mount path like /home/Data-Transfers/some-file.ext. But statting this file returns RESOURCE_TYPE_CONTAINER (it's always set to container). We would like to know the type so we can decide what rclone copy action to use (file or folder copy).

We can get rid of the part where we create a reference in UpdateReceivedOCMShare and call the gateway method to start the transfer instead. The resource will be created according to the source then.

Driver string `mapstructure:"driver"`
Drivers map[string]map[string]interface{} `mapstructure:"drivers"`
DatatxDriver string `mapstructure:"datatxdriver"`
DatatxDrivers map[string]map[string]interface{} `mapstructure:"datatxdrivers"`
Copy link
Contributor

Choose a reason for hiding this comment

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

This is wrong. The CreateTransfer should be called in the gateway service

@ishank011
Copy link
Contributor

Wrong again. Gateway.UpdateOCMShare -> Gateway.CreateTransfer -> Datatx.CreateTransfer -> Rclone driver

@redblom redblom force-pushed the datatx-initiate-transfer branch 2 times, most recently from 60250dd to 977701e Compare June 7, 2021 13:35
@redblom
Copy link
Contributor Author

redblom commented Jun 7, 2021

Wrong again. Gateway.UpdateOCMShare -> Gateway.CreateTransfer -> Datatx.CreateTransfer -> Rclone driver

@ishank011 : Ha, I honestly thought we were going to leave datatx.CreateTransfer unused in the current implementation.

datatx.CreateTransfer is designed with the push model in mind. It requests the grantee but if we want to use it we would like to provide it with the grantor instead. Perhaps change the signature of this method then?

@ishank011
Copy link
Contributor

@redblom you're right, that would be needed. The only way we could move to the push model is when we have notifications in OCM, and I think that needs some discussion. So we can change it now and then come back to it later. Can you please create a PR?

@redblom
Copy link
Contributor Author

redblom commented Jun 8, 2021

@ishank011 so am I going to do it really practical for now, like eg. having the ReceivedShare as parameter?

datatx.CreateTransferRequest{
   ReceivedShare, 
   Opaque,
}

@ishank011
Copy link
Contributor

We don't need the received share, right? We just need to rename grantee to creator. The rest of the info is already available from the reference. The source token would be passed as an opaque object like it currently is.


if share.GetShare().ShareType == ocm.Share_SHARE_TYPE_TRANSFER {
srcRemote := share.GetShare().GetOwner().GetIdp()
// TODO do we actually know for sure the home path of the src reva instance?
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ishank011 : I need some help here, I can't figure out how to retrieve the target uri in datatx.CreateTransfer from the cs3.storage.provider.v1beta1.Reference. I know it's stored when de OCM reference is created but ...

@redblom
Copy link
Contributor Author

redblom commented Jun 10, 2021

@ishank011 : I'm trying to come up with a practical model. So bare with me on this. How about:

When pushing a transfer you're at the src end. You want to start a transfer of a resource (under your control) to another account(grantee). This is all you know and that fits the current datatx.CreateTransferRequest (as far as we can tell now). Because you do not have all the info to actually start the transfer more steps that involve the user@destination need to take place. This needs to be worked out, but it probably will involve OCM notifications.

We, currently, on the other hand are pulling a transfer from the destination end. I see this as follows:
You can't pull what you don't know. So, there is a transfer already waiting to be started (instigated by the src user). Currently we implement this as a transfer type share waiting to be accepted (implemented through the OCM share mechanism). This implies that everything you need to know to actually start the transfer is already there. Now all that needs to take place is the start of the actual transfer. I would suggest to model this as datatx.PullTransferRequest that takes a transfer ID so it can retrieve everything that's already there at destination to start the transfer. In the current implementation that would mean we retrieve the necessary info from the received share (transfer id IS the share id).

So:
Leave type datatx.CreateTransferRequest as is (at least for now). This will support implementing push transfers in the future.
Create a new type datatx.PullTransferRequest for pulling a transfer that has a transfer id field to allow retrieval at the destination of the necessary data from the awaiting transfer (share).

@redblom redblom force-pushed the datatx-initiate-transfer branch from 2c512b3 to 76b7c6d Compare June 14, 2021 09:27
@redblom redblom force-pushed the datatx-initiate-transfer branch from 76b7c6d to db441d2 Compare June 14, 2021 12:04
@ishank011
Copy link
Contributor

ishank011 commented Jun 14, 2021

@redblom all the information can be stored as a URL string which can be resolved by the datatx service. Syntactically, it works but I'm not sure about the semantics

var endpoint string
meshProvider, err := s.GetInfoByDomain(ctx, &ocmprovider.GetInfoByDomainRequest{
	Domain: share.Creator.Idp,
})
for _, s := range meshProvider.ProviderInfo.Services {
	if strings.ToLower(s.Endpoint.Type.Name) == "webdav" {
		endpoint = s.Endpoint.Path
		break
	}
}
targetURI := fmt.Sprintf("datatx://%s@%s?name=%s", token, endpoint, share.Name)
ref := &provider.Reference{Path: targetURI}
res, err := s. CreateTransfer(CreateTransfer{Ref: ref, Creator: share.Creator})

And in datatx.CreateTransfer

ep, err := extractEndpointInfo(ctx, req.Ref.GetPath())
srcToken := ep.token
dstToken := token.ContextMustGetToken(ctx)
srcPath := joinURL(ep.endpoint, ep.filePath)
dstPath := path.Join(s.c.DataTransfersFolder, ep.filePath)       // DataTransfersFolder would have to be defined in datatx

Maybe we can change ref to a string called targetURI. Thoughts?

@ishank011
Copy link
Contributor

ishank011 commented Jun 14, 2021

Create a new type datatx.PullTransferRequest for pulling a transfer

Agree with this, but resolving the share ID won't work because datatx methods won't be able to call the gateway ones. You'll still need to pass the URL path as the source.

transfer id IS the share id

I don't think this is the right way to associate OCM shares and transfers. They're separate entities so should have different IDs. If you want to store the share associated with the transfer, you can but they shouldn't have the same ID.

@redblom
Copy link
Contributor Author

redblom commented Jun 17, 2021

@ishank011 thanks for your comments.
I actually thought about using the targetURI(as you've suggested in #1759 (comment)) in datatx.PullTransfer as opposed to specifying all the fields (endpoints, tokens) in the request. And yes, if the parameters change we don't need to change the API request so that's practical.
Semantics ... I actually don't know if this uri syntax is semantically correct for this specific purpose, pulling a transfer. I'm going to study List_of_URI_schemes) for a little bit. If we could make it part of this list ... :)
But for now yes, let's do this.

So, that still leaves the issue of linking the share to the transfer. I though hard about this, many questions come up for each decision. We must answer the following question:
Currently the user, when accepting, only knows about the share ID.
Assuming we want different IDs: share ID for accepting, transfer ID for status/cancel requests **

  1. Should the user only know about the share ID or also about the transfer ID ?
  • If both, how does the user know about the transfer ID? Can the user retrieve it with a API call, eg. OCMShareListReceivedRequest? Or, is it returned with the UpdateReceivedOCMShareResponse?
  • If only share ID, GetTransferStatus/CancelTransfer can be requested using the share ID.

** I'm actually not convinced about shares and transfers being different entities. OCM is currently THE initiation mechanism for both shares and transfers and we are specifying transfers as being a specific type of share. If it was up to me :) I'd say therefore they are similar beasts in the CS3MESH4EOSC world and so share ID can be a transfer ID.

@redblom
Copy link
Contributor Author

redblom commented Jul 2, 2021

And in datatx.CreateTransfer

ep, err := extractEndpointInfo(ctx, req.Ref.GetPath())
srcToken := ep.token
dstToken := token.ContextMustGetToken(ctx)
srcPath := joinURL(ep.endpoint, ep.filePath)
dstPath := path.Join(s.c.DataTransfersFolder, ep.filePath)       // DataTransfersFolder would have to be defined in datatx

@ishank011 Your not suggesting this to be done in gateway.datatx.PullTransfer I hope? That would be really confusing to me ... I would think that this should be done in the datatx service.
I'm asking because extractEndpointInfo currently is only available in the gateway.

@redblom
Copy link
Contributor Author

redblom commented Oct 13, 2021

Superseded by #2052

@redblom redblom closed this Oct 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants