From 283109358c3a1de0069e1fe25c604ffd343ee350 Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Wed, 21 Feb 2018 13:37:09 -0500 Subject: [PATCH] driver info should be set by both fingerprint and health check --- client/client.go | 18 +++++++- client/client_test.go | 15 +++++++ client/driver/docker.go | 4 +- client/driver/docker_test.go | 2 +- client/driver/mock_driver.go | 20 +++++---- client/fingerprint_manager.go | 27 +++++++----- client/fingerprint_manager_test.go | 71 ++++++++++++++++++++---------- client/structs/structs.go | 13 ++++++ nomad/structs/structs.go | 11 +++++ 9 files changed, 132 insertions(+), 49 deletions(-) diff --git a/client/client.go b/client/client.go index 1ae49675940d..17705fa74bb0 100644 --- a/client/client.go +++ b/client/client.go @@ -952,6 +952,18 @@ func (c *Client) updateNodeFromFingerprint(response *cstructs.FingerprintRespons c.config.Node.Resources.Merge(response.Resources) } + for name, new_val := range response.Drivers { + old_val := c.config.Node.Drivers[name] + if new_val.Equals(old_val) { + continue + } + if old_val == nil { + c.config.Node.Drivers[name] = new_val + } else { + c.config.Node.Drivers[name].MergeFingerprintInfo(new_val) + } + } + return c.config.Node } @@ -965,7 +977,11 @@ func (c *Client) updateNodeFromHealthCheck(response *cstructs.HealthCheckRespons if new_val.Equals(old_val) { continue } - c.config.Node.Drivers[name] = new_val + if old_val == nil { + c.config.Node.Drivers[name] = new_val + } else { + c.config.Node.Drivers[name].MergeHealthCheck(new_val) + } } return c.config.Node diff --git a/client/client_test.go b/client/client_test.go index da6a14fb7b4c..086b0ba0b402 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -278,6 +278,21 @@ func TestClient_Fingerprint_Periodic(t *testing.T) { if mockDriverStatus == "" { return false, fmt.Errorf("mock driver attribute should be set on the client") } + + // assert that the Driver information for the node is also set correctly + mockDriverInfo := node.Drivers["mock_driver"] + if mockDriverInfo == nil { + return false, fmt.Errorf("mock driver is nil when it should be set on node Drivers") + } + if !mockDriverInfo.Detected { + return false, fmt.Errorf("mock driver should be set as healthy") + } + if !mockDriverInfo.Healthy { + return false, fmt.Errorf("mock driver should be set as healthy") + } + if mockDriverInfo.HealthDescription == "" { + return false, fmt.Errorf("mock driver description should not be empty") + } return true, nil }, func(err error) { t.Fatalf("err: %v", err) diff --git a/client/driver/docker.go b/client/driver/docker.go index 343a13fc0284..e99571287631 100644 --- a/client/driver/docker.go +++ b/client/driver/docker.go @@ -560,7 +560,7 @@ func (d *DockerDriver) HealthCheck(req *cstructs.HealthCheckRequest, resp *cstru _, _, err := d.dockerClients() if err != nil { d.logger.Printf("[WARN] driver.docker: docker driver is available but is unresponsive to `docker ps`") - resp.AddDriverInfo("driver.docker", unhealthy) + resp.AddDriverInfo("docker", unhealthy) return err } @@ -570,7 +570,7 @@ func (d *DockerDriver) HealthCheck(req *cstructs.HealthCheckRequest, resp *cstru HealthDescription: "Docker driver is available and responsive", UpdateTime: time.Now(), } - resp.AddDriverInfo("driver.docker", healthy) + resp.AddDriverInfo("docker", healthy) return nil } diff --git a/client/driver/docker_test.go b/client/driver/docker_test.go index 12b4ad2a9e26..60ada85e2bdc 100644 --- a/client/driver/docker_test.go +++ b/client/driver/docker_test.go @@ -289,7 +289,7 @@ func TestDockerDriver_Check_DockerHealthStatus(t *testing.T) { err = dc.HealthCheck(request, &response) require.Nil(err) - driverInfo := response.Drivers["driver.docker"] + driverInfo := response.Drivers["docker"] require.NotNil(driverInfo) require.True(driverInfo.Healthy) } diff --git a/client/driver/mock_driver.go b/client/driver/mock_driver.go index 5c66b1223321..0c5db20fbe76 100644 --- a/client/driver/mock_driver.go +++ b/client/driver/mock_driver.go @@ -238,22 +238,24 @@ func (m *MockDriver) Fingerprint(req *cstructs.FingerprintRequest, resp *cstruct // HealthCheck implements the interface for HealthCheck, and indicates the current // health status of the mock driver. func (m *MockDriver) HealthCheck(req *cstructs.HealthCheckRequest, resp *cstructs.HealthCheckResponse) error { - if !m.shutdownFingerprintTime.IsZero() && time.Now().After(m.shutdownFingerprintTime) { + switch { + case !m.shutdownFingerprintTime.IsZero() && time.Now().After(m.shutdownFingerprintTime): notHealthy := &structs.DriverInfo{ Healthy: false, HealthDescription: "not running", UpdateTime: time.Now(), } - resp.AddDriverInfo(mockDriverName, notHealthy) + resp.AddDriverInfo("mock_driver", notHealthy) + return nil + default: + healthy := &structs.DriverInfo{ + Healthy: true, + HealthDescription: "running", + UpdateTime: time.Now(), + } + resp.AddDriverInfo("mock_driver", healthy) return nil } - healthy := &structs.DriverInfo{ - Healthy: true, - HealthDescription: "running", - UpdateTime: time.Now(), - } - resp.AddDriverInfo(mockDriverName, healthy) - return nil } // GetHealthCheckInterval implements the interface for HealthCheck and indicates diff --git a/client/fingerprint_manager.go b/client/fingerprint_manager.go index cfd2ca8385e4..4ef4c3cc89f0 100644 --- a/client/fingerprint_manager.go +++ b/client/fingerprint_manager.go @@ -114,7 +114,10 @@ func (fm *FingerprintManager) setupDrivers(drivers []string) error { go fm.runFingerprint(d, period, name) } - if hc, ok := d.(fingerprint.HealthCheck); ok { + // We should only run the health check task if the driver is detected + // Note that if the driver is detected later in a periodic health check, + // this won't automateically trigger the periodic health check. + if hc, ok := d.(fingerprint.HealthCheck); ok && detected { req := &cstructs.HealthCheckIntervalRequest{} resp := &cstructs.HealthCheckIntervalResponse{} hc.GetHealthCheckInterval(req, resp) @@ -140,6 +143,17 @@ func (fm *FingerprintManager) fingerprintDriver(name string, f fingerprint.Finge } fm.nodeLock.Lock() + + // TODO This is a temporary measure, as eventually all drivers will need to + // support this. Doing this so that we can enable this iteratively and also + // in a backwards compatible way, where node attributes for drivers will + // eventually be phased out. + di := &structs.DriverInfo{ + Attributes: response.Attributes, + Detected: response.Detected, + } + response.AddDriver(name, di) + if node := fm.updateNodeAttributes(&response); node != nil { fm.node = node } @@ -148,17 +162,6 @@ func (fm *FingerprintManager) fingerprintDriver(name string, f fingerprint.Finge if hc, ok := f.(fingerprint.HealthCheck); ok { fm.healthCheck(name, hc) } else { - // This is a temporary measure, as eventually all drivers will need to - // support this. Doing this so that we can enable this iteratively and also - // in a backwards compatible way, where node attributes for drivers will - // eventually be phased out. - - di := &structs.DriverInfo{ - Attributes: response.Attributes, - Detected: response.Detected, - Healthy: response.Detected, - UpdateTime: time.Now(), - } resp := &cstructs.HealthCheckResponse{ Drivers: map[string]*structs.DriverInfo{ diff --git a/client/fingerprint_manager_test.go b/client/fingerprint_manager_test.go index 58bf5aba02bc..ca633a40cbe2 100644 --- a/client/fingerprint_manager_test.go +++ b/client/fingerprint_manager_test.go @@ -56,6 +56,7 @@ func TestFingerprintManager_Fingerprint_Run(t *testing.T) { node := &structs.Node{ Attributes: make(map[string]string, 0), + Drivers: make(map[string]*structs.DriverInfo, 0), } conf := config.DefaultConfig() @@ -68,6 +69,9 @@ func TestFingerprintManager_Fingerprint_Run(t *testing.T) { for k, v := range r.Attributes { node.Attributes[k] = v } + for k, v := range r.Drivers { + node.Drivers[k] = v + } return node } updateHealthCheck := func(resp *cstructs.HealthCheckResponse) *structs.Node { @@ -87,6 +91,7 @@ func TestFingerprintManager_Fingerprint_Run(t *testing.T) { require.Nil(err) require.NotEqual("", node.Attributes["driver.raw_exec"]) + require.True(node.Drivers["raw_exec"].Detected) } func TestFingerprintManager_Fingerprint_Periodic(t *testing.T) { @@ -183,6 +188,7 @@ func TestFingerprintManager_HealthCheck_Driver(t *testing.T) { } conf := config.DefaultConfig() conf.Options = map[string]string{ + "driver.raw_exec.enable": "1", "test.shutdown_periodic_after": "true", "test.shutdown_periodic_duration": "2", } @@ -213,7 +219,7 @@ func TestFingerprintManager_HealthCheck_Driver(t *testing.T) { if mockDriverAttribute == "" { return false, fmt.Errorf("mock driver info should be set on the client attributes") } - mockDriverInfo := node.Drivers["driver.mock_driver"] + mockDriverInfo := node.Drivers["mock_driver"] if mockDriverInfo == nil { return false, fmt.Errorf("mock driver info should be set on the client") } @@ -222,27 +228,26 @@ func TestFingerprintManager_HealthCheck_Driver(t *testing.T) { } return true, nil }, func(err error) { - - // Ensure that a default driver without health checks enabled is registered and healthy on the client - testutil.WaitForResult(func() (bool, error) { - rawExecAttribute := node.Attributes["driver.raw_exec"] - if rawExecAttribute == "" { - return false, fmt.Errorf("raw exec info should be set on the client attributes") - } - rawExecInfo := node.Drivers["driver.raw_exec"] - if rawExecInfo == nil { - return false, fmt.Errorf("raw exec info should be set on the client") - } - if !rawExecInfo.Healthy { - return false, fmt.Errorf("raw exec info should be healthy") - } - return true, nil - }, func(err error) { - t.Fatalf("err: %v", err) - }) t.Fatalf("err: %v", err) }) + // Ensure that a default driver without health checks enabled is registered and healthy on the client + testutil.WaitForResult(func() (bool, error) { + rawExecAttribute := node.Attributes["driver.raw_exec"] + if rawExecAttribute == "" { + return false, fmt.Errorf("raw exec info should be set on the client attributes") + } + rawExecInfo := node.Drivers["raw_exec"] + if rawExecInfo == nil { + return false, fmt.Errorf("raw exec driver info should be set on the client") + } + if !rawExecInfo.Detected { + return false, fmt.Errorf("raw exec driver should be detected") + } + return true, nil + }, func(err error) { + t.Fatalf("err: %v", err) + }) } func TestFingerprintManager_HealthCheck_Periodic(t *testing.T) { @@ -252,12 +257,23 @@ func TestFingerprintManager_HealthCheck_Periodic(t *testing.T) { node := &structs.Node{ Drivers: make(map[string]*structs.DriverInfo, 0), } - updateNode := func(r *cstructs.FingerprintResponse) *structs.Node { + updateNode := func(resp *cstructs.FingerprintResponse) *structs.Node { + for k, v := range resp.Drivers { + if node.Drivers[k] == nil { + node.Drivers[k] = v + } else { + node.Drivers[k].MergeFingerprintInfo(v) + } + } return node } updateHealthCheck := func(resp *cstructs.HealthCheckResponse) *structs.Node { for k, v := range resp.Drivers { - node.Drivers[k] = v + if node.Drivers[k] == nil { + node.Drivers[k] = v + } else { + node.Drivers[k].MergeHealthCheck(v) + } } return node } @@ -289,10 +305,13 @@ func TestFingerprintManager_HealthCheck_Periodic(t *testing.T) { // Ensure the mock driver is registered and healthy on the client testutil.WaitForResult(func() (bool, error) { - mockDriverInfo := node.Drivers["driver.mock_driver"] + mockDriverInfo := node.Drivers["mock_driver"] if mockDriverInfo == nil { return false, fmt.Errorf("mock driver info should be set on the client") } + if !mockDriverInfo.Detected { + return false, fmt.Errorf("mock driver info should be detected") + } if !mockDriverInfo.Healthy { return false, fmt.Errorf("mock driver info should be healthy") } @@ -304,12 +323,16 @@ func TestFingerprintManager_HealthCheck_Periodic(t *testing.T) { // Ensure that the client health check eventually removes this attribute and // marks the driver as unhealthy testutil.WaitForResult(func() (bool, error) { - mockDriverInfo := node.Drivers["driver.mock_driver"] + + mockDriverInfo := node.Drivers["mock_driver"] if mockDriverInfo == nil { return false, fmt.Errorf("mock driver info should be set on the client") } + if !mockDriverInfo.Detected { + return false, fmt.Errorf("mock driver should be detected") + } if mockDriverInfo.Healthy { - return false, fmt.Errorf("mock driver info should not be healthy") + return false, fmt.Errorf("mock driver should not be healthy") } return true, nil }, func(err error) { diff --git a/client/structs/structs.go b/client/structs/structs.go index c36ae8501f42..3675a29b63e9 100644 --- a/client/structs/structs.go +++ b/client/structs/structs.go @@ -206,6 +206,19 @@ type FingerprintResponse struct { // Detected is a boolean indicating whether the fingerprinter detected // if the resource was available Detected bool + + // Drivers is a map of driver names to driver info. This allows the + // fingerprint method of each driver to set whether the driver is enabled or + // not, as well as its attributes + Drivers map[string]*structs.DriverInfo +} + +func (f *FingerprintResponse) AddDriver(name string, value *structs.DriverInfo) { + if f.Drivers == nil { + f.Drivers = make(map[string]*structs.DriverInfo, 0) + } + + f.Drivers[name] = value } // AddAttribute adds the name and value for a node attribute to the fingerprint diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 71a509564cd1..69b26d1bb160 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -1072,6 +1072,17 @@ type DriverInfo struct { UpdateTime time.Time } +func (di *DriverInfo) MergeHealthCheck(other *DriverInfo) { + di.Healthy = other.Healthy + di.HealthDescription = other.HealthDescription + di.UpdateTime = other.UpdateTime +} + +func (di *DriverInfo) MergeFingerprintInfo(other *DriverInfo) { + di.Detected = other.Detected + di.Attributes = other.Attributes +} + func (di *DriverInfo) Equals(other *DriverInfo) bool { if di == nil && other == nil { return true