Merge pull request #3781 from hashicorp/f-client-fingerprint-refactor

Refactor client fingerprinters to return a diff of node attributes

CHANGELOG.md

@@ -11,6 +11,8 @@ IMPROVEMENTS:
  * cli: Use ISO_8601 time format for cli output
    [[GH-3814](https://github.com/hashicorp/nomad/pull/3814)]
  * client: Allow '.' in environment variable names [[GH-3760](https://github.com/hashicorp/nomad/issues/3760)]
+ * client: Refactor client fingerprint methods to a request/response format
+   [[GH-3781](https://github.com/hashicorp/nomad/issues/3781)]
  * discovery: Allow `check_restart` to be specified in the `service` stanza.
    [[GH-3718](https://github.com/hashicorp/nomad/issues/3718)]
  * driver/docker; Support overriding image entrypoint [[GH-3788](https://github.com/hashicorp/nomad/issues/3788)]

client/client.go

@@ -23,6 +23,7 @@ import (
 	"github.com/hashicorp/nomad/client/driver"
 	"github.com/hashicorp/nomad/client/fingerprint"
 	"github.com/hashicorp/nomad/client/stats"
+	cstructs "github.com/hashicorp/nomad/client/structs"
 	"github.com/hashicorp/nomad/client/vaultclient"
 	"github.com/hashicorp/nomad/command/agent/consul"
 	"github.com/hashicorp/nomad/helper"
@@ -931,33 +932,42 @@ func (c *Client) fingerprint() error {
 
 	c.logger.Printf("[DEBUG] client: built-in fingerprints: %v", fingerprint.BuiltinFingerprints())
 
-	var applied []string
-	var skipped []string
+	var detectedFingerprints []string
+	var skippedFingerprints []string
 	for _, name := range fingerprint.BuiltinFingerprints() {
 		// Skip modules that are not in the whitelist if it is enabled.
 		if _, ok := whitelist[name]; whitelistEnabled && !ok {
-			skipped = append(skipped, name)
+			skippedFingerprints = append(skippedFingerprints, name)
 			continue
 		}
 		// Skip modules that are in the blacklist
 		if _, ok := blacklist[name]; ok {
-			skipped = append(skipped, name)
+			skippedFingerprints = append(skippedFingerprints, name)
 			continue
 		}
+
 		f, err := fingerprint.NewFingerprint(name, c.logger)
 		if err != nil {
 			return err
 		}
 
 		c.configLock.Lock()
-		applies, err := f.Fingerprint(c.config, c.config.Node)
+		request := &cstructs.FingerprintRequest{Config: c.config, Node: c.config.Node}
+		var response cstructs.FingerprintResponse
+		err = f.Fingerprint(request, &response)
 		c.configLock.Unlock()
 		if err != nil {
 			return err
 		}
-		if applies {
-			applied = append(applied, name)
+
+		// log the fingerprinters which have been applied
+		if response.Detected {
+			detectedFingerprints = append(detectedFingerprints, name)
 		}
+
+		// add the diff found from each fingerprinter
+		c.updateNodeFromFingerprint(&response)
+
 		p, period := f.Periodic()
 		if p {
 			// TODO: If more periodic fingerprinters are added, then
@@ -966,9 +976,10 @@ func (c *Client) fingerprint() error {
 			go c.fingerprintPeriodic(name, f, period)
 		}
 	}
-	c.logger.Printf("[DEBUG] client: applied fingerprints %v", applied)
-	if len(skipped) != 0 {
-		c.logger.Printf("[DEBUG] client: fingerprint modules skipped due to white/blacklist: %v", skipped)
+
+	c.logger.Printf("[DEBUG] client: detected fingerprints %v", detectedFingerprints)
+	if len(skippedFingerprints) != 0 {
+		c.logger.Printf("[DEBUG] client: fingerprint modules skipped due to white/blacklist: %v", skippedFingerprints)
 	}
 	return nil
 }
@@ -980,10 +991,17 @@ func (c *Client) fingerprintPeriodic(name string, f fingerprint.Fingerprint, d t
 		select {
 		case <-time.After(d):
 			c.configLock.Lock()
-			if _, err := f.Fingerprint(c.config, c.config.Node); err != nil {
+			request := &cstructs.FingerprintRequest{Config: c.config, Node: c.config.Node}
+			var response cstructs.FingerprintResponse
+			err := f.Fingerprint(request, &response)
+			c.configLock.Unlock()
+
+			if err != nil {
 				c.logger.Printf("[DEBUG] client: periodic fingerprinting for %v failed: %v", name, err)
+			} else {
+				c.updateNodeFromFingerprint(&response)
 			}
-			c.configLock.Unlock()
+
 		case <-c.shutdownCh:
 			return
 		}
@@ -997,52 +1015,86 @@ func (c *Client) setupDrivers() error {
 	whitelistEnabled := len(whitelist) > 0
 	blacklist := c.config.ReadStringListToMap("driver.blacklist")
 
-	var avail []string
-	var skipped []string
+	var detectedDrivers []string
+	var skippedDrivers []string
 	driverCtx := driver.NewDriverContext("", "", c.config, c.config.Node, c.logger, nil)
 	for name := range driver.BuiltinDrivers {
 		// Skip fingerprinting drivers that are not in the whitelist if it is
 		// enabled.
 		if _, ok := whitelist[name]; whitelistEnabled && !ok {
-			skipped = append(skipped, name)
+			skippedDrivers = append(skippedDrivers, name)
 			continue
 		}
 		// Skip fingerprinting drivers that are in the blacklist
 		if _, ok := blacklist[name]; ok {
-			skipped = append(skipped, name)
+			skippedDrivers = append(skippedDrivers, name)
 			continue
 		}
 
 		d, err := driver.NewDriver(name, driverCtx)
 		if err != nil {
 			return err
 		}
+
 		c.configLock.Lock()
-		applies, err := d.Fingerprint(c.config, c.config.Node)
+		request := &cstructs.FingerprintRequest{Config: c.config, Node: c.config.Node}
+		var response cstructs.FingerprintResponse
+		err = d.Fingerprint(request, &response)
 		c.configLock.Unlock()
 		if err != nil {
 			return err
 		}
-		if applies {
-			avail = append(avail, name)
+
+		// log the fingerprinters which have been applied
+		if response.Detected {
+			detectedDrivers = append(detectedDrivers, name)
 		}
 
+		c.updateNodeFromFingerprint(&response)
+
 		p, period := d.Periodic()
 		if p {
 			go c.fingerprintPeriodic(name, d, period)
 		}
 
 	}
 
-	c.logger.Printf("[DEBUG] client: available drivers %v", avail)
-
-	if len(skipped) != 0 {
-		c.logger.Printf("[DEBUG] client: drivers skipped due to white/blacklist: %v", skipped)
+	c.logger.Printf("[DEBUG] client: detected drivers %v", detectedDrivers)
+	if len(skippedDrivers) > 0 {
+		c.logger.Printf("[DEBUG] client: drivers skipped due to white/blacklist: %v", skippedDrivers)
 	}
 
 	return nil
 }
 
+// updateNodeFromFingerprint updates the node with the result of
+// fingerprinting the node from the diff that was created
+func (c *Client) updateNodeFromFingerprint(response *cstructs.FingerprintResponse) {
+	c.configLock.Lock()
+	defer c.configLock.Unlock()
+	for name, val := range response.Attributes {
+		if val == "" {
+			delete(c.config.Node.Attributes, name)
+		} else {
+			c.config.Node.Attributes[name] = val
+		}
+	}
+
+	// update node links and resources from the diff created from
+	// fingerprinting
+	for name, val := range response.Links {
+		if val == "" {
+			delete(c.config.Node.Links, name)
+		} else {
+			c.config.Node.Links[name] = val
+		}
+	}
+
+	if response.Resources != nil {
+		c.config.Node.Resources.Merge(response.Resources)
+	}
+}
+
 // retryIntv calculates a retry interval value given the base
 func (c *Client) retryIntv(base time.Duration) time.Duration {
 	if c.config.DevMode {

client/client_test.go

@@ -14,6 +14,7 @@ import (
 	"github.com/hashicorp/consul/lib/freeport"
 	memdb "github.com/hashicorp/go-memdb"
 	"github.com/hashicorp/nomad/client/config"
+	"github.com/hashicorp/nomad/client/driver"
 	"github.com/hashicorp/nomad/client/fingerprint"
 	"github.com/hashicorp/nomad/command/agent/consul"
 	"github.com/hashicorp/nomad/helper"
@@ -252,6 +253,48 @@ func TestClient_HasNodeChanged(t *testing.T) {
 	}
 }
 
+func TestClient_Fingerprint_Periodic(t *testing.T) {
+	if _, ok := driver.BuiltinDrivers["mock_driver"]; !ok {
+		t.Skip(`test requires mock_driver; run with "-tags nomad_test"`)
+	}
+	t.Parallel()
+
+	// these constants are only defined when nomad_test is enabled, so these fail
+	// our linter without explicit disabling.
+	c1 := testClient(t, func(c *config.Config) {
+		c.Options = map[string]string{
+			driver.ShutdownPeriodicAfter:    "true", // nolint: varcheck
+			driver.ShutdownPeriodicDuration: "3",    // nolint: varcheck
+		}
+	})
+	defer c1.Shutdown()
+
+	node := c1.config.Node
+	mockDriverName := "driver.mock_driver"
+
+	// Ensure the mock driver is registered on the client
+	testutil.WaitForResult(func() (bool, error) {
+		mockDriverStatus := node.Attributes[mockDriverName]
+		if mockDriverStatus == "" {
+			return false, fmt.Errorf("mock driver attribute should be set on the client")
+		}
+		return true, nil
+	}, func(err error) {
+		t.Fatalf("err: %v", err)
+	})
+
+	// Ensure that the client fingerprinter eventually removes this attribute
+	testutil.WaitForResult(func() (bool, error) {
+		mockDriverStatus := node.Attributes[mockDriverName]
+		if mockDriverStatus != "" {
+			return false, fmt.Errorf("mock driver attribute should not be set on the client")
+		}
+		return true, nil
+	}, func(err error) {
+		t.Fatalf("err: %v", err)
+	})
+}
+
 func TestClient_Fingerprint_InWhitelist(t *testing.T) {
 	t.Parallel()
 	c := testClient(t, func(c *config.Config) {

client/driver/docker.go

@@ -26,7 +26,6 @@ import (
 	"github.com/hashicorp/go-multierror"
 	"github.com/hashicorp/go-plugin"
 	"github.com/hashicorp/nomad/client/allocdir"
-	"github.com/hashicorp/nomad/client/config"
 	"github.com/hashicorp/nomad/client/driver/env"
 	"github.com/hashicorp/nomad/client/driver/executor"
 	dstructs "github.com/hashicorp/nomad/client/driver/structs"
@@ -477,42 +476,42 @@ func NewDockerDriver(ctx *DriverContext) Driver {
 	return &DockerDriver{DriverContext: *ctx}
 }
 
-func (d *DockerDriver) Fingerprint(cfg *config.Config, node *structs.Node) (bool, error) {
-	// Initialize docker API clients
+func (d *DockerDriver) Fingerprint(req *cstructs.FingerprintRequest, resp *cstructs.FingerprintResponse) error {
 	client, _, err := d.dockerClients()
 	if err != nil {
 		if d.fingerprintSuccess == nil || *d.fingerprintSuccess {
 			d.logger.Printf("[INFO] driver.docker: failed to initialize client: %s", err)
 		}
-		delete(node.Attributes, dockerDriverAttr)
 		d.fingerprintSuccess = helper.BoolToPtr(false)
-		return false, nil
+		resp.RemoveAttribute(dockerDriverAttr)
+		return nil
 	}
 
 	// This is the first operation taken on the client so we'll try to
 	// establish a connection to the Docker daemon. If this fails it means
 	// Docker isn't available so we'll simply disable the docker driver.
 	env, err := client.Version()
 	if err != nil {
-		delete(node.Attributes, dockerDriverAttr)
 		if d.fingerprintSuccess == nil || *d.fingerprintSuccess {
 			d.logger.Printf("[DEBUG] driver.docker: could not connect to docker daemon at %s: %s", client.Endpoint(), err)
 		}
 		d.fingerprintSuccess = helper.BoolToPtr(false)
-		return false, nil
+		resp.RemoveAttribute(dockerDriverAttr)
+		return nil
 	}
 
-	node.Attributes[dockerDriverAttr] = "1"
-	node.Attributes["driver.docker.version"] = env.Get("Version")
+	resp.AddAttribute(dockerDriverAttr, "1")
+	resp.AddAttribute("driver.docker.version", env.Get("Version"))
+	resp.Detected = true
 
 	privileged := d.config.ReadBoolDefault(dockerPrivilegedConfigOption, false)
 	if privileged {
-		node.Attributes[dockerPrivilegedConfigOption] = "1"
+		resp.AddAttribute(dockerPrivilegedConfigOption, "1")
 	}
 
 	// Advertise if this node supports Docker volumes
 	if d.config.ReadBoolDefault(dockerVolumesConfigOption, dockerVolumesConfigDefault) {
-		node.Attributes["driver."+dockerVolumesConfigOption] = "1"
+		resp.AddAttribute("driver."+dockerVolumesConfigOption, "1")
 	}
 
 	// Detect bridge IP address - #2785
@@ -530,7 +529,7 @@ func (d *DockerDriver) Fingerprint(cfg *config.Config, node *structs.Node) (bool
 			}
 
 			if n.IPAM.Config[0].Gateway != "" {
-				node.Attributes["driver.docker.bridge_ip"] = n.IPAM.Config[0].Gateway
+				resp.AddAttribute("driver.docker.bridge_ip", n.IPAM.Config[0].Gateway)
 			} else if d.fingerprintSuccess == nil {
 				// Docker 17.09.0-ce dropped the Gateway IP from the bridge network
 				// See https://github.com/moby/moby/issues/32648
@@ -541,7 +540,7 @@ func (d *DockerDriver) Fingerprint(cfg *config.Config, node *structs.Node) (bool
 	}
 
 	d.fingerprintSuccess = helper.BoolToPtr(true)
-	return true, nil
+	return nil
 }
 
 // Validate is used to validate the driver configuration