-
Notifications
You must be signed in to change notification settings - Fork 112
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 driver #1242
[WIP] Datatx driver #1242
Conversation
@redblom good progress! |
ea6aaf6
to
fac695b
Compare
This now implements rclone driver DoTransfer, GetTransferStatus.
This will echo the rclone job id.
Get status eg.:
This will echo the transfer status. |
fac695b
to
7e1d7b1
Compare
All 3 methods (DoTransfer, GetTransferStatus, CancelTransfer) of rclone driver implemented. Use reva cli rclone transfer commands: (get transfer status) (cancel transfer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The driver is supposed to be included in the GRPC service, not invoked directly by clients. Please take a look at the other GRPC services and make sure that this also follows that approach https://github.com/cs3org/reva/blob/master/internal/grpc/services/userprovider/userprovider.go
@ishank011 yeah I know, I'm working on those right now. These rclone clients are for driver testing purposes only, I don't expect them to be part of the final PR. |
7e1d7b1
to
81ef606
Compare
81ef606
to
e12d56e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a few comments. I'm not sure I fully understand how the token situation works. In OCM we transfer a token when sharing a file which is used to access resources on the file owner's provider, not sure how that translates here and how it connects to that, since these will be enabled only once an OCM share has been created, right? (https://wiki.cs3mesh4eosc.eu/wps/4/task44stuff/apis)
pkg/datatx/loader/loader.go
Outdated
@@ -0,0 +1,25 @@ | |||
// Copyright 2018-2020 CERN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Create a folder called manager
and move the loader, registry and rclone folders inside it. Refer how we do it in other packages.
@@ -33,14 +39,18 @@ func init() { | |||
rgrpc.Register("datatx", New) | |||
} | |||
|
|||
type config struct { | |||
func (c *config) init() { | |||
// set sane defaults |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Set default value for Driver to rclone
// (token is username for now, it is used like that in the current rclone driver implementation) | ||
// TODO: re-implement when the connection-string-remote syntax is available in rclone | ||
// TODO: implement proper token use | ||
srcToken := u.GetUsername() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do these tokens work? For the source token, we use the username and for destination, we use the grantee's ID?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the destination token should be available here, returned by OCM share request. But this is not implemented yet, see below: #1242 (comment).
Apparently a source token for the 'local' source file to be transferred should (at this point) be created somehow (see https://wiki.cs3mesh4eosc.eu/wps/4/task44stuff/apis#data-transfer 'generate token to read Hugo's file'). Maybe you could shed some light on (how to do) that.
So yes, the driver.DoTransfer token parameters are being 'abused' here. We need actual tokens.
txID := req.TxId.OpaqueId | ||
|
||
// TODO find the jobID belonging to the transfer id; let transfer id be the jobID for now | ||
var jobID int64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to declare jobID before initializing
pkg/datatx/rclone/rclone.go
Outdated
} | ||
|
||
// rcloneCancelTransferReqJSON the rclone job/stop method json request | ||
type rcloneCancelTransferReqJSON struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use a generic struct for rcloneCancelTransferReqJSON and rcloneStatusReqJSON, since they have the same fields? (same for rcloneCancelTransferResJSON)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will declare them within the function there used in, that's actually what I wanted but thought it was invalid to declare a struct inside a method (my lack of golang knowledge).
With data transfer a special transfer OCM share request (which will be a to be defined special datatx OCM protocol type) is done towards the anticipated destination and at destination a token is created and returned so the token is available at source site for use when doing the actual transfers (transfers are instigated at the source site so it must have a valid token). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Persistency should be implemented in the same interface, there shouldn't be the need for a new one. For example, refer how we do it for OCM invites https://github.com/cs3org/reva/blob/master/pkg/ocm/invite/manager/json/json.go
When the (datatx) share is received in the ocmd we know it's a datatx share through the protocol options parameter. However we currently cannot persist it as such. Share objects have no tx specifics. So the receiver does not know it's a datatx share either. How do we go about that? |
Why can't we persist it? On the recipient side, the request would look like as described in https://wiki.cs3mesh4eosc.eu/wps/4/task44stuff/apis, so it would know it's a datatx share. |
The datatx transfer ends up as a 'marshalled' Share object in the received_shares section of the shares file at the receiver's end. What options do I have to define there that it is a datatx share; so that the receiver (and the system) when accepting this share know that after reading this share from the shares file that this is a datatx share and the transfer can commence? In other words, in the shares file, how does a datatx share differ from a regular share? |
Add one more parameter to the schema? |
That is one solution I was also thinking of. So simplest:
Or maybe |
Sure, |
How about using the share ID instead of transfer ID in cs3 datatx api. The share ID is already there. That would simplify things. |
@ishank011 : I believe pkg.ocm.share manager interface method Save() needs an additional share type arg. |
@redblom sure, feel free to make changes as you need. |
@ishank011 please review #1578 |
@ishank011 : implementing this (#1578) I'm thinking we should not use
I think it makes more sense. Then we change the Now when
TODO: the transfer driver needs a destination path. In case this is user defined it should be specified in the |
When For WebDAV share references, we have the target These references are statted in the gateway storageprovider service https://github.com/cs3org/reva/blob/master/internal/grpc/services/gateway/storageprovider.go#L1369-L1395. We'll need to add a case for transfer refs to just be returned as it is. We have all the existing functionalities that will allow users to make changes to any files/folders located inside this reference. |
@ishank011 I'd like to see this merged first (create transfer type OCM share) #1578 |
@ishank011 What would be the resulting location here, eg. |
You're passing the |
@ishank011 I use single endpoints for each request. For transfer the rclone service is called through 2 possible endpoints, I experimented with accepted shares where I gave rclone the share path like |
Which service handles the requests at the |
@ishank011 I'm also not sure what should be returned here for the transfer case (https://github.com/cs3org/reva/blob/master/internal/grpc/services/gateway/storageprovider.go#L1369-L1395) exactly? Simply the provided resourceInfo? How should we establish here that this is a transfer ref? |
Use the path
Yes
Check the scheme |
Yes, that seems to work!
And that should read |
@ishank011 : I can set the scheme to
Then I can determine it at https://github.com/cs3org/reva/blob/master/internal/grpc/services/gateway/storageprovider.go#L1369-L1395 for the new case. Is that correct? |
This PR is superseded by #2052 |
See sciencemesh/sciencemesh#140
Datatx driver pkg setup.
note: Datatx interface methods' signatures still indefinite.