[SERVER-40224] server parameter on_update handlers shouldn't return status Created: 19/Mar/19  Updated: 06/Dec/22  Resolved: 09/Dec/21

Status: Closed
Project: Core Server
Component/s: IDL
Affects Version/s: None
Fix Version/s: None

Type: Bug Priority: Major - P3
Reporter: Mira Carey Assignee: Backlog - Service Architecture
Resolution: Won't Do Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Related
related to SERVER-61973 Remove TODOs linked to SERVER-40224 Closed
Assigned Teams:
Service Arch
Backwards Compatibility: Fully Compatible
Operating System: ALL
Participants:

 Description   

the on_update handler for server parameters with storage should not return a Status. Because we call it after storing the value, it can cause us to store an illegal value, report it via getParameter, but not have that updated value reflected in the rest of the system.

See onUpdateFTDCDirectorySize:

Status onUpdateFTDCDirectorySize(const std::int32_t potentialNewValue) {
    if (potentialNewValue < ftdcStartupParams.maxFileSizeMB.load()) {
        return Status(
            ErrorCodes::BadValue,
            str::stream()
                << "diagnosticDataCollectionDirectorySizeMB must be greater than or equal to '"
                << ftdcStartupParams.maxFileSizeMB.load()
                << "' which is the current value of diagnosticDataCollectionFileSizeMB.");
    }
 
    auto controller = getGlobalFTDCController();
    if (controller) {
        controller->setMaxDirectorySizeBytes(potentialNewValue * 1024 * 1024);
    }
 
    return Status::OK();
}

For an example of where we probably should have had a validator + on_update handler, but by merging into one have allowed the observable value and the use of that value to drift out of sync



 Comments   
Comment by Matt Diener (Inactive) [ 09/Feb/22 ]

The above commit removes some comments that assumed this issue would be resolved as done.

Comment by Githook User [ 09/Feb/22 ]

Author:

{'name': 'Matt Diener', 'email': 'matt.diener@mongodb.com', 'username': 'mattdiener'}

Message: SERVER-61973 Remove unnecessary TODOs that reference SERVER-40224
Branch: master
https://github.com/mongodb/mongo/commit/71ec8c546cca9b867aa3872286f6bf17b31384f8

Comment by Mira Carey [ 22/Apr/19 ]

My intent was that on_update handlers not be allowed to fail (at least for values which have backing storage).

My thinking is that set parameters shouldn't have two sources of truth (the actual param, then the value that gets flushed). And observable flaps (where the underlying storage flashes OLD -> UPDATED -> OLD and the code downstream of on_update does nothing), are even worse to reason about.

I think we want to aim for a world where parameters that have storage only have observers. And if we need on_update handlers that can fail, I think they shouldn't have storage (we'd have to come up with something for stashing arguments from startup, but that could get solved with something a bit simpler, since we could crash if those updates didn't take)

Comment by Sara Williamson [ 22/Apr/19 ]

acm we think this wound up in the security sprint (rather than the dev tools sprint) by accident, so sending this back to needs triage--but since we just triaged today I wanted to give you a heads up.

Generated at Thu Feb 08 04:54:23 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.