Add go-discover support to Nomad clients #4277

chelseakomlo · 2018-05-10T12:35:17Z

No description provided.

dadgar · 2018-05-21T22:17:29Z

command/agent/config-test-fixtures/basic.hcl

@@ -29,6 +30,13 @@ client {
 		foo = "bar"
 		baz = "zip"
 	}
+  server_join_info {


dadgar · 2018-05-21T22:17:36Z

command/agent/config-test-fixtures/basic.hcl

@@ -86,6 +94,12 @@ server {
 	redundancy_zone = "foo"
 	upgrade_version = "0.8.0"
 	encrypt = "abc"
+  server_join_info {


dadgar · 2018-05-21T22:19:07Z

command/agent/command.go

+			serverEnabled: true,
+		}
+
+		// This is for backwards compatibility, this should be removed in Nomad


Use // COMPAT: Remove in 0.10 and only use ServerJoin

Allows us to search backwards compatibility hacks

dadgar · 2018-05-21T22:20:34Z

command/agent/config.go

@@ -217,6 +217,9 @@ type ClientConfig struct {
 	// NoHostUUID disables using the host's UUID and will force generation of a
 	// random UUID.
 	NoHostUUID *bool `mapstructure:"no_host_uuid"`
+
+	// ServerJoin contains information that is used to attempt to join servers
+	ServerJoin *ServerJoin `mapstructure:"server_join_info"`


why _info?

+1, would suggest calling this server_join or server_join_config maybe?

This is left over from a rename, I'll move to just server_join

dadgar · 2018-05-21T22:21:29Z

command/agent/config-test-fixtures/basic.hcl

@@ -29,6 +30,13 @@ client {
 		foo = "bar"
 		baz = "zip"
 	}
+  server_join_info {
+    retry_join = [ "1.1.1.1", "2.2.2.2" ]
+      start_join = [ "1.1.1.1", "2.2.2.2" ]


Start_join isn't supported on client?

dadgar · 2018-05-21T22:24:26Z

command/agent/retry_join_test.go

+
+	require.Equal(1, len(output))
+	require.Equal(stubAddress, output[0])
+}


Can we do a client and server test

I changed existing tests to be server-specific, but I'll mark it in the test name as such.

dadgar · 2018-05-21T22:25:34Z

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

@@ -90,6 +90,22 @@ client {
  receive work. This may be specified as an IP address or DNS, with or without
  the port. If the port is omitted, the default port of `4647` is used.

+- `server_join` `(map[string]string` - Specifies the list of server information


I don't think map[string]string is the right type? It should be its own stanza similar to: https://www.nomadproject.io/docs/agent/configuration/acl.html

dadgar · 2018-05-21T22:27:28Z

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

@@ -148,6 +148,13 @@ server {
  made before exiting with a return code of 1. By default, this is set to 0
  which is interpreted as infinite retries.

+- `server_join` `(map[string]string` - Specifies the list of server information


Point this to the new stanza docs

dadgar · 2018-05-21T22:27:40Z

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

@@ -131,8 +131,8 @@ server {
  cluster again when starting. This flag allows the previous state to be used to
  rejoin the cluster.

- `retry_join` `(array<string>: [])` - Specifies a list of server addresses to
-  retry joining if the first attempt fails. This is similar to
+- `retry_join` `(array<string>: [])` - Specifies a list of server


Document that these should be deprecated in favor of server_join

dadgar · 2018-05-21T22:28:36Z

command/agent/command.go

-		discover: &discover.Discover{},
-		errCh:    c.retryJoinErrCh,
-		logger:   c.agent.logger,
+	if config.Server.Enabled && len(config.Server.RetryJoin) != 0 {


Make sure there is verification that:

On the server either, config.Server.RetryJoin/StartJoin/etc is set or the new ServerJoin is set and not both

On the client that StartupJoin isn't set.

preetapan · 2018-05-21T22:24:14Z

command/agent/agent_endpoint.go

@@ -222,7 +222,7 @@ func (s *HTTPServer) updateServers(resp http.ResponseWriter, req *http.Request)

 	// Set the servers list into the client
 	s.agent.logger.Printf("[TRACE] Adding servers %+q to the client's primary server list", servers)
-	if err := client.SetServers(servers); err != nil {
+	if _, err := client.SetServers(servers); err != nil {


Not sure why SetServers changed to return the number of endpoints if its not being used here where its called

We decided to change this so it has the same interface as Join for servers, as we both use them as handles to RetryJoiner.

preetapan · 2018-05-21T22:32:14Z

command/agent/command.go

+		}
+		go joiner.RetryJoin(serverJoinInfo)
+	}
+


Seems like only one of the two (either config.Server.RetryJoin or config.Server.RetryJoin) should be set? If both are set it should fail validation

Fixed in adding a Validate method for all edge cases like this.

preetapan · 2018-05-21T22:33:56Z

command/agent/config.go

+	// addresses, then the agent will error and exit.
+	StartJoin []string `mapstructure:"start_join"`
+
+	// RetryJoin is a list of addresses to join with retry enabled.


Should we mention here that we can use a single value in the array to find multiple servers using go discover syntax?

preetapan · 2018-05-21T22:34:57Z

command/agent/config.go

@@ -217,6 +217,9 @@ type ClientConfig struct {
 	// NoHostUUID disables using the host's UUID and will force generation of a
 	// random UUID.
 	NoHostUUID *bool `mapstructure:"no_host_uuid"`
+
+	// ServerJoin contains information that is used to attempt to join servers
+	ServerJoin *ServerJoin `mapstructure:"server_join_info"`


+1, would suggest calling this server_join or server_join_config maybe?

dadgar · 2018-05-24T17:11:25Z

command/agent/config-test-fixtures/basic.hcl

@@ -29,6 +30,12 @@ client {
 		foo = "bar"
 		baz = "zip"
 	}
+  server_join {


Fix the indentation

dadgar · 2018-05-24T17:12:21Z

command/agent/config.go

@@ -1055,6 +1088,9 @@ func (a *ServerConfig) Merge(b *ServerConfig) *ServerConfig {
 	if b.EncryptKey != "" {
 		result.EncryptKey = b.EncryptKey
 	}
+	if b.ServerJoin != nil {
+		result.ServerJoin = b.ServerJoin


This is overriding not merging. You need a merge method on the ServerJoin object

dadgar · 2018-05-24T17:13:54Z

command/agent/retry_join.go

+	// fields and the server_join stanza are not both set
+	if config.Server != nil && config.Server.ServerJoin != nil {
+		if len(config.Server.RetryJoin) != 0 {
+			return fmt.Errorf("server_join and retry_join cannot both be defined; try defining only server_join")


try defining only -> prefer

server_join and retry_join cannot both be defined; prefer setting the server_join parameter.

dadgar · 2018-05-24T17:17:06Z

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

@@ -90,6 +90,9 @@ client {
  receive work. This may be specified as an IP address or DNS, with or without
  the port. If the port is omitted, the default port of `4647` is used.

+- `server_join` <code>([ServerJoin][server_join]: nil)</code> - Specifies


Is this link clickable?

dadgar · 2018-05-24T17:18:43Z

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

@@ -196,6 +207,17 @@ unless configured otherwise:
 nomad-01.company.local => nomad-01.company.local:4648
 ```

+#### Via the go-discover interface
+
+As of Nomad 0.9, `retry-join` accepts a unified interface using the


0.8.4 and you should document it via the new stanza

dadgar · 2018-05-24T17:23:33Z

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

@@ -90,6 +90,9 @@ client {
  receive work. This may be specified as an IP address or DNS, with or without
  the port. If the port is omitted, the default port of `4647` is used.

+- `server_join` <code>([ServerJoin][server_join]: nil)</code> - Specifies
+  configuration which is specific to retry joining Nomad servers.


Specifies how the Nomad client will connect to Nomad servers. The start_join field is not supported on the client. The retry_join fields may directly specify the server address or use go-discover syntax for auto-discovery. See the documentation for more detail.

Can you use the text I wrote

dadgar · 2018-05-24T17:23:53Z

website/source/docs/agent/configuration/cloud-auto-join.html.md

+page_title: "Cloud Auto-join"
+sidebar_current: "docs-agent-cloud-auto-join"
+description: |-
+  Nomad supports automatic cluster joining using cloud metadata on various providers.


on -> from various cloud providers

dadgar · 2018-05-24T17:25:43Z

website/source/docs/agent/configuration/cloud-auto-join.html.md

+  Nomad supports automatic cluster joining using cloud metadata on various providers.
+---
+
+# Cloud Auto-joining


There should be a call out somewhere that this is only a subset of the cloud providers and for the most up to date documentation see the library itself

Maybe use the yellow here: https://www.nomadproject.io/guides/namespaces.html

dadgar · 2018-05-24T17:27:09Z

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

+page_title: "server_join Stanza - Agent Configuration"
+sidebar_current: "docs-agent-configuration-server_join"
+description: |-
+  The "server_join" stanza configures the Nomad agent to enable retry_join logic for connecting to Nomad servers.


The server_join stanza specifies how the Nomad agent will discover and connect to Nomad servers.

dadgar · 2018-05-24T17:28:21Z

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

+
+#### Via the go-discover interface
+
+As of Nomad 0.9, `retry-join` accepts a unified interface using the


0.8.4.

This above section looks like a copy of what is on the server page. We shouldn't duplicate. Probably remove that one?

dadgar · 2018-05-25T01:07:20Z

command/agent/retry_join.go

@@ -82,6 +82,33 @@ func (r *retryJoiner) Validate(config *Config) error {
 		}
 	}

+	if config.Server != nil {
+		dur, err := time.ParseDuration(config.Server.RetryInterval)


Just change the struct to parse directly into the time value to avoid this?

nomad/command/agent/config.go

Line 300 in c42a07b

HeartbeatGrace time.Duration `mapstructure:"heartbeat_grace"`

dadgar · 2018-05-25T01:09:25Z

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

@@ -90,6 +90,9 @@ client {
  receive work. This may be specified as an IP address or DNS, with or without
  the port. If the port is omitted, the default port of `4647` is used.

+- `server_join` <code>([ServerJoin][server_join]: nil)</code> - Specifies
+  configuration which is specific to retry joining Nomad servers.


Can you use the text I wrote

dadgar · 2018-05-25T01:10:30Z

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

@@ -131,24 +131,26 @@ server {
  cluster again when starting. This flag allows the previous state to be used to
  rejoin the cluster.

+- `server_join` - Specifies the [Server Join][server_join] stanza for


Use the same syntax as the rest of the parameters. <Name of the parameter> <type> - <Text describing>

dadgar · 2018-05-25T01:12:02Z

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

@@ -165,59 +167,6 @@ server {
  on the format of the string. This field is  deprecated in favor of


The address format link is broken

dadgar · 2018-05-25T21:28:07Z

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

@@ -106,7 +106,7 @@ nomad-01.company.local => nomad-01.company.local:4648

 #### Via the go-discover interface

-As of Nomad 0.9, `retry-join` accepts a unified interface using the
+As of Nomad 0.8.4, `retry-join` accepts a unified interface using the


retry-join -> retry_join

dadgar · 2018-05-29T20:35:15Z

command/agent/retry_join.go

@@ -70,7 +70,7 @@ func (r *retryJoiner) Validate(config *Config) error {
 			return fmt.Errorf("server_join and retry_max cannot both be defined; try defining only server_join")
 		}
 		if config.Server.RetryInterval != "0" {
-			return fmt.Errorf("server_join and retry_interval cannot both be defined; try defining only server_join")
+			return fmt.Errorf("server_join and retry_interval cannot both be defined; prefer setting the server_join parameter")


Please update all the errors to the suggested format

dadgar · 2018-05-29T20:49:47Z

command/agent/retry_join.go

-			return fmt.Errorf("server_join and retry_interval cannot both be defined; prefer setting the server_join parameter")
+		if config.Server.RetryInterval != "0" && config.Server.RetryInterval != "" {
+			// 30s is the default value that is set, ignore if this is the case
+			if config.Server.RetryInterval != "30s" {


Hmm this is quite odd. I wonder if there is a more elegant way to handle this.

clean up styling of new pages

documentation fixes

add additional validation case

This commit: * Improves how we combine the old retry-* fields and the new stanza and how it is validated * Handles the new stanza setting start_join * Fixes integration test to not bind to the standard port and instead be randomized. * Simplifies parsing of the old retry_interval * Fixes the errors from retry join being masked * Flags get parsed into new server_join stanza

preetapan · 2018-05-31T19:23:22Z

website/source/docs/commands/agent.html.md.erb

+  $ nomad agent -retry-join "127.0.0.1:4648"
+  ```
+
+    Note that `retry_join` can be defined for only servers as a command-line


reword as : retry-join can be defined as a command line flag only for servers. Clients can configure retry-join only in configuration files.

github-actions · 2023-03-03T02:47:08Z

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.

chelseakomlo force-pushed the f-retry-join-clients branch 3 times, most recently from d478c00 to 47ef99e Compare May 10, 2018 21:30

chelseakomlo force-pushed the f-retry-join-clients branch 6 times, most recently from ebd6dd4 to 5498a3a Compare May 19, 2018 19:18

chelseakomlo changed the title ~~[WIP] Add go-discover support to Nomad clients~~ Add go-discover support to Nomad clients May 21, 2018

chelseakomlo force-pushed the f-retry-join-clients branch 3 times, most recently from 606e3ba to 27151de Compare May 21, 2018 17:39

chelseakomlo requested review from preetapan and dadgar May 21, 2018 17:40

dadgar requested changes May 21, 2018

View reviewed changes

preetapan suggested changes May 21, 2018

View reviewed changes

chelseakomlo force-pushed the f-retry-join-clients branch 2 times, most recently from dbf77d0 to 7dcae2e Compare May 23, 2018 23:34

dadgar requested changes May 24, 2018

View reviewed changes

dadgar requested changes May 25, 2018

View reviewed changes

chelseakomlo force-pushed the f-retry-join-clients branch from e377e57 to 82f0a38 Compare May 25, 2018 21:13

dadgar reviewed May 25, 2018

View reviewed changes

chelseakomlo force-pushed the f-retry-join-clients branch 2 times, most recently from 5e381ce to 1707b3c Compare May 26, 2018 00:22

dadgar requested changes May 29, 2018

View reviewed changes

dadgar force-pushed the f-retry-join-clients branch from 27610f1 to 4f86d2c Compare May 30, 2018 20:46

chelseakomlo added 3 commits May 31, 2018 10:50

add server join info to server and client

1a854c4

add documentation

f68a040

update server_join naming and improve logging

02b89ae

chelseakomlo and others added 17 commits May 31, 2018 10:50

update documentation for server_join

0770f03

add further configuration validation for server_join

b4a0f2c

cloud auto join documentation

98f2947

add to sidebar navidation

ef90b44

clean up styling of new pages

set retryInterval and other code feedback

023cc2c

update documentation

e93dc53

links to server_join docs

58a6131

fix doc links and other improvements

02a64d0

RetryInterval should be a time.Duration

fa1d2de

update config parse test

94cf42e

documentation fixes

add stronger protections for nil pointers in server join merge

236ac65

ignore default values for retry interval

3d12b3f

add additional validation case

ensure default value of 30s is set for server_join stanza

4b20a69

indentation

907def0

validation errors

13f8e91

Fix website

92096d6

dadgar force-pushed the f-retry-join-clients branch from f9d2bea to 92096d6 Compare May 31, 2018 17:53

preetapan reviewed May 31, 2018

View reviewed changes

preetapan approved these changes May 31, 2018

View reviewed changes

retry-join flag docs

84b4e2c

dadgar merged commit d834439 into master Jun 1, 2018

dadgar deleted the f-retry-join-clients branch June 1, 2018 16:57

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


		#### Via the go-discover interface

		As of Nomad 0.9, `retry-join` accepts a unified interface using the

		@@ -165,59 +167,6 @@ server {
		on the format of the string. This field is deprecated in favor of

Add go-discover support to Nomad clients #4277

Add go-discover support to Nomad clients #4277

Conversation

chelseakomlo commented May 10, 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

chelseakomlo May 21, 2018 • edited Loading

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

chelseakomlo May 23, 2018 • edited Loading

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

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 3, 2023

chelseakomlo May 21, 2018 •

edited

Loading

chelseakomlo May 23, 2018 •

edited

Loading