fix(setupchecks): Skip checking for OPcache settings if running checks from CLI #46353

solracsf · 2024-07-08T08:45:29Z

Resolves: [Bug]: occ setupchecks - OPcache Configuration Checks should not be performed if opcache.enable_cli is disabled #46351

Summary

We don't recommend to enable OPcache on CLI, so disable the checks in this case.

Checklist

Code is properly formatted
Sign-off message is added to all commits
Tests (unit, integration, api and/or acceptance) are included
Screenshots before/after for front-end changes
Documentation (manuals or wiki) has been updated or is not required
Backports requested where applicable (ex: critical bugfixes)

Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>

solracsf · 2024-07-08T08:45:41Z

/backport to stable29

solracsf · 2024-07-08T08:45:45Z

/backport to stable28

kesselb · 2024-07-08T09:36:41Z

apps/settings/lib/SetupChecks/PhpOpcacheSetup.php

@@ -114,6 +114,11 @@ protected function getOpcacheSetupRecommendations(): array {
 	}

 	public function run(): SetupResult {
+		// Skip OPcache checks if running from CLI
+		if (PHP_SAPI === 'cli') {
+			return SetupResult::success($this->l10n->t('Checking from CLI, OPcache checks have been skipped.'));


Suggested change

return SetupResult::success($this->l10n->t('Checking from CLI, OPcache checks have been skipped.'));

return SetupResult::info($this->l10n->t('This check is skipped because OPcache is turned off.'));

Not sure in how far the output style of info differs from success, but I would indeed rate it as success when OPcache is disabled for CLI, since this is recommended for production systems. And in any case, the text should keep the info that it is skipped because we are running from CLI, or "turned off for CLI", else the information can be confusing.

apps/settings/lib/SetupChecks/PhpOpcacheSetup.php

come-nc

Why is recommended to disable opcache for CLI? This is unexpected to me, the cron and occ commands should also benefit from opcache, no?

apps/settings/lib/SetupChecks/PhpOpcacheSetup.php

solracsf · 2024-07-08T13:06:10Z

Why is recommended to disable opcache for CLI? This is unexpected to me, the cron and occ commands should also benefit from opcache, no?

nextcloud/documentation#1439

Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>

MichaIng

Looks good:

        php:
                ✓ PHP default charset: UTF-8
                ✓ PHP set_time_limit: The function is available.
                ✓ Freetype: Supported
                ✓ PHP getenv
                ✓ PHP memory limit: 512 MB
                ℹ PHP modules: This instance is missing some recommended PHP modules. For improved performance and better compatibility it is highly recommended to install them:
- bcmath for WebAuthn passwordless login
- gmp for WebAuthn passwordless login, and SFTP storage
                ✓ PHP opcache: Checking from CLI, OPcache checks have been skipped.
                ✓ PHP "output_buffering" option: Disabled
                ℹ PHP Imagick module: The PHP module "imagick" is not enabled although the theming app is. For favicon generation to work correctly, you need to install and enable this module.

After setting opcache.enable_cli=1:

        php:
                ✓ PHP default charset: UTF-8
                ✓ PHP set_time_limit: The function is available.
                ✓ Freetype: Supported
                ✓ PHP getenv
                ✓ PHP memory limit: 512 MB
                ℹ PHP modules: This instance is missing some recommended PHP modules. For improved performance and better compatibility it is highly recommended to install them:
- bcmath for WebAuthn passwordless login
- gmp for WebAuthn passwordless login, and SFTP storage
                ✓ PHP opcache: Correctly configured
                ✓ PHP "output_buffering" option: Disabled
                ℹ PHP Imagick module: The PHP module "imagick" is not enabled although the theming app is. For favicon generation to work correctly, you need to install and enable this module.

Not sure whether we can easily add correct indentation after line breaks?

In addition, with opcache.log_verbosity_level=3, I see a few pages with lines like:

Mon Jul  8 16:01:59 2024 (21777): Message Cached script '/var/www/nextcloud/lib/public/AppFramework/Http/Attribute/UseSession.php'
Mon Jul  8 16:01:59 2024 (21777): Message Added key '/var/www/nextcloud/lib/composer/composer/../../../lib/public/AppFramework/Http/Attribute/UseSession.php'

indicating how the OPcache is filled. On a Raspberry Pi 2, this increases the time of all occ calls by 1.5 - 2 seconds:

root@micha:/var/www/nextcloud# time ncc setupchecks &> /dev/null

real    0m17.633s
user    0m0.010s
sys     0m0.059s
root@micha:/var/www/nextcloud# time ncc setupchecks &> /dev/null

real    0m17.664s
user    0m0.041s
sys     0m0.030s
root@micha:/var/www/nextcloud# sed -i '/^opcache.enable_cli=/s/^/#/' /etc/php/8.2/mods-available/micha.ini # disable OPcache
root@micha:/var/www/nextcloud# time ncc setupchecks &> /dev/null

real    0m15.744s
user    0m0.025s
sys     0m0.047s
root@micha:/var/www/nextcloud# time ncc setupchecks &> /dev/null

real    0m15.813s
user    0m0.027s
sys     0m0.043s
root@micha:/var/www/nextcloud# time ncc &> /dev/null

real    0m2.557s
user    0m0.031s
sys     0m0.040s
root@micha:/var/www/nextcloud# time ncc &> /dev/null

real    0m2.562s
user    0m0.032s
sys     0m0.039s
root@micha:/var/www/nextcloud# sed -i '/^#opcache.enable_cli=/s/#//' /etc/php/8.2/mods-available/micha.ini # enable OPcache
root@micha:/var/www/nextcloud# time ncc &> /dev/null

real    0m4.123s
user    0m0.032s
sys     0m0.039s
root@micha:/var/www/nextcloud# time ncc &> /dev/null

real    0m4.054s
user    0m0.022s
sys     0m0.051s

@come-nc just to demonstrate that it really makes sense to have OPcache disabled for CLI.

come-nc · 2024-07-08T14:23:12Z

Then it should not even say "✓ PHP opcache: Correctly configured" when it is enabled, no?

MichaIng · 2024-07-08T15:11:58Z

But we might want to be able to test checks from CLI? Also, while it is good that we do not print a warning without opcache.enable_cli=1 anymore (which we did in the past), I am not sure whether we should now actively print a warning with opcache.enable_cli=1 🤔. It does not make sense, but it also does not break anything, and at least users/visitors are not affected by CLI performance.

come-nc · 2024-07-09T07:39:58Z

But we might want to be able to test checks from CLI?

There is the same problem with all checks on PHP configuration, running them from CLI tests the wrong configuration.
So the right way would be to use the API to get the results from server but occ has no access token for that. Experienced admin can always do it by hand with curl.

MichaIng · 2024-07-09T14:15:33Z

There is the same problem with all checks on PHP configuration, running them from CLI tests the wrong configuration.

Not necessarily the wrong configuration, only the wrong usage stats for sure. It would still be possible to configure PHP either for all SAPI's or CLI in particular the wrong and right way, and test whether occ setupchecks returns the correct results. As far as I can see, after migration to the new API, these checks are not tested? However, if we really want to have admins disabling OPcache for CLI now, and still want to be able to add PHPUnit tests:

		// We cannot check the PHP web SAPI config and OPcache usage stats from CLI
		// and building the OPcache usually worsens the performance of CLI calls.
		// Hence recommend to disable OPcache for CLI (which is the PHP default) and skip further OPcache checks.
		if (\OC::$CLI && !defined('PHPUNIT_RUN')) {
			if ($this->iniGetWrapper->getBool('opcache.enable_cli')) {
				return SetupResult::warning($this->l10n->t('OPcache is enabled for CLI. For better occ and background job performance, it is recommended to apply "opcache.enable_cli=0" to your PHP configuration.'));
			}
			return SetupResult::success($this->l10n->t('OPcache is disabled for CLI as recommended. Skipping further OPcache checks.'));
		}

However, while opcache.enable_cli=1 is probably set for older Nextcloud instances only, setup before we stopped recommending to set this, I would still vote to do such in a separate PR for NC30 only (and backporting the current one). So we can inform admins and developers about the changed check/recommendation in release notes, in case give appliances time to adjust etc.

come-nc · 2024-07-11T14:09:06Z

We should not have checks for PHPUNIT_RUN in the code, phpunit tests can mock the iniGetWrapper to return a config they want to test, and can set OC::$CLI according to what they want to test.

github-actions · 2024-07-23T01:50:44Z

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.)

solracsf · 2024-07-27T16:21:35Z

@come-nc are we good to go here as-is for now?

fix(setupchecks): Skip checking for OPcache if running from CLI

39f0524

Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>

solracsf added the 3. to review Waiting for reviews label Jul 8, 2024

solracsf added this to the Nextcloud 30 milestone Jul 8, 2024

solracsf requested review from kesselb, MichaIng and come-nc July 8, 2024 08:45

backportbot bot added the backport-request label Jul 8, 2024

kesselb reviewed Jul 8, 2024

View reviewed changes

kesselb approved these changes Jul 8, 2024

View reviewed changes

MichaIng reviewed Jul 8, 2024

View reviewed changes

apps/settings/lib/SetupChecks/PhpOpcacheSetup.php Outdated Show resolved Hide resolved

come-nc reviewed Jul 8, 2024

View reviewed changes

apps/settings/lib/SetupChecks/PhpOpcacheSetup.php Outdated Show resolved Hide resolved

fix: check both CLI and value

372c9d2

Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>

solracsf requested review from come-nc and MichaIng July 8, 2024 13:10

fix: typo

fa84f52

Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>

MichaIng approved these changes Jul 8, 2024

View reviewed changes

szaimen approved these changes Jul 9, 2024

View reviewed changes

github-actions bot added the feedback-requested label Jul 23, 2024

blizzz mentioned this pull request Jul 30, 2024

30.0.0 beta 2 #46870

Merged

come-nc approved these changes Aug 1, 2024

View reviewed changes

solracsf enabled auto-merge August 1, 2024 09:18

solracsf added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Aug 1, 2024

blizzz mentioned this pull request Aug 1, 2024

30.0.0 beta 3 #46946

Merged

solracsf merged commit 4488714 into master Aug 1, 2024
165 checks passed

solracsf deleted the skipOPcacheCLI branch August 1, 2024 18:02

This was referenced Aug 1, 2024

[stable28] fix(setupchecks): Skip checking for OPcache settings if running checks from CLI #46964

Merged

[stable29] fix(setupchecks): Skip checking for OPcache settings if running checks from CLI #46965

Merged

backportbot bot removed the backport-request label Aug 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(setupchecks): Skip checking for OPcache settings if running checks from CLI #46353

fix(setupchecks): Skip checking for OPcache settings if running checks from CLI #46353

solracsf commented Jul 8, 2024 •

edited

Loading

solracsf commented Jul 8, 2024

solracsf commented Jul 8, 2024

kesselb Jul 8, 2024

MichaIng Jul 8, 2024 •

edited

Loading

come-nc left a comment

solracsf commented Jul 8, 2024

MichaIng left a comment •

edited

Loading

come-nc commented Jul 8, 2024

MichaIng commented Jul 8, 2024

come-nc commented Jul 9, 2024

MichaIng commented Jul 9, 2024 •

edited

Loading

come-nc commented Jul 11, 2024

github-actions bot commented Jul 23, 2024

solracsf commented Jul 27, 2024

	return SetupResult::success($this->l10n->t('Checking from CLI, OPcache checks have been skipped.'));
	return SetupResult::info($this->l10n->t('This check is skipped because OPcache is turned off.'));

fix(setupchecks): Skip checking for OPcache settings if running checks from CLI #46353

fix(setupchecks): Skip checking for OPcache settings if running checks from CLI #46353

Conversation

solracsf commented Jul 8, 2024 • edited Loading

Summary

Checklist

solracsf commented Jul 8, 2024

solracsf commented Jul 8, 2024

kesselb Jul 8, 2024

Choose a reason for hiding this comment

MichaIng Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

come-nc left a comment

Choose a reason for hiding this comment

solracsf commented Jul 8, 2024

MichaIng left a comment • edited Loading

Choose a reason for hiding this comment

come-nc commented Jul 8, 2024

MichaIng commented Jul 8, 2024

come-nc commented Jul 9, 2024

MichaIng commented Jul 9, 2024 • edited Loading

come-nc commented Jul 11, 2024

github-actions bot commented Jul 23, 2024

solracsf commented Jul 27, 2024

solracsf commented Jul 8, 2024 •

edited

Loading

MichaIng Jul 8, 2024 •

edited

Loading

MichaIng left a comment •

edited

Loading

MichaIng commented Jul 9, 2024 •

edited

Loading