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

feat(cache): CLI should not fail if APCu is not available #46151

Merged
merged 1 commit into from
Oct 29, 2024

Conversation

MichaIng
Copy link
Member

@MichaIng MichaIng commented Jun 26, 2024

Summary

CLI should not fail if APCu is not available but fallback to NullCache. This can be the case if APCu is used without apc.enable_cli=1. APCu however runs within the PHP instance and hence cannot be shared between CLI and web or used as distributed cache. The CLI call can hence only create or invalidate entries for itself. For short-living CLI calls, this is theoretically a downsides regarding performance and resource usage, and Nextcloud must not have any issues using the dummy NullCache instead of an isolated freshly created and destroyed APCu instance.

This partly reverts #25770. The fallback was removed, because CLI calls started to hang after #25440. The reason however was not that a cache is generally required for CLI calls, but because the previously logged warning invoked the user backend, which invoked again the caching backend, causing a loop.

This commit re-adds the fallback without logging a warning, and for APCu only. For mentioned reasons, it is okay to fallback to NullCache silently. If Redis or memcached are configured but not available, then the web UI would fail as well, and it makes sense to abort and inform CLI calls as well then.

The motivation is to make apc.enable_cli=1 optional, and that hence the documentation about it can be removed. We should not enforce admins to enable APCu for CLI calls, which is reasonably disabled by default. This also reduces requirements for hosting providers to support Nextcloud.


I did some tests on a Raspberry Pi 2 with slow CPU and RAM, probably lowest end of hardware which can reasonably run a Nextcloud instance. I did some occ help, occ status and cron.php call loops with and without APCu, as well as with very large 256 MiB APCu size (PHP default is 32 MiB, my Nextcloud uses ~1 MiB). I could not find any performance differences. At least for those calls, building up and destroying the APCu cache did not take any significant time, compared to the ~2.5 seconds the whole processing for a single call (mostly loading all frameworks and scripts) took. But the php process memory usage raised by exactly the configured APCu size, which can make a difference if this leads to swapping. And it can push Linux I/O buffer/cache for regularly accessed files out of memory. So I find it reasonable to not use APCu for CLI calls in any case.

Since APCu and ArrayCache for the web instance cannot be invalidated from CLI, the documentation could mention this instead. Theoretically, config changes, Nextcloud or app upgrades can require a local cache invalidation. Hence, in case one faces issues, a PHP server restart could be required. The same is true when OPcache revalidation time is long or has been disabled. This commit however does not chance anything about this fact.

I dropped the idea to disable caching for CLI calls in the first place: With APCu for CLI disabled by default, this is practically no difference, but the fallback at least still allows admins to use APCu for CLI calls as well, whether for testing, debugging or whatever reason. And it does not hurt to have CLI use Redis/memcached, but it can then indeed be a performance benefit. And using these caching backends allows CLI to invalidate web cache, if required/beneficial in certain cases. However, at best, such requirements, if they really exist, are removed from Nextcloud, as of the impossibility to do this with APCu and ArrayCache. And dropping support for those fast (APCu) and simple caching options, which do not require to setup and run a dedicated caching server, is IMO raising the hurdle for setup Nextcloud too much.

TODO

Checklist

@MichaIng MichaIng added enhancement 3. to review Waiting for reviews feature: caching Related to our caching system: scssCacher, jsCombiner... labels Jun 26, 2024
@MichaIng MichaIng added this to the Nextcloud 30 milestone Jun 26, 2024
Copy link
Member

@nickvergessen nickvergessen 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 fine when you limit this to APCu.
But Redis and others should fail hard on CLI as well when they are not working properly.

@MichaIng
Copy link
Member Author

Makes sense. If Redis, memcached or ArrayCache fail, there is a general issue. Also they all do not need to be "created" for a CLI instance, the ArrayCache really is just a PHP array that does not consume any memory when not filled.

@MichaIng MichaIng force-pushed the enh/do-not-enforce-cache-for-cli branch 2 times, most recently from 3c35bfb to 8e2df5e Compare June 30, 2024 16:15
@MichaIng MichaIng changed the title feat(cache): CLI should not fail on missing memcache feat(cache): CLI should not fail if APCu is not available Jun 30, 2024
// CLI should not fail if APCu is not available but fallback to NullCache.
// This can be the case if APCu is used without apc.enable_cli=1.
// APCu however cannot be shared between Nextcloud (PHP) instances anyway.
$localCacheClass = self::NULL_CACHE;
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't ARRAY_CACHE be better? Could still help in cron and occ commands?

Copy link
Member Author

@MichaIng MichaIng Jul 1, 2024

Choose a reason for hiding this comment

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

Not sure. The question is whether cache entries created by typical cron/occ jobs are also reused by them within the same call. Otherwise, ArrayCache would just cause a raising memory usage (based which cache entries are created) without a performance benefit.

Checking my instance, most entries seem to be web UI related. Maybe someone has more insights or examples of apps/cron/occ jobs which are reusing cached content more often. But I guess this is not the case, and all a CLI ArrayCache would do is raising memory usage, while it runs.

EDIT: Maybe, when I find time, I'll add an access counter for ArrayCache entries on a local instance, and print/log the whole array with access counts when the CLI call ends, to get an idea 🙂.

@MichaIng MichaIng force-pushed the enh/do-not-enforce-cache-for-cli branch from 8e2df5e to 033c911 Compare July 3, 2024 20:58
Copy link
Contributor

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

@blizzz blizzz mentioned this pull request Jul 30, 2024
but fallback to NullCache. This can be the case if APCu is used without apc.enable_cli=1. APCu however runs within the PHP instance and hence cannot be shared between CLI and web or used as distributed cache. The CLI call can hence only create or invalidate entries for itself. For short-living CLI calls, this is theoretically a downsides regarding performance and resource usage, and Nextcloud must not have any issues using the dummy NullCache instead of an isolated freshly created and destroyed APCu instance.

This partly reverts #25770. The fallback was removed, because CLI calls started to hang after #25440. The reason however was not that a cache is generally required for CLI calls, but because the previously logged warning invoked the user backend, which invoked again the caching backend, causing a loop.

This commit re-adds the fallback without logging a warning, and for APCu only. For mentioned reasons, it is okay to fallback to NullCache silently. If Redis or memcached are configured but not available, then the web UI would fail as well, and it makes sense to abort and inform CLI calls as well then.

The motivation is to make apc.enable_cli=1 optional, and that hence the documentation about it can be removed. We should not enforce admins to enable APCu for CLI calls, which is reasonably disabled by default. This also reduces requirements for hosting providers to support Nextcloud.

Signed-off-by: MichaIng <[email protected]>
@AndyScherzinger AndyScherzinger force-pushed the enh/do-not-enforce-cache-for-cli branch from 033c911 to 89cd3b4 Compare August 1, 2024 08:02
@AndyScherzinger
Copy link
Member

🏓 @st3iny since requested for 2nd review 😃

@blizzz blizzz mentioned this pull request Aug 1, 2024
This was referenced Aug 5, 2024
@skjnldsv skjnldsv mentioned this pull request Aug 13, 2024
@skjnldsv skjnldsv modified the milestones: Nextcloud 30, Nextcloud 31 Aug 14, 2024
Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

🐘

@skjnldsv skjnldsv merged commit 2d5060d into master Oct 29, 2024
167 of 169 checks passed
@skjnldsv skjnldsv deleted the enh/do-not-enforce-cache-for-cli branch October 29, 2024 14:28
MichaIng added a commit to nextcloud/documentation that referenced this pull request Oct 29, 2024
After nextcloud/server#46151, Nextcloud does not require `apc.enable_cli` to be enabled anymore, but will fallback to the dummy NullCache instead for CLI calls. All mentions of this PHP configuration option are hence removed.

Additionally, one left mention of `opcache.enable_cli` is removed, which was never required.

Signed-off-by: MichaIng <[email protected]>
@MichaIng
Copy link
Member Author

MichaIng commented Oct 29, 2024

Docs update: nextcloud/documentation#12332

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement feature: caching Related to our caching system: scssCacher, jsCombiner... feedback-requested performance 🚀
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fall back to NullCache on CLI calls if configured cache is not available
4 participants