Merge pull request #2026 from jameinel/1.23-remove-le-feature-flag

Remove Leader Election as a feature flag

This should mean that leader election is just enabled by default.
This is Katco's request http://reviews.vapour.ws/r/1369/# but applied to 1.23 and with a couple of tests updated since they were patching the environment to set the flag for the test.

(Review request: http://reviews.vapour.ws/r/1378/)

feature/flags.go

@@ -18,10 +18,6 @@ const JES = "jes"
 // and server-side functionality.
 const Storage = "storage"
 
-// LeaderElection is the name of the feature to enable leadership hooks
-// and hook tools.
-const LeaderElection = "leader-election"
-
 // LogErrorStack is a developer feature flag to have the LoggedErrorStack
 // function in the utils package write out the error stack as defined by the
 // errors package to the logger.  The ability to log the error stack is very

worker/leadership/tracker.go

@@ -64,7 +64,22 @@ func NewTrackerWorker(tag names.UnitTag, leadership leadership.LeadershipManager
 				close(ticketCh)
 			}
 		}()
-		t.tomb.Kill(t.loop())
+		err := t.loop()
+		// TODO: jam 2015-04-02 is this the most elegant way to make
+		// sure we shutdown cleanly? Essentially the lowest level sees
+		// that we are dying, and propagates an ErrDying up to us so
+		// that we shut down, which we then are passing back into
+		// Tomb.Kill().
+		// Tomb.Kill() special cases the exact object ErrDying, and has
+		// no idea about errors.Cause and the general errors.Trace
+		// mechanisms that we use.
+		// So we explicitly unwrap before calling tomb.Kill() else
+		// tomb.Stop() thinks that we have a genuine error.
+		switch cause := errors.Cause(err); cause {
+		case tomb.ErrDying:
+			err = cause
+		}
+		t.tomb.Kill(err)
 	}()
 	return t
 }
@@ -222,7 +237,7 @@ func (t *tracker) isLeader() (bool, error) {
 		// Last time we looked, we were leader.
 		select {
 		case <-t.tomb.Dying():
-			return false, tomb.ErrDying
+			return false, errors.Trace(tomb.ErrDying)
 		case <-t.renewLease:
 			logger.Infof("%s renewing lease for %s leadership", t.unitName, t.serviceName)
 			t.renewLease = nil

worker/uniter/hook/hook.go

@@ -64,9 +64,7 @@ func (hi Info) Validate() error {
 		}
 	// TODO(fwereade): define these in charm/hooks...
 	case LeaderElected, LeaderDeposed, LeaderSettingsChanged:
-		if featureflag.Enabled(feature.LeaderElection) {
-			return nil
-		}
+		return nil
 	}
 	return fmt.Errorf("unknown hook kind %q", hi.Kind)
 }

worker/uniter/modes.go

@@ -8,13 +8,11 @@ import (
 	"time"
 
 	"github.com/juju/errors"
-	"github.com/juju/utils/featureflag"
 	"gopkg.in/juju/charm.v4"
 	"gopkg.in/juju/charm.v4/hooks"
 	"launchpad.net/tomb"
 
 	"github.com/juju/juju/apiserver/params"
-	"github.com/juju/juju/feature"
 	"github.com/juju/juju/state/watcher"
 	"github.com/juju/juju/worker"
 	"github.com/juju/juju/worker/uniter/hook"
@@ -46,40 +44,37 @@ func ModeContinue(u *Uniter) (next Mode, err error) {
 		return nil, errors.Trace(err)
 	}
 
-	if featureflag.Enabled(feature.LeaderElection) {
-		// Check for any leadership change, and enact it if possible.
-		logger.Infof("checking leadership status")
-		// If we've already accepted leadership, we don't need to do it again.
-		canAcceptLeader := !opState.Leader
-		select {
-		// If the unit's shutting down, we shouldn't accept it.
-		case <-u.f.UnitDying():
+	// Check for any leadership change, and enact it if possible.
+	logger.Infof("checking leadership status")
+	// If we've already accepted leadership, we don't need to do it again.
+	canAcceptLeader := !opState.Leader
+	select {
+	// If the unit's shutting down, we shouldn't accept it.
+	case <-u.f.UnitDying():
+		canAcceptLeader = false
+	default:
+		// If we're in an unexpected mode (eg pending hook) we shouldn't try either.
+		if opState.Kind != operation.Continue {
 			canAcceptLeader = false
-		default:
-			// If we're in an unexpected mode (eg pending hook) we shouldn't try either.
-			if opState.Kind != operation.Continue {
-				canAcceptLeader = false
-			}
 		}
-
-		// NOTE: the Wait() looks scary, but a ClaimLeadership ticket should always
-		// complete quickly; worst-case is API latency time, but it's designed that
-		// it should be vanishingly rare to hit that code path.
-		isLeader := u.leadershipTracker.ClaimLeader().Wait()
-		var creator creator
-		switch {
-		case isLeader && canAcceptLeader:
-			creator = newAcceptLeadershipOp()
-		case opState.Leader && !isLeader:
-			creator = newResignLeadershipOp()
-		}
-		if creator != nil {
-			return continueAfter(u, creator)
-		}
-		logger.Infof("leadership status is up-to-date")
 	}
 
+	// NOTE: the Wait() looks scary, but a ClaimLeadership ticket should always
+	// complete quickly; worst-case is API latency time, but it's designed that
+	// it should be vanishingly rare to hit that code path.
+	isLeader := u.leadershipTracker.ClaimLeader().Wait()
 	var creator creator
+	switch {
+	case isLeader && canAcceptLeader:
+		creator = newAcceptLeadershipOp()
+	case opState.Leader && !isLeader:
+		creator = newResignLeadershipOp()
+	}
+	if creator != nil {
+		return continueAfter(u, creator)
+	}
+	logger.Infof("leadership status is up-to-date")
+
 	switch opState.Kind {
 	case operation.RunAction:
 		// TODO(fwereade): we *should* handle interrupted actions, and make sure
@@ -201,12 +196,10 @@ func ModeAbide(u *Uniter) (next Mode, err error) {
 		return nil, errors.Trace(err)
 	}
 
-	if featureflag.Enabled(feature.LeaderElection) {
-		if !opState.Leader && !u.ranLeaderSettingsChanged {
-			creator := newSimpleRunHookOp(hook.LeaderSettingsChanged)
-			if err := u.runOperation(creator); err != nil {
-				return nil, errors.Trace(err)
-			}
+	if !opState.Leader && !u.ranLeaderSettingsChanged {
+		creator := newSimpleRunHookOp(hook.LeaderSettingsChanged)
+		if err := u.runOperation(creator); err != nil {
+			return nil, errors.Trace(err)
 		}
 	}
 
@@ -244,18 +237,16 @@ func ModeAbide(u *Uniter) (next Mode, err error) {
 func modeAbideAliveLoop(u *Uniter) (Mode, error) {
 	var leaderElected, leaderDeposed <-chan struct{}
 	for {
-		if featureflag.Enabled(feature.LeaderElection) {
-			// We expect one or none of these vars to be non-nil; and if none
-			// are, we set the one that should trigger when our leadership state
-			// differs from what we have recorded locally.
-			if leaderElected == nil && leaderDeposed == nil {
-				if u.operationState().Leader {
-					logger.Infof("waiting to lose leadership")
-					leaderDeposed = u.leadershipTracker.WaitMinion().Ready()
-				} else {
-					logger.Infof("waiting to gain leadership")
-					leaderElected = u.leadershipTracker.WaitLeader().Ready()
-				}
+		// We expect one or none of these vars to be non-nil; and if none
+		// are, we set the one that should trigger when our leadership state
+		// differs from what we have recorded locally.
+		if leaderElected == nil && leaderDeposed == nil {
+			if u.operationState().Leader {
+				logger.Infof("waiting to lose leadership")
+				leaderDeposed = u.leadershipTracker.WaitMinion().Ready()
+			} else {
+				logger.Infof("waiting to gain leadership")
+				leaderElected = u.leadershipTracker.WaitLeader().Ready()
 			}
 		}
 		lastCollectMetrics := time.Unix(u.operationState().CollectMetricsTime, 0)
@@ -314,18 +305,16 @@ func modeAbideDyingLoop(u *Uniter) (next Mode, err error) {
 	if err := u.relations.SetDying(); err != nil {
 		return nil, errors.Trace(err)
 	}
-	if featureflag.Enabled(feature.LeaderElection) {
-		if u.operationState().Leader {
-			if err := u.runOperation(newResignLeadershipOp()); err != nil {
-				return nil, errors.Trace(err)
-			}
-			// TODO(fwereade): we ought to inform the tracker that we're shutting down
-			// (and no longer wish to continue renewing our lease) so that the tracker
-			// can then report minionhood at all times, and thus prevent the is-leader
-			// and leader-set hook tools from acting in a correct but misleading way
-			// (ie continuing to act as though leader after leader-deposed has run).
-			// tool from returning true but misleading reports of leadership while dying.
+	if u.operationState().Leader {
+		if err := u.runOperation(newResignLeadershipOp()); err != nil {
+			return nil, errors.Trace(err)
 		}
+		// TODO(fwereade): we ought to inform the tracker that we're shutting down
+		// (and no longer wish to continue renewing our lease) so that the tracker
+		// can then report minionhood at all times, and thus prevent the is-leader
+		// and leader-set hook tools from acting in a correct but misleading way
+		// (ie continuing to act as though leader after leader-deposed has run).
+		// tool from returning true but misleading reports of leadership while dying.
 	}
 	for {
 		if len(u.relations.GetInfo()) == 0 {
@@ -383,10 +372,8 @@ func ModeHookError(u *Uniter) (next Mode, err error) {
 	u.f.WantResolvedEvent()
 	u.f.WantUpgradeEvent(true)
 	var leaderDeposed <-chan struct{}
-	if featureflag.Enabled(feature.LeaderElection) {
-		if opState.Leader {
-			leaderDeposed = u.leadershipTracker.WaitMinion().Ready()
-		}
+	if opState.Leader {
+		leaderDeposed = u.leadershipTracker.WaitMinion().Ready()
 	}
 	for {
 		// We set status inside the loop so we can be sure we *reset* status after a

worker/uniter/runner/jujuc/server.go

@@ -69,9 +69,7 @@ func allEnabledCommands() map[string]creator {
 	if featureflag.Enabled(feature.Storage) {
 		add(storageCommands)
 	}
-	if featureflag.Enabled(feature.LeaderElection) {
-		add(leaderCommands)
-	}
+	add(leaderCommands)
 	return all
 }
 

worker/uniter/uniter.go

@@ -13,14 +13,12 @@ import (
 	"github.com/juju/loggo"
 	"github.com/juju/names"
 	"github.com/juju/utils/exec"
-	"github.com/juju/utils/featureflag"
 	"github.com/juju/utils/fslock"
 	corecharm "gopkg.in/juju/charm.v4"
 	"launchpad.net/tomb"
 
 	"github.com/juju/juju/api/uniter"
 	"github.com/juju/juju/apiserver/params"
-	"github.com/juju/juju/feature"
 	coreleadership "github.com/juju/juju/leadership"
 	"github.com/juju/juju/version"
 	"github.com/juju/juju/worker"
@@ -154,11 +152,7 @@ func (u *Uniter) loop(unitTag names.UnitTag) (err error) {
 	go func() { u.tomb.Kill(u.f.Wait()) }()
 
 	// Start handling leader settings events, or not, as appropriate.
-	if featureflag.Enabled(feature.LeaderElection) {
-		u.f.WantLeaderSettingsEvents(!u.operationState().Leader)
-	} else {
-		u.f.WantLeaderSettingsEvents(false)
-	}
+	u.f.WantLeaderSettingsEvents(!u.operationState().Leader)
 
 	// Run modes until we encounter an error.
 	mode := ModeContinue
@@ -394,13 +388,15 @@ func (u *Uniter) runOperation(creator creator) error {
 	if err != nil {
 		return errors.Annotatef(err, "cannot create operation")
 	}
-	if featureflag.Enabled(feature.LeaderElection) {
-		before := u.operationState()
-		defer func() {
-			if after := u.operationState(); before.Leader != after.Leader {
-				u.f.WantLeaderSettingsEvents(before.Leader)
-			}
-		}()
-	}
+	before := u.operationState()
+	defer func() {
+		// Check that if we lose leadership as a result of this
+		// operation, we want to start getting leader settings events,
+		// or if we gain leadership we want to stop receiving those
+		// events.
+		if after := u.operationState(); before.Leader != after.Leader {
+			u.f.WantLeaderSettingsEvents(before.Leader)
+		}
+	}()
 	return u.operationExecutor.Run(op)
 }