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

[stable25] Quota value as float for 32-bit systems #35734

Merged
merged 4 commits into from
Jan 2, 2023

Conversation

PVince81
Copy link
Member

Summary

TODO

Checklist

@PVince81
Copy link
Member Author

/backport to stable24

@szaimen
Copy link
Contributor

szaimen commented Dec 12, 2022

@PVince81 I am under the impression that this error does again only happen on 32-bit due to the php int limit. Are you sure that this does not break on 64-bit?

@emc02
Copy link

emc02 commented Dec 12, 2022

same problem here...
"message":"Typed property OC\Files\Storage\Wrapper\Quota::$quota must be int or null, float used in file '/var/www/html/nextcloud/lib/private/Files/Storage/Wrapper/Quota.php

Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

I’m afraid converting int to float on 64bits may have side effects.

We should just remove the type hint and only use a phpdoc type hint of int|float, with an explanation that it may be float on 32bits.

@szaimen
Copy link
Contributor

szaimen commented Dec 12, 2022

We should just remove the type hint and only use a phpdoc type hint of int|float, with an explanation that it may be float on 32bits.

Sounds good to me 👍

@gfggfgfsg

This comment was marked as off-topic.

@emc02
Copy link

emc02 commented Dec 12, 2022

What lines should be changed? I could test it on my 32bit raspberry system.

@PVince81 PVince81 force-pushed the stable25-quota-as-float-for-32bit-folks branch from e4e27b2 to 885c4ab Compare December 13, 2022 17:15
@PVince81
Copy link
Member Author

PVince81 commented Dec 13, 2022

@homeessentials1 @emc02

cd /path/to/nextcloud
curl -L https://github.com/nextcloud/server/pull/35734.patch -o 35734-quota-32bit-fix.patch
patch -p1 < 35734-quota-32bit-fix.patch

@PVince81
Copy link
Member Author

can someone else take over, possibly someone with access to a 32-bit system ?
I don't have time to dig deeper and try every combination.
We need a solution that works both for 32-bit and 64-bit systems, for positive and negative quota values (negative values are for special cases like "unlimied")

@PVince81 PVince81 removed their assignment Dec 15, 2022
@emc02
Copy link

emc02 commented Dec 15, 2022

can someone else take over,

I am sorry, I have not the experience to do that, but I can test various possible solutions if wanted and report back

@PVince81
Copy link
Member Author

it looks like so far there were three solutions:

  • no type: causing warnings ?
  • float: breaking negative values ?
  • int: broke with 32-bits

maybe more changes are needed

@come-nc can you take over maybe ?

@come-nc
Copy link
Contributor

come-nc commented Dec 15, 2022

@come-nc can you take over maybe ?

Next week if I have the time, I will.
I think we should setup a 32bit CI for stable25.

@come-nc come-nc force-pushed the stable25-quota-as-float-for-32bit-folks branch from ffc0e7c to 442cc3c Compare December 19, 2022 13:46
PVince81 and others added 2 commits December 19, 2022 14:47
Signed-off-by: Vincent Petry <vincent@nextcloud.com>
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@MichaIng
Copy link
Member

Many thanks. Those two failing tests at least are exactly about what we experience here. Probably starting to investigate from what the tests do is a better approach to find the faulty int cast/declaration.

@j-be
Copy link

j-be commented Mar 19, 2023

@MichaIng My pleasure. I was in the meantime able to figure out how to run stable26 and master with Debian Unstable (PHP8.2) and on first glance it looks like both i386 and amd64 behave more or less the same. There are no additional errors on i386 and all failures there happen in amd64, too. 🤔 Not sure what to make of this.

Full run logs should be available at https://j-be.github.io/nextcloud-ci/ by the time you are reading this, but it seems to me that GitHub Pages does heavy caching. So consider accessing it with cache disabled, or Ctrl+F5, or something like that.

@MichaIng
Copy link
Member

MichaIng commented Mar 22, 2023

I just upgraded to stable NC26. Indeed something has changed. I can now assign the quota and it shows up correctly in GUI. It was also assigned correctly to the database:

$ mysql -e "select * from nextcloud.oc_preferences where configkey = 'quota';"
+----------+-------+-----------+-------------+
| userid   | appid | configkey | configvalue |
+----------+-------+-----------+-------------+
| USERNAME | files | quota     | 100 GB      |
+----------+-------+-----------+-------------+

However, when I try to use the files app with that user, I get a 5xx error and:

{"reqId":"5nw7l5SMC4dbmgQ7kYOT","level":3,"time":"2023-03-22T13:01:32+01:00","remoteAddr":"192.168.1.1","user":"USERNAME","app":"index","method":"GET","url":"/nextcloud/apps/files/","message":"Cannot assign float to property OC\\Files\\Storage\\Wrapper\\Quota::$quota of type ?int in file '/var/www/nextcloud/lib/private/Files/Storage/Wrapper/Quota.php' line 68","userAgent":"Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/111.0.0.0 Safari/537.36 OPR/98.0.0.0 (Edition developer)","version":"26.0.0.11","exception":{"Exception":"Exception","Message":"Cannot assign float to property OC\\Files\\Storage\\Wrapper\\Quota::$quota of type ?int in file '/var/www/nextcloud/lib/private/Files/Storage/Wrapper/Quota.php' line 68","Code":0,"Trace":[{"file":"/var/www/nextcloud/lib/private/AppFramework/App.php","line":183,"function":"dispatch","class":"OC\\AppFramework\\Http\\Dispatcher","type":"->"},{"file":"/var/www/nextcloud/lib/private/Route/Router.php","line":315,"function":"main","class":"OC\\AppFramework\\App","type":"::"},{"file":"/var/www/nextcloud/lib/base.php","line":1055,"function":"match","class":"OC\\Route\\Router","type":"->"},{"file":"/var/www/nextcloud/index.php","line":36,"function":"handleRequest","class":"OC","type":"::"}],"File":"/var/www/nextcloud/lib/private/AppFramework/Http/Dispatcher.php","Line":169,"Previous":{"Exception":"TypeError","Message":"Cannot assign float to property OC\\Files\\Storage\\Wrapper\\Quota::$quota of type ?int","Code":0,"Trace":[{"function":"getQuota","class":"OC\\Files\\Storage\\Wrapper\\Quota","type":"->"},{"file":"/var/www/nextcloud/lib/private/Files/Storage/Wrapper/Wrapper.php","line":524,"function":"call_user_func_array"},{"function":"__call","class":"OC\\Files\\Storage\\Wrapper\\Wrapper","type":"->"},{"file":"/var/www/nextcloud/lib/private/Files/Storage/Wrapper/Wrapper.php","line":524,"function":"call_user_func_array"},{"file":"/var/www/nextcloud/lib/private/legacy/OC_Helper.php","line":522,"function":"__call","class":"OC\\Files\\Storage\\Wrapper\\Wrapper","type":"->"},{"file":"/var/www/nextcloud/apps/files/lib/Controller/ViewController.php","line":142,"function":"getStorageInfo","class":"OC_Helper","type":"::"},{"file":"/var/www/nextcloud/apps/files/lib/Controller/ViewController.php","line":243,"function":"getStorageInfo","class":"OCA\\Files\\Controller\\ViewController","type":"->"},{"file":"/var/www/nextcloud/lib/private/AppFramework/Http/Dispatcher.php","line":230,"function":"index","class":"OCA\\Files\\Controller\\ViewController","type":"->"},{"file":"/var/www/nextcloud/lib/private/AppFramework/Http/Dispatcher.php","line":137,"function":"executeController","class":"OC\\AppFramework\\Http\\Dispatcher","type":"->"},{"file":"/var/www/nextcloud/lib/private/AppFramework/App.php","line":183,"function":"dispatch","class":"OC\\AppFramework\\Http\\Dispatcher","type":"->"},{"file":"/var/www/nextcloud/lib/private/Route/Router.php","line":315,"function":"main","class":"OC\\AppFramework\\App","type":"::"},{"file":"/var/www/nextcloud/lib/base.php","line":1055,"function":"match","class":"OC\\Route\\Router","type":"->"},{"file":"/var/www/nextcloud/index.php","line":36,"function":"handleRequest","class":"OC","type":"::"}],"File":"/var/www/nextcloud/lib/private/Files/Storage/Wrapper/Quota.php","Line":68},"CustomMessage":"--"}}

On stable24 and stable25 I see commits which fixed this (actually the ones from this PR), but never made it into master, it seems:

This is the last commit on master, which broke it: bd91c56#diff-b89bc6bd7d5336d68032bb5a5c1f309df16f27173f9f58d9f338ec495d3f951b

Still does not explain why there are issues with stable25 which are not there with stable24 and even some solved with master (assigning the quota). But step by step.

@MichaIng
Copy link
Member

/backport to master

@MichaIng
Copy link
Member

MichaIng commented Mar 22, 2023

I forward-ported this PR here: #37344
With the patch applied onto my stable NC26 instance, quota works as expected, can be assigned, read from database, is correctly applied for users etc. Not sure what changed from NC25 to NC26, but something has fixed it in the meantime, probably another int=>float solved somewhere in the depth of what gets called when dealing with quota.
And it should solve the to new failing tests on master/stable26

About the failing tests on stable25/24:

  1. Solved in master with 94ecae4. I've actually no idea why it converts 2147483648 int into 2147483648.0 float, as it is 2 GiB and shouldn't cause an issue. The trailing comma , is not interpreted somehow as decimal separator instead of array value separator, is it? 😄
  2. 3./4./5. solved in master with 0753be3.

Not sure whether it makes sense to backport those fixes, as the tests do not fail in 64-bit CI and 32-bit CI will most likely never be applied on stable25/24.

EDIT: Much likely on NC26 that was fixed with #36120, which never was backported to NC25.

@lapineige
Copy link

Upgraded to Nextcloud 26 to see if that would change anything.
First it gave me an internal server error when accessing the File app. Other apps worked as far as I could see.

I… can't access to the user page to change the quota.
I had not time to check from the command line (I mean database query).

That applying the patch again from that v26 could work ? Can someone provide guidance to me, I don't really know how to do it (what's the command) ? 🙏

@MichaIng
Copy link
Member

First it gave me an internal server error when accessing the File app.

As I reported above + fix here: #37344

cd /var/www/nextcloud
curl -sSfL 'https://github.com/nextcloud/server/pull/37344.patch' | patch -p1

@lapineige
Copy link

lapineige commented Mar 24, 2023

Thanks a lot !
I did it. File app was working again ! 🚀
And changed quota value from command line before this, worked without fixing anything, but after the patch is was displayed with the new value. Changing from the web interface worked too.

Thanks a lot, problem solved for me ! 🎉

edit : oh no, my bad, File app loads again partially, but the file list is still unavailable…

@MichaIng
Copy link
Member

Assure to force-reload the page via CTRL+C to clear browser cache, and in case restart PHP to assure OPcache and APCu are cleared.

@lapineige
Copy link

lapineige commented Mar 25, 2023

PHP (8.1) was restarted right away after applying the patch.
I loaded the page in private navigation so I had no cache.

To be precise, I have no longer any error shown, in the file app page nor in the logs (from admin UI).
Yet only the recent file bar and the top "Add notes, lists or links…" loads.

@MichaIng
Copy link
Member

Mysterious. It works perfectly fine here, including navigating, uploading files via web UI etc with quota applied. And I use this as my daily driver (although with an account without quota).

Please also check the browser console for errors.

@lapineige
Copy link

I tested without quota, same result.
I don't see any particular error in the browser console after looking at it quickly, I will try to go deeper as I don't understand all the warnings.

@MichaIng
Copy link
Member

Very strange. It works perfectly fine here. NC26, the patch applied exactly as posted above, and the user is perfectly able to browse files with and without quota applied, even showing a nice correct warning that quota is nearly full with correct percentage. Without any error messages in browser or backend, difficult to find the reason.

While is should not make a difference: We use PHP 8.2 here.

@lapineige
Copy link

I wonder if this could be linked to a quota value being set when I was in a semi-patched version.
But since that time I did upgrade, set the quota again, upgrade again (to v26), patch it, set the quota again…

@Timmy93
Copy link

Timmy93 commented Mar 29, 2023

First it gave me an internal server error when accessing the File app.

As I reported above + fix here: #37344

cd /var/www/nextcloud
curl -sSfL 'https://github.com/nextcloud/server/pull/37344.patch' | patch -p1

Upgraded to NC 26.0.0.11 on Raspberry Pi 4 32bit. I had the quota problem, this patch helped me. Thank guys!

PS. I run this modified command since nextcloud files are accessible only to www-data user

cd /var/www/nextcloud
curl -sSfL 'https://github.com/nextcloud/server/pull/37344.patch' | sudo -u www-data patch -p1

@lapineige
Copy link

I noticed something new today : while testing this, I disabled the quota limit for my account a while ago. Today when going to the files app, I had a "You storage space is almost full (98%)" or something similar. Storage isn't full at all. I don't know where this 98% is coming from.

@lapineige
Copy link

lapineige commented Apr 13, 2023

That message is no longer shown.

Also in server diagnostic I have this :

Your web server is not yet properly configured for file synchronisation because the WebDAV interface seems not to work.

Indeed client sync is broken, client tries repeatedly but always fails.
It happened since that quota bug appeared.

edit : if I set up a quota (not unlimited), this disappears.

@MichaIng MichaIng added the feature: 32bits Bug specific to 32bits architectures label Apr 21, 2023
@MichaIng
Copy link
Member

PR up which backports most of the changes done in #36120: #37877
Wait until all tests succeeded and until I tested it on my own instance before testing it on yours, since I was pretty intrusive, probably too much, with the backports and since there is possibly one conflict between a change done earlier on NC25, also related to quota, but solving it by casting everything to float.

@lapineige
Copy link

lapineige commented May 10, 2023

Can we test it now ?
How ?

edit : oh sorry I'm on Nextcloud 26.0.0 now, so it's irrelevant for me.

@lapineige
Copy link

lapineige commented May 10, 2023

I noticed something new today : while testing this, I disabled the quota limit for my account a while ago. Today when going to the files app, I had a "You storage space is almost full (98%)" or something similar. Storage isn't full at all. I don't know where this 98% is coming from.

This is the related bug ? #37940
32 bit system too.

@lapineige
Copy link

lapineige commented May 10, 2023

I'm a bit confused about the situation right now, and the various issues/PR where this problem (if it is really the same one) is mentioned.
Should I open a new issue, ask to reopen #34674, ... ?

edit : #37940 sounds very similar to the (second ?) bug I'm encountering. 32 bit system issue too, bug introduced by a new "int" typing...

@lapineige
Copy link

For the record migrating from 26.0.0 to 26.0.1 doesn't change anything regarding this issue.

@come-nc
Copy link
Contributor

come-nc commented May 11, 2023

I'm a bit confused about the situation right now, and the various issues/PR where this problem (if it is really the same one) is mentioned. Should I open a new issue, ask to reopen #34674, ... ?

edit : #37940 sounds very similar to the (second ?) bug I'm encountering. 32 bit system issue too, bug introduced by a new "int" typing...

If your bug is the same as #37940, comment there, if your bug is different, open a ticket for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish feature: files feature: 32bits Bug specific to 32bits architectures
Projects
None yet
Development

Successfully merging this pull request may close these issues.