-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Add command to remove obsolete storages from the filecache #40779
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
Conversation
I don't think I can add unit tests... mocking the connection is complex. The other option is to access to the DB, but we need a way to fill the data, which means we need to use additional components. |
This is imho a very bad idea...
We could directly add the description when the command is final to this PR making it available immediately. I can support, but coding should be finished. |
We can provide a way to list the potential unused storages, but there could be storages wrongly marked as unused (some extra info in https://github.com/owncloud/enterprise/issues/5630#issuecomment-1540375335). Fully automating the process seems too risky, so I think it's better to delegate the final decision to the admin. We also need to decide which relevant info we want to show to the admin. The numeric id of the storage won't tell anything, and the storage id could be hashed. I don't find any relevant info we could use to help the admin to decide which storage he wants to delete. |
@jvillafanez If ok to you:
|
Yes, feel free to adjust the description.
As a recommendation it's a good thing to have. I mean, assuming you're targeting the right storage, single user mode shouldn't be needed because nobody should be using that storage; the problem is if you target a wrong storage. |
Still need to test with postgresql and oracle. Hopefully no changes are needed |
SonarCloud Quality Gate failed. |
0d3dc97
to
2a480bf
Compare
2a480bf
to
407d807
Compare
@jvillafanez Mind to post an example of |
The |
|
That's part of the id of the storage. It could be nicer, but it is what it is. I'll adjust the headers |
I'm having problems to connect to an oracle DB, so the PR is currently untested with oracle. Worst case, the command won't work. I don't expect bigger issues. |
Tested with oracle, works now. Columns names are changed now as requested:
|
@phil-davis @pako81 mind to support with a review, I cant as my oc server dev env is down atm. |
The code looks OK to me.
Then I did:
My real data dir is shown in the list. That makes me a bit nervous - I don't want to remove the real data dir that has users and some file. Is that "normal" that this command can show the real local data dir? I didn't try removing |
I agree the data directory shouldn't appear, but I don't think we can easily exclude it. There is no user mounting that (and there shouldn't be), so that's why it appears.
There is no safety net. For whatever reason, there is a view with the data directory as root, and that's why it appears in the filecache. We probably should use a custom access to the data directory so the data doesn't end up in the filecache. It seems too late to do that. Worst case, I think only the data in the filecache will be removed. The actual files in the FS will remain. I expect the data to reappear at some point. In addition, I think it usually has few files, so removing it isn't probably worthy. |
I tried removing it:
Then 10 seconds later:
And I can still login as the users that are on my dev system, and their files still exist. I am not sure why |
Note that the corresponding documentation PR contains the following text as IMPORTANT tag:
I think that anyone using that command got notified, reading the docs first of course. |
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.
LGTM. Manually tested (see comments in the PR).
CI is green except sonarcloud - how to proceed? (force merge?) |
SonarCloud is not a "required" result. We can ignore that, since we agree that unit test coverage is difficult for this command. It's fine to merge from my PoV. Who else needs to review and agree to this? |
Going to review this and start manual testing in a while. |
Manual tests seem to be ok 👍 Tested with both auth methods: One small remark: the entry related to the user creating the mountpoint over the Web UI (
Not a big issue I would say as there are anyway no orphaned |
Let's merge this? |
I suppose this will be released in the normal flow of 10.13 release. |
correct. |
Using it does not do a next line:
|
But I cleaned a lot:
|
Took about 15 minutes to clean all of them, great! |
@IljaN This is what you asked today. this is a good question: why should so many entries in the main storage be deleted? @jvillafanez any clue? |
I think any file accessed through the ownCloud's You can remove that storage, but it will eventually grow to the same number. We'll have to replace the |
If I run: I can see storages that are still there but empty:
weird this one:
|
@mmattel we should also tell in the documentation, after running the command we have to run:
or better:
@jvillafanez @pako81 @phil-davis what do you think? |
Is the table usually big enough that "optimize" makes much performance difference |
For any command that should run, I am not in favour of database sql commands or mysql versions of it. This should be managed by the occ command internally. Maybe to extend that one. |
Addon, what to do when NOT running mysql/mariadb ? --> extend the internal command for the functionality. I ran
I am very reluctant to show these details to customers... |
If anything, I'd say something generic like "if you've deleted a huge amount of rows, check your DB to optimize / compact the table to improve performance". I assume that if they delete millions of rows and then the DB doesn't improve the performance automatically, they should check the DB manual to know what they can do or what is the problem |
We should stay generic as @jvillafanez described and not recommend specific commands. In MariaDB for example the index size stays the same after a lot of deletions but the empty space is reserved for subsequent inserts. |
I think in the case shares are created for files located on those mountpoints, upon removing the mount point configuration from the web UI, the candidates are not properly found by the command as entries in oc_mounts pointing to that specific storage_id will exist for the shares' receivers. So the condition |
Description
Provide an occ command to remove obsolete storages from the DB. The command is limited to the
oc_storages
andoc_filecache
tables.The recommended action plan is:
oc_storages
table and check which storages aren't used any longer.occ files:remove-storage
command with those storages.In case the storage is still in use, a new storage id is likely to be assigned, and you'll need to rescan the storage. This also means that the clients will think the files are new and they'll download the files.
Anyway, the files won't be touched in the backend.
Related Issue
https://github.com/owncloud/enterprise/issues/5630
Motivation and Context
This will help to keep the DB more tidy by removing entries that won't be needed any longer. It will also help with the performance because the dataset will be reduced
How Has This Been Tested?
Manually tested
occ files:scan
to bring the data into the DBfiles:remove-storage
command with the target storage idScreenshots (if appropriate):
Types of changes
Checklist:
@mmattel for documenting the command when it's merged