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

add delete home to the storage provider #95

Merged
merged 2 commits into from
Feb 9, 2021

Conversation

C0rby
Copy link
Contributor

@C0rby C0rby commented Nov 4, 2020

Added a delete home method to the storage provider to be able to clean up when removing a user.

@labkode
Copy link
Member

labkode commented Nov 5, 2020

This API call should be part of an Admin API, we have discussed it in the past and now with this need arising is the right moment to create it. @C0rby can you add this new API here: cs3/admin/v1beta1/admin_api.proto with that method?
Note that the method must accept a user ID as argument, this API will be used by site administrators or by robot accounts to automate actions.

@C0rby
Copy link
Contributor Author

C0rby commented Nov 5, 2020

Cool.
Just for clarification, should I move the method to the admin_api or copy it to have it in both places?

@refs
Copy link
Member

refs commented Nov 5, 2020

This API call should be part of an Admin API, we have discussed it in the past and now with this need arising is the right moment to create it. @C0rby can you add this new API here: cs3/admin/v1beta1/admin_api.proto with that method?
Note that the method must accept a user ID as argument, this API will be used by site administrators or by robot accounts to automate actions.

thanks for the clarification, I meant userid, not an entire user 👯

@C0rby C0rby force-pushed the storageprovider-deletehome branch from b9c5fb8 to f671ad5 Compare November 5, 2020 10:26
@C0rby C0rby force-pushed the storageprovider-deletehome branch from f671ad5 to 49b3aed Compare November 17, 2020 11:48
@C0rby C0rby requested review from butonic and refs November 17, 2020 12:03
Copy link
Member

@labkode labkode left a comment

Choose a reason for hiding this comment

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

@C0rby I've added some comments, overall looks pretty good.

@C0rby C0rby force-pushed the storageprovider-deletehome branch 2 times, most recently from 4475d7d to f8e2003 Compare November 26, 2020 10:14
@C0rby C0rby requested a review from labkode November 27, 2020 08:48
@labkode
Copy link
Member

labkode commented Nov 30, 2020

@C0rby @refs any update on this? Will be nice to merge it in.

@C0rby C0rby force-pushed the storageprovider-deletehome branch from f8e2003 to 77d64c9 Compare November 30, 2020 15:22
@C0rby
Copy link
Contributor Author

C0rby commented Nov 30, 2020

I rebased the branch.
From my POV we can merge, right @butonic?

@kulmann
Copy link
Member

kulmann commented Dec 24, 2020

@C0rby could you rebase please? Would be nice to get this in. :-)

@phil-davis
Copy link

@C0rby are you online this week? Can you rebase this and we can try to push it along. There are things that depend on this that are waiting.

@phil-davis
Copy link

@labkode @butonic @refs looks like time to review.

There is no response from drone CI - but that will be related to the drone troubles over Christmas...

@C0rby
Copy link
Contributor Author

C0rby commented Jan 19, 2021

Well, new year new luck.

Can we merge this? @refs, @butonic, @labkode

@refs
Copy link
Member

refs commented Jan 19, 2021

@C0rby it LGTM, could we get @ishank011 to have another pair of eyes? :)

@ishank011
Copy link
Contributor

@C0rby looks good but how will ProvisionStorage be called? I'm guessing from the ProvisionUser method in the admin API? How will we supply the quota parameters to it then?

@C0rby
Copy link
Contributor Author

C0rby commented Jan 25, 2021

I see your point.
@butonic, what do you say? Add the quota to the ProvisionUser api?

@butonic
Copy link
Contributor

butonic commented Feb 1, 2021

As discussed at the CS3 we should pick a more generic name for CreateHome. @ishank011 proposed CreateStorageSpace which captures the difference between creating a storage vs creating something inside a storage. Something being called a space.

Other options were CreateTree, CreateSpace, CreateSubTree, and CreateRoot but they are either too abstract / graph theory influenced (anything with tree or root) or would invent a new term (space).

Storage space seems to be a good middle ground. Furthermore it can be used not only for user homes but also other spaces eg. project or group spaces.

The method needs an owner (which can be a user or a group), optionally quota and opaque data as usual.

cc @ishank011 @C0rby

@C0rby
Copy link
Contributor Author

C0rby commented Feb 3, 2021

@jfd, wouldn't CreateStorageSpace also need a path parameter or something like that?

@butonic
Copy link
Contributor

butonic commented Feb 3, 2021

CreateStorageSpace where?

In the gateway? there it would need an indication of which storage should create the space. might be done by path or by storageid.

In a storage provider? No, It is up to the storage provider to determine where to put the new root of the storeage.

In the storage registry? Yes, it needs to know where to mount a storage space. Not only a strorage provitder. But this is a bigger change.

For now, I would limit this change to the StorageProvider Api in the context of the UserProvisioning api: the Gateway can determine the responsible storage provider by calling storageregistry->GetHome() and then using a storageregistry->GetProvider(<home>). It can then send the CreateStorageSpace(<owner>, <quota>) to it, just as it is currently sending a CreateHome()

@butonic
Copy link
Contributor

butonic commented Feb 3, 2021

@C0rby While you are at it could you also add an UpdateStorageSpace so we can adjust the quota? We need that for the quota related tests.

@butonic
Copy link
Contributor

butonic commented Feb 3, 2021

blocks owncloud/ocis#1290
blocks owncloud/ocis#1313
blocks owncloud/product#247

The storage space api should make it possible to provision and deprovision spaces ('home', 'shares', 'project' etc...) for users. Using this api we should also be able to deprovision spacess

Co-authored-by: Jörn Friedrich Dreyer <[email protected]>
@C0rby C0rby force-pushed the storageprovider-deletehome branch from 07ef96d to 064a93b Compare February 4, 2021 12:05
@C0rby
Copy link
Contributor Author

C0rby commented Feb 4, 2021

@ishank011 @labkode, @butonic and I now implemented the storage API how we think it should be.
I removed the admin API again and only added the storage space stuff.

@ishank011 ishank011 merged commit 29454ca into cs3org:master Feb 9, 2021
@C0rby C0rby deleted the storageprovider-deletehome branch February 9, 2021 09:23
@phil-davis
Copy link

@C0rby is there a PR to cs3org/reva that implements this?

@C0rby
Copy link
Contributor Author

C0rby commented Feb 15, 2021

@phil-davis, not yet. We still have to do that. It will most likely happen within the coming sprint.

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.

7 participants