LGTM On Wed, Nov 4, 2015 at 4:08 PM 'Klaus Aehlig' via ganeti-devel < [email protected]> wrote:
> Locking the configuration is naturally idempotent. However, > the corresponding WConfD call had a check refusing to lock > the config, if the caller has already locked it, arguing that > this should not happen. That argument misses that we have the > built-in assumption that daemons might be restarted at any time, > including the moment where a request is processed, but the caller > did not get the answer yet. So allow retries, hower logging that > they occurred (as this should only happen rarely). > > Signed-off-by: Klaus Aehlig <[email protected]> > --- > src/Ganeti/WConfd/Core.hs | 28 +++++++++++++++++----------- > 1 file changed, 17 insertions(+), 11 deletions(-) > > diff --git a/src/Ganeti/WConfd/Core.hs b/src/Ganeti/WConfd/Core.hs > index e60108c..6fc6a6d 100644 > --- a/src/Ganeti/WConfd/Core.hs > +++ b/src/Ganeti/WConfd/Core.hs > @@ -43,7 +43,7 @@ module Ganeti.WConfd.Core where > import Control.Arrow ((&&&)) > import Control.Concurrent (myThreadId) > import Control.Lens.Setter (set) > -import Control.Monad (liftM, unless, when) > +import Control.Monad (liftM, unless) > import qualified Data.Map as M > import qualified Data.Set as S > import Language.Haskell.TH (Name) > @@ -54,7 +54,7 @@ import Ganeti.BasicTypes > import qualified Ganeti.Constants as C > import qualified Ganeti.JSON as J > import qualified Ganeti.Locking.Allocation as L > -import Ganeti.Logging (logDebug) > +import Ganeti.Logging (logDebug, logWarning) > import Ganeti.Locking.Locks ( GanetiLocks(ConfigLock, BGL) > , LockLevel(LevelConfig) > , lockLevel, LockLevel > @@ -114,16 +114,22 @@ lockConfig > -> Bool -- ^ set to 'True' if the lock should be shared > -> WConfdMonad (J.MaybeForJSON ConfigData) > lockConfig cid shared = do > - let reqtype = if shared then ReqShared else ReqExclusive > - -- warn if we already have the lock, this shouldn't happen > + let (reqtype, owntype) = if shared > + then (ReqShared, L.OwnShared) > + else (ReqExclusive, L.OwnExclusive) > la <- readLockAllocation > - when (L.holdsLock cid ConfigLock L.OwnShared la) > - . failError $ "Client " ++ show cid ++ > - " already holds a config lock" > - waiting <- tryUpdateLocks cid [(ConfigLock, reqtype)] > - liftM J.MaybeForJSON $ case waiting of > - [] -> liftM Just CW.readConfig > - _ -> return Nothing > + if L.holdsLock cid ConfigLock owntype la > + then do > + -- warn if we already have the lock, but continue (with no-op) > + -- on the locks > + logWarning $ "Client " ++ show cid ++ " asked to lock the config" > + ++ " while owning the lock" > + liftM (J.MaybeForJSON . Just) CW.readConfig > + else do > + waiting <- tryUpdateLocks cid [(ConfigLock, reqtype)] > + liftM J.MaybeForJSON $ case waiting of > + [] -> liftM Just CW.readConfig > + _ -> return Nothing > > -- | Release the config lock, if the client currently holds it. > unlockConfig > -- > 2.6.0.rc2.230.g3dd15c0 > > -- Lisa Velden Software Engineer [email protected] Google Germany GmbH Dienerstraße 12 80331 München Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg
