[SERVER-33044] Move the fcvLock from being held for the duration of setFCV to only while setting targetVersion Created: 31/Jan/18  Updated: 16/Feb/18  Resolved: 16/Feb/18

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

Type: Task Priority: Major - P3
Reporter: Dianna Hohensee (Inactive) Assignee: Dianna Hohensee (Inactive)
Resolution: Won't Fix Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Related
is related to SERVER-33043 Only setFCV should take the fcvLock (... Closed
Sprint: Storage 2018-02-26
Participants:

 Description   

Currently setFCV and addShard take the fcvLock in exclusive mode so that they don't run together at all.

This ticket must make sure that addShard is safe to run entirely before or after setFCV sets a targetVersion. addShard should be explored to ensure it's safe, and make necessary modifications. One such mod might be to make sure addShard sends a valid FCV when it calls setFCV on the new shard – if in transition, does the function used to acquire a version to put in the call return some non "3.6"/"4.0" string?

shardCollection also takes the fcvLock, specifically for the v3.4 <=> v3.6 transition, due to UUIDs. This will be pulled out eventually (SERVER-33045) but needs to continue working correctly until then. Make sure this doesn't break that either.



 Comments   
Comment by Dianna Hohensee (Inactive) [ 16/Feb/18 ]

Closing as Won't Fix because we no longer want to minimize the duration of fcvLock in setFCV. It would require introducing a mutex replacement to prevent more than one thread executing the command at a time, and we don't want a mutex and fcvLock being held at the same time.

Comment by Dianna Hohensee (Inactive) [ 13/Feb/18 ]

Note: we must reintroduce the original setFCV command specific mutex, '_kFeatureCompatibilityVersionLock', to prevent setFCV from running with itself. '_kFeatureCompatibilityVersionLock' was repurposed into the current 'fcvLock' here

Comment by Esha Maharishi (Inactive) [ 01/Feb/18 ]

I think it'd be better defensive programming to lock around any changes to the fcv state.

In general, you should always take a lock when writing a variable that can be accessed from multiple threads (in this case, the variable is ServerGlobalParams::FeatureCompatibility::_version), regardless of whether the any code cares about the particular write being done.

Comment by Dianna Hohensee (Inactive) [ 01/Feb/18 ]

We are not adding the lock around unsetting targetVersion. The existing use cases where this is needed are only differentiating between upgrading/upgraded and downgrading/downgraded. There is no need for locking unsetting because there is no need to lock the change from upgrading to upgraded, or the downgrading equivalent.

Comment by Esha Maharishi (Inactive) [ 01/Feb/18 ]

Also, I think shardCollection already correctly handles seeing an "upgrading" or "downgrading" state (it propagates the UUID if 'upgrading' or 'fullyUpgraded').

Perhaps addShard should do something similar - send setFCV(<theUpgradeVersion>) if upgrading or fullyUpgraded, else send setFCV(<theDowngradeVersion>)?

Comment by Esha Maharishi (Inactive) [ 01/Feb/18 ]

This ticket means "while setting or unsetting targetVersion", right?

E.g., during setFCV:

1) take the fcvLock in X mode
2) set 'targetVersion' field (indicates upgrading or downgrading), and set 'version' field to the desired version. this write will also update the in-memory fcv state variable
3) release fcvLock
4) // do upgrade/downgrade work
5) take fcvLock in X mode
6) unset 'targetVersion' field (indicates fully upgraded or fully downgraded), and set 'version' field to the desired version. this write will also update the in-memory fcv state variable
7) release fcvLock

This way, any readers of the in-memory fcv state variable must see one of the fcv states: fully downgraded, upgrading, fully upgraded, or downgrading.

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