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 opcache validate/re-validate setupCheck (or warning in Updater) #45498

Open
joshtrichards opened this issue May 24, 2024 · 2 comments
Open

Comments

@joshtrichards
Copy link
Member

joshtrichards commented May 24, 2024

Inform operator if restarting mod_php/fpm is more likely than not to be needed after triggering an Upgrade so people are less surprised when this happens and/or to remind people that adjust these values but then later forget. :)

Options:

  • An info level message in existing PhpOpcacheSetup setupcheck
  • Something within updatenotification app that gets displayed on the Updater screen
  • Something in Updater itself

Or:

  • Just tell people to restart always 🤷‍♂️

Currently going to focus on clarifying in the documentation: nextcloud/documentation#11872

Related:

@joshtrichards joshtrichards added enhancement 0. Needs triage Pending check for reproducibility or if it fits our roadmap feature: install and update feature: settings labels May 24, 2024
@joshtrichards joshtrichards changed the title Add opcache validate/re-validate value setupCheck or to Updater Add opcache validate/re-validate setupCheck (or warning in Updater) May 24, 2024
@joshtrichards
Copy link
Member Author

joshtrichards commented May 27, 2024

pseudo code (all wrapped behind a check that opcache.enable === 1)

// will revalidate on every request so no problem
if `opcache.validate_timestamps === 1` and `opcache.revalidate_freq === 0` {
	return;
}

// handle the default (for PHP)
if `opcache.validate_timestamps === 1` and `opcache.revalidate_freq === 2` {
	return; 
	// note: technically this could still create some issues but it's less likely at least (and our bigger concerns are the below)
}

// handle the optimization scenario of validation being completely disabled
if `opcache.validate_timestamps === 0` {
	WARN: You presumably know what you're doing since you disabled validation, but as a consequence you're expected to restart mod_php or fpm after upgrading Server or Apps
	// note: For Server, technically it would be best if they did this after they push out the code update (i.e. after Updater runs or they upload the new code), but before db/app upgrades
}

// handle the optimization scenario of validation being on but it being big enough that it's likely to create server (and app) upgrade problems
if `opcache.validate_timestamps === 1` and `opcache.revalidate_freq > 2` {
	WARN: You presumably know what you're doing since you increased validation frequency from the default, but as a consequence you're expected to restart mod_php or fpm after ugprading Server or Apps
	// note: For Server, technically it would be best if they did this after they push out the code update (i.e. after Updater runs or they upload the new code), but before db/app upgrades
}

// handle when setup check is ran from the CLI
if (setupCheck ran from the CLI) {
        WARN: Unable to check your opcache [maybe with some further explanation/suggestion to check from web]
}

I was originally leaning towards putting this in the updatenotification app or Updater itself, but opcache validation impacts app upgrades too. So I'm thinking a standard setupcheck for now, maybe with a only documented in config.php.sample parameter that can be used to disable the general setupcheck for this.

This would allow those operators that adjust validation to not be permanently nagged by the warning in the general setupchecks... but still would allow us to later add in friendly and non-intrusive reminders in updatenotifcation / Updater and app store / occ app that ignores this flag and just prints a reminder to "Update done! Please restart mod_php/fpm".

This approach has the benefit of:

  • alerting admins that may not even know they did this (forgot or just followed some tutorial) to revisit their config
  • requiring one more "Yes I really want to do this" decision by adding the override
  • still cover situations where this is likely to be a problem by allowing us to add polite reminders when/where it's most critical to restart things
  • not just making the default that we tell people to be "restart" (though that's tempting)
  • providing a "heads up" to those that are using shared hosting (or cloud VPSes or particular distributions or whatever) where the validation values have been changed without them even knowing

@joshtrichards joshtrichards added 1. to develop Accepted and waiting to be taken care of and removed 0. Needs triage Pending check for reproducibility or if it fits our roadmap labels May 27, 2024
@joshtrichards
Copy link
Member Author

Both AIO and the micro-services Docker images sets opcache.revalidate_freq to 60.

That's obviously more than the default of 2.

Need to decide how to handle that. The higher the number goes the higher the likelihood of people experiencing weird behavior when making config changes or upgrades. 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant