Wait to initialize autopilot until all servers are >= 0.8.0 #2897

kyhavlov · 2017-04-12T22:31:17Z

No description provided.

slackpad · 2017-04-12T22:40:30Z

consul/autopilot.go

@@ -33,6 +34,8 @@ func (s *Server) stopAutopilot() {
 	s.autopilotWaitGroup.Wait()
 }

+var minAutopilotVersion, _ = version.NewVersion("0.8.0")


Let's use version.Must(version.NewVersion()) here - can you update the other constant versions in AP to do that as well?

That way things will blow up if there's an error. Unlikely to ever happen but an error into an _ feels like it'll bite us some day.

slackpad · 2017-04-12T22:58:43Z

consul/autopilot.go

 // pruneDeadServers removes up to numPeers/2 failed servers
 func (s *Server) pruneDeadServers() error {
 	state := s.fsm.State()
 	_, autopilotConf, err := state.AutopilotConfig()
-	if err != nil {
+	if err != nil || autopilotConf == nil {


It feels weird to have this down here. I'd change pruneDeadServers to take the config as an argument like PromoteNonVoters and then make a helper that gets called in both paths. Since initializeAutopilot also fetches the state I think you can move its logic here and consolidate into one function:

func (s *Server) getAutopilotConfig() (*structs.AutopilotConfig, bool) { state := s.fsm.State() _, config, err := state.AutopilotConfig() if err != nil { s.logger.Printf("[ERR] autopilot: Error retrieving config from state store: %v", err) return nil, false } if config != nil { return config, true } // do the version check here and possibly return nil, false config = *s.config.AutopilotConfig req := structs.AutopilotSetConfigRequest{Config:config} if _, err = s.raftApply(structs.AutopilotRequestType, req); err != nil { s.logger.Printf("[ERR] autopilot: Failed to initialize autopilot config: %v", err) return nil, false } return config, true }

Then in the switch it's:

autopilotConfig, ok := s.getAutopilotConfig() if !ok { continue }

Actually maybe getOrCreateAutpilotConfig so it's clear it has a side-effect.

slackpad · 2017-04-12T23:03:02Z

consul/autopilot.go

@@ -68,11 +84,26 @@ func (s *Server) autopilotLoop() {
 	}
 }

+// lowestServerVersion returns the lowest version among the alive servers


I'd factor this out into a companion to https://github.com/hashicorp/consul/blob/master/consul/util.go#L65-L92, like AreServersAtLeastOnVersion. Or, since have util depend on consul/server might be weird you could move them both into a new Go file in the Consul package like "compatibility.go" so they live side-by-side.

slackpad · 2017-04-12T23:04:38Z

consul/leader.go

@@ -153,12 +153,6 @@ func (s *Server) establishLeadership() error {
 		return err
 	}

-	// Setup autopilot config if we are the leader and need to


I think this endpoint will crash if the AP config hasn't been made yet:

https://github.com/hashicorp/consul/blob/master/consul/operator_autopilot_endpoint.go#L24-L30

^ that endpoint still needs an update (it can call get or create prolly and return an error if it can't get a config)

slackpad · 2017-04-13T01:06:05Z

consul/leader.go

-func (s *Server) initializeAutopilot() error {
-	// Bail if the config has already been initialized
+// getAutopilotConfig is used to get the autopilot config, initializing it if necessary
+func (s *Server) getAutopilotConfig() (*structs.AutopilotConfig, bool) {


What do you think of getOrCreateAutpilotConfig?

slackpad · 2017-04-13T01:06:27Z

consul/leader.go

-	req := structs.AutopilotSetConfigRequest{
-		Config: *s.config.AutopilotConfig,
+	if !ServersMeetMinimumVersion(s.LANMembers(), minAutopilotVersion) {
+		s.logger.Printf("can't initialize autopilot until all servers are >= %s", minAutopilotVersion.String())


These should have the autopilot prefix added.

slackpad · 2017-04-13T01:07:41Z

consul/leader.go

 	if _, err = s.raftApply(structs.AutopilotRequestType, req); err != nil {
-		return fmt.Errorf("failed to initialize autopilot config")
+		s.logger.Printf("failed to initialize autopilot config")


Add the error onto the end of this one.

slackpad · 2017-04-13T01:07:56Z

consul/util.go

+
+// ServersMeetMinimumVersion returns whether the given alive servers are at least on the
+// given Consul version
+func ServersMeetMinimumVersion(members []serf.Member, minVersion *version.Version) bool {


This would be good to add a test for.

slackpad · 2017-04-13T01:08:19Z

consul/autopilot.go

@@ -33,6 +34,8 @@ func (s *Server) stopAutopilot() {
 	s.autopilotWaitGroup.Wait()
 }

+var minAutopilotVersion = version.Must(version.NewVersion("0.8.0"))


Can you change the other NewVersion calls to use this pattern too?

slackpad

Looks good - I'd bump the log level for the one down to a WARN and the operator AP endpoint needs a nil check, then I think we are good.

slackpad · 2017-04-13T03:36:26Z

consul/leader.go

-	req := structs.AutopilotSetConfigRequest{
-		Config: *s.config.AutopilotConfig,
+	if !ServersMeetMinimumVersion(s.LANMembers(), minAutopilotVersion) {
+		s.logger.Printf("[ERR] autopilot: can't initialize until all servers are >= %s", minAutopilotVersion.String())


This seems like a WARN.

Wait to initialize autopilot until all servers are >= 0.8.0

7d36403

kyhavlov force-pushed the autopilot-config-fix branch from 45efc53 to 7d36403 Compare April 12, 2017 23:05

slackpad suggested changes Apr 12, 2017

View reviewed changes

Reorganize version check logic for autopilot

e467da5

slackpad reviewed Apr 13, 2017

View reviewed changes

Add formatting to autopilot init messages

196dd81

kyhavlov force-pushed the autopilot-config-fix branch from c4a67fe to 196dd81 Compare April 13, 2017 01:39

slackpad reviewed Apr 13, 2017

View reviewed changes

slackpad mentioned this pull request Apr 13, 2017

Panic in leader loop when stopping autopilot #2896

Closed

Add nil check to operator autopilot endpoint

76d96e2

slackpad approved these changes Apr 13, 2017

View reviewed changes

slackpad mentioned this pull request Apr 13, 2017

Electing a 0.8.x leader during an upgrade can cause a panic in older servers #2889

Closed

kyhavlov merged commit 19540bb into master Apr 13, 2017

kyhavlov deleted the autopilot-config-fix branch April 13, 2017 17:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wait to initialize autopilot until all servers are >= 0.8.0 #2897

Wait to initialize autopilot until all servers are >= 0.8.0 #2897

kyhavlov commented Apr 12, 2017

slackpad Apr 12, 2017

slackpad Apr 12, 2017

slackpad Apr 12, 2017

slackpad Apr 12, 2017

slackpad Apr 12, 2017

slackpad Apr 12, 2017

slackpad Apr 12, 2017

slackpad Apr 13, 2017

slackpad Apr 13, 2017

slackpad Apr 13, 2017

slackpad Apr 13, 2017

slackpad Apr 13, 2017

slackpad Apr 13, 2017

slackpad left a comment

slackpad Apr 13, 2017

Wait to initialize autopilot until all servers are >= 0.8.0 #2897

Wait to initialize autopilot until all servers are >= 0.8.0 #2897

Conversation

kyhavlov commented Apr 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slackpad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment