nextcloud / server

☁️ Nextcloud server, a safe home for all your data
https://nextcloud.com
GNU Affero General Public License v3.0
27.28k stars 4.06k forks source link

Add opcache validate/re-validate setupCheck (or warning in Updater) #45498

Open joshtrichards opened 5 months ago

joshtrichards commented 5 months ago

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:

Or:

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

Related:

joshtrichards commented 5 months ago

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:

joshtrichards commented 5 months ago

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. :thinking: