From 53a5bc2bb35b1e41665c670ce882ed01fb8b30b9 Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Fri, 9 Mar 2018 12:28:01 -0500 Subject: [PATCH] Code review feedback --- client/client.go | 76 ++++++++++--- client/fingerprint_manager.go | 173 +++++++++++++---------------- client/fingerprint_manager_test.go | 41 +++---- client/structs/structs.go | 13 --- 4 files changed, 156 insertions(+), 147 deletions(-) diff --git a/client/client.go b/client/client.go index 7a82824aa..6de239d38 100644 --- a/client/client.go +++ b/client/client.go @@ -259,7 +259,8 @@ func NewClient(cfg *config.Config, consulCatalog consul.CatalogAPI, consulServic } fingerprintManager := NewFingerprintManager(c.GetConfig, c.config.Node, - c.shutdownCh, c.updateNodeFromFingerprint, c.updateNodeFromHealthCheck, c.logger) + c.shutdownCh, c.updateNodeFromFingerprint, c.updateNodeFromHealthCheck, + c.updateNodeFromDriver, c.logger) // Fingerprint the node and scan for drivers if err := fingerprintManager.Run(); err != nil { @@ -1006,23 +1007,66 @@ func (c *Client) updateNodeFromFingerprint(response *cstructs.FingerprintRespons c.config.Node.Resources.Merge(response.Resources) } - // XXX -------------- - // All drivers only expose DriverInfo - // This becomes a separate setter. So we have: - // * updateNodeFromFingerprinters - // * updateNodeFromDriver(fingerprint, health *DriverInfo) - /// TODO is anything updating the Node.Attributes based on updating - // driverinfos + if nodeHasChanged { + c.updateNode() + } - for name, newVal := range response.Drivers { - oldVal := c.config.Node.Drivers[name] - if oldVal == nil && newVal != nil { - c.config.Node.Drivers[name] = newVal - } else if newVal != nil && newVal.Detected != oldVal.Detected { - c.config.Node.Drivers[name].MergeFingerprintInfo(newVal) + return c.config.Node +} + +// updateNodeFromDriver receives either a fingerprint of the driver or its +// health and merges this into a single DriverInfo object +func (c *Client) updateNodeFromDriver(name string, fingerprint, health *structs.DriverInfo) *structs.Node { + c.configLock.Lock() + defer c.configLock.Unlock() + + var hasChanged bool + + if fingerprint != nil { + if c.config.Node.Drivers[name] == nil { + hasChanged = true + c.config.Node.Drivers[name] = fingerprint + } else { + if c.config.Node.Drivers[name].Detected != fingerprint.Detected { + hasChanged = true + c.config.Node.Drivers[name].Detected = fingerprint.Detected + } + + for attrName, newVal := range fingerprint.Attributes { + oldVal := c.config.Node.Drivers[name].Attributes[attrName] + if oldVal == newVal { + continue + } + + hasChanged = true + if newVal == "" { + delete(c.config.Node.Attributes, attrName) + } else { + c.config.Node.Attributes[attrName] = newVal + } + } } } - if nodeHasChanged { + + if health != nil { + if c.config.Node.Drivers[name] == nil { + hasChanged = true + c.config.Node.Drivers[name] = health + } else { + oldVal := c.config.Node.Drivers[name] + if !health.HealthCheckEquals(oldVal) { + hasChanged = true + c.config.Node.Drivers[name].MergeHealthCheck(health) + } else { + // make sure we accurately reflect the last time a health check has been + // performed for the driver. + oldVal.UpdateTime = health.UpdateTime + } + } + } + + if hasChanged { + c.config.Node.Drivers[name].UpdateTime = time.Now() c.updateNode() } @@ -1060,6 +1104,7 @@ func watcher(driver Driver) { // updateNodeFromHealthCheck receives a health check response and updates the // node accordingly +// TODO is this even needed? func (c *Client) updateNodeFromHealthCheck(response *cstructs.HealthCheckResponse) *structs.Node { c.configLock.Lock() defer c.configLock.Unlock() @@ -1083,7 +1128,6 @@ func (c *Client) updateNodeFromHealthCheck(response *cstructs.HealthCheckRespons if oldVal == nil { c.config.Node.Drivers[name] = newVal } else { - c.config.Node.Drivers[name].MergeHealthCheck(newVal) } } diff --git a/client/fingerprint_manager.go b/client/fingerprint_manager.go index 6a8618f96..9f257c375 100644 --- a/client/fingerprint_manager.go +++ b/client/fingerprint_manager.go @@ -28,7 +28,9 @@ type FingerprintManager struct { // updateHealthCheck is a callback to the client to update the state of the // node for resources that require a health check updateHealthCheck func(*cstructs.HealthCheckResponse) *structs.Node - logger *log.Logger + + updateNodeFromDriver func(string, *structs.DriverInfo, *structs.DriverInfo) *structs.Node + logger *log.Logger } // NewFingerprintManager is a constructor that creates and returns an instance @@ -38,11 +40,13 @@ func NewFingerprintManager(getConfig func() *config.Config, shutdownCh chan struct{}, updateNodeAttributes func(*cstructs.FingerprintResponse) *structs.Node, updateHealthCheck func(*cstructs.HealthCheckResponse) *structs.Node, + updateNodeFromDriver func(string, *structs.DriverInfo, *structs.DriverInfo) *structs.Node, logger *log.Logger) *FingerprintManager { return &FingerprintManager{ getConfig: getConfig, updateNodeAttributes: updateNodeAttributes, updateHealthCheck: updateHealthCheck, + updateNodeFromDriver: updateNodeFromDriver, node: node, shutdownCh: shutdownCh, logger: logger, @@ -53,9 +57,14 @@ func NewFingerprintManager(getConfig func() *config.Config, func (fm *FingerprintManager) runFingerprint(f fingerprint.Fingerprint, period time.Duration, name string) { fm.logger.Printf("[DEBUG] client.fingerprint_manager: fingerprinting %s every %v", name, period) + timer := time.NewTimer(period) + defer timer.Stop() + for { select { - case <-time.After(period): + case <-timer.C: + timer.Reset(period) + _, err := fm.fingerprint(name, f) if err != nil { fm.logger.Printf("[DEBUG] client.fingerprint_manager: periodic fingerprinting for %v failed: %+v", name, err) @@ -68,26 +77,6 @@ func (fm *FingerprintManager) runFingerprint(f fingerprint.Fingerprint, period t } } -// XXX Do we need this for now -// runHealthCheck runs each health check individually on an ongoing basis -func (fm *FingerprintManager) runHealthCheck(hc fingerprint.HealthCheck, period time.Duration, name string) { - fm.logger.Printf("[DEBUG] client.fingerprint_manager: healthchecking %s every %v", name, period) - - for { - select { - case <-time.After(period): - err := fm.healthCheck(name, hc) - if err != nil { - fm.logger.Printf("[DEBUG] client.fingerprint_manager: health checking for %v failed: %v", name, err) - continue - } - - case <-fm.shutdownCh: - return - } - } -} - // setupDrivers is used to fingerprint the node to see if these drivers are // supported func (fm *FingerprintManager) setupDrivers(drivers []string) error { @@ -100,62 +89,81 @@ func (fm *FingerprintManager) setupDrivers(drivers []string) error { return err } - if hc, ok := d.(fingerprint.HealthCheck); ok { - fm.healthCheck(name, hc) - } - detected, err := fm.fingerprintDriver(name, d) if err != nil { fm.logger.Printf("[DEBUG] client.fingerprint_manager: fingerprinting for %v failed: %+v", name, err) return err } + if hc, ok := d.(fingerprint.HealthCheck); ok { + fm.runHealthCheck(name, hc) + } else { + // for drivers which are not of type health check, update them to have their + // health status match the status of whether they are detected or not. + healthInfo := &structs.DriverInfo{ + Healthy: detected, + UpdateTime: time.Now(), + } + if node := fm.updateNodeFromDriver(name, nil, healthInfo); node != nil { + fm.nodeLock.Lock() + fm.node = node + fm.nodeLock.Unlock() + } + } + + go fm.watchDriver(d, name) + // log the fingerprinters which have been applied if detected { availDrivers = append(availDrivers, name) } - - // 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 { - p, period := d.Periodic() - if p { - go fm.runFingerprint(d, period, name) - } - - req := &cstructs.HealthCheckIntervalRequest{} - resp := &cstructs.HealthCheckIntervalResponse{} - hc.GetHealthCheckInterval(req, resp) - if resp.Eligible { - go fm.runHealthCheck(hc, resp.Period, name) - } - } else { - p, period := d.Periodic() - if p { - go fm.runFingerprintDriver(d, period, name) - } - } } fm.logger.Printf("[DEBUG] client.fingerprint_manager: detected drivers %v", availDrivers) return nil } -func (fm *FingerprintManager) runFingerprintDriver(f fingerprint.Fingerprint, period time.Duration, name string) { - fm.logger.Printf("[DEBUG] client.fingerprint_manager: fingerprinting driver %s every %v", name, period) +// watchDrivers facilitates the different periods between fingerprint and +// health checking a driver +func (fm *FingerprintManager) watchDriver(d driver.Driver, name string) { + _, fingerprintPeriod := d.Periodic() + fm.logger.Printf("[DEBUG] client.fingerprint_manager: fingerprinting driver %s every %v", name, fingerprintPeriod) + + var healthCheckPeriod time.Duration + hc, isHealthCheck := d.(fingerprint.HealthCheck) + if isHealthCheck { + req := &cstructs.HealthCheckIntervalRequest{} + var resp cstructs.HealthCheckIntervalResponse + hc.GetHealthCheckInterval(req, &resp) + if resp.Eligible { + fm.logger.Printf("[DEBUG] client.fingerprint_manager: health checking driver %s every %v", name, healthCheckPeriod) + healthCheckPeriod = resp.Period + } + } + + t1 := time.NewTimer(fingerprintPeriod) + defer t1.Stop() + t2 := time.NewTimer(healthCheckPeriod) + defer t2.Stop() for { select { - case <-time.After(period): - _, err := fm.fingerprintDriver(name, f) - if err != nil { - fm.logger.Printf("[DEBUG] client.fingerprint_manager: periodic fingerprinting for driver %v failed: %+v", name, err) - continue - } - case <-fm.shutdownCh: return + case <-t1.C: + t1.Reset(fingerprintPeriod) + _, err := fm.fingerprintDriver(name, d) + if err != nil { + fm.logger.Printf("[DEBUG] client.fingerprint_manager: periodic fingerprinting for driver %v failed: %+v", name, err) + } + case <-t2.C: + if isHealthCheck { + t2.Reset(healthCheckPeriod) + err := fm.runHealthCheck(name, hc) + if err != nil { + fm.logger.Printf("[DEBUG] client.fingerprint_manager: health checking for %v failed: %v", name, err) + } + } } } } @@ -176,6 +184,12 @@ func (fm *FingerprintManager) fingerprintDriver(name string, f fingerprint.Finge return false, err } + if node := fm.updateNodeAttributes(&response); node != nil { + fm.nodeLock.Lock() + fm.node = node + fm.nodeLock.Unlock() + } + // COMPAT: Remove in 0.9: As of Nomad 0.8 there is a temporary measure to // update all driver attributes to its corresponding driver info object, // as eventually all drivers will need to @@ -188,43 +202,17 @@ func (fm *FingerprintManager) fingerprintDriver(name string, f fingerprint.Finge strings.Replace(copy, "driver.", "", 1) strippedAttributes[k] = v } + di := &structs.DriverInfo{ Attributes: strippedAttributes, Detected: response.Detected, } - response.AddDriver(name, di) - - if node := fm.updateNodeAttributes(&response); node != nil { + if node := fm.updateNodeFromDriver(name, di, nil); node != nil { fm.nodeLock.Lock() fm.node = node fm.nodeLock.Unlock() } - // COMPAT: Remove in 0.9: As of Nomad 0.8, there is a driver info for every - // driver. As a compatibility mechanism, we proactively set this driver info - // for drivers which do not yet support health checks. - if _, ok := f.(fingerprint.HealthCheck); !ok { - resp := &cstructs.HealthCheckResponse{ - Drivers: map[string]*structs.DriverInfo{ - name: &structs.DriverInfo{ - // This achieves backwards compatibility- before, we only relied on the - // status of the fingerprint method to determine whether a driver was - // enabled. For drivers without health checks yet enabled, we should always - // set this to true, and then once we enable health checks, this can - // dynamically update this value. - Healthy: true, - UpdateTime: time.Now(), - }, - }, - } - - if node := fm.updateHealthCheck(resp); node != nil { - fm.nodeLock.Lock() - fm.node = node - fm.nodeLock.Unlock() - } - } - return response.Detected, nil } @@ -253,12 +241,12 @@ func (fm *FingerprintManager) fingerprint(name string, f fingerprint.Fingerprint } // healthcheck checks the health of the specified resource. -func (fm *FingerprintManager) healthCheck(name string, hc fingerprint.HealthCheck) error { +func (fm *FingerprintManager) runHealthCheck(name string, hc fingerprint.HealthCheck) error { request := &cstructs.HealthCheckRequest{} var response cstructs.HealthCheckResponse err := hc.HealthCheck(request, &response) - if node := fm.updateHealthCheck(&response); node != nil { + if node := fm.updateNodeFromDriver(name, nil, response.Drivers[name]); node != nil { fm.nodeLock.Lock() fm.node = node fm.nodeLock.Unlock() @@ -294,17 +282,6 @@ func (fm *FingerprintManager) setupFingerprinters(fingerprints []string) error { if p { go fm.runFingerprint(f, period, name) } - - // XXX This code path doesn't exist right now - if hc, ok := f.(fingerprint.HealthCheck); ok { - req := &cstructs.HealthCheckIntervalRequest{} - var resp cstructs.HealthCheckIntervalResponse - if err := hc.GetHealthCheckInterval(req, &resp); err != nil { - if resp.Eligible { - go fm.runHealthCheck(hc, resp.Period, name) - } - } - } } fm.logger.Printf("[DEBUG] client.fingerprint_manager: detected fingerprints %v", appliedFingerprints) diff --git a/client/fingerprint_manager_test.go b/client/fingerprint_manager_test.go index b37272871..28b4c3186 100644 --- a/client/fingerprint_manager_test.go +++ b/client/fingerprint_manager_test.go @@ -6,7 +6,6 @@ import ( "github.com/hashicorp/nomad/client/config" "github.com/hashicorp/nomad/client/driver" - cstructs "github.com/hashicorp/nomad/client/structs" "github.com/hashicorp/nomad/nomad/structs" "github.com/hashicorp/nomad/testutil" "github.com/stretchr/testify/require" @@ -37,6 +36,7 @@ func TestFingerprintManager_Run_MockDriver(t *testing.T) { make(chan struct{}), testClient.updateNodeFromFingerprint, testClient.updateNodeFromHealthCheck, + testClient.updateNodeFromDriver, testLogger(), ) @@ -70,6 +70,7 @@ func TestFingerprintManager_Run_ResourcesFingerprint(t *testing.T) { make(chan struct{}), testClient.updateNodeFromFingerprint, testClient.updateNodeFromHealthCheck, + testClient.updateNodeFromDriver, testLogger(), ) @@ -105,6 +106,7 @@ func TestFingerprintManager_Fingerprint_Run(t *testing.T) { make(chan struct{}), testClient.updateNodeFromFingerprint, testClient.updateNodeFromHealthCheck, + testClient.updateNodeFromDriver, testLogger(), ) @@ -149,6 +151,7 @@ func TestFingerprintManager_Fingerprint_Periodic(t *testing.T) { shutdownCh, testClient.updateNodeFromFingerprint, testClient.updateNodeFromHealthCheck, + testClient.updateNodeFromDriver, testLogger(), ) @@ -194,24 +197,13 @@ func TestFingerprintManager_HealthCheck_Driver(t *testing.T) { node := &structs.Node{ Attributes: make(map[string]string, 0), + Links: make(map[string]string, 0), + Resources: &structs.Resources{}, Drivers: make(map[string]*structs.DriverInfo, 0), } - updateNode := func(r *cstructs.FingerprintResponse) *structs.Node { - if r.Attributes != nil { - for k, v := range r.Attributes { - node.Attributes[k] = v - } - } - return node - } - updateHealthCheck := func(resp *cstructs.HealthCheckResponse) *structs.Node { - if resp.Drivers != nil { - for k, v := range resp.Drivers { - node.Drivers[k] = v - } - } - return node - } + testConfig := config.Config{Node: node} + testClient := &Client{config: &testConfig} + conf := config.DefaultConfig() conf.Options = map[string]string{ "driver.raw_exec.enable": "1", @@ -231,8 +223,9 @@ func TestFingerprintManager_HealthCheck_Driver(t *testing.T) { getConfig, node, shutdownCh, - updateNode, - updateHealthCheck, + testClient.updateNodeFromFingerprint, + testClient.updateNodeFromHealthCheck, + testClient.updateNodeFromDriver, testLogger(), ) @@ -327,6 +320,7 @@ func TestFingerprintManager_HealthCheck_Periodic(t *testing.T) { shutdownCh, testClient.updateNodeFromFingerprint, testClient.updateNodeFromHealthCheck, + testClient.updateNodeFromDriver, testLogger(), ) @@ -384,7 +378,7 @@ func TestFingerprintManager_HealthCheck_Periodic(t *testing.T) { if mockDriverInfo == nil { return false, fmt.Errorf("mock driver info should be set on the client") } - if !mockDriverInfo.Detected { + if mockDriverInfo.Detected { return false, fmt.Errorf("mock driver should be detected") } if mockDriverInfo.Healthy { @@ -427,6 +421,7 @@ func TestFimgerprintManager_Run_InWhitelist(t *testing.T) { shutdownCh, testClient.updateNodeFromFingerprint, testClient.updateNodeFromHealthCheck, + testClient.updateNodeFromDriver, testLogger(), ) @@ -466,6 +461,7 @@ func TestFingerprintManager_Run_InBlacklist(t *testing.T) { shutdownCh, testClient.updateNodeFromFingerprint, testClient.updateNodeFromHealthCheck, + testClient.updateNodeFromDriver, testLogger(), ) @@ -506,6 +502,7 @@ func TestFingerprintManager_Run_Combination(t *testing.T) { shutdownCh, testClient.updateNodeFromFingerprint, testClient.updateNodeFromHealthCheck, + testClient.updateNodeFromDriver, testLogger(), ) @@ -550,6 +547,7 @@ func TestFingerprintManager_Run_WhitelistDrivers(t *testing.T) { shutdownCh, testClient.updateNodeFromFingerprint, testClient.updateNodeFromHealthCheck, + testClient.updateNodeFromDriver, testLogger(), ) @@ -590,6 +588,7 @@ func TestFingerprintManager_Run_AllDriversBlacklisted(t *testing.T) { shutdownCh, testClient.updateNodeFromFingerprint, testClient.updateNodeFromHealthCheck, + testClient.updateNodeFromDriver, testLogger(), ) @@ -634,6 +633,7 @@ func TestFingerprintManager_Run_DriversWhiteListBlacklistCombination(t *testing. shutdownCh, testClient.updateNodeFromFingerprint, testClient.updateNodeFromHealthCheck, + testClient.updateNodeFromDriver, testLogger(), ) @@ -677,6 +677,7 @@ func TestFingerprintManager_Run_DriversInBlacklist(t *testing.T) { shutdownCh, testClient.updateNodeFromFingerprint, testClient.updateNodeFromHealthCheck, + testClient.updateNodeFromDriver, testLogger(), ) diff --git a/client/structs/structs.go b/client/structs/structs.go index d1c15ee56..af28b55ca 100644 --- a/client/structs/structs.go +++ b/client/structs/structs.go @@ -361,19 +361,6 @@ 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