Add Autopilot feature from Consul #3670

kyhavlov · 2017-12-18T22:31:43Z

This adds the OSS components for autopilot as well as the config fields/docs for the enterprise autopilot features. The autopilot logic is in the agent/consul/autopilot package over in the Consul repo.

schmichael

I'm a little unclear on what Datacenter/dc represents for Autopilot. I don't think we have any other datacenter-specific APIs in Nomad, but I'm probably forgetting something. 😅

schmichael · 2018-01-10T00:28:09Z

api/operator_autopilot.go

+)
+
+// AutopilotConfiguration is used for querying/setting the Autopilot configuration.
+// Autopilot helps manage operator tasks related to Consul servers like removing


Consul servers -> Nomad servers

schmichael · 2018-01-10T00:28:25Z

api/operator_autopilot.go

+	// The status of the SerfHealth check for the server.
+	SerfStatus string
+
+	// Version is the Consul version of the server.


Nomad version

schmichael · 2018-01-10T00:28:38Z

api/operator_autopilot.go

+	DisableUpgradeMigration bool
+
+	// (Enterprise-only) UpgradeVersionTag is the node tag to use for version info when
+	// performing upgrade migrations. If left blank, the Consul version will be used.


Nomad version

schmichael · 2018-01-10T00:36:58Z

api/operator_autopilot.go

+	return res, nil
+}
+
+// AutopilotServerHealth


Incomplete comment

schmichael · 2018-01-10T00:48:48Z

command/agent/operator_endpoint.go

+	}
+}
+
+// OperatorServerHealth is used to get the health of the servers in the local DC


I think remove "in the local DC" (?) as DCs are defined differently in Consul and Nomad.

schmichael · 2018-01-10T00:54:22Z

nomad/autopilot.go

+	"github.com/hashicorp/serf/serf"
+)
+
+// AutopilotDelegate is a Consul delegate for autopilot operations.


Consul delegate -> Nomad delegate

schmichael · 2018-01-10T00:55:12Z

nomad/autopilot.go

+	return server, nil
+}
+
+// Heartbeat a metric for monitoring if we're the leader


// NotifyHealth heartbeats a metric ...

schmichael · 2018-01-10T01:08:57Z

nomad/util.go

@@ -80,6 +83,12 @@ func isNomadServer(m serf.Member) (bool, *serverParts) {
 		}
 	}

+	// If the server is missing the rpc_addr tag, default to the serf advertise addr
+	rpc_ip := net.ParseIP(m.Tags["rpc_addr"])


rpcIP would be a more idiomatic name

schmichael · 2018-01-10T01:22:01Z

website/source/guides/cluster/autopilot.html.md

+    "redundancy_zone_tag": "az",
+    "disable_upgrade_migration": false,
+    "upgrade_version_tag": ""
+}


schmichael · 2018-01-10T01:23:34Z

website/source/guides/cluster/autopilot.html.md

+## Server Health Checking
+
+An internal health check runs on the leader to track the stability of servers.
+</br>A server is considered healthy if all of the following conditions are true:


<br> not </br> but is it necessary?

preetapan · 2018-01-10T15:39:35Z

api/operator_autopilot_test.go

+	if err != nil {
+		t.Fatalf("err: %v", err)
+	}
+	if !config.CleanupDeadServers {


WE've been trying to be consistent using testify/assert for new tests and when we touch existing tests, would be nice if you could rewrite this to use asserts

preetapan · 2018-01-10T15:51:03Z

website/source/guides/cluster/autopilot.html.md

+An internal health check runs on the leader to track the stability of servers.
+</br>A server is considered healthy if all of the following conditions are true:
+
+- It has a SerfHealth status of 'Alive'


I think the term "SerfHealth" is specific to Consul and refers to the serfstatus health check. I would suggest calling this Serf status or rewrite the sentence to be "The server's status according to serf is "Alive", to avoid confusion.

preetapan · 2018-01-10T15:58:38Z

command/agent/agent.go

+	}
+	if agentConfig.Autopilot != nil {
+		if agentConfig.Autopilot.CleanupDeadServers != nil {
+			conf.AutopilotConfig.CleanupDeadServers = *agentConfig.Autopilot.CleanupDeadServers


why do a couple of these do *agentConfig?

I didn't want to override the defaults in the server config with different defaults from the user config

preetapan · 2018-01-10T16:04:11Z

nomad/server.go

@@ -271,6 +279,9 @@ func NewServer(config *Config, consulCatalog consul.CatalogAPI, logger *log.Logg
 	// Create the periodic dispatcher for launching periodic jobs.
 	s.periodicDispatcher = NewPeriodicDispatch(s.logger, s)

+	// Initialize the stats fetcher that autopilot will use.
+	s.statsFetcher = NewStatsFetcher(logger, s.connPool, s.config.Region)
+
 	// Setup Vault
 	if err := s.setupVaultClient(); err != nil {


The Leave method here also calls RemovePeer and currently does not handle raft protocol 3, I left a TODO in there in the master branch.

Another TODO https://github.com/hashicorp/nomad/blob/master/api/operator.go#L72

The last of these (raft ID related) should be taken care of now

kyhavlov · 2018-01-16T21:41:31Z

@schmichael those references to datacenter instead of region were just mistakes, I think.

This should be ready for another look now.

schmichael · 2018-01-16T23:31:52Z

command/operator_autopilot.go

+
+func (c *OperatorAutopilotCommand) Help() string {
+	helpText := `
+Usage: consul operator autopilot <subcommand> [options]


consul -> nomad

schmichael · 2018-01-16T23:32:12Z

command/operator_autopilot_get.go

+
+func (c *OperatorAutopilotGetCommand) Help() string {
+	helpText := `
+Usage: consul operator autopilot get-config [options]


consul -> nomad

schmichael · 2018-01-16T23:33:06Z

command/operator_autopilot_set.go

+
+func (c *OperatorAutopilotSetCommand) Help() string {
+	helpText := `
+Usage: consul operator autopilot set-config [options]


consul -> nomad

schmichael · 2018-01-16T23:33:13Z

command/operator_autopilot_set.go

+Set Config Options:
+
+  -cleanup-dead-servers=[true|false]
+     Controls whether Consul will automatically remove dead servers when


consul -> nomad

schmichael · 2018-01-16T23:33:19Z

command/operator_autopilot_set.go

+     new ones are successfully added. Must be one of [true|false].
+
+  -disable-upgrade-migration=[true|false]
+     (Enterprise-only) Controls whether Consul will avoid promoting


consul -> nomad

schmichael · 2018-01-16T23:33:41Z

command/operator_autopilot_set.go

+
+  -upgrade-version-tag=<value>
+     (Enterprise-only) The node_meta tag to use for version info when
+     performing upgrade migrations. If left blank, the Consul version


consul -> nomad

schmichael · 2018-01-16T23:34:20Z

nomad/structs/config/autopilot.go

+	DisableUpgradeMigration *bool `mapstructure:"disable_upgrade_migration"`
+
+	// (Enterprise-only) UpgradeVersionTag is the node tag to use for version info when
+	// performing upgrade migrations. If left blank, the Consul version will be used.


consul -> nomad

dadgar · 2018-01-19T20:58:01Z

website/source/guides/cluster/autopilot.html.md

+deploying just one server in each.
+
+If the `RedundancyZoneTag` setting is set, Nomad will use its value to look for a
+zone in each server's specified [`-meta`](/docs/agent/configuration/client.html#meta)


This is only defined on clients that aren't participating in Raft/Serf.

dadgar · 2018-02-07T23:24:45Z

api/operator_autopilot.go

+
+// AutopilotGetConfiguration is used to query the current Autopilot configuration.
+func (op *Operator) AutopilotGetConfiguration(q *QueryOptions) (*AutopilotConfiguration, error) {
+	r, err := op.c.newRequest("GET", "/v1/operator/autopilot/configuration")


152-166 can be replaced by:

var resp AutopilotConfiguration qm, err := n.client.query("/v1/operator/autopilot/configuration", &resp, q) if err != nil { return nil, nil, err } return resp, qm, nil

All of the APIs should use this format. There is a query as well as write helper for PUTs

dadgar · 2018-02-07T23:25:27Z

api/operator_autopilot.go

+}
+
+// AutopilotGetConfiguration is used to query the current Autopilot configuration.
+func (op *Operator) AutopilotGetConfiguration(q *QueryOptions) (*AutopilotConfiguration, error) {


Should also return the query meta result: (*AutopilotConfiguration, *QueryMeta, error) and in the cases of writes, WriteMeta

dadgar · 2018-02-07T23:31:43Z

command/agent/operator_endpoint.go

+	if !hasID && !hasAddress {
+		resp.WriteHeader(http.StatusBadRequest)
+		fmt.Fprint(resp, "Must specify either ?id with the server's ID or ?address with IP:port of peer to remove")
+		return nil, nil


Change to be return nil, CodedError(http.StatusBadRequest, error_string) here and else where were this is used

dadgar · 2018-02-07T23:33:19Z

command/agent/operator_endpoint.go

+		return reply, nil
+
+	default:
+		resp.WriteHeader(http.StatusMethodNotAllowed)


return nil, CodedError(404, ErrInvalidMethod)

dadgar · 2018-02-07T23:34:24Z

command/agent/operator_endpoint.go

+
+	case "PUT":
+		var args structs.AutopilotSetConfigRequest
+		s.parseRegion(req, &args.Region)


parseWriteRequest

dadgar · 2018-02-07T23:46:10Z

nomad/operator_endpoint.go

+	// Check management permissions
+	if aclObj, err := op.srv.ResolveToken(args.AuthToken); err != nil {
+		return err
+	} else if aclObj != nil && !aclObj.IsManagement() {


Why not OperatorWrite?

That seems fine to me; the rest of the raft methods in here take a management token though

dadgar · 2018-02-07T23:47:33Z

nomad/server.go

@@ -777,6 +801,8 @@ func (s *Server) setupRPC(tlsWrap tlsutil.RegionWrapper) error {
 	}
 	s.rpcListener = list

+	s.logger.Printf("[INFO] nomad: RPC listening on %q", s.rpcListener.Addr().String())


Is this useful? I would say lets remove

dadgar · 2018-02-07T23:58:03Z

website/source/api/operator.html.md

+| ---------------- | ----------------- | --------------- |
+| `NO`             | `none`            | `operator:read` |
+
+### Parameters


I think we remove both of these parameters (dc should be region) but they are both covered generically on this page: https://www.nomadproject.io/api/#consistency-modes

dadgar · 2018-02-07T23:58:17Z

website/source/api/operator.html.md

+
+### Parameters
+
+- `dc` `(string: "")` - Specifies the datacenter to query. This will default to


Remove all of these across endpoints

dadgar · 2018-02-07T23:58:59Z

website/source/docs/agent/configuration/autopilot.html.md

+  <tr>
+    <th width="120">Placement</th>
+    <td>
+      <code>**acl**</code>


acl -> autopilot

github-actions · 2023-03-13T02:14:29Z

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

kyhavlov added 3 commits December 18, 2017 14:29

vendor: add autopilot and flags packages from consul

448e328

Add autopilot docs

54e6b7f

Add autopilot functionality based on Consul's autopilot

c2d0c11

kyhavlov requested review from preetapan and dadgar December 18, 2017 22:31

schmichael reviewed Jan 10, 2018

View reviewed changes

preetapan requested changes Jan 10, 2018

View reviewed changes

Add raft remove by id endpoint/command

2544587

schmichael reviewed Jan 16, 2018

View reviewed changes

Fix comments/text referring to consul

46254e1

schmichael approved these changes Jan 18, 2018

View reviewed changes

kyhavlov changed the base branch from 0.8 to master January 18, 2018 21:26

kyhavlov added 2 commits January 18, 2018 13:29

Merge branch 'master' into autopilot

4e9735d

Formatting/test adjustments

4962c8b

kyhavlov force-pushed the autopilot branch from ab85e08 to 4962c8b Compare January 18, 2018 23:03

kyhavlov merged commit 0dab89f into master Jan 19, 2018

kyhavlov deleted the autopilot branch January 19, 2018 20:53

dadgar reviewed Jan 19, 2018

View reviewed changes

dadgar reviewed Feb 8, 2018

View reviewed changes

kyhavlov mentioned this pull request Feb 13, 2018

Clean up some leftover autopilot differences from Consul #3852

Merged

github-actions bot locked as resolved and limited conversation to collaborators Mar 13, 2023


		### Parameters

		- `dc` `(string: "")` - Specifies the datacenter to query. This will default to

Add Autopilot feature from Consul #3670

Add Autopilot feature from Consul #3670

Conversation

kyhavlov commented Dec 18, 2017

schmichael left a 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

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

kyhavlov commented Jan 16, 2018

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

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

github-actions bot commented Mar 13, 2023