From 6f0ed6184b9152bef8ba0ad6fd3b8a5c850b6ac8 Mon Sep 17 00:00:00 2001 From: Alex Dadgar Date: Wed, 26 Sep 2018 18:14:36 -0700 Subject: [PATCH] Fix client reloading and pass the plugin loaders to server and client --- CHANGELOG.md | 1 + client/config/config.go | 8 ++ client/testing.go | 10 +- command/agent/agent.go | 210 ++++++++++++++++++------------ command/agent/command.go | 27 ++-- nomad/config.go | 8 ++ nomad/testing.go | 6 + plugins/shared/catalog/testing.go | 65 +++++++++ plugins/shared/loader/init.go | 11 +- 9 files changed, 251 insertions(+), 95 deletions(-) create mode 100644 plugins/shared/catalog/testing.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 2c43b556c..60f849bff 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ IMPROVEMENTS: BUG FIXES: * core: Fixed bug in reconciler where allocs already stopped were being unnecessarily updated [[GH-4764](https://github.com/hashicorp/nomad/issues/4764)] + * client: Fix an issue reloading the client config [[GH-4730](https://github.com/hashicorp/nomad/issues/4730)] ## 0.8.6 (September 26, 2018) diff --git a/client/config/config.go b/client/config/config.go index 07ab2bf34..dfdd8cbee 100644 --- a/client/config/config.go +++ b/client/config/config.go @@ -13,6 +13,7 @@ import ( "github.com/hashicorp/nomad/helper" "github.com/hashicorp/nomad/nomad/structs" "github.com/hashicorp/nomad/nomad/structs/config" + "github.com/hashicorp/nomad/plugins/shared/loader" "github.com/hashicorp/nomad/version" ) @@ -206,6 +207,13 @@ type Config struct { // This period is meant to be long enough for a leader election to take // place, and a small jitter is applied to avoid a thundering herd. RPCHoldTimeout time.Duration + + // PluginLoader is used to load plugins. + PluginLoader loader.PluginCatalog + + // PluginSingletonLoader is a plugin loader that will returns singleton + // instances of the plugins. + PluginSingletonLoader loader.PluginCatalog } func (c *Config) Copy() *Config { diff --git a/client/testing.go b/client/testing.go index cde5e8b80..a67caa0dd 100644 --- a/client/testing.go +++ b/client/testing.go @@ -8,12 +8,16 @@ import ( "github.com/hashicorp/nomad/helper" "github.com/hashicorp/nomad/helper/testlog" "github.com/hashicorp/nomad/nomad/structs" + "github.com/hashicorp/nomad/plugins/shared/catalog" + "github.com/hashicorp/nomad/plugins/shared/singleton" "github.com/mitchellh/go-testing-interface" ) // TestClient creates an in-memory client for testing purposes. func TestClient(t testing.T, cb func(c *config.Config)) *Client { conf := config.DefaultConfig() + logger := testlog.HCLogger(t) + conf.Logger = logger conf.VaultConfig.Enabled = helper.BoolToPtr(false) conf.DevMode = true conf.Node = &structs.Node{ @@ -32,12 +36,14 @@ func TestClient(t testing.T, cb func(c *config.Config)) *Client { } conf.Options[fingerprint.TightenNetworkTimeoutsConfig] = "true" + // Set the plugin loaders + conf.PluginLoader = catalog.TestPluginLoader(t) + conf.PluginSingletonLoader = singleton.NewSingletonLoader(logger, conf.PluginLoader) + if cb != nil { cb(conf) } - logger := testlog.HCLogger(t) - conf.Logger = logger catalog := consul.NewMockCatalog(logger) mockService := consulApi.NewMockConsulServiceClient(t, logger) client, err := NewClient(conf, catalog, mockService) diff --git a/command/agent/agent.go b/command/agent/agent.go index dc7e21bcc..0dc02044c 100644 --- a/command/agent/agent.go +++ b/command/agent/agent.go @@ -110,7 +110,6 @@ func NewAgent(config *Config, logOutput io.Writer, inmem *metrics.InmemSink) (*A return nil, fmt.Errorf("Failed to initialize Consul client: %v", err) } - // TODO setup plugin loader if err := a.setupPlugins(); err != nil { return nil, err } @@ -129,14 +128,13 @@ func NewAgent(config *Config, logOutput io.Writer, inmem *metrics.InmemSink) (*A } // convertServerConfig takes an agent config and log output and returns a Nomad -// Config. -func convertServerConfig(agentConfig *Config, logger log.Logger, logOutput io.Writer) (*nomad.Config, error) { +// Config. There may be missing fields that must be set by the agent. To do this +// call finalizeServerConfig +func convertServerConfig(agentConfig *Config) (*nomad.Config, error) { conf := agentConfig.NomadConfig if conf == nil { conf = nomad.DefaultConfig() } - conf.Logger = logger - conf.LogOutput = logOutput conf.DevMode = agentConfig.DevMode conf.Build = agentConfig.Version.VersionNumber() if agentConfig.Region != "" { @@ -335,61 +333,73 @@ func convertServerConfig(agentConfig *Config, logger log.Logger, logOutput io.Wr // serverConfig is used to generate a new server configuration struct // for initializing a nomad server. func (a *Agent) serverConfig() (*nomad.Config, error) { - return convertServerConfig(a.config, a.logger, a.logOutput) + c, err := convertServerConfig(a.config) + if err != nil { + return nil, err + } + + a.finalizeServerConfig(c) + return c, nil } -// clientConfig is used to generate a new client configuration struct -// for initializing a Nomad client. +// finalizeServerConfig sets configuration fields on the server config that are +// not staticly convertable and are from the agent. +func (a *Agent) finalizeServerConfig(c *nomad.Config) { + // Setup the logging + c.Logger = a.logger + c.LogOutput = a.logOutput + + // Setup the plugin loaders + c.PluginLoader = a.pluginLoader + c.PluginSingletonLoader = a.pluginSingletonLoader +} + +// clientConfig is used to generate a new client configuration struct for +// initializing a Nomad client. func (a *Agent) clientConfig() (*clientconfig.Config, error) { - // Setup the configuration - conf := a.config.ClientConfig - if conf == nil { - conf = clientconfig.DefaultConfig() + c, err := convertClientConfig(a.config) + if err != nil { + return nil, err } + if err := a.finalizeClientConfig(c); err != nil { + return nil, err + } + + return c, nil +} + +// finalizeClientConfig sets configuration fields on the client config that are +// not staticly convertable and are from the agent. +func (a *Agent) finalizeClientConfig(c *clientconfig.Config) error { + // Setup the logging + c.Logger = a.logger + c.LogOutput = a.logOutput + // If we are running a server, append both its bind and advertise address so // we are able to at least talk to the local server even if that isn't // configured explicitly. This handles both running server and client on one // host and -dev mode. - conf.Servers = a.config.Client.Servers if a.server != nil { if a.config.AdvertiseAddrs == nil || a.config.AdvertiseAddrs.RPC == "" { - return nil, fmt.Errorf("AdvertiseAddrs is nil or empty") + return fmt.Errorf("AdvertiseAddrs is nil or empty") } else if a.config.normalizedAddrs == nil || a.config.normalizedAddrs.RPC == "" { - return nil, fmt.Errorf("normalizedAddrs is nil or empty") + return fmt.Errorf("normalizedAddrs is nil or empty") } - conf.Servers = append(conf.Servers, + c.Servers = append(c.Servers, a.config.normalizedAddrs.RPC, a.config.AdvertiseAddrs.RPC) } - conf.Logger = a.logger - conf.LogOutput = a.logOutput - conf.LogLevel = a.config.LogLevel - conf.DevMode = a.config.DevMode - if a.config.Region != "" { - conf.Region = a.config.Region - } - if a.config.DataDir != "" { - conf.StateDir = filepath.Join(a.config.DataDir, "client") - conf.AllocDir = filepath.Join(a.config.DataDir, "alloc") - } - if a.config.Client.StateDir != "" { - conf.StateDir = a.config.Client.StateDir - } - if a.config.Client.AllocDir != "" { - conf.AllocDir = a.config.Client.AllocDir - } - if a.config.Client.NetworkInterface != "" { - conf.NetworkInterface = a.config.Client.NetworkInterface - } - conf.ChrootEnv = a.config.Client.ChrootEnv - conf.Options = a.config.Client.Options - // Logging deprecation messages about consul related configuration in client + // Setup the plugin loaders + c.PluginLoader = a.pluginLoader + c.PluginSingletonLoader = a.pluginSingletonLoader + + // Log deprecation messages about Consul related configuration in client // options var invalidConsulKeys []string - for key := range conf.Options { + for key := range c.Options { if strings.HasPrefix(key, "consul") { invalidConsulKeys = append(invalidConsulKeys, fmt.Sprintf("options.%s", key)) } @@ -401,34 +411,68 @@ func (a *Agent) clientConfig() (*clientconfig.Config, error) { to configure Nomad to work with Consul.`) } - if a.config.Client.NetworkSpeed != 0 { - conf.NetworkSpeed = a.config.Client.NetworkSpeed + return nil +} + +// convertClientConfig takes an agent config and log output and returns a client +// Config. There may be missing fields that must be set by the agent. To do this +// call finalizeServerConfig +func convertClientConfig(agentConfig *Config) (*clientconfig.Config, error) { + // Setup the configuration + conf := agentConfig.ClientConfig + if conf == nil { + conf = clientconfig.DefaultConfig() } - if a.config.Client.CpuCompute != 0 { - conf.CpuCompute = a.config.Client.CpuCompute + + conf.Servers = agentConfig.Client.Servers + conf.LogLevel = agentConfig.LogLevel + conf.DevMode = agentConfig.DevMode + if agentConfig.Region != "" { + conf.Region = agentConfig.Region } - if a.config.Client.MemoryMB != 0 { - conf.MemoryMB = a.config.Client.MemoryMB + if agentConfig.DataDir != "" { + conf.StateDir = filepath.Join(agentConfig.DataDir, "client") + conf.AllocDir = filepath.Join(agentConfig.DataDir, "alloc") } - if a.config.Client.MaxKillTimeout != "" { - dur, err := time.ParseDuration(a.config.Client.MaxKillTimeout) + if agentConfig.Client.StateDir != "" { + conf.StateDir = agentConfig.Client.StateDir + } + if agentConfig.Client.AllocDir != "" { + conf.AllocDir = agentConfig.Client.AllocDir + } + if agentConfig.Client.NetworkInterface != "" { + conf.NetworkInterface = agentConfig.Client.NetworkInterface + } + conf.ChrootEnv = agentConfig.Client.ChrootEnv + conf.Options = agentConfig.Client.Options + if agentConfig.Client.NetworkSpeed != 0 { + conf.NetworkSpeed = agentConfig.Client.NetworkSpeed + } + if agentConfig.Client.CpuCompute != 0 { + conf.CpuCompute = agentConfig.Client.CpuCompute + } + if agentConfig.Client.MemoryMB != 0 { + conf.MemoryMB = agentConfig.Client.MemoryMB + } + if agentConfig.Client.MaxKillTimeout != "" { + dur, err := time.ParseDuration(agentConfig.Client.MaxKillTimeout) if err != nil { return nil, fmt.Errorf("Error parsing max kill timeout: %s", err) } conf.MaxKillTimeout = dur } - conf.ClientMaxPort = uint(a.config.Client.ClientMaxPort) - conf.ClientMinPort = uint(a.config.Client.ClientMinPort) + conf.ClientMaxPort = uint(agentConfig.Client.ClientMaxPort) + conf.ClientMinPort = uint(agentConfig.Client.ClientMinPort) // Setup the node conf.Node = new(structs.Node) - conf.Node.Datacenter = a.config.Datacenter - conf.Node.Name = a.config.NodeName - conf.Node.Meta = a.config.Client.Meta - conf.Node.NodeClass = a.config.Client.NodeClass + conf.Node.Datacenter = agentConfig.Datacenter + conf.Node.Name = agentConfig.NodeName + conf.Node.Meta = agentConfig.Client.Meta + conf.Node.NodeClass = agentConfig.Client.NodeClass // Set up the HTTP advertise address - conf.Node.HTTPAddr = a.config.AdvertiseAddrs.HTTP + conf.Node.HTTPAddr = agentConfig.AdvertiseAddrs.HTTP // Reserve resources on the node. // COMPAT(0.10): Remove in 0.10 @@ -437,58 +481,58 @@ func (a *Agent) clientConfig() (*clientconfig.Config, error) { r = new(structs.Resources) conf.Node.Reserved = r } - r.CPU = a.config.Client.Reserved.CPU - r.MemoryMB = a.config.Client.Reserved.MemoryMB - r.DiskMB = a.config.Client.Reserved.DiskMB - r.IOPS = a.config.Client.Reserved.IOPS + r.CPU = agentConfig.Client.Reserved.CPU + r.MemoryMB = agentConfig.Client.Reserved.MemoryMB + r.DiskMB = agentConfig.Client.Reserved.DiskMB + r.IOPS = agentConfig.Client.Reserved.IOPS res := conf.Node.ReservedResources if res == nil { res = new(structs.NodeReservedResources) conf.Node.ReservedResources = res } - res.Cpu.CpuShares = int64(a.config.Client.Reserved.CPU) - res.Memory.MemoryMB = int64(a.config.Client.Reserved.MemoryMB) - res.Disk.DiskMB = int64(a.config.Client.Reserved.DiskMB) - res.Networks.ReservedHostPorts = a.config.Client.Reserved.ReservedPorts + res.Cpu.CpuShares = int64(agentConfig.Client.Reserved.CPU) + res.Memory.MemoryMB = int64(agentConfig.Client.Reserved.MemoryMB) + res.Disk.DiskMB = int64(agentConfig.Client.Reserved.DiskMB) + res.Networks.ReservedHostPorts = agentConfig.Client.Reserved.ReservedPorts - conf.Version = a.config.Version + conf.Version = agentConfig.Version - if *a.config.Consul.AutoAdvertise && a.config.Consul.ClientServiceName == "" { + if *agentConfig.Consul.AutoAdvertise && agentConfig.Consul.ClientServiceName == "" { return nil, fmt.Errorf("client_service_name must be set when auto_advertise is enabled") } - conf.ConsulConfig = a.config.Consul - conf.VaultConfig = a.config.Vault + conf.ConsulConfig = agentConfig.Consul + conf.VaultConfig = agentConfig.Vault // Set up Telemetry configuration - conf.StatsCollectionInterval = a.config.Telemetry.collectionInterval - conf.PublishNodeMetrics = a.config.Telemetry.PublishNodeMetrics - conf.PublishAllocationMetrics = a.config.Telemetry.PublishAllocationMetrics - conf.DisableTaggedMetrics = a.config.Telemetry.DisableTaggedMetrics - conf.BackwardsCompatibleMetrics = a.config.Telemetry.BackwardsCompatibleMetrics + conf.StatsCollectionInterval = agentConfig.Telemetry.collectionInterval + conf.PublishNodeMetrics = agentConfig.Telemetry.PublishNodeMetrics + conf.PublishAllocationMetrics = agentConfig.Telemetry.PublishAllocationMetrics + conf.DisableTaggedMetrics = agentConfig.Telemetry.DisableTaggedMetrics + conf.BackwardsCompatibleMetrics = agentConfig.Telemetry.BackwardsCompatibleMetrics // Set the TLS related configs - conf.TLSConfig = a.config.TLSConfig + conf.TLSConfig = agentConfig.TLSConfig conf.Node.TLSEnabled = conf.TLSConfig.EnableHTTP // Set the GC related configs - conf.GCInterval = a.config.Client.GCInterval - conf.GCParallelDestroys = a.config.Client.GCParallelDestroys - conf.GCDiskUsageThreshold = a.config.Client.GCDiskUsageThreshold - conf.GCInodeUsageThreshold = a.config.Client.GCInodeUsageThreshold - conf.GCMaxAllocs = a.config.Client.GCMaxAllocs - if a.config.Client.NoHostUUID != nil { - conf.NoHostUUID = *a.config.Client.NoHostUUID + conf.GCInterval = agentConfig.Client.GCInterval + conf.GCParallelDestroys = agentConfig.Client.GCParallelDestroys + conf.GCDiskUsageThreshold = agentConfig.Client.GCDiskUsageThreshold + conf.GCInodeUsageThreshold = agentConfig.Client.GCInodeUsageThreshold + conf.GCMaxAllocs = agentConfig.Client.GCMaxAllocs + if agentConfig.Client.NoHostUUID != nil { + conf.NoHostUUID = *agentConfig.Client.NoHostUUID } else { // Default no_host_uuid to true conf.NoHostUUID = true } // Setup the ACLs - conf.ACLEnabled = a.config.ACL.Enabled - conf.ACLTokenTTL = a.config.ACL.TokenTTL - conf.ACLPolicyTTL = a.config.ACL.PolicyTTL + conf.ACLEnabled = agentConfig.ACL.Enabled + conf.ACLTokenTTL = agentConfig.ACL.TokenTTL + conf.ACLPolicyTTL = agentConfig.ACL.PolicyTTL return conf, nil } diff --git a/command/agent/command.go b/command/agent/command.go index e15555c33..5c049be50 100644 --- a/command/agent/command.go +++ b/command/agent/command.go @@ -751,25 +751,36 @@ func (c *Command) handleReload() { if s := c.agent.Server(); s != nil { c.agent.logger.Debug("starting reload of server config") - sconf, err := convertServerConfig(newConf, c.agent.logger, c.logOutput) + sconf, err := convertServerConfig(newConf) if err != nil { c.agent.logger.Error("failed to convert server config", "error", err) return - } else { - if err := s.Reload(sconf); err != nil { - c.agent.logger.Error("reloading server config failed", "error", err) - return - } + } + + // Finalize the config to get the agent objects injected in + c.agent.finalizeServerConfig(sconf) + + // Reload the config + if err := s.Reload(sconf); err != nil { + c.agent.logger.Error("reloading server config failed", "error", err) + return } } if s := c.agent.Client(); s != nil { - clientConfig, err := c.agent.clientConfig() c.agent.logger.Debug("starting reload of client config") + clientConfig, err := convertClientConfig(newConf) if err != nil { - c.agent.logger.Error("reloading client config failed", "error", err) + c.agent.logger.Error("failed to convert client config", "error", err) return } + + // Finalize the config to get the agent objects injected in + if err := c.agent.finalizeClientConfig(clientConfig); err != nil { + c.agent.logger.Error("failed to finalize client config", "error", err) + return + } + if err := c.agent.Client().Reload(clientConfig); err != nil { c.agent.logger.Error("reloading client config failed", "error", err) return diff --git a/nomad/config.go b/nomad/config.go index 2e0b49ed7..c712e5833 100644 --- a/nomad/config.go +++ b/nomad/config.go @@ -14,6 +14,7 @@ import ( "github.com/hashicorp/nomad/helper/uuid" "github.com/hashicorp/nomad/nomad/structs" "github.com/hashicorp/nomad/nomad/structs/config" + "github.com/hashicorp/nomad/plugins/shared/loader" "github.com/hashicorp/nomad/scheduler" "github.com/hashicorp/raft" "github.com/hashicorp/serf/serf" @@ -295,6 +296,13 @@ type Config struct { // autopilot tasks, such as promoting eligible non-voters and removing // dead servers. AutopilotInterval time.Duration + + // PluginLoader is used to load plugins. + PluginLoader loader.PluginCatalog + + // PluginSingletonLoader is a plugin loader that will returns singleton + // instances of the plugins. + PluginSingletonLoader loader.PluginCatalog } // CheckVersion is used to check if the ProtocolVersion is valid diff --git a/nomad/testing.go b/nomad/testing.go index 9696aa386..7d0f9fcd0 100644 --- a/nomad/testing.go +++ b/nomad/testing.go @@ -12,6 +12,8 @@ import ( "github.com/hashicorp/nomad/helper/testlog" "github.com/hashicorp/nomad/nomad/mock" "github.com/hashicorp/nomad/nomad/structs" + "github.com/hashicorp/nomad/plugins/shared/catalog" + "github.com/hashicorp/nomad/plugins/shared/singleton" "github.com/mitchellh/go-testing-interface" ) @@ -69,6 +71,10 @@ func TestServer(t testing.T, cb func(*Config)) *Server { config.ServerHealthInterval = 50 * time.Millisecond config.AutopilotInterval = 100 * time.Millisecond + // Set the plugin loaders + config.PluginLoader = catalog.TestPluginLoader(t) + config.PluginSingletonLoader = singleton.NewSingletonLoader(config.Logger, config.PluginLoader) + // Invoke the callback if any if cb != nil { cb(config) diff --git a/plugins/shared/catalog/testing.go b/plugins/shared/catalog/testing.go new file mode 100644 index 000000000..59bcc6cf1 --- /dev/null +++ b/plugins/shared/catalog/testing.go @@ -0,0 +1,65 @@ +package catalog + +import ( + "github.com/hashicorp/nomad/helper/testlog" + "github.com/hashicorp/nomad/nomad/structs/config" + "github.com/hashicorp/nomad/plugins/shared/loader" + "github.com/mitchellh/go-testing-interface" +) + +// TestPluginLoader returns a plugin loader populated only with internal plugins +func TestPluginLoader(t testing.T) loader.PluginCatalog { + return TestPluginLoaderWithOptions(t, "", nil, nil) +} + +// TestPluginLoaderWithOptions allows configuring the plugin loader fully. +func TestPluginLoaderWithOptions(t testing.T, + pluginDir string, + options map[string]string, + configs []*config.PluginConfig) loader.PluginCatalog { + + // Get a logger + logger := testlog.HCLogger(t) + + // Get the registered plugins + catalog := Catalog() + + // Create our map of plugins + internal := make(map[loader.PluginID]*loader.InternalPluginConfig, len(catalog)) + + for id, reg := range catalog { + if reg.Config == nil { + logger.Warn("skipping loading internal plugin because it is missing its configuration", "plugin", id) + continue + } + + pluginConfig := reg.Config.Config + if reg.ConfigLoader != nil { + pc, err := reg.ConfigLoader(options) + if err != nil { + t.Fatalf("failed to retrieve config for internal plugin %v: %v", id, err) + } + + pluginConfig = pc + } + + internal[id] = &loader.InternalPluginConfig{ + Factory: reg.Config.Factory, + Config: pluginConfig, + } + } + + // Build the plugin loader + config := &loader.PluginLoaderConfig{ + Logger: logger, + PluginDir: "", + Configs: configs, + InternalPlugins: internal, + } + l, err := loader.NewPluginLoader(config) + if err != nil { + t.Fatalf("failed to create plugin loader: %v", err) + } + + return l +} diff --git a/plugins/shared/loader/init.go b/plugins/shared/loader/init.go index 5ca4dca48..1f9a48f54 100644 --- a/plugins/shared/loader/init.go +++ b/plugins/shared/loader/init.go @@ -32,8 +32,6 @@ func validateConfig(config *PluginLoaderConfig) error { return fmt.Errorf("nil config passed") } else if config.Logger == nil { multierror.Append(&mErr, fmt.Errorf("nil logger passed")) - } else if config.PluginDir == "" { - multierror.Append(&mErr, fmt.Errorf("invalid plugin dir %q passed", config.PluginDir)) } // Validate that all plugins have a binary name @@ -149,9 +147,18 @@ func (l *PluginLoader) initInternal(plugins map[PluginID]*InternalPluginConfig, // scan scans the plugin directory and retrieves potentially eligible binaries func (l *PluginLoader) scan() ([]os.FileInfo, error) { + if l.pluginDir == "" { + return nil, nil + } + // Capture the list of binaries in the plugins folder f, err := os.Open(l.pluginDir) if err != nil { + // There are no plugins to scan + if os.IsNotExist(err) { + return nil, nil + } + return nil, fmt.Errorf("failed to open plugin directory %q: %v", l.pluginDir, err) } files, err := f.Readdirnames(-1)