From aa4e3a4b77592f14aec53c1c9dfdaa7ac73b0af9 Mon Sep 17 00:00:00 2001 From: Peter Kazmierczak Date: Wed, 3 Aug 2022 09:23:33 +0200 Subject: [PATCH 01/68] TemplateChangeModeScript --- client/allocrunner/taskrunner/template/template.go | 4 ++++ client/allocrunner/taskrunner/template_hook.go | 3 +++ nomad/structs/structs.go | 4 ++++ 3 files changed, 11 insertions(+) diff --git a/client/allocrunner/taskrunner/template/template.go b/client/allocrunner/taskrunner/template/template.go index 3eed8034e818..a19162b77377 100644 --- a/client/allocrunner/taskrunner/template/template.go +++ b/client/allocrunner/taskrunner/template/template.go @@ -108,6 +108,8 @@ type TaskTemplateManagerConfig struct { // NomadToken is the Nomad token or identity claim for the task NomadToken string + + Handle interfaces.ScriptExecutor } // Validate validates the configuration. @@ -436,6 +438,8 @@ func (tm *TaskTemplateManager) onTemplateRendered(handledRenders map[string]time signals[tmpl.ChangeSignal] = struct{}{} case structs.TemplateChangeModeRestart: restart = true + case structs.TemplateChangeModeScript: + // do stuff case structs.TemplateChangeModeNoop: continue } diff --git a/client/allocrunner/taskrunner/template_hook.go b/client/allocrunner/taskrunner/template_hook.go index cdc6ee16f7b3..0f634c83284c 100644 --- a/client/allocrunner/taskrunner/template_hook.go +++ b/client/allocrunner/taskrunner/template_hook.go @@ -42,6 +42,8 @@ type templateHookConfig struct { // nomadNamespace is the job's Nomad namespace nomadNamespace string + + handle *DriverHandle } type templateHook struct { @@ -131,6 +133,7 @@ func (h *templateHook) newManager() (unblock chan struct{}, err error) { MaxTemplateEventRate: template.DefaultMaxTemplateEventRate, NomadNamespace: h.config.nomadNamespace, NomadToken: h.nomadToken, + Handle: h.config.handle, }) if err != nil { h.logger.Error("failed to create template manager", "error", err) diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index f486f4ce7e13..65e410b9ea93 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -7677,6 +7677,10 @@ const ( // TemplateChangeModeRestart marks that the task should be restarted if the // template is re-rendered TemplateChangeModeRestart = "restart" + + // TemplateChangeModeScript marks that the task should trigger a script if + // the template is re-rendered + TemplateChangeModeScript = "script" ) var ( From cb79fb1206a665d18760ac8f5f2c45f2966b5744 Mon Sep 17 00:00:00 2001 From: Peter Kazmierczak Date: Thu, 4 Aug 2022 10:58:34 +0200 Subject: [PATCH 02/68] updated documentation --- website/content/api-docs/json-jobs.mdx | 1 + website/content/docs/job-specification/template.mdx | 1 + 2 files changed, 2 insertions(+) diff --git a/website/content/api-docs/json-jobs.mdx b/website/content/api-docs/json-jobs.mdx index ffc7c89a116a..0aa3a29fc851 100644 --- a/website/content/api-docs/json-jobs.mdx +++ b/website/content/api-docs/json-jobs.mdx @@ -1055,6 +1055,7 @@ README][ct]. - `"noop"` - take no action (continue running the task) - `"restart"` - restart the task - `"signal"` - send a configurable signal to the task + - `"script"` - run a script - `ChangeSignal` - Specifies the signal to send to the task as a string like "SIGUSR1" or "SIGINT". This option is required if the `ChangeMode` is diff --git a/website/content/docs/job-specification/template.mdx b/website/content/docs/job-specification/template.mdx index 8704bce7b1c2..4f18d3de9977 100644 --- a/website/content/docs/job-specification/template.mdx +++ b/website/content/docs/job-specification/template.mdx @@ -53,6 +53,7 @@ refer to the [Learn Go Template Syntax][gt_learn] Learn guide. - `"noop"` - take no action (continue running the task) - `"restart"` - restart the task - `"signal"` - send a configurable signal to the task + - `"script"` - run a script - `change_signal` `(string: "")` - Specifies the signal to send to the task as a string like `"SIGUSR1"` or `"SIGINT"`. This option is required if the From cf3dffad325736e35741a2dab4fb1abfc0807dae Mon Sep 17 00:00:00 2001 From: Peter Kazmierczak Date: Thu, 4 Aug 2022 11:01:39 +0200 Subject: [PATCH 03/68] changelog entry --- .changelog/13972.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/13972.txt diff --git a/.changelog/13972.txt b/.changelog/13972.txt new file mode 100644 index 000000000000..330faea98a13 --- /dev/null +++ b/.changelog/13972.txt @@ -0,0 +1,3 @@ +```release-note:improvement +template: add script change_mode that allows scripts to be executed on template change +``` \ No newline at end of file From 03e6dab4142747a0593bd3c043d6fe62aa0d64a3 Mon Sep 17 00:00:00 2001 From: Peter Kazmierczak Date: Thu, 4 Aug 2022 11:22:58 +0200 Subject: [PATCH 04/68] wip --- client/allocrunner/taskrunner/template/template.go | 5 +++-- client/allocrunner/taskrunner/template_hook.go | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/client/allocrunner/taskrunner/template/template.go b/client/allocrunner/taskrunner/template/template.go index a19162b77377..53c324fe88cc 100644 --- a/client/allocrunner/taskrunner/template/template.go +++ b/client/allocrunner/taskrunner/template/template.go @@ -109,7 +109,8 @@ type TaskTemplateManagerConfig struct { // NomadToken is the Nomad token or identity claim for the task NomadToken string - Handle interfaces.ScriptExecutor + // ExecHandle is used to execute scripts + ExecHandle interfaces.ScriptExecutor } // Validate validates the configuration. @@ -439,7 +440,7 @@ func (tm *TaskTemplateManager) onTemplateRendered(handledRenders map[string]time case structs.TemplateChangeModeRestart: restart = true case structs.TemplateChangeModeScript: - // do stuff + tm.config.ExecHandle.Exec(time.Minute, "", []string{}) // FIXME: figure out how to pass this case structs.TemplateChangeModeNoop: continue } diff --git a/client/allocrunner/taskrunner/template_hook.go b/client/allocrunner/taskrunner/template_hook.go index 0f634c83284c..b2dfe7be03af 100644 --- a/client/allocrunner/taskrunner/template_hook.go +++ b/client/allocrunner/taskrunner/template_hook.go @@ -133,7 +133,7 @@ func (h *templateHook) newManager() (unblock chan struct{}, err error) { MaxTemplateEventRate: template.DefaultMaxTemplateEventRate, NomadNamespace: h.config.nomadNamespace, NomadToken: h.nomadToken, - Handle: h.config.handle, + ExecHandle: h.config.handle, }) if err != nil { h.logger.Error("failed to create template manager", "error", err) From 3f2b1ec3668e7444c71cac32fe7eafe58ace8917 Mon Sep 17 00:00:00 2001 From: Peter Kazmierczak Date: Thu, 4 Aug 2022 19:36:47 +0200 Subject: [PATCH 05/68] handle script execution --- .../taskrunner/template/template.go | 28 +++++++++++++++++-- .../allocrunner/taskrunner/template_hook.go | 3 +- nomad/structs/structs.go | 10 +++++++ 3 files changed, 37 insertions(+), 4 deletions(-) diff --git a/client/allocrunner/taskrunner/template/template.go b/client/allocrunner/taskrunner/template/template.go index 53c324fe88cc..433078004fdd 100644 --- a/client/allocrunner/taskrunner/template/template.go +++ b/client/allocrunner/taskrunner/template/template.go @@ -109,8 +109,8 @@ type TaskTemplateManagerConfig struct { // NomadToken is the Nomad token or identity claim for the task NomadToken string - // ExecHandle is used to execute scripts - ExecHandle interfaces.ScriptExecutor + // Handle is used to execute scripts + Handle interfaces.ScriptExecutor } // Validate validates the configuration. @@ -395,6 +395,11 @@ func (tm *TaskTemplateManager) onTemplateRendered(handledRenders map[string]time var handling []string signals := make(map[string]struct{}) + scripts := []struct { + path string + args []string + timeout time.Duration + }{} restart := false var splay time.Duration @@ -440,7 +445,16 @@ func (tm *TaskTemplateManager) onTemplateRendered(handledRenders map[string]time case structs.TemplateChangeModeRestart: restart = true case structs.TemplateChangeModeScript: - tm.config.ExecHandle.Exec(time.Minute, "", []string{}) // FIXME: figure out how to pass this + duration, _ := time.ParseDuration(strconv.Itoa(tmpl.ChangeScriptTimeout) + "s") + scripts = append(scripts, struct { + path string + args []string + timeout time.Duration + }{ + path: tmpl.ChangeScriptPath, + args: strings.Split(tmpl.ChangeScriptArguments, ","), + timeout: duration, + }) case structs.TemplateChangeModeNoop: continue } @@ -496,6 +510,14 @@ func (tm *TaskTemplateManager) onTemplateRendered(handledRenders map[string]time SetFailsTask(). SetDisplayMessage(fmt.Sprintf("Template failed to send signals %v: %v", flat, err))) } + } else if len(scripts) != 0 { + for _, script := range scripts { + _, _, err := tm.config.Handle.Exec(script.timeout, script.path, script.args) + if err != nil { + structs.NewTaskEvent(structs.TaskDriverMessage). + SetDisplayMessage(fmt.Sprintf("Template failed to run script on change: %v", err)) + } + } } } diff --git a/client/allocrunner/taskrunner/template_hook.go b/client/allocrunner/taskrunner/template_hook.go index b2dfe7be03af..74f264e281dc 100644 --- a/client/allocrunner/taskrunner/template_hook.go +++ b/client/allocrunner/taskrunner/template_hook.go @@ -43,6 +43,7 @@ type templateHookConfig struct { // nomadNamespace is the job's Nomad namespace nomadNamespace string + // handle is the driver handle that allows driver operations handle *DriverHandle } @@ -133,7 +134,7 @@ func (h *templateHook) newManager() (unblock chan struct{}, err error) { MaxTemplateEventRate: template.DefaultMaxTemplateEventRate, NomadNamespace: h.config.nomadNamespace, NomadToken: h.nomadToken, - ExecHandle: h.config.handle, + Handle: h.config.handle, }) if err != nil { h.logger.Error("failed to create template manager", "error", err) diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 65e410b9ea93..38584d54dc92 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -7708,6 +7708,16 @@ type Template struct { // requires it. ChangeSignal string + // ChangeScriptPath is the path to the script that is being triggered if the + // change mode requires it + ChangeScriptPath string + // ChangeScriptArguments is a coma-separated list of arguments passed to the + // script + ChangeScriptArguments string + // ChangeScriptTimeout is the amount of seconds we wait for the script to + // run + ChangeScriptTimeout int + // Splay is used to avoid coordinated restarts of processes by applying a // random wait between 0 and the given splay value before signalling the // application of a change From 634e2d473eed24a1882c6318397469c42ed75854 Mon Sep 17 00:00:00 2001 From: Peter Kazmierczak Date: Thu, 4 Aug 2022 19:42:26 +0200 Subject: [PATCH 06/68] documentation --- website/content/api-docs/json-jobs.mdx | 11 +++++++++++ website/content/docs/job-specification/template.mdx | 11 +++++++++++ 2 files changed, 22 insertions(+) diff --git a/website/content/api-docs/json-jobs.mdx b/website/content/api-docs/json-jobs.mdx index 0aa3a29fc851..f5cdf2b2f5f1 100644 --- a/website/content/api-docs/json-jobs.mdx +++ b/website/content/api-docs/json-jobs.mdx @@ -1061,6 +1061,17 @@ README][ct]. "SIGUSR1" or "SIGINT". This option is required if the `ChangeMode` is `signal`. +- `ChangeScriptPath` - Specifies the full path to a script that is to be + executed on template change. This option is required if the `change_mode` is + `script`. + +- `ChangeScriptArguments` - Comma-separated list of arguments that are passed to + the script that is to be executed on template change. This option is required if + the `change_mode` is `script`. + +- `ChangeScriptTimeout` - Timeout for script execution in seconds. This option + is required if the `change_mode` is `script`. + - `DestPath` - Specifies the location where the resulting template should be rendered, relative to the task directory. diff --git a/website/content/docs/job-specification/template.mdx b/website/content/docs/job-specification/template.mdx index 4f18d3de9977..71cdf4259a34 100644 --- a/website/content/docs/job-specification/template.mdx +++ b/website/content/docs/job-specification/template.mdx @@ -59,6 +59,17 @@ refer to the [Learn Go Template Syntax][gt_learn] Learn guide. string like `"SIGUSR1"` or `"SIGINT"`. This option is required if the `change_mode` is `signal`. +- `change_script_path` `(string: "")` - Specifies the full path to a script that + is to be executed on template change. This option is required if the + `change_mode` is `script`. + +- `change_script_arguments` `(string: "")` - Comma-separated list of arguments + that are passed to the script that is to be executed on template change. This + option is required if the `change_mode` is `script`. + +- `change_script_timeout` `(int: 0)` - Timeout for script execution in seconds. + This option is required if the `change_mode` is `script`. + - `data` `(string: "")` - Specifies the raw template to execute. One of `source` or `data` must be specified, but not both. This is useful for smaller templates, but we recommend using `source` for larger templates. From f7a53cb00c6e711d2d0a898794c8e5c275e88038 Mon Sep 17 00:00:00 2001 From: Peter Kazmierczak Date: Thu, 4 Aug 2022 20:22:53 +0200 Subject: [PATCH 07/68] timeout can just be time.Duration --- client/allocrunner/taskrunner/template/template.go | 3 +-- nomad/structs/structs.go | 8 ++++++-- website/content/api-docs/json-jobs.mdx | 8 ++++---- website/content/docs/job-specification/template.mdx | 8 ++++---- 4 files changed, 15 insertions(+), 12 deletions(-) diff --git a/client/allocrunner/taskrunner/template/template.go b/client/allocrunner/taskrunner/template/template.go index 433078004fdd..3e2055e353de 100644 --- a/client/allocrunner/taskrunner/template/template.go +++ b/client/allocrunner/taskrunner/template/template.go @@ -445,7 +445,6 @@ func (tm *TaskTemplateManager) onTemplateRendered(handledRenders map[string]time case structs.TemplateChangeModeRestart: restart = true case structs.TemplateChangeModeScript: - duration, _ := time.ParseDuration(strconv.Itoa(tmpl.ChangeScriptTimeout) + "s") scripts = append(scripts, struct { path string args []string @@ -453,7 +452,7 @@ func (tm *TaskTemplateManager) onTemplateRendered(handledRenders map[string]time }{ path: tmpl.ChangeScriptPath, args: strings.Split(tmpl.ChangeScriptArguments, ","), - timeout: duration, + timeout: tmpl.ChangeScriptTimeout, }) case structs.TemplateChangeModeNoop: continue diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 38584d54dc92..8174b2dbdfd1 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -7686,7 +7686,7 @@ const ( var ( // TemplateChangeModeInvalidError is the error for when an invalid change // mode is given - TemplateChangeModeInvalidError = errors.New("Invalid change mode. Must be one of the following: noop, signal, restart") + TemplateChangeModeInvalidError = errors.New("Invalid change mode. Must be one of the following: noop, signal, script, restart") ) // Template represents a template configuration to be rendered for a given task @@ -7716,7 +7716,7 @@ type Template struct { ChangeScriptArguments string // ChangeScriptTimeout is the amount of seconds we wait for the script to // run - ChangeScriptTimeout int + ChangeScriptTimeout time.Duration // Splay is used to avoid coordinated restarts of processes by applying a // random wait between 0 and the given splay value before signalling the @@ -7816,6 +7816,10 @@ func (t *Template) Validate() error { if t.Envvars { _ = multierror.Append(&mErr, fmt.Errorf("cannot use signals with env var templates")) } + case TemplateChangeModeScript: + if t.ChangeScriptPath == "" || t.ChangeScriptTimeout == 0 { + _ = multierror.Append(&mErr, fmt.Errorf("must specify script path and timeout value when change mode is signal")) + } default: _ = multierror.Append(&mErr, TemplateChangeModeInvalidError) } diff --git a/website/content/api-docs/json-jobs.mdx b/website/content/api-docs/json-jobs.mdx index f5cdf2b2f5f1..1216101aa4f5 100644 --- a/website/content/api-docs/json-jobs.mdx +++ b/website/content/api-docs/json-jobs.mdx @@ -1066,11 +1066,11 @@ README][ct]. `script`. - `ChangeScriptArguments` - Comma-separated list of arguments that are passed to - the script that is to be executed on template change. This option is required if - the `change_mode` is `script`. + the script that is to be executed on template change. -- `ChangeScriptTimeout` - Timeout for script execution in seconds. This option - is required if the `change_mode` is `script`. +- `ChangeScriptTimeout` - Timeout for script execution specified using a label + suffix like "30s" or "1h", This option is required if the `change_mode` is + `script`. - `DestPath` - Specifies the location where the resulting template should be rendered, relative to the task directory. diff --git a/website/content/docs/job-specification/template.mdx b/website/content/docs/job-specification/template.mdx index 71cdf4259a34..aa7f981143da 100644 --- a/website/content/docs/job-specification/template.mdx +++ b/website/content/docs/job-specification/template.mdx @@ -64,11 +64,11 @@ refer to the [Learn Go Template Syntax][gt_learn] Learn guide. `change_mode` is `script`. - `change_script_arguments` `(string: "")` - Comma-separated list of arguments - that are passed to the script that is to be executed on template change. This - option is required if the `change_mode` is `script`. + that are passed to the script that is to be executed on template change. -- `change_script_timeout` `(int: 0)` - Timeout for script execution in seconds. - This option is required if the `change_mode` is `script`. +- `change_script_timeout` `(string: "5s")` - Timeout for script execution + specified using a label suffix like "30s" or "1h", This option is required if + the `change_mode` is `script`. - `data` `(string: "")` - Specifies the raw template to execute. One of `source` or `data` must be specified, but not both. This is useful for smaller From 56d0113a7fd713f41bb8c83a8f17ba3b4f6aae63 Mon Sep 17 00:00:00 2001 From: Peter Kazmierczak Date: Thu, 4 Aug 2022 20:31:10 +0200 Subject: [PATCH 08/68] tasks & jobspec update --- api/tasks.go | 40 +++++++++++++++++++++++------------ command/agent/job_endpoint.go | 31 +++++++++++++++------------ jobspec/parse_task.go | 3 +++ 3 files changed, 46 insertions(+), 28 deletions(-) diff --git a/api/tasks.go b/api/tasks.go index 2fc3c5643947..c2a4fd72da77 100644 --- a/api/tasks.go +++ b/api/tasks.go @@ -792,20 +792,23 @@ func (wc *WaitConfig) Copy() *WaitConfig { } type Template struct { - SourcePath *string `mapstructure:"source" hcl:"source,optional"` - DestPath *string `mapstructure:"destination" hcl:"destination,optional"` - EmbeddedTmpl *string `mapstructure:"data" hcl:"data,optional"` - ChangeMode *string `mapstructure:"change_mode" hcl:"change_mode,optional"` - ChangeSignal *string `mapstructure:"change_signal" hcl:"change_signal,optional"` - Splay *time.Duration `mapstructure:"splay" hcl:"splay,optional"` - Perms *string `mapstructure:"perms" hcl:"perms,optional"` - Uid *int `mapstructure:"uid" hcl:"uid,optional"` - Gid *int `mapstructure:"gid" hcl:"gid,optional"` - LeftDelim *string `mapstructure:"left_delimiter" hcl:"left_delimiter,optional"` - RightDelim *string `mapstructure:"right_delimiter" hcl:"right_delimiter,optional"` - Envvars *bool `mapstructure:"env" hcl:"env,optional"` - VaultGrace *time.Duration `mapstructure:"vault_grace" hcl:"vault_grace,optional"` - Wait *WaitConfig `mapstructure:"wait" hcl:"wait,block"` + SourcePath *string `mapstructure:"source" hcl:"source,optional"` + DestPath *string `mapstructure:"destination" hcl:"destination,optional"` + EmbeddedTmpl *string `mapstructure:"data" hcl:"data,optional"` + ChangeMode *string `mapstructure:"change_mode" hcl:"change_mode,optional"` + ChangeScriptPath *string `mapstructure:"change_mode_path" hcl:"change_mode_path,optional"` + ChangeScriptArguments *string `mapstructure:"change_script_arguments" hcl:"change_script_arguments,optional"` + ChangeScriptTimeout *time.Duration `mapstructure:"change_script_timeout" hcl:"change_script_timeout,optional"` + ChangeSignal *string `mapstructure:"change_signal" hcl:"change_signal,optional"` + Splay *time.Duration `mapstructure:"splay" hcl:"splay,optional"` + Perms *string `mapstructure:"perms" hcl:"perms,optional"` + Uid *int `mapstructure:"uid" hcl:"uid,optional"` + Gid *int `mapstructure:"gid" hcl:"gid,optional"` + LeftDelim *string `mapstructure:"left_delimiter" hcl:"left_delimiter,optional"` + RightDelim *string `mapstructure:"right_delimiter" hcl:"right_delimiter,optional"` + Envvars *bool `mapstructure:"env" hcl:"env,optional"` + VaultGrace *time.Duration `mapstructure:"vault_grace" hcl:"vault_grace,optional"` + Wait *WaitConfig `mapstructure:"wait" hcl:"wait,block"` } func (tmpl *Template) Canonicalize() { @@ -831,6 +834,15 @@ func (tmpl *Template) Canonicalize() { sig := *tmpl.ChangeSignal tmpl.ChangeSignal = stringToPtr(strings.ToUpper(sig)) } + if tmpl.ChangeScriptPath == nil { + tmpl.ChangeScriptPath = stringToPtr("") + } + if tmpl.ChangeScriptArguments == nil { + tmpl.ChangeScriptPath = stringToPtr("") + } + if tmpl.ChangeScriptTimeout == nil { + tmpl.ChangeScriptTimeout = timeToPtr(5 * time.Second) + } if tmpl.Splay == nil { tmpl.Splay = timeToPtr(5 * time.Second) } diff --git a/command/agent/job_endpoint.go b/command/agent/job_endpoint.go index f76ca8333c44..99dec5243fea 100644 --- a/command/agent/job_endpoint.go +++ b/command/agent/job_endpoint.go @@ -1219,20 +1219,23 @@ func ApiTaskToStructsTask(job *structs.Job, group *structs.TaskGroup, } structsTask.Templates = append(structsTask.Templates, &structs.Template{ - SourcePath: *template.SourcePath, - DestPath: *template.DestPath, - EmbeddedTmpl: *template.EmbeddedTmpl, - ChangeMode: *template.ChangeMode, - ChangeSignal: *template.ChangeSignal, - Splay: *template.Splay, - Perms: *template.Perms, - Uid: uid, - Gid: gid, - LeftDelim: *template.LeftDelim, - RightDelim: *template.RightDelim, - Envvars: *template.Envvars, - VaultGrace: *template.VaultGrace, - Wait: ApiWaitConfigToStructsWaitConfig(template.Wait), + SourcePath: *template.SourcePath, + DestPath: *template.DestPath, + EmbeddedTmpl: *template.EmbeddedTmpl, + ChangeMode: *template.ChangeMode, + ChangeSignal: *template.ChangeSignal, + ChangeScriptPath: *template.ChangeScriptPath, + ChangeScriptArguments: *template.ChangeScriptArguments, + ChangeScriptTimeout: *template.ChangeScriptTimeout, + Splay: *template.Splay, + Perms: *template.Perms, + Uid: uid, + Gid: gid, + LeftDelim: *template.LeftDelim, + RightDelim: *template.RightDelim, + Envvars: *template.Envvars, + VaultGrace: *template.VaultGrace, + Wait: ApiWaitConfigToStructsWaitConfig(template.Wait), }) } } diff --git a/jobspec/parse_task.go b/jobspec/parse_task.go index 87299146719a..165710ee86ea 100644 --- a/jobspec/parse_task.go +++ b/jobspec/parse_task.go @@ -437,6 +437,9 @@ func parseTemplates(result *[]*api.Template, list *ast.ObjectList) error { valid := []string{ "change_mode", "change_signal", + "change_script_path", + "change_script_arguments", + "change_script_timeout", "data", "destination", "left_delimiter", From efbd9cfb4d5f432a264cca56d4c1f072d68b0f7d Mon Sep 17 00:00:00 2001 From: Peter Kazmierczak Date: Thu, 4 Aug 2022 20:39:47 +0200 Subject: [PATCH 09/68] typos --- api/tasks.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api/tasks.go b/api/tasks.go index c2a4fd72da77..a0a1d3bc8c46 100644 --- a/api/tasks.go +++ b/api/tasks.go @@ -796,7 +796,7 @@ type Template struct { DestPath *string `mapstructure:"destination" hcl:"destination,optional"` EmbeddedTmpl *string `mapstructure:"data" hcl:"data,optional"` ChangeMode *string `mapstructure:"change_mode" hcl:"change_mode,optional"` - ChangeScriptPath *string `mapstructure:"change_mode_path" hcl:"change_mode_path,optional"` + ChangeScriptPath *string `mapstructure:"change_script_path" hcl:"change_script_path,optional"` ChangeScriptArguments *string `mapstructure:"change_script_arguments" hcl:"change_script_arguments,optional"` ChangeScriptTimeout *time.Duration `mapstructure:"change_script_timeout" hcl:"change_script_timeout,optional"` ChangeSignal *string `mapstructure:"change_signal" hcl:"change_signal,optional"` @@ -838,7 +838,7 @@ func (tmpl *Template) Canonicalize() { tmpl.ChangeScriptPath = stringToPtr("") } if tmpl.ChangeScriptArguments == nil { - tmpl.ChangeScriptPath = stringToPtr("") + tmpl.ChangeScriptArguments = stringToPtr("") } if tmpl.ChangeScriptTimeout == nil { tmpl.ChangeScriptTimeout = timeToPtr(5 * time.Second) From fbee6a2a8f58a099c85578bba498ff2e20320376 Mon Sep 17 00:00:00 2001 From: Peter Kazmierczak Date: Fri, 5 Aug 2022 15:25:21 +0200 Subject: [PATCH 10/68] adjusted tests --- api/jobs_test.go | 58 ++++++++------ command/agent/job_endpoint_test.go | 54 +++++++------ nomad/structs/diff_test.go | 122 ++++++++++++++++++++--------- 3 files changed, 147 insertions(+), 87 deletions(-) diff --git a/api/jobs_test.go b/api/jobs_test.go index b585a6ac9b77..1584bd3cbdfc 100644 --- a/api/jobs_test.go +++ b/api/jobs_test.go @@ -758,34 +758,40 @@ func TestJobs_Canonicalize(t *testing.T) { LogConfig: DefaultLogConfig(), Templates: []*Template{ { - SourcePath: stringToPtr(""), - DestPath: stringToPtr("local/file.yml"), - EmbeddedTmpl: stringToPtr("---"), - ChangeMode: stringToPtr("restart"), - ChangeSignal: stringToPtr(""), - Splay: timeToPtr(5 * time.Second), - Perms: stringToPtr("0644"), - Uid: intToPtr(0), - Gid: intToPtr(0), - LeftDelim: stringToPtr("{{"), - RightDelim: stringToPtr("}}"), - Envvars: boolToPtr(false), - VaultGrace: timeToPtr(0), + SourcePath: stringToPtr(""), + DestPath: stringToPtr("local/file.yml"), + EmbeddedTmpl: stringToPtr("---"), + ChangeMode: stringToPtr("restart"), + ChangeSignal: stringToPtr(""), + ChangeScriptPath: stringToPtr(""), + ChangeScriptArguments: stringToPtr(""), + ChangeScriptTimeout: timeToPtr(5 * time.Second), + Splay: timeToPtr(5 * time.Second), + Perms: stringToPtr("0644"), + Uid: intToPtr(0), + Gid: intToPtr(0), + LeftDelim: stringToPtr("{{"), + RightDelim: stringToPtr("}}"), + Envvars: boolToPtr(false), + VaultGrace: timeToPtr(0), }, { - SourcePath: stringToPtr(""), - DestPath: stringToPtr("local/file.env"), - EmbeddedTmpl: stringToPtr("FOO=bar\n"), - ChangeMode: stringToPtr("restart"), - ChangeSignal: stringToPtr(""), - Splay: timeToPtr(5 * time.Second), - Perms: stringToPtr("0644"), - Uid: intToPtr(0), - Gid: intToPtr(0), - LeftDelim: stringToPtr("{{"), - RightDelim: stringToPtr("}}"), - Envvars: boolToPtr(true), - VaultGrace: timeToPtr(0), + SourcePath: stringToPtr(""), + DestPath: stringToPtr("local/file.env"), + EmbeddedTmpl: stringToPtr("FOO=bar\n"), + ChangeMode: stringToPtr("restart"), + ChangeSignal: stringToPtr(""), + ChangeScriptPath: stringToPtr(""), + ChangeScriptArguments: stringToPtr(""), + ChangeScriptTimeout: timeToPtr(5 * time.Second), + Splay: timeToPtr(5 * time.Second), + Perms: stringToPtr("0644"), + Uid: intToPtr(0), + Gid: intToPtr(0), + LeftDelim: stringToPtr("{{"), + RightDelim: stringToPtr("}}"), + Envvars: boolToPtr(true), + VaultGrace: timeToPtr(0), }, }, }, diff --git a/command/agent/job_endpoint_test.go b/command/agent/job_endpoint_test.go index b4dc7ff05c94..95bdc5bb6ad6 100644 --- a/command/agent/job_endpoint_test.go +++ b/command/agent/job_endpoint_test.go @@ -2726,18 +2726,21 @@ func TestJobs_ApiJobToStructsJob(t *testing.T) { }, Templates: []*api.Template{ { - SourcePath: helper.StringToPtr("source"), - DestPath: helper.StringToPtr("dest"), - EmbeddedTmpl: helper.StringToPtr("embedded"), - ChangeMode: helper.StringToPtr("change"), - ChangeSignal: helper.StringToPtr("signal"), - Splay: helper.TimeToPtr(1 * time.Minute), - Perms: helper.StringToPtr("666"), - Uid: helper.IntToPtr(1000), - Gid: helper.IntToPtr(1000), - LeftDelim: helper.StringToPtr("abc"), - RightDelim: helper.StringToPtr("def"), - Envvars: helper.BoolToPtr(true), + SourcePath: helper.StringToPtr("source"), + DestPath: helper.StringToPtr("dest"), + EmbeddedTmpl: helper.StringToPtr("embedded"), + ChangeMode: helper.StringToPtr("change"), + ChangeSignal: helper.StringToPtr("signal"), + ChangeScriptPath: helper.StringToPtr("/bin/foo"), + ChangeScriptArguments: helper.StringToPtr("-h"), + ChangeScriptTimeout: helper.TimeToPtr(5 * time.Second), + Splay: helper.TimeToPtr(1 * time.Minute), + Perms: helper.StringToPtr("666"), + Uid: helper.IntToPtr(1000), + Gid: helper.IntToPtr(1000), + LeftDelim: helper.StringToPtr("abc"), + RightDelim: helper.StringToPtr("def"), + Envvars: helper.BoolToPtr(true), Wait: &api.WaitConfig{ Min: helper.TimeToPtr(5 * time.Second), Max: helper.TimeToPtr(10 * time.Second), @@ -3133,18 +3136,21 @@ func TestJobs_ApiJobToStructsJob(t *testing.T) { }, Templates: []*structs.Template{ { - SourcePath: "source", - DestPath: "dest", - EmbeddedTmpl: "embedded", - ChangeMode: "change", - ChangeSignal: "SIGNAL", - Splay: 1 * time.Minute, - Perms: "666", - Uid: 1000, - Gid: 1000, - LeftDelim: "abc", - RightDelim: "def", - Envvars: true, + SourcePath: "source", + DestPath: "dest", + EmbeddedTmpl: "embedded", + ChangeMode: "change", + ChangeSignal: "SIGNAL", + ChangeScriptPath: "/bin/foo", + ChangeScriptArguments: "-h", + ChangeScriptTimeout: 5 * time.Second, + Splay: 1 * time.Minute, + Perms: "666", + Uid: 1000, + Gid: 1000, + LeftDelim: "abc", + RightDelim: "def", + Envvars: true, Wait: &structs.WaitConfig{ Min: helper.TimeToPtr(5 * time.Second), Max: helper.TimeToPtr(10 * time.Second), diff --git a/nomad/structs/diff_test.go b/nomad/structs/diff_test.go index d081a72bed54..e4893cb5f28c 100644 --- a/nomad/structs/diff_test.go +++ b/nomad/structs/diff_test.go @@ -7037,61 +7037,73 @@ func TestTaskDiff(t *testing.T) { Old: &Task{ Templates: []*Template{ { - SourcePath: "foo", - DestPath: "bar", - EmbeddedTmpl: "baz", - ChangeMode: "bam", - ChangeSignal: "SIGHUP", - Splay: 1, - Perms: "0644", - Uid: 1001, - Gid: 21, + SourcePath: "foo", + DestPath: "bar", + EmbeddedTmpl: "baz", + ChangeMode: "bam", + ChangeSignal: "SIGHUP", + ChangeScriptPath: "/bin/foo", + ChangeScriptArguments: "-debug", + ChangeScriptTimeout: 5, + Splay: 1, + Perms: "0644", + Uid: 1001, + Gid: 21, Wait: &WaitConfig{ Min: helper.TimeToPtr(5 * time.Second), Max: helper.TimeToPtr(5 * time.Second), }, }, { - SourcePath: "foo2", - DestPath: "bar2", - EmbeddedTmpl: "baz2", - ChangeMode: "bam2", - ChangeSignal: "SIGHUP2", - Splay: 2, - Perms: "0666", - Uid: 1000, - Gid: 20, - Envvars: true, + SourcePath: "foo2", + DestPath: "bar2", + EmbeddedTmpl: "baz2", + ChangeMode: "bam2", + ChangeSignal: "SIGHUP2", + ChangeScriptPath: "/bin/foo2", + ChangeScriptArguments: "-debugs", + ChangeScriptTimeout: 6, + Splay: 2, + Perms: "0666", + Uid: 1000, + Gid: 20, + Envvars: true, }, }, }, New: &Task{ Templates: []*Template{ { - SourcePath: "foo", - DestPath: "bar", - EmbeddedTmpl: "baz new", - ChangeMode: "bam", - ChangeSignal: "SIGHUP", - Splay: 1, - Perms: "0644", - Uid: 1001, - Gid: 21, + SourcePath: "foo", + DestPath: "bar", + EmbeddedTmpl: "baz new", + ChangeMode: "bam", + ChangeSignal: "SIGHUP", + ChangeScriptPath: "/bin/foo", + ChangeScriptArguments: "-debug", + ChangeScriptTimeout: 5, + Splay: 1, + Perms: "0644", + Uid: 1001, + Gid: 21, Wait: &WaitConfig{ Min: helper.TimeToPtr(5 * time.Second), Max: helper.TimeToPtr(10 * time.Second), }, }, { - SourcePath: "foo3", - DestPath: "bar3", - EmbeddedTmpl: "baz3", - ChangeMode: "bam3", - ChangeSignal: "SIGHUP3", - Splay: 3, - Perms: "0776", - Uid: 1002, - Gid: 22, + SourcePath: "foo3", + DestPath: "bar3", + EmbeddedTmpl: "baz3", + ChangeMode: "bam3", + ChangeSignal: "SIGHUP3", + ChangeScriptPath: "/bin/foo3", + ChangeScriptArguments: "-debugss", + ChangeScriptTimeout: 7, + Splay: 3, + Perms: "0776", + Uid: 1002, + Gid: 22, Wait: &WaitConfig{ Min: helper.TimeToPtr(5 * time.Second), Max: helper.TimeToPtr(10 * time.Second), @@ -7138,6 +7150,24 @@ func TestTaskDiff(t *testing.T) { Old: "", New: "bam3", }, + { + Type: DiffTypeAdded, + Name: "ChangeScriptArguments", + Old: "", + New: "-debugss", + }, + { + Type: DiffTypeAdded, + Name: "ChangeScriptPath", + Old: "", + New: "/bin/foo3", + }, + { + Type: DiffTypeAdded, + Name: "ChangeScriptTimeout", + Old: "", + New: "7", + }, { Type: DiffTypeAdded, Name: "ChangeSignal", @@ -7230,6 +7260,24 @@ func TestTaskDiff(t *testing.T) { Old: "bam2", New: "", }, + { + Type: DiffTypeAdded, + Name: "ChangeScriptArguments", + Old: "-debugs", + New: "", + }, + { + Type: DiffTypeAdded, + Name: "ChangeScriptPath", + Old: "/bin/foo2", + New: "", + }, + { + Type: DiffTypeAdded, + Name: "ChangeScriptTimeout", + Old: "6", + New: "", + }, { Type: DiffTypeDeleted, Name: "ChangeSignal", From 5aa40b5c95c13aafe334a6ac63c253fa61e0e629 Mon Sep 17 00:00:00 2001 From: Peter Kazmierczak Date: Mon, 8 Aug 2022 14:15:08 +0200 Subject: [PATCH 11/68] wip --- .../taskrunner/template/template_test.go | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/client/allocrunner/taskrunner/template/template_test.go b/client/allocrunner/taskrunner/template/template_test.go index b22fe7e1881c..65e1052d511e 100644 --- a/client/allocrunner/taskrunner/template/template_test.go +++ b/client/allocrunner/taskrunner/template/template_test.go @@ -1201,6 +1201,56 @@ func TestTaskTemplateManager_Signal_Error(t *testing.T) { require.Contains(harness.mockHooks.KillEvent.DisplayMessage, "failed to send signals") } +func TestTaskTemplateManager_Script(t *testing.T) { + ci.Parallel(t) + require := require.New(t) + + // Make a template that renders based on a key in Consul and triggers a script + key1 := "foo" + content1 := "bar" + content2 := "baz" + embedded1 := fmt.Sprintf(`{{key "%s"}}`, key1) + file1 := "my.tmpl" + template := &structs.Template{ + EmbeddedTmpl: embedded1, + DestPath: file1, + ChangeMode: structs.TemplateChangeModeScript, + ChangeScriptPath: "", // FIXME + ChangeScriptArguments: "", // FIXME + ChangeScriptTimeout: 5 * time.Second, + } + + harness := newTestHarness(t, []*structs.Template{template}, true, false) + harness.start(t) + defer harness.stop() + + // harness.mockHooks.SignalError = fmt.Errorf("test error") + + // Write the key to Consul + harness.consul.SetKV(t, key1, []byte(content1)) + + // Wait a little + select { + case <-harness.mockHooks.UnblockCh: + case <-time.After(time.Duration(2*testutil.TestMultiplier()) * time.Second): + t.Fatalf("Should have received unblock: %+v", harness.mockHooks) + } + + // Write the key to Consul + harness.consul.SetKV(t, key1, []byte(content2)) + + // Wait for kill channel + select { + case <-harness.mockHooks.KillCh: + break + case <-time.After(time.Duration(1*testutil.TestMultiplier()) * time.Second): + t.Fatalf("Should have received a signals: %+v", harness.mockHooks) + } + + require.NotNil(harness.mockHooks.KillEvent) + require.Contains(harness.mockHooks.KillEvent.DisplayMessage, "failed to send signals") +} + // TestTaskTemplateManager_FiltersProcessEnvVars asserts that we only render // environment variables found in task env-vars and not read the nomad host // process environment variables. nomad host process environment variables From a8fcabab49a3b7ce3b8aaceea2e65dd5c526acde Mon Sep 17 00:00:00 2001 From: Peter Kazmierczak Date: Mon, 8 Aug 2022 15:43:38 +0200 Subject: [PATCH 12/68] corrected diff_test --- nomad/structs/diff_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/nomad/structs/diff_test.go b/nomad/structs/diff_test.go index e4893cb5f28c..9124ff0ec320 100644 --- a/nomad/structs/diff_test.go +++ b/nomad/structs/diff_test.go @@ -7261,19 +7261,19 @@ func TestTaskDiff(t *testing.T) { New: "", }, { - Type: DiffTypeAdded, + Type: DiffTypeDeleted, Name: "ChangeScriptArguments", Old: "-debugs", New: "", }, { - Type: DiffTypeAdded, + Type: DiffTypeDeleted, Name: "ChangeScriptPath", Old: "/bin/foo2", New: "", }, { - Type: DiffTypeAdded, + Type: DiffTypeDeleted, Name: "ChangeScriptTimeout", Old: "6", New: "", From a2dd1762e5bb0a3482b52fecc61a505d38d92cdd Mon Sep 17 00:00:00 2001 From: Peter Kazmierczak Date: Mon, 8 Aug 2022 19:10:29 +0200 Subject: [PATCH 13/68] applied Derek's comments --- api/jobs_test.go | 60 +++--- api/tasks.go | 52 ++--- .../taskrunner/template/template.go | 21 +- .../taskrunner/template/template_test.go | 12 +- nomad/structs/diff_test.go | 192 ++++++++++-------- nomad/structs/structs.go | 32 ++- 6 files changed, 193 insertions(+), 176 deletions(-) diff --git a/api/jobs_test.go b/api/jobs_test.go index 1584bd3cbdfc..78db8f991d14 100644 --- a/api/jobs_test.go +++ b/api/jobs_test.go @@ -758,40 +758,36 @@ func TestJobs_Canonicalize(t *testing.T) { LogConfig: DefaultLogConfig(), Templates: []*Template{ { - SourcePath: stringToPtr(""), - DestPath: stringToPtr("local/file.yml"), - EmbeddedTmpl: stringToPtr("---"), - ChangeMode: stringToPtr("restart"), - ChangeSignal: stringToPtr(""), - ChangeScriptPath: stringToPtr(""), - ChangeScriptArguments: stringToPtr(""), - ChangeScriptTimeout: timeToPtr(5 * time.Second), - Splay: timeToPtr(5 * time.Second), - Perms: stringToPtr("0644"), - Uid: intToPtr(0), - Gid: intToPtr(0), - LeftDelim: stringToPtr("{{"), - RightDelim: stringToPtr("}}"), - Envvars: boolToPtr(false), - VaultGrace: timeToPtr(0), + SourcePath: stringToPtr(""), + DestPath: stringToPtr("local/file.yml"), + EmbeddedTmpl: stringToPtr("---"), + ChangeMode: stringToPtr("restart"), + ChangeSignal: stringToPtr(""), + ChangeScriptConfig: &ChangeScriptConfig{Path: stringToPtr(""), Args: []*string{}, Timeout: timeToPtr(5 * time.Second)}, + Splay: timeToPtr(5 * time.Second), + Perms: stringToPtr("0644"), + Uid: intToPtr(0), + Gid: intToPtr(0), + LeftDelim: stringToPtr("{{"), + RightDelim: stringToPtr("}}"), + Envvars: boolToPtr(false), + VaultGrace: timeToPtr(0), }, { - SourcePath: stringToPtr(""), - DestPath: stringToPtr("local/file.env"), - EmbeddedTmpl: stringToPtr("FOO=bar\n"), - ChangeMode: stringToPtr("restart"), - ChangeSignal: stringToPtr(""), - ChangeScriptPath: stringToPtr(""), - ChangeScriptArguments: stringToPtr(""), - ChangeScriptTimeout: timeToPtr(5 * time.Second), - Splay: timeToPtr(5 * time.Second), - Perms: stringToPtr("0644"), - Uid: intToPtr(0), - Gid: intToPtr(0), - LeftDelim: stringToPtr("{{"), - RightDelim: stringToPtr("}}"), - Envvars: boolToPtr(true), - VaultGrace: timeToPtr(0), + SourcePath: stringToPtr(""), + DestPath: stringToPtr("local/file.env"), + EmbeddedTmpl: stringToPtr("FOO=bar\n"), + ChangeMode: stringToPtr("restart"), + ChangeSignal: stringToPtr(""), + ChangeScriptConfig: &ChangeScriptConfig{Path: stringToPtr(""), Args: []*string{}, Timeout: timeToPtr(5 * time.Second)}, + Splay: timeToPtr(5 * time.Second), + Perms: stringToPtr("0644"), + Uid: intToPtr(0), + Gid: intToPtr(0), + LeftDelim: stringToPtr("{{"), + RightDelim: stringToPtr("}}"), + Envvars: boolToPtr(true), + VaultGrace: timeToPtr(0), }, }, }, diff --git a/api/tasks.go b/api/tasks.go index a0a1d3bc8c46..e09bcdf670f0 100644 --- a/api/tasks.go +++ b/api/tasks.go @@ -791,24 +791,28 @@ func (wc *WaitConfig) Copy() *WaitConfig { return nwc } +type ChangeScriptConfig struct { + Path *string `mapstructure:"path" hcl:"path"` + Args []*string `mapstructure:"args" hcl:"args,optional"` + Timeout *time.Duration `mapstructure:"timeout" hcl:"timeout"` +} + type Template struct { - SourcePath *string `mapstructure:"source" hcl:"source,optional"` - DestPath *string `mapstructure:"destination" hcl:"destination,optional"` - EmbeddedTmpl *string `mapstructure:"data" hcl:"data,optional"` - ChangeMode *string `mapstructure:"change_mode" hcl:"change_mode,optional"` - ChangeScriptPath *string `mapstructure:"change_script_path" hcl:"change_script_path,optional"` - ChangeScriptArguments *string `mapstructure:"change_script_arguments" hcl:"change_script_arguments,optional"` - ChangeScriptTimeout *time.Duration `mapstructure:"change_script_timeout" hcl:"change_script_timeout,optional"` - ChangeSignal *string `mapstructure:"change_signal" hcl:"change_signal,optional"` - Splay *time.Duration `mapstructure:"splay" hcl:"splay,optional"` - Perms *string `mapstructure:"perms" hcl:"perms,optional"` - Uid *int `mapstructure:"uid" hcl:"uid,optional"` - Gid *int `mapstructure:"gid" hcl:"gid,optional"` - LeftDelim *string `mapstructure:"left_delimiter" hcl:"left_delimiter,optional"` - RightDelim *string `mapstructure:"right_delimiter" hcl:"right_delimiter,optional"` - Envvars *bool `mapstructure:"env" hcl:"env,optional"` - VaultGrace *time.Duration `mapstructure:"vault_grace" hcl:"vault_grace,optional"` - Wait *WaitConfig `mapstructure:"wait" hcl:"wait,block"` + SourcePath *string `mapstructure:"source" hcl:"source,optional"` + DestPath *string `mapstructure:"destination" hcl:"destination,optional"` + EmbeddedTmpl *string `mapstructure:"data" hcl:"data,optional"` + ChangeMode *string `mapstructure:"change_mode" hcl:"change_mode,optional"` + ChangeScriptConfig *ChangeScriptConfig `mapstructure:"change_script_config" hcl:"change_script_config,optional"` + ChangeSignal *string `mapstructure:"change_signal" hcl:"change_signal,optional"` + Splay *time.Duration `mapstructure:"splay" hcl:"splay,optional"` + Perms *string `mapstructure:"perms" hcl:"perms,optional"` + Uid *int `mapstructure:"uid" hcl:"uid,optional"` + Gid *int `mapstructure:"gid" hcl:"gid,optional"` + LeftDelim *string `mapstructure:"left_delimiter" hcl:"left_delimiter,optional"` + RightDelim *string `mapstructure:"right_delimiter" hcl:"right_delimiter,optional"` + Envvars *bool `mapstructure:"env" hcl:"env,optional"` + VaultGrace *time.Duration `mapstructure:"vault_grace" hcl:"vault_grace,optional"` + Wait *WaitConfig `mapstructure:"wait" hcl:"wait,block"` } func (tmpl *Template) Canonicalize() { @@ -834,14 +838,12 @@ func (tmpl *Template) Canonicalize() { sig := *tmpl.ChangeSignal tmpl.ChangeSignal = stringToPtr(strings.ToUpper(sig)) } - if tmpl.ChangeScriptPath == nil { - tmpl.ChangeScriptPath = stringToPtr("") - } - if tmpl.ChangeScriptArguments == nil { - tmpl.ChangeScriptArguments = stringToPtr("") - } - if tmpl.ChangeScriptTimeout == nil { - tmpl.ChangeScriptTimeout = timeToPtr(5 * time.Second) + if tmpl.ChangeScriptConfig == nil { + tmpl.ChangeScriptConfig = &ChangeScriptConfig{ + stringToPtr(""), + []*string{}, + timeToPtr(5 * time.Second), + } } if tmpl.Splay == nil { tmpl.Splay = timeToPtr(5 * time.Second) diff --git a/client/allocrunner/taskrunner/template/template.go b/client/allocrunner/taskrunner/template/template.go index 3e2055e353de..cf18fc9c904e 100644 --- a/client/allocrunner/taskrunner/template/template.go +++ b/client/allocrunner/taskrunner/template/template.go @@ -395,11 +395,7 @@ func (tm *TaskTemplateManager) onTemplateRendered(handledRenders map[string]time var handling []string signals := make(map[string]struct{}) - scripts := []struct { - path string - args []string - timeout time.Duration - }{} + scripts := []structs.ChangeScriptConfig{} restart := false var splay time.Duration @@ -445,15 +441,7 @@ func (tm *TaskTemplateManager) onTemplateRendered(handledRenders map[string]time case structs.TemplateChangeModeRestart: restart = true case structs.TemplateChangeModeScript: - scripts = append(scripts, struct { - path string - args []string - timeout time.Duration - }{ - path: tmpl.ChangeScriptPath, - args: strings.Split(tmpl.ChangeScriptArguments, ","), - timeout: tmpl.ChangeScriptTimeout, - }) + scripts = append(scripts, *tmpl.ChangeScriptConfig) case structs.TemplateChangeModeNoop: continue } @@ -509,9 +497,10 @@ func (tm *TaskTemplateManager) onTemplateRendered(handledRenders map[string]time SetFailsTask(). SetDisplayMessage(fmt.Sprintf("Template failed to send signals %v: %v", flat, err))) } - } else if len(scripts) != 0 { + } + if len(scripts) != 0 { for _, script := range scripts { - _, _, err := tm.config.Handle.Exec(script.timeout, script.path, script.args) + _, _, err := tm.config.Handle.Exec(script.Timeout, script.Path, script.Args) if err != nil { structs.NewTaskEvent(structs.TaskDriverMessage). SetDisplayMessage(fmt.Sprintf("Template failed to run script on change: %v", err)) diff --git a/client/allocrunner/taskrunner/template/template_test.go b/client/allocrunner/taskrunner/template/template_test.go index 65e1052d511e..248262258885 100644 --- a/client/allocrunner/taskrunner/template/template_test.go +++ b/client/allocrunner/taskrunner/template/template_test.go @@ -1182,7 +1182,7 @@ func TestTaskTemplateManager_Signal_Error(t *testing.T) { // Wait a little select { case <-harness.mockHooks.UnblockCh: - case <-time.After(time.Duration(2*testutil.TestMultiplier()) * time.Second): + case <-time.After(time.Duration(30 * time.Second)): t.Fatalf("Should have received unblock: %+v", harness.mockHooks) } @@ -1212,12 +1212,10 @@ func TestTaskTemplateManager_Script(t *testing.T) { embedded1 := fmt.Sprintf(`{{key "%s"}}`, key1) file1 := "my.tmpl" template := &structs.Template{ - EmbeddedTmpl: embedded1, - DestPath: file1, - ChangeMode: structs.TemplateChangeModeScript, - ChangeScriptPath: "", // FIXME - ChangeScriptArguments: "", // FIXME - ChangeScriptTimeout: 5 * time.Second, + EmbeddedTmpl: embedded1, + DestPath: file1, + ChangeMode: structs.TemplateChangeModeScript, + ChangeScriptConfig: &structs.ChangeScriptConfig{}, } harness := newTestHarness(t, []*structs.Template{template}, true, false) diff --git a/nomad/structs/diff_test.go b/nomad/structs/diff_test.go index 9124ff0ec320..bc05a474ddbb 100644 --- a/nomad/structs/diff_test.go +++ b/nomad/structs/diff_test.go @@ -7037,73 +7037,81 @@ func TestTaskDiff(t *testing.T) { Old: &Task{ Templates: []*Template{ { - SourcePath: "foo", - DestPath: "bar", - EmbeddedTmpl: "baz", - ChangeMode: "bam", - ChangeSignal: "SIGHUP", - ChangeScriptPath: "/bin/foo", - ChangeScriptArguments: "-debug", - ChangeScriptTimeout: 5, - Splay: 1, - Perms: "0644", - Uid: 1001, - Gid: 21, + SourcePath: "foo", + DestPath: "bar", + EmbeddedTmpl: "baz", + ChangeMode: "bam", + ChangeSignal: "SIGHUP", + ChangeScriptConfig: &ChangeScriptConfig{ + Path: "/bin/foo", + Args: []string{"-debug"}, + Timeout: 5, + }, + Splay: 1, + Perms: "0644", + Uid: 1001, + Gid: 21, Wait: &WaitConfig{ Min: helper.TimeToPtr(5 * time.Second), Max: helper.TimeToPtr(5 * time.Second), }, }, { - SourcePath: "foo2", - DestPath: "bar2", - EmbeddedTmpl: "baz2", - ChangeMode: "bam2", - ChangeSignal: "SIGHUP2", - ChangeScriptPath: "/bin/foo2", - ChangeScriptArguments: "-debugs", - ChangeScriptTimeout: 6, - Splay: 2, - Perms: "0666", - Uid: 1000, - Gid: 20, - Envvars: true, + SourcePath: "foo2", + DestPath: "bar2", + EmbeddedTmpl: "baz2", + ChangeMode: "bam2", + ChangeSignal: "SIGHUP2", + ChangeScriptConfig: &ChangeScriptConfig{ + Path: "/bin/foo2", + Args: []string{"-debugs"}, + Timeout: 6, + }, + Splay: 2, + Perms: "0666", + Uid: 1000, + Gid: 20, + Envvars: true, }, }, }, New: &Task{ Templates: []*Template{ { - SourcePath: "foo", - DestPath: "bar", - EmbeddedTmpl: "baz new", - ChangeMode: "bam", - ChangeSignal: "SIGHUP", - ChangeScriptPath: "/bin/foo", - ChangeScriptArguments: "-debug", - ChangeScriptTimeout: 5, - Splay: 1, - Perms: "0644", - Uid: 1001, - Gid: 21, + SourcePath: "foo", + DestPath: "bar", + EmbeddedTmpl: "baz new", + ChangeMode: "bam", + ChangeSignal: "SIGHUP", + ChangeScriptConfig: &ChangeScriptConfig{ + Path: "/bin/foo", + Args: []string{"-debug"}, + Timeout: 5, + }, + Splay: 1, + Perms: "0644", + Uid: 1001, + Gid: 21, Wait: &WaitConfig{ Min: helper.TimeToPtr(5 * time.Second), Max: helper.TimeToPtr(10 * time.Second), }, }, { - SourcePath: "foo3", - DestPath: "bar3", - EmbeddedTmpl: "baz3", - ChangeMode: "bam3", - ChangeSignal: "SIGHUP3", - ChangeScriptPath: "/bin/foo3", - ChangeScriptArguments: "-debugss", - ChangeScriptTimeout: 7, - Splay: 3, - Perms: "0776", - Uid: 1002, - Gid: 22, + SourcePath: "foo3", + DestPath: "bar3", + EmbeddedTmpl: "baz3", + ChangeMode: "bam3", + ChangeSignal: "SIGHUP3", + ChangeScriptConfig: &ChangeScriptConfig{ + Path: "/bin/foo3", + Args: []string{"-debugss"}, + Timeout: 7, + }, + Splay: 3, + Perms: "0776", + Uid: 1002, + Gid: 22, Wait: &WaitConfig{ Min: helper.TimeToPtr(5 * time.Second), Max: helper.TimeToPtr(10 * time.Second), @@ -7150,24 +7158,6 @@ func TestTaskDiff(t *testing.T) { Old: "", New: "bam3", }, - { - Type: DiffTypeAdded, - Name: "ChangeScriptArguments", - Old: "", - New: "-debugss", - }, - { - Type: DiffTypeAdded, - Name: "ChangeScriptPath", - Old: "", - New: "/bin/foo3", - }, - { - Type: DiffTypeAdded, - Name: "ChangeScriptTimeout", - Old: "", - New: "7", - }, { Type: DiffTypeAdded, Name: "ChangeSignal", @@ -7248,6 +7238,30 @@ func TestTaskDiff(t *testing.T) { }, }, }, + { + Type: DiffTypeAdded, + Name: "ChangeScriptConfig", + Fields: []*FieldDiff{ + { + Type: DiffTypeAdded, + Name: "Args", + Old: "", + New: "-debugss", + }, + { + Type: DiffTypeAdded, + Name: "Path", + Old: "", + New: "/bin/foo3", + }, + { + Type: DiffTypeAdded, + Name: "Timeout", + Old: "", + New: "7", + }, + }, + }, }, }, { @@ -7260,24 +7274,6 @@ func TestTaskDiff(t *testing.T) { Old: "bam2", New: "", }, - { - Type: DiffTypeDeleted, - Name: "ChangeScriptArguments", - Old: "-debugs", - New: "", - }, - { - Type: DiffTypeDeleted, - Name: "ChangeScriptPath", - Old: "/bin/foo2", - New: "", - }, - { - Type: DiffTypeDeleted, - Name: "ChangeScriptTimeout", - Old: "6", - New: "", - }, { Type: DiffTypeDeleted, Name: "ChangeSignal", @@ -7339,6 +7335,32 @@ func TestTaskDiff(t *testing.T) { New: "", }, }, + Objects: []*ObjectDiff{ + { + Type: DiffTypeDeleted, + Name: "ChangeScriptConfig", + Fields: []*FieldDiff{ + { + Type: DiffTypeDeleted, + Name: "Args", + Old: "-debugs", + New: "", + }, + { + Type: DiffTypeDeleted, + Name: "Path", + Old: "/bin/foo2", + New: "", + }, + { + Type: DiffTypeDeleted, + Name: "Timeout", + Old: "6", + New: "", + }, + }, + }, + }, }, }, }, diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 8174b2dbdfd1..f2620cce15a4 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -7689,6 +7689,19 @@ var ( TemplateChangeModeInvalidError = errors.New("Invalid change mode. Must be one of the following: noop, signal, script, restart") ) +// ChangeScriptConfig holds the configuration for the script that is executed if +// change mode is set to script +type ChangeScriptConfig struct { + // Path is the full path to the script + Path string + // Args is a slice of arguments passed to the script + Args []string + // Timeout is the amount of seconds we wait for the script to finish + Timeout time.Duration + // TODO: add a field that users could use to specify whether a task should + // fail if the script fails +} + // Template represents a template configuration to be rendered for a given task type Template struct { // SourcePath is the path to the template to be rendered @@ -7708,15 +7721,9 @@ type Template struct { // requires it. ChangeSignal string - // ChangeScriptPath is the path to the script that is being triggered if the - // change mode requires it - ChangeScriptPath string - // ChangeScriptArguments is a coma-separated list of arguments passed to the - // script - ChangeScriptArguments string - // ChangeScriptTimeout is the amount of seconds we wait for the script to - // run - ChangeScriptTimeout time.Duration + // ChangeScriptConfig is the configuration of the script. It's required if + // ChangeMode is set to script. + ChangeScriptConfig *ChangeScriptConfig // Splay is used to avoid coordinated restarts of processes by applying a // random wait between 0 and the given splay value before signalling the @@ -7817,9 +7824,12 @@ func (t *Template) Validate() error { _ = multierror.Append(&mErr, fmt.Errorf("cannot use signals with env var templates")) } case TemplateChangeModeScript: - if t.ChangeScriptPath == "" || t.ChangeScriptTimeout == 0 { + if t.ChangeScriptConfig.Path == "" { _ = multierror.Append(&mErr, fmt.Errorf("must specify script path and timeout value when change mode is signal")) } + if t.ChangeScriptConfig.Timeout == 0 { + _ = multierror.Append(&mErr, fmt.Errorf("must specify script timeout value when change mode is script, timeout value must be greater than 0")) + } default: _ = multierror.Append(&mErr, TemplateChangeModeInvalidError) } @@ -7942,7 +7952,7 @@ type AllocState struct { // they are assigned to is down, their state is migrated to the replacement // allocation. // -// Minimal set of fields from plugins/drivers/task_handle.go:TaskHandle +// Minimal set of fields from plugins/drivers/task_handle.go:TaskHandle type TaskHandle struct { // Version of driver state. Used by the driver to gracefully handle // plugin upgrades. From ae15843d4539f2dd3744220f68af781cd2504131 Mon Sep 17 00:00:00 2001 From: Peter Kazmierczak Date: Tue, 9 Aug 2022 09:59:03 +0200 Subject: [PATCH 14/68] corrections --- api/jobs_test.go | 4 +- api/tasks.go | 4 +- command/agent/job_endpoint.go | 52 ++++++++++++++---------- command/agent/job_endpoint_test.go | 65 ++++++++++++++++-------------- 4 files changed, 70 insertions(+), 55 deletions(-) diff --git a/api/jobs_test.go b/api/jobs_test.go index 78db8f991d14..63bcbbe90a78 100644 --- a/api/jobs_test.go +++ b/api/jobs_test.go @@ -763,7 +763,7 @@ func TestJobs_Canonicalize(t *testing.T) { EmbeddedTmpl: stringToPtr("---"), ChangeMode: stringToPtr("restart"), ChangeSignal: stringToPtr(""), - ChangeScriptConfig: &ChangeScriptConfig{Path: stringToPtr(""), Args: []*string{}, Timeout: timeToPtr(5 * time.Second)}, + ChangeScriptConfig: &ChangeScriptConfig{Path: stringToPtr(""), Args: &[]string{}, Timeout: timeToPtr(5 * time.Second)}, Splay: timeToPtr(5 * time.Second), Perms: stringToPtr("0644"), Uid: intToPtr(0), @@ -779,7 +779,7 @@ func TestJobs_Canonicalize(t *testing.T) { EmbeddedTmpl: stringToPtr("FOO=bar\n"), ChangeMode: stringToPtr("restart"), ChangeSignal: stringToPtr(""), - ChangeScriptConfig: &ChangeScriptConfig{Path: stringToPtr(""), Args: []*string{}, Timeout: timeToPtr(5 * time.Second)}, + ChangeScriptConfig: &ChangeScriptConfig{Path: stringToPtr(""), Args: &[]string{}, Timeout: timeToPtr(5 * time.Second)}, Splay: timeToPtr(5 * time.Second), Perms: stringToPtr("0644"), Uid: intToPtr(0), diff --git a/api/tasks.go b/api/tasks.go index e09bcdf670f0..42388a7a5287 100644 --- a/api/tasks.go +++ b/api/tasks.go @@ -793,7 +793,7 @@ func (wc *WaitConfig) Copy() *WaitConfig { type ChangeScriptConfig struct { Path *string `mapstructure:"path" hcl:"path"` - Args []*string `mapstructure:"args" hcl:"args,optional"` + Args *[]string `mapstructure:"args" hcl:"args,optional"` Timeout *time.Duration `mapstructure:"timeout" hcl:"timeout"` } @@ -841,7 +841,7 @@ func (tmpl *Template) Canonicalize() { if tmpl.ChangeScriptConfig == nil { tmpl.ChangeScriptConfig = &ChangeScriptConfig{ stringToPtr(""), - []*string{}, + &[]string{}, timeToPtr(5 * time.Second), } } diff --git a/command/agent/job_endpoint.go b/command/agent/job_endpoint.go index 99dec5243fea..dc7d0c4cd720 100644 --- a/command/agent/job_endpoint.go +++ b/command/agent/job_endpoint.go @@ -1219,23 +1219,21 @@ func ApiTaskToStructsTask(job *structs.Job, group *structs.TaskGroup, } structsTask.Templates = append(structsTask.Templates, &structs.Template{ - SourcePath: *template.SourcePath, - DestPath: *template.DestPath, - EmbeddedTmpl: *template.EmbeddedTmpl, - ChangeMode: *template.ChangeMode, - ChangeSignal: *template.ChangeSignal, - ChangeScriptPath: *template.ChangeScriptPath, - ChangeScriptArguments: *template.ChangeScriptArguments, - ChangeScriptTimeout: *template.ChangeScriptTimeout, - Splay: *template.Splay, - Perms: *template.Perms, - Uid: uid, - Gid: gid, - LeftDelim: *template.LeftDelim, - RightDelim: *template.RightDelim, - Envvars: *template.Envvars, - VaultGrace: *template.VaultGrace, - Wait: ApiWaitConfigToStructsWaitConfig(template.Wait), + SourcePath: *template.SourcePath, + DestPath: *template.DestPath, + EmbeddedTmpl: *template.EmbeddedTmpl, + ChangeMode: *template.ChangeMode, + ChangeSignal: *template.ChangeSignal, + ChangeScriptConfig: apiChangeScriptConfigToStructsChangeScriptConfig(template.ChangeScriptConfig), + Splay: *template.Splay, + Perms: *template.Perms, + Uid: uid, + Gid: gid, + LeftDelim: *template.LeftDelim, + RightDelim: *template.RightDelim, + Envvars: *template.Envvars, + VaultGrace: *template.VaultGrace, + Wait: apiWaitConfigToStructsWaitConfig(template.Wait), }) } } @@ -1254,16 +1252,28 @@ func ApiTaskToStructsTask(job *structs.Job, group *structs.TaskGroup, } } -// ApiWaitConfigToStructsWaitConfig is a copy and type conversion between the API +// apiWaitConfigToStructsWaitConfig is a copy and type conversion between the API // representation of a WaitConfig from a struct representation of a WaitConfig. -func ApiWaitConfigToStructsWaitConfig(waitConfig *api.WaitConfig) *structs.WaitConfig { +func apiWaitConfigToStructsWaitConfig(waitConfig *api.WaitConfig) *structs.WaitConfig { if waitConfig == nil { return nil } return &structs.WaitConfig{ - Min: &*waitConfig.Min, - Max: &*waitConfig.Max, + Min: waitConfig.Min, + Max: waitConfig.Max, + } +} + +func apiChangeScriptConfigToStructsChangeScriptConfig(changeScriptConfig *api.ChangeScriptConfig) *structs.ChangeScriptConfig { + if changeScriptConfig == nil { + return nil + } + + return &structs.ChangeScriptConfig{ + Path: *changeScriptConfig.Path, + Args: *changeScriptConfig.Args, + Timeout: *changeScriptConfig.Timeout, } } diff --git a/command/agent/job_endpoint_test.go b/command/agent/job_endpoint_test.go index 95bdc5bb6ad6..21040fee1bec 100644 --- a/command/agent/job_endpoint_test.go +++ b/command/agent/job_endpoint_test.go @@ -2726,21 +2726,23 @@ func TestJobs_ApiJobToStructsJob(t *testing.T) { }, Templates: []*api.Template{ { - SourcePath: helper.StringToPtr("source"), - DestPath: helper.StringToPtr("dest"), - EmbeddedTmpl: helper.StringToPtr("embedded"), - ChangeMode: helper.StringToPtr("change"), - ChangeSignal: helper.StringToPtr("signal"), - ChangeScriptPath: helper.StringToPtr("/bin/foo"), - ChangeScriptArguments: helper.StringToPtr("-h"), - ChangeScriptTimeout: helper.TimeToPtr(5 * time.Second), - Splay: helper.TimeToPtr(1 * time.Minute), - Perms: helper.StringToPtr("666"), - Uid: helper.IntToPtr(1000), - Gid: helper.IntToPtr(1000), - LeftDelim: helper.StringToPtr("abc"), - RightDelim: helper.StringToPtr("def"), - Envvars: helper.BoolToPtr(true), + SourcePath: helper.StringToPtr("source"), + DestPath: helper.StringToPtr("dest"), + EmbeddedTmpl: helper.StringToPtr("embedded"), + ChangeMode: helper.StringToPtr("change"), + ChangeSignal: helper.StringToPtr("signal"), + ChangeScriptConfig: &api.ChangeScriptConfig{ + Path: helper.StringToPtr("/bin/foo"), + Args: &[]string{"-h"}, + Timeout: helper.TimeToPtr(5 * time.Second), + }, + Splay: helper.TimeToPtr(1 * time.Minute), + Perms: helper.StringToPtr("666"), + Uid: helper.IntToPtr(1000), + Gid: helper.IntToPtr(1000), + LeftDelim: helper.StringToPtr("abc"), + RightDelim: helper.StringToPtr("def"), + Envvars: helper.BoolToPtr(true), Wait: &api.WaitConfig{ Min: helper.TimeToPtr(5 * time.Second), Max: helper.TimeToPtr(10 * time.Second), @@ -3136,21 +3138,23 @@ func TestJobs_ApiJobToStructsJob(t *testing.T) { }, Templates: []*structs.Template{ { - SourcePath: "source", - DestPath: "dest", - EmbeddedTmpl: "embedded", - ChangeMode: "change", - ChangeSignal: "SIGNAL", - ChangeScriptPath: "/bin/foo", - ChangeScriptArguments: "-h", - ChangeScriptTimeout: 5 * time.Second, - Splay: 1 * time.Minute, - Perms: "666", - Uid: 1000, - Gid: 1000, - LeftDelim: "abc", - RightDelim: "def", - Envvars: true, + SourcePath: "source", + DestPath: "dest", + EmbeddedTmpl: "embedded", + ChangeMode: "change", + ChangeSignal: "SIGNAL", + ChangeScriptConfig: &structs.ChangeScriptConfig{ + Path: "/bin/foo", + Args: []string{"-h"}, + Timeout: 5 * time.Second, + }, + Splay: 1 * time.Minute, + Perms: "666", + Uid: 1000, + Gid: 1000, + LeftDelim: "abc", + RightDelim: "def", + Envvars: true, Wait: &structs.WaitConfig{ Min: helper.TimeToPtr(5 * time.Second), Max: helper.TimeToPtr(10 * time.Second), @@ -3492,6 +3496,7 @@ func TestJobs_ApiJobToStructsJobUpdate(t *testing.T) { } // TestJobs_Matching_Resources asserts: +// // api.{Default,Min}Resources == structs.{Default,Min}Resources // // While this is an odd place to test that, this is where both are imported, From f9b12c6ff27c3fd19e7f20a6d2b7cbd7fc4b74fb Mon Sep 17 00:00:00 2001 From: Peter Kazmierczak Date: Tue, 9 Aug 2022 17:39:49 +0200 Subject: [PATCH 15/68] revamped tests Co-authored-by: Derek Strickland --- .../taskrunner/template/template.go | 23 ++++++--- .../taskrunner/template/template_test.go | 47 ++++++++++--------- nomad/structs/structs.go | 4 ++ 3 files changed, 45 insertions(+), 29 deletions(-) diff --git a/client/allocrunner/taskrunner/template/template.go b/client/allocrunner/taskrunner/template/template.go index cf18fc9c904e..4149ffcface6 100644 --- a/client/allocrunner/taskrunner/template/template.go +++ b/client/allocrunner/taskrunner/template/template.go @@ -498,13 +498,22 @@ func (tm *TaskTemplateManager) onTemplateRendered(handledRenders map[string]time SetDisplayMessage(fmt.Sprintf("Template failed to send signals %v: %v", flat, err))) } } - if len(scripts) != 0 { - for _, script := range scripts { - _, _, err := tm.config.Handle.Exec(script.Timeout, script.Path, script.Args) - if err != nil { - structs.NewTaskEvent(structs.TaskDriverMessage). - SetDisplayMessage(fmt.Sprintf("Template failed to run script on change: %v", err)) - } + } + if len(scripts) != 0 { + for _, script := range scripts { + _, exitCode, err := tm.config.Handle.Exec(script.Timeout, script.Path, script.Args) + if err != nil { + structs.NewTaskEvent(structs.TaskHookFailed). + SetDisplayMessage( + fmt.Sprintf( + "Template failed to run script %v on change: %v Exit code: %v", script.Path, err, exitCode, + )) + } else { + tm.config.Events.EmitEvent(structs.NewTaskEvent(structs.TaskHookMessage). + SetDisplayMessage( + fmt.Sprintf( + "Template ran a script from %v with exit code: %v", script.Path, exitCode, + ))) } } } diff --git a/client/allocrunner/taskrunner/template/template_test.go b/client/allocrunner/taskrunner/template/template_test.go index 248262258885..ee09788a4216 100644 --- a/client/allocrunner/taskrunner/template/template_test.go +++ b/client/allocrunner/taskrunner/template/template_test.go @@ -24,6 +24,7 @@ import ( ctestutil "github.com/hashicorp/consul/sdk/testutil" "github.com/hashicorp/nomad/ci" "github.com/hashicorp/nomad/client/allocdir" + "github.com/hashicorp/nomad/client/allocrunner/taskrunner/interfaces" "github.com/hashicorp/nomad/client/config" "github.com/hashicorp/nomad/client/taskenv" "github.com/hashicorp/nomad/helper" @@ -123,6 +124,14 @@ func (m *MockTaskHooks) EmitEvent(event *structs.TaskEvent) { func (m *MockTaskHooks) SetState(state string, event *structs.TaskEvent) {} +// MockExecutor is implementing script executor interface +type MockExecutor struct { +} + +func (m *MockExecutor) Exec(timeout time.Duration, cmd string, args []string) ([]byte, int, error) { + return []byte{}, 0, nil +} + // testHarness is used to test the TaskTemplateManager by spinning up // Consul/Vault as needed type testHarness struct { @@ -138,6 +147,7 @@ type testHarness struct { consul *ctestutil.TestServer emitRate time.Duration nomadNamespace string + driver interfaces.ScriptExecutor } // newTestHarness returns a harness starting a dev consul and vault server, @@ -211,6 +221,7 @@ func (h *testHarness) startWithErr() error { VaultToken: h.vaultToken, TaskDir: h.taskDir, EnvBuilder: h.envBuilder, + Handle: h.driver, MaxTemplateEventRate: h.emitRate, }) @@ -1182,7 +1193,7 @@ func TestTaskTemplateManager_Signal_Error(t *testing.T) { // Wait a little select { case <-harness.mockHooks.UnblockCh: - case <-time.After(time.Duration(30 * time.Second)): + case <-time.After(time.Duration(2*testutil.TestMultiplier()) * time.Second): t.Fatalf("Should have received unblock: %+v", harness.mockHooks) } @@ -1205,25 +1216,29 @@ func TestTaskTemplateManager_Script(t *testing.T) { ci.Parallel(t) require := require.New(t) - // Make a template that renders based on a key in Consul and triggers a script + m := MockExecutor{} + key1 := "foo" content1 := "bar" - content2 := "baz" embedded1 := fmt.Sprintf(`{{key "%s"}}`, key1) file1 := "my.tmpl" template := &structs.Template{ - EmbeddedTmpl: embedded1, - DestPath: file1, - ChangeMode: structs.TemplateChangeModeScript, - ChangeScriptConfig: &structs.ChangeScriptConfig{}, + EmbeddedTmpl: embedded1, + DestPath: file1, + ChangeMode: structs.TemplateChangeModeScript, + ChangeScriptConfig: &structs.ChangeScriptConfig{ + Path: "/bin/foo", + Args: []string{}, + Timeout: 5 * time.Second, + }, } harness := newTestHarness(t, []*structs.Template{template}, true, false) + // inject a mock driver into the test harness + harness.driver = &m harness.start(t) defer harness.stop() - // harness.mockHooks.SignalError = fmt.Errorf("test error") - // Write the key to Consul harness.consul.SetKV(t, key1, []byte(content1)) @@ -1234,19 +1249,7 @@ func TestTaskTemplateManager_Script(t *testing.T) { t.Fatalf("Should have received unblock: %+v", harness.mockHooks) } - // Write the key to Consul - harness.consul.SetKV(t, key1, []byte(content2)) - - // Wait for kill channel - select { - case <-harness.mockHooks.KillCh: - break - case <-time.After(time.Duration(1*testutil.TestMultiplier()) * time.Second): - t.Fatalf("Should have received a signals: %+v", harness.mockHooks) - } - - require.NotNil(harness.mockHooks.KillEvent) - require.Contains(harness.mockHooks.KillEvent.DisplayMessage, "failed to send signals") + require.Contains(harness.mockHooks.Events, "Template ran a script") } // TestTaskTemplateManager_FiltersProcessEnvVars asserts that we only render diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index f2620cce15a4..fd5842fa9247 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -8135,6 +8135,10 @@ const ( // TaskHookFailed indicates that one of the hooks for a task failed. TaskHookFailed = "Task hook failed" + // TaskHookMessage indicates that one of the hooks for a task emitted a + // message. + TaskHookMessage = "Task hook message" + // TaskRestoreFailed indicates Nomad was unable to reattach to a // restored task. TaskRestoreFailed = "Failed Restoring Task" From 55d33da6280488a85c78b4d15b56bfa73f606619 Mon Sep 17 00:00:00 2001 From: Peter Kazmierczak Date: Tue, 9 Aug 2022 20:19:26 +0200 Subject: [PATCH 16/68] revamped test --- .../taskrunner/template/template_test.go | 73 ++++++++++++++----- 1 file changed, 56 insertions(+), 17 deletions(-) diff --git a/client/allocrunner/taskrunner/template/template_test.go b/client/allocrunner/taskrunner/template/template_test.go index ee09788a4216..c56e5a7502bc 100644 --- a/client/allocrunner/taskrunner/template/template_test.go +++ b/client/allocrunner/taskrunner/template/template_test.go @@ -1212,43 +1212,82 @@ func TestTaskTemplateManager_Signal_Error(t *testing.T) { require.Contains(harness.mockHooks.KillEvent.DisplayMessage, "failed to send signals") } -func TestTaskTemplateManager_Script(t *testing.T) { +func TestTaskTemplateManager_ScriptExecution(t *testing.T) { ci.Parallel(t) require := require.New(t) - m := MockExecutor{} - - key1 := "foo" - content1 := "bar" - embedded1 := fmt.Sprintf(`{{key "%s"}}`, key1) - file1 := "my.tmpl" - template := &structs.Template{ - EmbeddedTmpl: embedded1, - DestPath: file1, - ChangeMode: structs.TemplateChangeModeScript, + // Make a template that renders based on a key in Consul and triggers script + key1 := "bam" + key2 := "bar" + content1_1 := "cat" + content1_2 := "dog" + t1 := &structs.Template{ + EmbeddedTmpl: ` +FOO={{key "bam"}} +`, + DestPath: "test.env", + ChangeMode: structs.TemplateChangeModeScript, + ChangeScriptConfig: &structs.ChangeScriptConfig{ + Path: "/bin/foo", + Args: []string{}, + Timeout: 5 * time.Second, + }, + Envvars: true, + } + t2 := &structs.Template{ + EmbeddedTmpl: ` +BAR={{key "bar"}} +`, + DestPath: "test2.env", + ChangeMode: structs.TemplateChangeModeScript, ChangeScriptConfig: &structs.ChangeScriptConfig{ Path: "/bin/foo", Args: []string{}, Timeout: 5 * time.Second, }, + Envvars: true, } - harness := newTestHarness(t, []*structs.Template{template}, true, false) - // inject a mock driver into the test harness + m := MockExecutor{} + harness := newTestHarness(t, []*structs.Template{t1, t2}, true, false) harness.driver = &m harness.start(t) defer harness.stop() + // Ensure no unblock + select { + case <-harness.mockHooks.UnblockCh: + t.Fatalf("Task unblock should not have been called") + case <-time.After(time.Duration(1*testutil.TestMultiplier()) * time.Second): + } + // Write the key to Consul - harness.consul.SetKV(t, key1, []byte(content1)) + harness.consul.SetKV(t, key1, []byte(content1_1)) + harness.consul.SetKV(t, key2, []byte(content1_1)) - // Wait a little + // Wait for the unblock select { case <-harness.mockHooks.UnblockCh: - case <-time.After(time.Duration(2*testutil.TestMultiplier()) * time.Second): - t.Fatalf("Should have received unblock: %+v", harness.mockHooks) + case <-time.After(time.Duration(5*testutil.TestMultiplier()) * time.Second): + t.Fatalf("Task unblock should have been called") } + // Update the keys in Consul + harness.consul.SetKV(t, key1, []byte(content1_2)) + + // Wait for restart + timeout := time.After(time.Duration(1*testutil.TestMultiplier()) * time.Second) +OUTER: + for { + select { + case <-harness.mockHooks.RestartCh: + break OUTER + case <-harness.mockHooks.SignalCh: + t.Fatalf("Signal with restart policy: %+v", harness.mockHooks) + case <-timeout: + t.Fatalf("Should have received a restart: %+v", harness.mockHooks) + } + } require.Contains(harness.mockHooks.Events, "Template ran a script") } From 611738bf7d46e3ce6489dacb5f266c5a87a50d73 Mon Sep 17 00:00:00 2001 From: Derek Strickland Date: Tue, 9 Aug 2022 14:45:33 -0400 Subject: [PATCH 17/68] Refactor test to listen for event channel --- .../taskrunner/template/template_test.go | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/client/allocrunner/taskrunner/template/template_test.go b/client/allocrunner/taskrunner/template/template_test.go index c56e5a7502bc..b30253c21dce 100644 --- a/client/allocrunner/taskrunner/template/template_test.go +++ b/client/allocrunner/taskrunner/template/template_test.go @@ -1214,7 +1214,6 @@ func TestTaskTemplateManager_Signal_Error(t *testing.T) { func TestTaskTemplateManager_ScriptExecution(t *testing.T) { ci.Parallel(t) - require := require.New(t) // Make a template that renders based on a key in Consul and triggers script key1 := "bam" @@ -1276,19 +1275,29 @@ BAR={{key "bar"}} harness.consul.SetKV(t, key1, []byte(content1_2)) // Wait for restart - timeout := time.After(time.Duration(1*testutil.TestMultiplier()) * time.Second) + timeout := time.After(time.Duration(5*testutil.TestMultiplier()) * time.Second) OUTER: for { select { case <-harness.mockHooks.RestartCh: + require.Fail(t, "restart not expected") + case <-harness.mockHooks.EmitEventCh: break OUTER case <-harness.mockHooks.SignalCh: - t.Fatalf("Signal with restart policy: %+v", harness.mockHooks) + require.Fail(t, "signal not expected") case <-timeout: - t.Fatalf("Should have received a restart: %+v", harness.mockHooks) + require.Fail(t, "should have received an event") + } + } + + eventPublished := false + for _, ev := range harness.mockHooks.Events { + if strings.Contains(ev.DisplayMessage, "Template ran a script") { + eventPublished = true + break } } - require.Contains(harness.mockHooks.Events, "Template ran a script") + require.True(t, eventPublished) } // TestTaskTemplateManager_FiltersProcessEnvVars asserts that we only render From 1277dd6be09cb486b362bad2ebfda321cad7e844 Mon Sep 17 00:00:00 2001 From: Peter Kazmierczak Date: Wed, 10 Aug 2022 10:43:23 +0200 Subject: [PATCH 18/68] diffs corrected --- nomad/structs/diff.go | 41 ++++++++++++++++++++++++++++++++++++++ nomad/structs/diff_test.go | 40 ++++++++++++++++++++++++++----------- 2 files changed, 69 insertions(+), 12 deletions(-) diff --git a/nomad/structs/diff.go b/nomad/structs/diff.go index 8c16123e1c82..11ae5ca855ab 100644 --- a/nomad/structs/diff.go +++ b/nomad/structs/diff.go @@ -1649,6 +1649,40 @@ func waitConfigDiff(old, new *WaitConfig, contextual bool) *ObjectDiff { return diff } +// changeScriptConfigDiff returns the diff of two ChangeScriptConfig objects. If +// contextual diff is enabled, all fields will be returned, even if no diff +// occurred. +func changeScriptConfigDiff(old, new *ChangeScriptConfig, contextual bool) *ObjectDiff { + diff := &ObjectDiff{Type: DiffTypeNone, Name: "ChangeScriptConfig"} + var oldPrimitiveFlat, newPrimitiveFlat map[string]string + + if reflect.DeepEqual(old, new) { + return nil + } else if old == nil { + old = &ChangeScriptConfig{} + diff.Type = DiffTypeAdded + newPrimitiveFlat = flatmap.Flatten(new, nil, true) + } else if new == nil { + new = &ChangeScriptConfig{} + diff.Type = DiffTypeDeleted + oldPrimitiveFlat = flatmap.Flatten(old, nil, true) + } else { + diff.Type = DiffTypeEdited + oldPrimitiveFlat = flatmap.Flatten(old, nil, true) + newPrimitiveFlat = flatmap.Flatten(new, nil, true) + } + + // Diff the primitive fields. + diff.Fields = fieldDiffs(oldPrimitiveFlat, newPrimitiveFlat, contextual) + + // Args diffs + if setDiff := stringSetDiff(old.Args, new.Args, "Args", contextual); setDiff != nil { + diff.Objects = append(diff.Objects, setDiff) + } + + return diff +} + // templateDiff returns the diff of two Consul Template objects. If contextual diff is // enabled, all fields will be returned, even if no diff occurred. func templateDiff(old, new *Template, contextual bool) *ObjectDiff { @@ -1679,6 +1713,13 @@ func templateDiff(old, new *Template, contextual bool) *ObjectDiff { diff.Objects = append(diff.Objects, waitDiffs) } + // ChangeScriptConfig diffs + if changeScriptDiffs := changeScriptConfigDiff( + old.ChangeScriptConfig, new.ChangeScriptConfig, contextual, + ); changeScriptDiffs != nil { + diff.Objects = append(diff.Objects, changeScriptDiffs) + } + return diff } diff --git a/nomad/structs/diff_test.go b/nomad/structs/diff_test.go index bc05a474ddbb..8562fc88c2ef 100644 --- a/nomad/structs/diff_test.go +++ b/nomad/structs/diff_test.go @@ -7242,12 +7242,6 @@ func TestTaskDiff(t *testing.T) { Type: DiffTypeAdded, Name: "ChangeScriptConfig", Fields: []*FieldDiff{ - { - Type: DiffTypeAdded, - Name: "Args", - Old: "", - New: "-debugss", - }, { Type: DiffTypeAdded, Name: "Path", @@ -7261,6 +7255,20 @@ func TestTaskDiff(t *testing.T) { New: "7", }, }, + Objects: []*ObjectDiff{ + { + Type: DiffTypeAdded, + Name: "Args", + Fields: []*FieldDiff{ + { + Type: DiffTypeAdded, + Name: "Args", + Old: "", + New: "-debugss", + }, + }, + }, + }, }, }, }, @@ -7340,12 +7348,6 @@ func TestTaskDiff(t *testing.T) { Type: DiffTypeDeleted, Name: "ChangeScriptConfig", Fields: []*FieldDiff{ - { - Type: DiffTypeDeleted, - Name: "Args", - Old: "-debugs", - New: "", - }, { Type: DiffTypeDeleted, Name: "Path", @@ -7359,6 +7361,20 @@ func TestTaskDiff(t *testing.T) { New: "", }, }, + Objects: []*ObjectDiff{ + { + Type: DiffTypeDeleted, + Name: "Args", + Fields: []*FieldDiff{ + { + Type: DiffTypeDeleted, + Name: "Args", + Old: "-debugs", + New: "", + }, + }, + }, + }, }, }, }, From ac8d910755f86f2ee41b3b59fcb5503bc8d9f5ab Mon Sep 17 00:00:00 2001 From: Peter Kazmierczak Date: Wed, 10 Aug 2022 16:39:39 +0200 Subject: [PATCH 19/68] CI-driven debugging :/ --- client/allocrunner/taskrunner/template/template_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/client/allocrunner/taskrunner/template/template_test.go b/client/allocrunner/taskrunner/template/template_test.go index b30253c21dce..4224143ce9dc 100644 --- a/client/allocrunner/taskrunner/template/template_test.go +++ b/client/allocrunner/taskrunner/template/template_test.go @@ -1268,7 +1268,7 @@ BAR={{key "bar"}} select { case <-harness.mockHooks.UnblockCh: case <-time.After(time.Duration(5*testutil.TestMultiplier()) * time.Second): - t.Fatalf("Task unblock should have been called") + // t.Fatalf("Task unblock should have been called") } // Update the keys in Consul @@ -1291,8 +1291,10 @@ OUTER: } eventPublished := false + fmt.Printf("[WTF] len of mockHooks events: %v\n", len(harness.mockHooks.Events)) for _, ev := range harness.mockHooks.Events { - if strings.Contains(ev.DisplayMessage, "Template ran a script") { + fmt.Printf("[WTF] ev display message: %v\n", ev.DisplayMessage) + if strings.Contains(ev.DisplayMessage, t1.ChangeScriptConfig.Path) { eventPublished = true break } From 2270765d5df3235da54dd39a1c9db6d632b69aed Mon Sep 17 00:00:00 2001 From: Peter Kazmierczak Date: Wed, 10 Aug 2022 17:41:20 +0200 Subject: [PATCH 20/68] fixed test --- .../taskrunner/template/template_test.go | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/client/allocrunner/taskrunner/template/template_test.go b/client/allocrunner/taskrunner/template/template_test.go index 4224143ce9dc..417e297fab83 100644 --- a/client/allocrunner/taskrunner/template/template_test.go +++ b/client/allocrunner/taskrunner/template/template_test.go @@ -1281,25 +1281,16 @@ OUTER: select { case <-harness.mockHooks.RestartCh: require.Fail(t, "restart not expected") - case <-harness.mockHooks.EmitEventCh: - break OUTER + case ev := <-harness.mockHooks.EmitEventCh: + if strings.Contains(ev.DisplayMessage, t1.ChangeScriptConfig.Path) { + break OUTER + } case <-harness.mockHooks.SignalCh: require.Fail(t, "signal not expected") case <-timeout: require.Fail(t, "should have received an event") } } - - eventPublished := false - fmt.Printf("[WTF] len of mockHooks events: %v\n", len(harness.mockHooks.Events)) - for _, ev := range harness.mockHooks.Events { - fmt.Printf("[WTF] ev display message: %v\n", ev.DisplayMessage) - if strings.Contains(ev.DisplayMessage, t1.ChangeScriptConfig.Path) { - eventPublished = true - break - } - } - require.True(t, eventPublished) } // TestTaskTemplateManager_FiltersProcessEnvVars asserts that we only render From 8d61a2505c5a69a384094296828a361d3da14a3f Mon Sep 17 00:00:00 2001 From: pkazmierczak Date: Wed, 10 Aug 2022 21:29:36 +0200 Subject: [PATCH 21/68] FailTask --- api/jobs_test.go | 4 +- api/tasks.go | 8 ++-- .../taskrunner/template/template.go | 8 ++++ .../taskrunner/template/template_test.go | 14 ++++--- command/agent/job_endpoint.go | 7 ++-- command/agent/job_endpoint_test.go | 14 ++++--- jobspec/parse_task.go | 4 +- nomad/structs/diff_test.go | 40 +++++++++++++------ nomad/structs/structs.go | 5 ++- 9 files changed, 67 insertions(+), 37 deletions(-) diff --git a/api/jobs_test.go b/api/jobs_test.go index 63bcbbe90a78..48df91ef7755 100644 --- a/api/jobs_test.go +++ b/api/jobs_test.go @@ -763,7 +763,7 @@ func TestJobs_Canonicalize(t *testing.T) { EmbeddedTmpl: stringToPtr("---"), ChangeMode: stringToPtr("restart"), ChangeSignal: stringToPtr(""), - ChangeScriptConfig: &ChangeScriptConfig{Path: stringToPtr(""), Args: &[]string{}, Timeout: timeToPtr(5 * time.Second)}, + ChangeScriptConfig: &ChangeScriptConfig{Path: stringToPtr(""), Args: &[]string{}, Timeout: timeToPtr(5 * time.Second), false}, Splay: timeToPtr(5 * time.Second), Perms: stringToPtr("0644"), Uid: intToPtr(0), @@ -779,7 +779,7 @@ func TestJobs_Canonicalize(t *testing.T) { EmbeddedTmpl: stringToPtr("FOO=bar\n"), ChangeMode: stringToPtr("restart"), ChangeSignal: stringToPtr(""), - ChangeScriptConfig: &ChangeScriptConfig{Path: stringToPtr(""), Args: &[]string{}, Timeout: timeToPtr(5 * time.Second)}, + ChangeScriptConfig: &ChangeScriptConfig{Path: stringToPtr(""), Args: &[]string{}, Timeout: timeToPtr(5 * time.Second), false}, Splay: timeToPtr(5 * time.Second), Perms: stringToPtr("0644"), Uid: intToPtr(0), diff --git a/api/tasks.go b/api/tasks.go index 42388a7a5287..e16e5d4047e9 100644 --- a/api/tasks.go +++ b/api/tasks.go @@ -792,9 +792,10 @@ func (wc *WaitConfig) Copy() *WaitConfig { } type ChangeScriptConfig struct { - Path *string `mapstructure:"path" hcl:"path"` - Args *[]string `mapstructure:"args" hcl:"args,optional"` - Timeout *time.Duration `mapstructure:"timeout" hcl:"timeout"` + Path *string `mapstructure:"path" hcl:"path"` + Args *[]string `mapstructure:"args" hcl:"args,optional"` + Timeout *time.Duration `mapstructure:"timeout" hcl:"timeout"` + FailTask *bool `mapstructure:"fail_task" hcl:"fail_task"` } type Template struct { @@ -843,6 +844,7 @@ func (tmpl *Template) Canonicalize() { stringToPtr(""), &[]string{}, timeToPtr(5 * time.Second), + boolToPtr(false), } } if tmpl.Splay == nil { diff --git a/client/allocrunner/taskrunner/template/template.go b/client/allocrunner/taskrunner/template/template.go index 4149ffcface6..7f777ed65494 100644 --- a/client/allocrunner/taskrunner/template/template.go +++ b/client/allocrunner/taskrunner/template/template.go @@ -508,6 +508,14 @@ func (tm *TaskTemplateManager) onTemplateRendered(handledRenders map[string]time fmt.Sprintf( "Template failed to run script %v on change: %v Exit code: %v", script.Path, err, exitCode, )) + if script.FailTask { + tm.config.Lifecycle.Kill(context.Background(), + structs.NewTaskEvent(structs.TaskKilling). + SetFailsTask(). + SetDisplayMessage( + fmt.Sprintf("Template failed to run script %v and the task is being killed", script.Path), + )) + } } else { tm.config.Events.EmitEvent(structs.NewTaskEvent(structs.TaskHookMessage). SetDisplayMessage( diff --git a/client/allocrunner/taskrunner/template/template_test.go b/client/allocrunner/taskrunner/template/template_test.go index 417e297fab83..5b1c254f9a81 100644 --- a/client/allocrunner/taskrunner/template/template_test.go +++ b/client/allocrunner/taskrunner/template/template_test.go @@ -1227,9 +1227,10 @@ FOO={{key "bam"}} DestPath: "test.env", ChangeMode: structs.TemplateChangeModeScript, ChangeScriptConfig: &structs.ChangeScriptConfig{ - Path: "/bin/foo", - Args: []string{}, - Timeout: 5 * time.Second, + Path: "/bin/foo", + Args: []string{}, + Timeout: 5 * time.Second, + FailTask: false, }, Envvars: true, } @@ -1240,9 +1241,10 @@ BAR={{key "bar"}} DestPath: "test2.env", ChangeMode: structs.TemplateChangeModeScript, ChangeScriptConfig: &structs.ChangeScriptConfig{ - Path: "/bin/foo", - Args: []string{}, - Timeout: 5 * time.Second, + Path: "/bin/foo", + Args: []string{}, + Timeout: 5 * time.Second, + FailTask: false, }, Envvars: true, } diff --git a/command/agent/job_endpoint.go b/command/agent/job_endpoint.go index dc7d0c4cd720..afc9b1ad4eba 100644 --- a/command/agent/job_endpoint.go +++ b/command/agent/job_endpoint.go @@ -1271,9 +1271,10 @@ func apiChangeScriptConfigToStructsChangeScriptConfig(changeScriptConfig *api.Ch } return &structs.ChangeScriptConfig{ - Path: *changeScriptConfig.Path, - Args: *changeScriptConfig.Args, - Timeout: *changeScriptConfig.Timeout, + Path: *changeScriptConfig.Path, + Args: *changeScriptConfig.Args, + Timeout: *changeScriptConfig.Timeout, + FailTask: *changeScriptConfig.FailTask, } } diff --git a/command/agent/job_endpoint_test.go b/command/agent/job_endpoint_test.go index 21040fee1bec..5533585b497a 100644 --- a/command/agent/job_endpoint_test.go +++ b/command/agent/job_endpoint_test.go @@ -2732,9 +2732,10 @@ func TestJobs_ApiJobToStructsJob(t *testing.T) { ChangeMode: helper.StringToPtr("change"), ChangeSignal: helper.StringToPtr("signal"), ChangeScriptConfig: &api.ChangeScriptConfig{ - Path: helper.StringToPtr("/bin/foo"), - Args: &[]string{"-h"}, - Timeout: helper.TimeToPtr(5 * time.Second), + Path: helper.StringToPtr("/bin/foo"), + Args: &[]string{"-h"}, + Timeout: helper.TimeToPtr(5 * time.Second), + FailTask: helper.BoolToPtr(false), }, Splay: helper.TimeToPtr(1 * time.Minute), Perms: helper.StringToPtr("666"), @@ -3144,9 +3145,10 @@ func TestJobs_ApiJobToStructsJob(t *testing.T) { ChangeMode: "change", ChangeSignal: "SIGNAL", ChangeScriptConfig: &structs.ChangeScriptConfig{ - Path: "/bin/foo", - Args: []string{"-h"}, - Timeout: 5 * time.Second, + Path: "/bin/foo", + Args: []string{"-h"}, + Timeout: 5 * time.Second, + FailTask: false, }, Splay: 1 * time.Minute, Perms: "666", diff --git a/jobspec/parse_task.go b/jobspec/parse_task.go index 165710ee86ea..4dd211f0150f 100644 --- a/jobspec/parse_task.go +++ b/jobspec/parse_task.go @@ -437,9 +437,7 @@ func parseTemplates(result *[]*api.Template, list *ast.ObjectList) error { valid := []string{ "change_mode", "change_signal", - "change_script_path", - "change_script_arguments", - "change_script_timeout", + "change_script_config", "data", "destination", "left_delimiter", diff --git a/nomad/structs/diff_test.go b/nomad/structs/diff_test.go index 8562fc88c2ef..ee65c0bdbf09 100644 --- a/nomad/structs/diff_test.go +++ b/nomad/structs/diff_test.go @@ -7043,9 +7043,10 @@ func TestTaskDiff(t *testing.T) { ChangeMode: "bam", ChangeSignal: "SIGHUP", ChangeScriptConfig: &ChangeScriptConfig{ - Path: "/bin/foo", - Args: []string{"-debug"}, - Timeout: 5, + Path: "/bin/foo", + Args: []string{"-debug"}, + Timeout: 5, + FailTask: false, }, Splay: 1, Perms: "0644", @@ -7063,9 +7064,10 @@ func TestTaskDiff(t *testing.T) { ChangeMode: "bam2", ChangeSignal: "SIGHUP2", ChangeScriptConfig: &ChangeScriptConfig{ - Path: "/bin/foo2", - Args: []string{"-debugs"}, - Timeout: 6, + Path: "/bin/foo2", + Args: []string{"-debugs"}, + Timeout: 6, + FailTask: false, }, Splay: 2, Perms: "0666", @@ -7084,9 +7086,10 @@ func TestTaskDiff(t *testing.T) { ChangeMode: "bam", ChangeSignal: "SIGHUP", ChangeScriptConfig: &ChangeScriptConfig{ - Path: "/bin/foo", - Args: []string{"-debug"}, - Timeout: 5, + Path: "/bin/foo", + Args: []string{"-debug"}, + Timeout: 5, + FailTask: false, }, Splay: 1, Perms: "0644", @@ -7104,9 +7107,10 @@ func TestTaskDiff(t *testing.T) { ChangeMode: "bam3", ChangeSignal: "SIGHUP3", ChangeScriptConfig: &ChangeScriptConfig{ - Path: "/bin/foo3", - Args: []string{"-debugss"}, - Timeout: 7, + Path: "/bin/foo3", + Args: []string{"-debugss"}, + Timeout: 7, + FailTask: false, }, Splay: 3, Perms: "0776", @@ -7242,6 +7246,12 @@ func TestTaskDiff(t *testing.T) { Type: DiffTypeAdded, Name: "ChangeScriptConfig", Fields: []*FieldDiff{ + { + Type: DiffTypeAdded, + Name: "FailTask", + Old: "", + New: "false", + }, { Type: DiffTypeAdded, Name: "Path", @@ -7348,6 +7358,12 @@ func TestTaskDiff(t *testing.T) { Type: DiffTypeDeleted, Name: "ChangeScriptConfig", Fields: []*FieldDiff{ + { + Type: DiffTypeDeleted, + Name: "FailTask", + Old: "false", + New: "", + }, { Type: DiffTypeDeleted, Name: "Path", diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index fd5842fa9247..944b310ddae5 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -7698,8 +7698,9 @@ type ChangeScriptConfig struct { Args []string // Timeout is the amount of seconds we wait for the script to finish Timeout time.Duration - // TODO: add a field that users could use to specify whether a task should - // fail if the script fails + // FailTask indicates whether a task should fail in case script execution + // fails or log script failure and don't interrupt the task + FailTask bool } // Template represents a template configuration to be rendered for a given task From 4839c672a9735755bbb3723ae5b88776ebf1724c Mon Sep 17 00:00:00 2001 From: pkazmierczak Date: Wed, 10 Aug 2022 21:35:31 +0200 Subject: [PATCH 22/68] s/suck/such; --- client/allocrunner/taskrunner/template/template.go | 2 +- website/content/api-docs/json-jobs.mdx | 4 ++-- website/content/docs/job-specification/template.mdx | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/client/allocrunner/taskrunner/template/template.go b/client/allocrunner/taskrunner/template/template.go index 7f777ed65494..1dd3f51eb707 100644 --- a/client/allocrunner/taskrunner/template/template.go +++ b/client/allocrunner/taskrunner/template/template.go @@ -520,7 +520,7 @@ func (tm *TaskTemplateManager) onTemplateRendered(handledRenders map[string]time tm.config.Events.EmitEvent(structs.NewTaskEvent(structs.TaskHookMessage). SetDisplayMessage( fmt.Sprintf( - "Template ran a script from %v with exit code: %v", script.Path, exitCode, + "Template successfully ran a script from %v with exit code: %v", script.Path, exitCode, ))) } } diff --git a/website/content/api-docs/json-jobs.mdx b/website/content/api-docs/json-jobs.mdx index 1216101aa4f5..721c2f3eb746 100644 --- a/website/content/api-docs/json-jobs.mdx +++ b/website/content/api-docs/json-jobs.mdx @@ -1092,14 +1092,14 @@ README][ct]. - `Uid` - Specifies the rendered template owner's user ID. ~> **Caveat:** Works only on Unix-based systems. Be careful when using - containerized drivers, suck as `docker` or `podman`, as groups and users + containerized drivers, such as `docker` or `podman`, as groups and users inside the container may have different IDs than on the host system. This feature will also **not** work with Docker Desktop. - `Gid` - Specifies the rendered template owner's group ID. ~> **Caveat:** Works only on Unix-based systems. Be careful when using - containerized drivers, suck as `docker` or `podman`, as groups and users + containerized drivers, such as `docker` or `podman`, as groups and users inside the container may have different IDs than on the host system. This feature will also **not** work with Docker Desktop. diff --git a/website/content/docs/job-specification/template.mdx b/website/content/docs/job-specification/template.mdx index aa7f981143da..1435b58f2932 100644 --- a/website/content/docs/job-specification/template.mdx +++ b/website/content/docs/job-specification/template.mdx @@ -99,14 +99,14 @@ refer to the [Learn Go Template Syntax][gt_learn] Learn guide. - `uid` `(int: 0)` - Specifies the rendered template owner's user ID. ~> **Caveat:** Works only on Unix-based systems. Be careful when using - containerized drivers, suck as `docker` or `podman`, as groups and users + containerized drivers, such as `docker` or `podman`, as groups and users inside the container may have different IDs than on the host system. This feature will also **not** work with Docker Desktop. - `gid` `(int: 0)` - Specifies the rendered template owner's group ID. ~> **Caveat:** Works only on Unix-based systems. Be careful when using - containerized drivers, suck as `docker` or `podman`, as groups and users + containerized drivers, such as `docker` or `podman`, as groups and users inside the container may have different IDs than on the host system. This feature will also **not** work with Docker Desktop. From 5f98282bcd98fb5f63fa7e15ca2b70da562af56e Mon Sep 17 00:00:00 2001 From: Peter Kazmierczak Date: Thu, 11 Aug 2022 09:29:15 +0200 Subject: [PATCH 23/68] jobs_test correction --- api/jobs_test.go | 66 ++++++++++++++++++++++++++++-------------------- 1 file changed, 38 insertions(+), 28 deletions(-) diff --git a/api/jobs_test.go b/api/jobs_test.go index 48df91ef7755..443bd00fd0c6 100644 --- a/api/jobs_test.go +++ b/api/jobs_test.go @@ -758,36 +758,46 @@ func TestJobs_Canonicalize(t *testing.T) { LogConfig: DefaultLogConfig(), Templates: []*Template{ { - SourcePath: stringToPtr(""), - DestPath: stringToPtr("local/file.yml"), - EmbeddedTmpl: stringToPtr("---"), - ChangeMode: stringToPtr("restart"), - ChangeSignal: stringToPtr(""), - ChangeScriptConfig: &ChangeScriptConfig{Path: stringToPtr(""), Args: &[]string{}, Timeout: timeToPtr(5 * time.Second), false}, - Splay: timeToPtr(5 * time.Second), - Perms: stringToPtr("0644"), - Uid: intToPtr(0), - Gid: intToPtr(0), - LeftDelim: stringToPtr("{{"), - RightDelim: stringToPtr("}}"), - Envvars: boolToPtr(false), - VaultGrace: timeToPtr(0), + SourcePath: stringToPtr(""), + DestPath: stringToPtr("local/file.yml"), + EmbeddedTmpl: stringToPtr("---"), + ChangeMode: stringToPtr("restart"), + ChangeSignal: stringToPtr(""), + ChangeScriptConfig: &ChangeScriptConfig{ + Path: stringToPtr(""), + Args: &[]string{}, + Timeout: timeToPtr(5 * time.Second), + FailTask: boolToPtr(false), + }, + Splay: timeToPtr(5 * time.Second), + Perms: stringToPtr("0644"), + Uid: intToPtr(0), + Gid: intToPtr(0), + LeftDelim: stringToPtr("{{"), + RightDelim: stringToPtr("}}"), + Envvars: boolToPtr(false), + VaultGrace: timeToPtr(0), }, { - SourcePath: stringToPtr(""), - DestPath: stringToPtr("local/file.env"), - EmbeddedTmpl: stringToPtr("FOO=bar\n"), - ChangeMode: stringToPtr("restart"), - ChangeSignal: stringToPtr(""), - ChangeScriptConfig: &ChangeScriptConfig{Path: stringToPtr(""), Args: &[]string{}, Timeout: timeToPtr(5 * time.Second), false}, - Splay: timeToPtr(5 * time.Second), - Perms: stringToPtr("0644"), - Uid: intToPtr(0), - Gid: intToPtr(0), - LeftDelim: stringToPtr("{{"), - RightDelim: stringToPtr("}}"), - Envvars: boolToPtr(true), - VaultGrace: timeToPtr(0), + SourcePath: stringToPtr(""), + DestPath: stringToPtr("local/file.env"), + EmbeddedTmpl: stringToPtr("FOO=bar\n"), + ChangeMode: stringToPtr("restart"), + ChangeSignal: stringToPtr(""), + ChangeScriptConfig: &ChangeScriptConfig{ + Path: stringToPtr(""), + Args: &[]string{}, + Timeout: timeToPtr(5 * time.Second), + FailTask: boolToPtr(false), + }, + Splay: timeToPtr(5 * time.Second), + Perms: stringToPtr("0644"), + Uid: intToPtr(0), + Gid: intToPtr(0), + LeftDelim: stringToPtr("{{"), + RightDelim: stringToPtr("}}"), + Envvars: boolToPtr(true), + VaultGrace: timeToPtr(0), }, }, }, From 8a6d5911374963d2cf2de6cf86d0699997220296 Mon Sep 17 00:00:00 2001 From: Peter Kazmierczak Date: Thu, 11 Aug 2022 10:29:53 +0200 Subject: [PATCH 24/68] documentation --- .../change_script_config.mdx | 50 +++++++++++++++++++ .../docs/job-specification/template.mdx | 12 ++--- website/data/docs-nav-data.json | 4 ++ 3 files changed, 57 insertions(+), 9 deletions(-) create mode 100644 website/content/docs/job-specification/change_script_config.mdx diff --git a/website/content/docs/job-specification/change_script_config.mdx b/website/content/docs/job-specification/change_script_config.mdx new file mode 100644 index 000000000000..32ad7c9594f7 --- /dev/null +++ b/website/content/docs/job-specification/change_script_config.mdx @@ -0,0 +1,50 @@ +--- +layout: docs +page_title: change_script_config Stanza - Job Specification +description: The "change_script_config" stanza configures a script to be run on template re-render. +--- + +# `change_script_config` Stanza + + + +The `change_script_config` stanza allows operators to configure scripts that +will be executed on template change. This stanza is only used when template +`change_mode` is set to `script`. + +```hcl +job "docs" { + group "example" { + task "server" { + template { + source = "local/redis.conf.tpl" + destination = "local/redis.conf" + change_mode = "script" + change_script_config { + path = "/bin/foo" + args = ["-verbose", "-debug"] + timeout = "5s" + fail_task = false + } + } + } + } +} +``` + +## `change_script_config` Parameters + +- `path` `(string: "")` - Specifies the full path to a script that + is to be executed on template change. This option is required is the + `change_mode` is `script`. + +- `args` `(array: [])` - List of arguments that are passed to the script + that is to be executed on template change. + +- `timeout` `(string: "5s")` - Timeout for script execution specified using a + label suffix like "30s" or "1h". This option is required if the `change_mode` is + `script`. + +- `fail_task` `(bool: false)` - If `true`, Nomad will kill the task if the + script execution fails. If `false`, script failure will be logged but the task + will continue uninterrupted. \ No newline at end of file diff --git a/website/content/docs/job-specification/template.mdx b/website/content/docs/job-specification/template.mdx index 1435b58f2932..e20f6faacb19 100644 --- a/website/content/docs/job-specification/template.mdx +++ b/website/content/docs/job-specification/template.mdx @@ -59,17 +59,10 @@ refer to the [Learn Go Template Syntax][gt_learn] Learn guide. string like `"SIGUSR1"` or `"SIGINT"`. This option is required if the `change_mode` is `signal`. -- `change_script_path` `(string: "")` - Specifies the full path to a script that - is to be executed on template change. This option is required if the +- `change_script_config` ([ChangeScriptConfig][]: nil) - Configures + the script triggered on template change. This option is required if the `change_mode` is `script`. -- `change_script_arguments` `(string: "")` - Comma-separated list of arguments - that are passed to the script that is to be executed on template change. - -- `change_script_timeout` `(string: "5s")` - Timeout for script execution - specified using a label suffix like "30s" or "1h", This option is required if - the `change_mode` is `script`. - - `data` `(string: "")` - Specifies the raw template to execute. One of `source` or `data` must be specified, but not both. This is useful for smaller templates, but we recommend using `source` for larger templates. @@ -570,6 +563,7 @@ options](/docs/configuration/client#options): files on the client host via the `file` function. By default templates can access files only within the [task working directory]. +[change_script_config]: /docs/job-specification/change_script_config 'Nomad change_script_config Job Specification' [ct]: https://github.com/hashicorp/consul-template 'Consul Template by HashiCorp' [ct_api]: https://github.com/hashicorp/consul-template/blob/master/docs/templating-language.md 'Consul Template API by HashiCorp' [ct_api_connect]: https://github.com/hashicorp/consul-template/blob/master/docs/templating-language.md#connect 'Consul Template API by HashiCorp - connect' diff --git a/website/data/docs-nav-data.json b/website/data/docs-nav-data.json index c543d7e58cfc..92e907999f69 100644 --- a/website/data/docs-nav-data.json +++ b/website/data/docs-nav-data.json @@ -1358,6 +1358,10 @@ "title": "affinity", "path": "job-specification/affinity" }, + { + "title": "change_script_config", + "path": "job-specification/change_script_config" + }, { "title": "check", "path": "job-specification/check" From 21f15ad61437ff73fb17660aad30acd8a7052cfb Mon Sep 17 00:00:00 2001 From: Peter Kazmierczak Date: Thu, 11 Aug 2022 10:42:45 +0200 Subject: [PATCH 25/68] typo in the docs --- website/content/docs/job-specification/template.mdx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/website/content/docs/job-specification/template.mdx b/website/content/docs/job-specification/template.mdx index e20f6faacb19..934b33d5df3c 100644 --- a/website/content/docs/job-specification/template.mdx +++ b/website/content/docs/job-specification/template.mdx @@ -563,7 +563,7 @@ options](/docs/configuration/client#options): files on the client host via the `file` function. By default templates can access files only within the [task working directory]. -[change_script_config]: /docs/job-specification/change_script_config 'Nomad change_script_config Job Specification' +[changescriptconfig]: /docs/job-specification/change_script_config 'Nomad change_script_config Job Specification' [ct]: https://github.com/hashicorp/consul-template 'Consul Template by HashiCorp' [ct_api]: https://github.com/hashicorp/consul-template/blob/master/docs/templating-language.md 'Consul Template API by HashiCorp' [ct_api_connect]: https://github.com/hashicorp/consul-template/blob/master/docs/templating-language.md#connect 'Consul Template API by HashiCorp - connect' From 1f8b044dcaf610aedf2e8ed8945efad83da975ae Mon Sep 17 00:00:00 2001 From: Peter Kazmierczak Date: Thu, 11 Aug 2022 11:18:47 +0200 Subject: [PATCH 26/68] test FailTask --- .../taskrunner/template/template_test.go | 95 +++++++++++++++++-- 1 file changed, 88 insertions(+), 7 deletions(-) diff --git a/client/allocrunner/taskrunner/template/template_test.go b/client/allocrunner/taskrunner/template/template_test.go index 5b1c254f9a81..f6f82c0f322a 100644 --- a/client/allocrunner/taskrunner/template/template_test.go +++ b/client/allocrunner/taskrunner/template/template_test.go @@ -124,12 +124,14 @@ func (m *MockTaskHooks) EmitEvent(event *structs.TaskEvent) { func (m *MockTaskHooks) SetState(state string, event *structs.TaskEvent) {} -// MockExecutor is implementing script executor interface -type MockExecutor struct { +// mockExecutor implements script executor interface +type mockExecutor struct { + DesiredExit int + DesiredErr error } -func (m *MockExecutor) Exec(timeout time.Duration, cmd string, args []string) ([]byte, int, error) { - return []byte{}, 0, nil +func (m *mockExecutor) Exec(timeout time.Duration, cmd string, args []string) ([]byte, int, error) { + return []byte{}, m.DesiredExit, m.DesiredErr } // testHarness is used to test the TaskTemplateManager by spinning up @@ -1249,9 +1251,9 @@ BAR={{key "bar"}} Envvars: true, } - m := MockExecutor{} + me := mockExecutor{} harness := newTestHarness(t, []*structs.Template{t1, t2}, true, false) - harness.driver = &m + harness.driver = &me harness.start(t) defer harness.stop() @@ -1270,7 +1272,7 @@ BAR={{key "bar"}} select { case <-harness.mockHooks.UnblockCh: case <-time.After(time.Duration(5*testutil.TestMultiplier()) * time.Second): - // t.Fatalf("Task unblock should have been called") + t.Fatalf("Task unblock should have been called") } // Update the keys in Consul @@ -1295,6 +1297,85 @@ OUTER: } } +// TestTaskTemplateManager_ScriptExecutionFailTask tests whether we fail the +// task upon script execution failure if that's how it's configured. +func TestTaskTemplateManager_ScriptExecutionFailTask(t *testing.T) { + ci.Parallel(t) + require := require.New(t) + + // Make a template that renders based on a key in Consul and triggers script + key1 := "bam" + key2 := "bar" + content1_1 := "cat" + content1_2 := "dog" + t1 := &structs.Template{ + EmbeddedTmpl: ` +FOO={{key "bam"}} +`, + DestPath: "test.env", + ChangeMode: structs.TemplateChangeModeScript, + ChangeScriptConfig: &structs.ChangeScriptConfig{ + Path: "/bin/foo", + Args: []string{}, + Timeout: 5 * time.Second, + FailTask: true, + }, + Envvars: true, + } + t2 := &structs.Template{ + EmbeddedTmpl: ` +BAR={{key "bar"}} +`, + DestPath: "test2.env", + ChangeMode: structs.TemplateChangeModeScript, + ChangeScriptConfig: &structs.ChangeScriptConfig{ + Path: "/bin/foo", + Args: []string{}, + Timeout: 5 * time.Second, + FailTask: false, + }, + Envvars: true, + } + + me := mockExecutor{DesiredExit: 1, DesiredErr: fmt.Errorf("Script failed")} + harness := newTestHarness(t, []*structs.Template{t1, t2}, true, false) + harness.driver = &me + harness.start(t) + defer harness.stop() + + // Ensure no unblock + select { + case <-harness.mockHooks.UnblockCh: + t.Fatalf("Task unblock should not have been called") + case <-time.After(time.Duration(1*testutil.TestMultiplier()) * time.Second): + } + + // Write the key to Consul + harness.consul.SetKV(t, key1, []byte(content1_1)) + harness.consul.SetKV(t, key2, []byte(content1_1)) + + // Wait for the unblock + select { + case <-harness.mockHooks.UnblockCh: + case <-time.After(time.Duration(5*testutil.TestMultiplier()) * time.Second): + t.Fatalf("Task unblock should have been called") + } + + // Update the keys in Consul + harness.consul.SetKV(t, key1, []byte(content1_2)) + + // Wait for kill channel + select { + case <-harness.mockHooks.KillCh: + break + case <-time.After(time.Duration(1*testutil.TestMultiplier()) * time.Second): + t.Fatalf("Should have received a signals: %+v", harness.mockHooks) + } + + require.NotNil(harness.mockHooks.KillEvent) + require.Contains(harness.mockHooks.KillEvent.DisplayMessage, "failed to run script") +} + // TestTaskTemplateManager_FiltersProcessEnvVars asserts that we only render // environment variables found in task env-vars and not read the nomad host // process environment variables. nomad host process environment variables From bcd7213c5dc3677bdc85b7578c29b8a6db547d14 Mon Sep 17 00:00:00 2001 From: Peter Kazmierczak Date: Thu, 11 Aug 2022 14:52:33 +0200 Subject: [PATCH 27/68] fail_on_error --- api/jobs_test.go | 16 +++++----- api/tasks.go | 8 ++--- .../taskrunner/template/template.go | 2 +- .../taskrunner/template/template_test.go | 32 +++++++++---------- command/agent/job_endpoint.go | 8 ++--- command/agent/job_endpoint_test.go | 16 +++++----- nomad/structs/diff_test.go | 32 +++++++++---------- nomad/structs/structs.go | 4 +-- .../change_script_config.mdx | 10 +++--- 9 files changed, 64 insertions(+), 64 deletions(-) diff --git a/api/jobs_test.go b/api/jobs_test.go index 54c7cd6d6652..622844128171 100644 --- a/api/jobs_test.go +++ b/api/jobs_test.go @@ -764,10 +764,10 @@ func TestJobs_Canonicalize(t *testing.T) { ChangeMode: stringToPtr("restart"), ChangeSignal: stringToPtr(""), ChangeScriptConfig: &ChangeScriptConfig{ - Path: stringToPtr(""), - Args: &[]string{}, - Timeout: timeToPtr(5 * time.Second), - FailTask: boolToPtr(false), + Path: stringToPtr(""), + Args: &[]string{}, + Timeout: timeToPtr(5 * time.Second), + FailOnError: boolToPtr(false), }, Splay: timeToPtr(5 * time.Second), Perms: stringToPtr("0644"), @@ -785,10 +785,10 @@ func TestJobs_Canonicalize(t *testing.T) { ChangeMode: stringToPtr("restart"), ChangeSignal: stringToPtr(""), ChangeScriptConfig: &ChangeScriptConfig{ - Path: stringToPtr(""), - Args: &[]string{}, - Timeout: timeToPtr(5 * time.Second), - FailTask: boolToPtr(false), + Path: stringToPtr(""), + Args: &[]string{}, + Timeout: timeToPtr(5 * time.Second), + FailOnError: boolToPtr(false), }, Splay: timeToPtr(5 * time.Second), Perms: stringToPtr("0644"), diff --git a/api/tasks.go b/api/tasks.go index 74f513c0acad..8f739c54a873 100644 --- a/api/tasks.go +++ b/api/tasks.go @@ -792,10 +792,10 @@ func (wc *WaitConfig) Copy() *WaitConfig { } type ChangeScriptConfig struct { - Path *string `mapstructure:"path" hcl:"path"` - Args *[]string `mapstructure:"args" hcl:"args,optional"` - Timeout *time.Duration `mapstructure:"timeout" hcl:"timeout"` - FailTask *bool `mapstructure:"fail_task" hcl:"fail_task"` + Path *string `mapstructure:"path" hcl:"path"` + Args *[]string `mapstructure:"args" hcl:"args,optional"` + Timeout *time.Duration `mapstructure:"timeout" hcl:"timeout"` + FailOnError *bool `mapstructure:"fail_on_error" hcl:"fail_on_error"` } type Template struct { diff --git a/client/allocrunner/taskrunner/template/template.go b/client/allocrunner/taskrunner/template/template.go index 1dd3f51eb707..21691e334870 100644 --- a/client/allocrunner/taskrunner/template/template.go +++ b/client/allocrunner/taskrunner/template/template.go @@ -508,7 +508,7 @@ func (tm *TaskTemplateManager) onTemplateRendered(handledRenders map[string]time fmt.Sprintf( "Template failed to run script %v on change: %v Exit code: %v", script.Path, err, exitCode, )) - if script.FailTask { + if script.FailOnError { tm.config.Lifecycle.Kill(context.Background(), structs.NewTaskEvent(structs.TaskKilling). SetFailsTask(). diff --git a/client/allocrunner/taskrunner/template/template_test.go b/client/allocrunner/taskrunner/template/template_test.go index f6f82c0f322a..22056d275f12 100644 --- a/client/allocrunner/taskrunner/template/template_test.go +++ b/client/allocrunner/taskrunner/template/template_test.go @@ -1229,10 +1229,10 @@ FOO={{key "bam"}} DestPath: "test.env", ChangeMode: structs.TemplateChangeModeScript, ChangeScriptConfig: &structs.ChangeScriptConfig{ - Path: "/bin/foo", - Args: []string{}, - Timeout: 5 * time.Second, - FailTask: false, + Path: "/bin/foo", + Args: []string{}, + Timeout: 5 * time.Second, + FailOnError: false, }, Envvars: true, } @@ -1243,10 +1243,10 @@ BAR={{key "bar"}} DestPath: "test2.env", ChangeMode: structs.TemplateChangeModeScript, ChangeScriptConfig: &structs.ChangeScriptConfig{ - Path: "/bin/foo", - Args: []string{}, - Timeout: 5 * time.Second, - FailTask: false, + Path: "/bin/foo", + Args: []string{}, + Timeout: 5 * time.Second, + FailOnError: false, }, Envvars: true, } @@ -1315,10 +1315,10 @@ FOO={{key "bam"}} DestPath: "test.env", ChangeMode: structs.TemplateChangeModeScript, ChangeScriptConfig: &structs.ChangeScriptConfig{ - Path: "/bin/foo", - Args: []string{}, - Timeout: 5 * time.Second, - FailTask: true, + Path: "/bin/foo", + Args: []string{}, + Timeout: 5 * time.Second, + FailOnError: true, }, Envvars: true, } @@ -1329,10 +1329,10 @@ BAR={{key "bar"}} DestPath: "test2.env", ChangeMode: structs.TemplateChangeModeScript, ChangeScriptConfig: &structs.ChangeScriptConfig{ - Path: "/bin/foo", - Args: []string{}, - Timeout: 5 * time.Second, - FailTask: false, + Path: "/bin/foo", + Args: []string{}, + Timeout: 5 * time.Second, + FailOnError: false, }, Envvars: true, } diff --git a/command/agent/job_endpoint.go b/command/agent/job_endpoint.go index afc9b1ad4eba..2c2fb0faf05a 100644 --- a/command/agent/job_endpoint.go +++ b/command/agent/job_endpoint.go @@ -1271,10 +1271,10 @@ func apiChangeScriptConfigToStructsChangeScriptConfig(changeScriptConfig *api.Ch } return &structs.ChangeScriptConfig{ - Path: *changeScriptConfig.Path, - Args: *changeScriptConfig.Args, - Timeout: *changeScriptConfig.Timeout, - FailTask: *changeScriptConfig.FailTask, + Path: *changeScriptConfig.Path, + Args: *changeScriptConfig.Args, + Timeout: *changeScriptConfig.Timeout, + FailOnError: *changeScriptConfig.FailOnError, } } diff --git a/command/agent/job_endpoint_test.go b/command/agent/job_endpoint_test.go index 5533585b497a..89315a19b170 100644 --- a/command/agent/job_endpoint_test.go +++ b/command/agent/job_endpoint_test.go @@ -2732,10 +2732,10 @@ func TestJobs_ApiJobToStructsJob(t *testing.T) { ChangeMode: helper.StringToPtr("change"), ChangeSignal: helper.StringToPtr("signal"), ChangeScriptConfig: &api.ChangeScriptConfig{ - Path: helper.StringToPtr("/bin/foo"), - Args: &[]string{"-h"}, - Timeout: helper.TimeToPtr(5 * time.Second), - FailTask: helper.BoolToPtr(false), + Path: helper.StringToPtr("/bin/foo"), + Args: &[]string{"-h"}, + Timeout: helper.TimeToPtr(5 * time.Second), + FailOnError: helper.BoolToPtr(false), }, Splay: helper.TimeToPtr(1 * time.Minute), Perms: helper.StringToPtr("666"), @@ -3145,10 +3145,10 @@ func TestJobs_ApiJobToStructsJob(t *testing.T) { ChangeMode: "change", ChangeSignal: "SIGNAL", ChangeScriptConfig: &structs.ChangeScriptConfig{ - Path: "/bin/foo", - Args: []string{"-h"}, - Timeout: 5 * time.Second, - FailTask: false, + Path: "/bin/foo", + Args: []string{"-h"}, + Timeout: 5 * time.Second, + FailOnError: false, }, Splay: 1 * time.Minute, Perms: "666", diff --git a/nomad/structs/diff_test.go b/nomad/structs/diff_test.go index ee65c0bdbf09..685fffcd4ff1 100644 --- a/nomad/structs/diff_test.go +++ b/nomad/structs/diff_test.go @@ -7043,10 +7043,10 @@ func TestTaskDiff(t *testing.T) { ChangeMode: "bam", ChangeSignal: "SIGHUP", ChangeScriptConfig: &ChangeScriptConfig{ - Path: "/bin/foo", - Args: []string{"-debug"}, - Timeout: 5, - FailTask: false, + Path: "/bin/foo", + Args: []string{"-debug"}, + Timeout: 5, + FailOnError: false, }, Splay: 1, Perms: "0644", @@ -7064,10 +7064,10 @@ func TestTaskDiff(t *testing.T) { ChangeMode: "bam2", ChangeSignal: "SIGHUP2", ChangeScriptConfig: &ChangeScriptConfig{ - Path: "/bin/foo2", - Args: []string{"-debugs"}, - Timeout: 6, - FailTask: false, + Path: "/bin/foo2", + Args: []string{"-debugs"}, + Timeout: 6, + FailOnError: false, }, Splay: 2, Perms: "0666", @@ -7086,10 +7086,10 @@ func TestTaskDiff(t *testing.T) { ChangeMode: "bam", ChangeSignal: "SIGHUP", ChangeScriptConfig: &ChangeScriptConfig{ - Path: "/bin/foo", - Args: []string{"-debug"}, - Timeout: 5, - FailTask: false, + Path: "/bin/foo", + Args: []string{"-debug"}, + Timeout: 5, + FailOnError: false, }, Splay: 1, Perms: "0644", @@ -7107,10 +7107,10 @@ func TestTaskDiff(t *testing.T) { ChangeMode: "bam3", ChangeSignal: "SIGHUP3", ChangeScriptConfig: &ChangeScriptConfig{ - Path: "/bin/foo3", - Args: []string{"-debugss"}, - Timeout: 7, - FailTask: false, + Path: "/bin/foo3", + Args: []string{"-debugss"}, + Timeout: 7, + FailOnError: false, }, Splay: 3, Perms: "0776", diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index d3a3f1517f4d..df0094e1e1f3 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -7707,9 +7707,9 @@ type ChangeScriptConfig struct { Args []string // Timeout is the amount of seconds we wait for the script to finish Timeout time.Duration - // FailTask indicates whether a task should fail in case script execution + // FailOnError indicates whether a task should fail in case script execution // fails or log script failure and don't interrupt the task - FailTask bool + FailOnError bool } // Template represents a template configuration to be rendered for a given task diff --git a/website/content/docs/job-specification/change_script_config.mdx b/website/content/docs/job-specification/change_script_config.mdx index 32ad7c9594f7..eb427b83be7b 100644 --- a/website/content/docs/job-specification/change_script_config.mdx +++ b/website/content/docs/job-specification/change_script_config.mdx @@ -21,10 +21,10 @@ job "docs" { destination = "local/redis.conf" change_mode = "script" change_script_config { - path = "/bin/foo" - args = ["-verbose", "-debug"] - timeout = "5s" - fail_task = false + path = "/bin/foo" + args = ["-verbose", "-debug"] + timeout = "5s" + fail_on_error = false } } } @@ -45,6 +45,6 @@ job "docs" { label suffix like "30s" or "1h". This option is required if the `change_mode` is `script`. -- `fail_task` `(bool: false)` - If `true`, Nomad will kill the task if the +- `fail_on_error` `(bool: false)` - If `true`, Nomad will kill the task if the script execution fails. If `false`, script failure will be logged but the task will continue uninterrupted. \ No newline at end of file From de9c7c2ce82c6176c637b067ffe9d3bb8568ab16 Mon Sep 17 00:00:00 2001 From: Peter Kazmierczak Date: Thu, 11 Aug 2022 14:59:46 +0200 Subject: [PATCH 28/68] addressed Derek's comments --- api/tasks.go | 8 ++++---- client/allocrunner/taskrunner/template/template.go | 4 ++-- .../allocrunner/taskrunner/template/template_test.go | 10 +++++----- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/api/tasks.go b/api/tasks.go index 8f739c54a873..a7eb48e4034c 100644 --- a/api/tasks.go +++ b/api/tasks.go @@ -841,10 +841,10 @@ func (tmpl *Template) Canonicalize() { } if tmpl.ChangeScriptConfig == nil { tmpl.ChangeScriptConfig = &ChangeScriptConfig{ - stringToPtr(""), - &[]string{}, - timeToPtr(5 * time.Second), - boolToPtr(false), + Path: stringToPtr(""), + Args: &[]string{}, + Timeout: timeToPtr(5 * time.Second), + FailOnError: boolToPtr(false), } } if tmpl.Splay == nil { diff --git a/client/allocrunner/taskrunner/template/template.go b/client/allocrunner/taskrunner/template/template.go index 21691e334870..c22125bb0b55 100644 --- a/client/allocrunner/taskrunner/template/template.go +++ b/client/allocrunner/taskrunner/template/template.go @@ -395,7 +395,7 @@ func (tm *TaskTemplateManager) onTemplateRendered(handledRenders map[string]time var handling []string signals := make(map[string]struct{}) - scripts := []structs.ChangeScriptConfig{} + scripts := []*structs.ChangeScriptConfig{} restart := false var splay time.Duration @@ -441,7 +441,7 @@ func (tm *TaskTemplateManager) onTemplateRendered(handledRenders map[string]time case structs.TemplateChangeModeRestart: restart = true case structs.TemplateChangeModeScript: - scripts = append(scripts, *tmpl.ChangeScriptConfig) + scripts = append(scripts, tmpl.ChangeScriptConfig) case structs.TemplateChangeModeNoop: continue } diff --git a/client/allocrunner/taskrunner/template/template_test.go b/client/allocrunner/taskrunner/template/template_test.go index 22056d275f12..c69f9743aa50 100644 --- a/client/allocrunner/taskrunner/template/template_test.go +++ b/client/allocrunner/taskrunner/template/template_test.go @@ -1260,7 +1260,7 @@ BAR={{key "bar"}} // Ensure no unblock select { case <-harness.mockHooks.UnblockCh: - t.Fatalf("Task unblock should not have been called") + require.Fail(t, "Task unblock should not have been called") case <-time.After(time.Duration(1*testutil.TestMultiplier()) * time.Second): } @@ -1272,7 +1272,7 @@ BAR={{key "bar"}} select { case <-harness.mockHooks.UnblockCh: case <-time.After(time.Duration(5*testutil.TestMultiplier()) * time.Second): - t.Fatalf("Task unblock should have been called") + require.Fail(t, "Task unblock should have been called") } // Update the keys in Consul @@ -1346,7 +1346,7 @@ BAR={{key "bar"}} // Ensure no unblock select { case <-harness.mockHooks.UnblockCh: - t.Fatalf("Task unblock should not have been called") + require.Fail("Task unblock should not have been called") case <-time.After(time.Duration(1*testutil.TestMultiplier()) * time.Second): } @@ -1358,7 +1358,7 @@ BAR={{key "bar"}} select { case <-harness.mockHooks.UnblockCh: case <-time.After(time.Duration(5*testutil.TestMultiplier()) * time.Second): - t.Fatalf("Task unblock should have been called") + require.Fail("Task unblock should have been called") } // Update the keys in Consul @@ -1369,7 +1369,7 @@ BAR={{key "bar"}} case <-harness.mockHooks.KillCh: break case <-time.After(time.Duration(1*testutil.TestMultiplier()) * time.Second): - t.Fatalf("Should have received a signals: %+v", harness.mockHooks) + require.Fail("Should have received a signals: %+v", harness.mockHooks) } require.NotNil(harness.mockHooks.KillEvent) From 986a01859db7a6170894d1cdcfe9351e25b8f833 Mon Sep 17 00:00:00 2001 From: Peter Kazmierczak Date: Thu, 11 Aug 2022 15:09:12 +0200 Subject: [PATCH 29/68] updated json-jobs.mdx --- website/content/api-docs/json-jobs.mdx | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/website/content/api-docs/json-jobs.mdx b/website/content/api-docs/json-jobs.mdx index 721c2f3eb746..0d32a66bb0bb 100644 --- a/website/content/api-docs/json-jobs.mdx +++ b/website/content/api-docs/json-jobs.mdx @@ -1061,9 +1061,23 @@ README][ct]. "SIGUSR1" or "SIGINT". This option is required if the `ChangeMode` is `signal`. -- `ChangeScriptPath` - Specifies the full path to a script that is to be - executed on template change. This option is required if the `change_mode` is - `script`. +- `ChangeScriptConfig` - Configures the script triggered on template change. + This option is required if the `change_mode` is `script`. + + The `ChangeScriptConfig` object supports the following attributes: + + - `path` - Specifies the full path to a script that is to be executed on + template change. This option is required is the `change_mode` is `script`. + + - `args` - List of arguments that are passed to the script that is to be + executed on template change. + + - `timeout` - Timeout for script execution specified using a label suffix like + "30s" or "1h". This option is required if the `change_mode` is `script`. + + - `fail_on_error` - If `true`, Nomad will kill the task if the script + execution fails. If `false`, script failure will be logged but the task will + continue uninterrupted. - `ChangeScriptArguments` - Comma-separated list of arguments that are passed to the script that is to be executed on template change. From df9f13bca121d027a8a5bba0956d40854a1b2d1f Mon Sep 17 00:00:00 2001 From: Peter Kazmierczak Date: Thu, 11 Aug 2022 15:25:04 +0200 Subject: [PATCH 30/68] typo in diff_test --- nomad/structs/diff_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nomad/structs/diff_test.go b/nomad/structs/diff_test.go index 685fffcd4ff1..851e1a91262b 100644 --- a/nomad/structs/diff_test.go +++ b/nomad/structs/diff_test.go @@ -7248,7 +7248,7 @@ func TestTaskDiff(t *testing.T) { Fields: []*FieldDiff{ { Type: DiffTypeAdded, - Name: "FailTask", + Name: "FailOnError", Old: "", New: "false", }, @@ -7360,7 +7360,7 @@ func TestTaskDiff(t *testing.T) { Fields: []*FieldDiff{ { Type: DiffTypeDeleted, - Name: "FailTask", + Name: "FailOnError", Old: "false", New: "", }, From 6ada97e1b9ee393ff832440d25c4dba717925de8 Mon Sep 17 00:00:00 2001 From: Peter Kazmierczak Date: Thu, 11 Aug 2022 17:27:42 +0200 Subject: [PATCH 31/68] removed obsolete documentation --- website/content/api-docs/json-jobs.mdx | 7 ------- 1 file changed, 7 deletions(-) diff --git a/website/content/api-docs/json-jobs.mdx b/website/content/api-docs/json-jobs.mdx index 0d32a66bb0bb..ed973c096e0b 100644 --- a/website/content/api-docs/json-jobs.mdx +++ b/website/content/api-docs/json-jobs.mdx @@ -1079,13 +1079,6 @@ README][ct]. execution fails. If `false`, script failure will be logged but the task will continue uninterrupted. -- `ChangeScriptArguments` - Comma-separated list of arguments that are passed to - the script that is to be executed on template change. - -- `ChangeScriptTimeout` - Timeout for script execution specified using a label - suffix like "30s" or "1h", This option is required if the `change_mode` is - `script`. - - `DestPath` - Specifies the location where the resulting template should be rendered, relative to the task directory. From d8fd77bc59fe7419ecd467c218716ce9f9ab7788 Mon Sep 17 00:00:00 2001 From: Peter Kazmierczak Date: Thu, 11 Aug 2022 19:31:07 +0200 Subject: [PATCH 32/68] better canonicalization --- api/tasks.go | 28 +++++++++++++++++++--------- nomad/structs/structs.go | 5 +---- 2 files changed, 20 insertions(+), 13 deletions(-) diff --git a/api/tasks.go b/api/tasks.go index a7eb48e4034c..c1b5e1f237dd 100644 --- a/api/tasks.go +++ b/api/tasks.go @@ -794,16 +794,31 @@ func (wc *WaitConfig) Copy() *WaitConfig { type ChangeScriptConfig struct { Path *string `mapstructure:"path" hcl:"path"` Args *[]string `mapstructure:"args" hcl:"args,optional"` - Timeout *time.Duration `mapstructure:"timeout" hcl:"timeout"` + Timeout *time.Duration `mapstructure:"timeout" hcl:"timeout,optional"` FailOnError *bool `mapstructure:"fail_on_error" hcl:"fail_on_error"` } +func (ch *ChangeScriptConfig) Canonicalize() { + if ch.Path == nil { + ch.Path = stringToPtr("") + } + if ch.Args == nil { + ch.Args = &[]string{} + } + if ch.Timeout == nil { + ch.Timeout = timeToPtr(5 * time.Second) + } + if ch.FailOnError == nil { + ch.FailOnError = boolToPtr(false) + } +} + type Template struct { SourcePath *string `mapstructure:"source" hcl:"source,optional"` DestPath *string `mapstructure:"destination" hcl:"destination,optional"` EmbeddedTmpl *string `mapstructure:"data" hcl:"data,optional"` ChangeMode *string `mapstructure:"change_mode" hcl:"change_mode,optional"` - ChangeScriptConfig *ChangeScriptConfig `mapstructure:"change_script_config" hcl:"change_script_config,optional"` + ChangeScriptConfig *ChangeScriptConfig `mapstructure:"change_script_config" hcl:"change_script_config,block"` ChangeSignal *string `mapstructure:"change_signal" hcl:"change_signal,optional"` Splay *time.Duration `mapstructure:"splay" hcl:"splay,optional"` Perms *string `mapstructure:"perms" hcl:"perms,optional"` @@ -839,13 +854,8 @@ func (tmpl *Template) Canonicalize() { sig := *tmpl.ChangeSignal tmpl.ChangeSignal = stringToPtr(strings.ToUpper(sig)) } - if tmpl.ChangeScriptConfig == nil { - tmpl.ChangeScriptConfig = &ChangeScriptConfig{ - Path: stringToPtr(""), - Args: &[]string{}, - Timeout: timeToPtr(5 * time.Second), - FailOnError: boolToPtr(false), - } + if tmpl.ChangeScriptConfig != nil { + tmpl.ChangeScriptConfig.Canonicalize() } if tmpl.Splay == nil { tmpl.Splay = timeToPtr(5 * time.Second) diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index df0094e1e1f3..e3300658aff7 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -7835,10 +7835,7 @@ func (t *Template) Validate() error { } case TemplateChangeModeScript: if t.ChangeScriptConfig.Path == "" { - _ = multierror.Append(&mErr, fmt.Errorf("must specify script path and timeout value when change mode is signal")) - } - if t.ChangeScriptConfig.Timeout == 0 { - _ = multierror.Append(&mErr, fmt.Errorf("must specify script timeout value when change mode is script, timeout value must be greater than 0")) + _ = multierror.Append(&mErr, fmt.Errorf("must specify script path value when change mode is script")) } default: _ = multierror.Append(&mErr, TemplateChangeModeInvalidError) From 4673f953a226ecbc90ce55dd914430289f1a6f55 Mon Sep 17 00:00:00 2001 From: Peter Kazmierczak Date: Thu, 11 Aug 2022 19:31:18 +0200 Subject: [PATCH 33/68] make timeout optional --- website/content/api-docs/json-jobs.mdx | 2 +- .../content/docs/job-specification/change_script_config.mdx | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/website/content/api-docs/json-jobs.mdx b/website/content/api-docs/json-jobs.mdx index ed973c096e0b..40eef6946b6a 100644 --- a/website/content/api-docs/json-jobs.mdx +++ b/website/content/api-docs/json-jobs.mdx @@ -1073,7 +1073,7 @@ README][ct]. executed on template change. - `timeout` - Timeout for script execution specified using a label suffix like - "30s" or "1h". This option is required if the `change_mode` is `script`. + "30s" or "1h". - `fail_on_error` - If `true`, Nomad will kill the task if the script execution fails. If `false`, script failure will be logged but the task will diff --git a/website/content/docs/job-specification/change_script_config.mdx b/website/content/docs/job-specification/change_script_config.mdx index eb427b83be7b..bb482af2f02f 100644 --- a/website/content/docs/job-specification/change_script_config.mdx +++ b/website/content/docs/job-specification/change_script_config.mdx @@ -42,8 +42,7 @@ job "docs" { that is to be executed on template change. - `timeout` `(string: "5s")` - Timeout for script execution specified using a - label suffix like "30s" or "1h". This option is required if the `change_mode` is - `script`. + label suffix like "30s" or "1h". - `fail_on_error` `(bool: false)` - If `true`, Nomad will kill the task if the script execution fails. If `false`, script failure will be logged but the task From f8f4fddc1d60cc0edc20ad6a846da808eb80d986 Mon Sep 17 00:00:00 2001 From: Peter Kazmierczak Date: Thu, 11 Aug 2022 20:39:45 +0200 Subject: [PATCH 34/68] hcl parser fixes --- jobspec/parse_task.go | 43 +++++++++++++++++++++++++++++++++ jobspec/parse_test.go | 12 ++++++++- jobspec/test-fixtures/basic.hcl | 11 +++++++-- jobspec2/parse_job.go | 19 +++++++++++++++ 4 files changed, 82 insertions(+), 3 deletions(-) diff --git a/jobspec/parse_task.go b/jobspec/parse_task.go index 243a49bdd80d..01eb65f5e418 100644 --- a/jobspec/parse_task.go +++ b/jobspec/parse_task.go @@ -433,6 +433,14 @@ func parseArtifactOption(result map[string]string, list *ast.ObjectList) error { func parseTemplates(result *[]*api.Template, list *ast.ObjectList) error { for _, o := range list.Elem().Items { + // we'll need a list of all ast objects for later + var listVal *ast.ObjectList + if ot, ok := o.Val.(*ast.ObjectType); ok { + listVal = ot.List + } else { + return fmt.Errorf("should be an object") + } + // Check for invalid keys valid := []string{ "change_mode", @@ -458,6 +466,7 @@ func parseTemplates(result *[]*api.Template, list *ast.ObjectList) error { if err := hcl.DecodeObject(&m, o.Val); err != nil { return err } + delete(m, "change_script_config") // change_script_config is its own object templ := &api.Template{ ChangeMode: stringToPtr("restart"), @@ -479,6 +488,40 @@ func parseTemplates(result *[]*api.Template, list *ast.ObjectList) error { return err } + // If we have change_script_config, parse it + if o := listVal.Filter("change_script_config"); len(o.Items) > 0 { + if len(o.Items) != 1 { + return fmt.Errorf( + "change_script_config -> expected single stanza, got %d", len(o.Items), + ) + } + var m map[string]interface{} + changeScriptConfigBlock := o.Items[0] + + // check for invalid fields + valid := []string{"path", "args", "timeout", "fail_on_error"} + if err := checkHCLKeys(changeScriptConfigBlock.Val, valid); err != nil { + return multierror.Prefix(err, "change_script_config ->") + } + + if err := hcl.DecodeObject(&m, changeScriptConfigBlock.Val); err != nil { + return err + } + + templ.ChangeScriptConfig = &api.ChangeScriptConfig{} + dec, err := mapstructure.NewDecoder(&mapstructure.DecoderConfig{ + DecodeHook: mapstructure.StringToTimeDurationHookFunc(), + WeaklyTypedInput: true, + Result: templ.ChangeScriptConfig, + }) + if err != nil { + return err + } + if err := dec.Decode(m); err != nil { + return err + } + } + *result = append(*result, templ) } diff --git a/jobspec/parse_test.go b/jobspec/parse_test.go index 0d7467c1692f..a22af4754643 100644 --- a/jobspec/parse_test.go +++ b/jobspec/parse_test.go @@ -22,6 +22,10 @@ const ( // templateChangeModeRestart marks that the task should be restarted if the templateChangeModeRestart = "restart" + + // templateChangeModeScript marks that ac script should be executed on + // template re-render + templateChangeModeScript = "script" ) // Helper functions below are only used by this test suite @@ -382,7 +386,13 @@ func TestParse(t *testing.T) { { SourcePath: stringToPtr("bar"), DestPath: stringToPtr("bar"), - ChangeMode: stringToPtr(templateChangeModeRestart), + ChangeMode: stringToPtr(templateChangeModeScript), + ChangeScriptConfig: &api.ChangeScriptConfig{ + Args: &[]string{"-debug", "-verbose"}, + Path: stringToPtr("/bin/foo"), + Timeout: timeToPtr(5 * time.Second), + FailOnError: boolToPtr(false), + }, Splay: timeToPtr(5 * time.Second), Perms: stringToPtr("777"), Uid: intToPtr(1001), diff --git a/jobspec/test-fixtures/basic.hcl b/jobspec/test-fixtures/basic.hcl index 273ae6ebdbf2..aae539b69b46 100644 --- a/jobspec/test-fixtures/basic.hcl +++ b/jobspec/test-fixtures/basic.hcl @@ -315,8 +315,15 @@ job "binstore-storagelocker" { } template { - source = "bar" - destination = "bar" + source = "bar" + destination = "bar" + change_mode = "script" + change_script_config { + path = "/bin/foo" + args = ["-debug", "-verbose"] + timeout = "5s" + fail_on_error = false + } perms = "777" uid = 1001 gid = 20 diff --git a/jobspec2/parse_job.go b/jobspec2/parse_job.go index 25ec2381ad80..80ef84ba534b 100644 --- a/jobspec2/parse_job.go +++ b/jobspec2/parse_job.go @@ -116,6 +116,25 @@ func normalizeTemplates(templates []*api.Template) { if t.Splay == nil { t.Splay = durationToPtr(5 * time.Second) } + normalizeChangeScriptConfig(t.ChangeScriptConfig) + } +} + +func normalizeChangeScriptConfig(ch *api.ChangeScriptConfig) { + if ch == nil { + return + } + + if ch.Args == nil { + ch.Args = &[]string{} + } + + if ch.Timeout == nil { + ch.Timeout = durationToPtr(5 * time.Second) + } + + if ch.FailOnError == nil { + ch.FailOnError = boolToPtr(false) } } From 3b68123102f0178752782cd86aec99d55ca2ebc4 Mon Sep 17 00:00:00 2001 From: Peter Kazmierczak Date: Thu, 11 Aug 2022 20:57:38 +0200 Subject: [PATCH 35/68] canonicalize test isn't needed --- api/jobs_test.go | 44 ++++++++++++++++---------------------------- 1 file changed, 16 insertions(+), 28 deletions(-) diff --git a/api/jobs_test.go b/api/jobs_test.go index 622844128171..ea3761fa3240 100644 --- a/api/jobs_test.go +++ b/api/jobs_test.go @@ -763,20 +763,14 @@ func TestJobs_Canonicalize(t *testing.T) { EmbeddedTmpl: stringToPtr("---"), ChangeMode: stringToPtr("restart"), ChangeSignal: stringToPtr(""), - ChangeScriptConfig: &ChangeScriptConfig{ - Path: stringToPtr(""), - Args: &[]string{}, - Timeout: timeToPtr(5 * time.Second), - FailOnError: boolToPtr(false), - }, - Splay: timeToPtr(5 * time.Second), - Perms: stringToPtr("0644"), - Uid: intToPtr(-1), - Gid: intToPtr(-1), - LeftDelim: stringToPtr("{{"), - RightDelim: stringToPtr("}}"), - Envvars: boolToPtr(false), - VaultGrace: timeToPtr(0), + Splay: timeToPtr(5 * time.Second), + Perms: stringToPtr("0644"), + Uid: intToPtr(-1), + Gid: intToPtr(-1), + LeftDelim: stringToPtr("{{"), + RightDelim: stringToPtr("}}"), + Envvars: boolToPtr(false), + VaultGrace: timeToPtr(0), }, { SourcePath: stringToPtr(""), @@ -784,20 +778,14 @@ func TestJobs_Canonicalize(t *testing.T) { EmbeddedTmpl: stringToPtr("FOO=bar\n"), ChangeMode: stringToPtr("restart"), ChangeSignal: stringToPtr(""), - ChangeScriptConfig: &ChangeScriptConfig{ - Path: stringToPtr(""), - Args: &[]string{}, - Timeout: timeToPtr(5 * time.Second), - FailOnError: boolToPtr(false), - }, - Splay: timeToPtr(5 * time.Second), - Perms: stringToPtr("0644"), - Uid: intToPtr(-1), - Gid: intToPtr(-1), - LeftDelim: stringToPtr("{{"), - RightDelim: stringToPtr("}}"), - Envvars: boolToPtr(true), - VaultGrace: timeToPtr(0), + Splay: timeToPtr(5 * time.Second), + Perms: stringToPtr("0644"), + Uid: intToPtr(-1), + Gid: intToPtr(-1), + LeftDelim: stringToPtr("{{"), + RightDelim: stringToPtr("}}"), + Envvars: boolToPtr(true), + VaultGrace: timeToPtr(0), }, }, }, From 87fd05fd46ed74be02ba38df529a9a43ebd7a574 Mon Sep 17 00:00:00 2001 From: Peter Kazmierczak Date: Fri, 12 Aug 2022 11:12:27 +0200 Subject: [PATCH 36/68] pointer non-sense --- api/tasks.go | 4 ++-- command/agent/job_endpoint.go | 2 +- command/agent/job_endpoint_test.go | 2 +- jobspec/parse_test.go | 2 +- jobspec2/parse_job.go | 2 +- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/api/tasks.go b/api/tasks.go index c1b5e1f237dd..46061b7c9e8e 100644 --- a/api/tasks.go +++ b/api/tasks.go @@ -793,7 +793,7 @@ func (wc *WaitConfig) Copy() *WaitConfig { type ChangeScriptConfig struct { Path *string `mapstructure:"path" hcl:"path"` - Args *[]string `mapstructure:"args" hcl:"args,optional"` + Args []string `mapstructure:"args" hcl:"args,optional"` Timeout *time.Duration `mapstructure:"timeout" hcl:"timeout,optional"` FailOnError *bool `mapstructure:"fail_on_error" hcl:"fail_on_error"` } @@ -803,7 +803,7 @@ func (ch *ChangeScriptConfig) Canonicalize() { ch.Path = stringToPtr("") } if ch.Args == nil { - ch.Args = &[]string{} + ch.Args = []string{} } if ch.Timeout == nil { ch.Timeout = timeToPtr(5 * time.Second) diff --git a/command/agent/job_endpoint.go b/command/agent/job_endpoint.go index 2c2fb0faf05a..31c8e5538c0f 100644 --- a/command/agent/job_endpoint.go +++ b/command/agent/job_endpoint.go @@ -1272,7 +1272,7 @@ func apiChangeScriptConfigToStructsChangeScriptConfig(changeScriptConfig *api.Ch return &structs.ChangeScriptConfig{ Path: *changeScriptConfig.Path, - Args: *changeScriptConfig.Args, + Args: changeScriptConfig.Args, Timeout: *changeScriptConfig.Timeout, FailOnError: *changeScriptConfig.FailOnError, } diff --git a/command/agent/job_endpoint_test.go b/command/agent/job_endpoint_test.go index 89315a19b170..90a74aa3f88d 100644 --- a/command/agent/job_endpoint_test.go +++ b/command/agent/job_endpoint_test.go @@ -2733,7 +2733,7 @@ func TestJobs_ApiJobToStructsJob(t *testing.T) { ChangeSignal: helper.StringToPtr("signal"), ChangeScriptConfig: &api.ChangeScriptConfig{ Path: helper.StringToPtr("/bin/foo"), - Args: &[]string{"-h"}, + Args: []string{"-h"}, Timeout: helper.TimeToPtr(5 * time.Second), FailOnError: helper.BoolToPtr(false), }, diff --git a/jobspec/parse_test.go b/jobspec/parse_test.go index a22af4754643..fa095e3dbffa 100644 --- a/jobspec/parse_test.go +++ b/jobspec/parse_test.go @@ -388,7 +388,7 @@ func TestParse(t *testing.T) { DestPath: stringToPtr("bar"), ChangeMode: stringToPtr(templateChangeModeScript), ChangeScriptConfig: &api.ChangeScriptConfig{ - Args: &[]string{"-debug", "-verbose"}, + Args: []string{"-debug", "-verbose"}, Path: stringToPtr("/bin/foo"), Timeout: timeToPtr(5 * time.Second), FailOnError: boolToPtr(false), diff --git a/jobspec2/parse_job.go b/jobspec2/parse_job.go index 80ef84ba534b..2fc0e5bdfb61 100644 --- a/jobspec2/parse_job.go +++ b/jobspec2/parse_job.go @@ -126,7 +126,7 @@ func normalizeChangeScriptConfig(ch *api.ChangeScriptConfig) { } if ch.Args == nil { - ch.Args = &[]string{} + ch.Args = []string{} } if ch.Timeout == nil { From 3d1ebdcd8422aa50bc17cd3b4bc9a861b8567b2b Mon Sep 17 00:00:00 2001 From: Peter Kazmierczak Date: Fri, 12 Aug 2022 11:14:01 +0200 Subject: [PATCH 37/68] fixed conditional --- .../taskrunner/template/template.go | 43 +++++++++---------- 1 file changed, 20 insertions(+), 23 deletions(-) diff --git a/client/allocrunner/taskrunner/template/template.go b/client/allocrunner/taskrunner/template/template.go index c22125bb0b55..076e5fca9132 100644 --- a/client/allocrunner/taskrunner/template/template.go +++ b/client/allocrunner/taskrunner/template/template.go @@ -499,33 +499,30 @@ func (tm *TaskTemplateManager) onTemplateRendered(handledRenders map[string]time } } } - if len(scripts) != 0 { - for _, script := range scripts { - _, exitCode, err := tm.config.Handle.Exec(script.Timeout, script.Path, script.Args) - if err != nil { - structs.NewTaskEvent(structs.TaskHookFailed). - SetDisplayMessage( - fmt.Sprintf( - "Template failed to run script %v on change: %v Exit code: %v", script.Path, err, exitCode, + for _, script := range scripts { + _, exitCode, err := tm.config.Handle.Exec(script.Timeout, script.Path, script.Args) + if err != nil { + structs.NewTaskEvent(structs.TaskHookFailed). + SetDisplayMessage( + fmt.Sprintf( + "Template failed to run script %v on change: %v Exit code: %v", script.Path, err, exitCode, + )) + if script.FailOnError { + tm.config.Lifecycle.Kill(context.Background(), + structs.NewTaskEvent(structs.TaskKilling). + SetFailsTask(). + SetDisplayMessage( + fmt.Sprintf("Template failed to run script %v and the task is being killed", script.Path), )) - if script.FailOnError { - tm.config.Lifecycle.Kill(context.Background(), - structs.NewTaskEvent(structs.TaskKilling). - SetFailsTask(). - SetDisplayMessage( - fmt.Sprintf("Template failed to run script %v and the task is being killed", script.Path), - )) - } - } else { - tm.config.Events.EmitEvent(structs.NewTaskEvent(structs.TaskHookMessage). - SetDisplayMessage( - fmt.Sprintf( - "Template successfully ran a script from %v with exit code: %v", script.Path, exitCode, - ))) } + } else { + tm.config.Events.EmitEvent(structs.NewTaskEvent(structs.TaskHookMessage). + SetDisplayMessage( + fmt.Sprintf( + "Template successfully ran a script from %v with exit code: %v", script.Path, exitCode, + ))) } } - } // allTemplatesNoop returns whether all the managed templates have change mode noop. From 675a66753baa3913f97e1cd50249c9fffa119966 Mon Sep 17 00:00:00 2001 From: Peter Kazmierczak Date: Tue, 16 Aug 2022 10:12:14 +0200 Subject: [PATCH 38/68] set poststart for template hook --- .../taskrunner/template/template.go | 51 +++++++++++++------ .../taskrunner/template/template_test.go | 6 +-- .../allocrunner/taskrunner/template_hook.go | 8 ++- 3 files changed, 42 insertions(+), 23 deletions(-) diff --git a/client/allocrunner/taskrunner/template/template.go b/client/allocrunner/taskrunner/template/template.go index 076e5fca9132..515629fb5a66 100644 --- a/client/allocrunner/taskrunner/template/template.go +++ b/client/allocrunner/taskrunner/template/template.go @@ -54,6 +54,9 @@ type TaskTemplateManager struct { // runner is the consul-template runner runner *manager.Runner + // handle is used to execute scripts + handle interfaces.ScriptExecutor + // signals is a lookup map from the string representation of a signal to its // actual signal signals map[string]os.Signal @@ -66,6 +69,11 @@ type TaskTemplateManager struct { shutdownLock sync.Mutex } +// SetDriverHandle sets the executor +func (ttm *TaskTemplateManager) SetDriverHandle(executor interfaces.ScriptExecutor) { + ttm.handle = executor +} + // TaskTemplateManagerConfig is used to configure an instance of the // TaskTemplateManager type TaskTemplateManagerConfig struct { @@ -108,9 +116,6 @@ type TaskTemplateManagerConfig struct { // NomadToken is the Nomad token or identity claim for the task NomadToken string - - // Handle is used to execute scripts - Handle interfaces.ScriptExecutor } // Validate validates the configuration. @@ -500,13 +505,33 @@ func (tm *TaskTemplateManager) onTemplateRendered(handledRenders map[string]time } } for _, script := range scripts { - _, exitCode, err := tm.config.Handle.Exec(script.Timeout, script.Path, script.Args) - if err != nil { - structs.NewTaskEvent(structs.TaskHookFailed). - SetDisplayMessage( - fmt.Sprintf( - "Template failed to run script %v on change: %v Exit code: %v", script.Path, err, exitCode, - )) + if tm.handle != nil { + _, exitCode, err := tm.handle.Exec(script.Timeout, script.Path, script.Args) + if err != nil { + structs.NewTaskEvent(structs.TaskHookFailed). + SetDisplayMessage( + fmt.Sprintf( + "Template failed to run script %v on change: %v Exit code: %v", script.Path, err, exitCode, + )) + if script.FailOnError { + tm.config.Lifecycle.Kill(context.Background(), + structs.NewTaskEvent(structs.TaskKilling). + SetFailsTask(). + SetDisplayMessage( + fmt.Sprintf("Template failed to run script %v and the task is being killed", script.Path), + )) + } + } else { + tm.config.Events.EmitEvent(structs.NewTaskEvent(structs.TaskHookMessage). + SetDisplayMessage( + fmt.Sprintf( + "Template successfully ran a script from %v with exit code: %v", script.Path, exitCode, + ))) + } + } else { + tm.config.Events.EmitEvent(structs.NewTaskEvent(structs.TaskHookFailed). + SetDisplayMessage("Template failed to run a script: task is lacking a driver"), + ) if script.FailOnError { tm.config.Lifecycle.Kill(context.Background(), structs.NewTaskEvent(structs.TaskKilling). @@ -515,12 +540,6 @@ func (tm *TaskTemplateManager) onTemplateRendered(handledRenders map[string]time fmt.Sprintf("Template failed to run script %v and the task is being killed", script.Path), )) } - } else { - tm.config.Events.EmitEvent(structs.NewTaskEvent(structs.TaskHookMessage). - SetDisplayMessage( - fmt.Sprintf( - "Template successfully ran a script from %v with exit code: %v", script.Path, exitCode, - ))) } } } diff --git a/client/allocrunner/taskrunner/template/template_test.go b/client/allocrunner/taskrunner/template/template_test.go index c69f9743aa50..00c52ff24e62 100644 --- a/client/allocrunner/taskrunner/template/template_test.go +++ b/client/allocrunner/taskrunner/template/template_test.go @@ -223,9 +223,9 @@ func (h *testHarness) startWithErr() error { VaultToken: h.vaultToken, TaskDir: h.taskDir, EnvBuilder: h.envBuilder, - Handle: h.driver, MaxTemplateEventRate: h.emitRate, }) + h.manager.SetDriverHandle(h.driver) return err } @@ -1219,7 +1219,6 @@ func TestTaskTemplateManager_ScriptExecution(t *testing.T) { // Make a template that renders based on a key in Consul and triggers script key1 := "bam" - key2 := "bar" content1_1 := "cat" content1_2 := "dog" t1 := &structs.Template{ @@ -1266,7 +1265,6 @@ BAR={{key "bar"}} // Write the key to Consul harness.consul.SetKV(t, key1, []byte(content1_1)) - harness.consul.SetKV(t, key2, []byte(content1_1)) // Wait for the unblock select { @@ -1305,7 +1303,6 @@ func TestTaskTemplateManager_ScriptExecutionFailTask(t *testing.T) { // Make a template that renders based on a key in Consul and triggers script key1 := "bam" - key2 := "bar" content1_1 := "cat" content1_2 := "dog" t1 := &structs.Template{ @@ -1352,7 +1349,6 @@ BAR={{key "bar"}} // Write the key to Consul harness.consul.SetKV(t, key1, []byte(content1_1)) - harness.consul.SetKV(t, key2, []byte(content1_1)) // Wait for the unblock select { diff --git a/client/allocrunner/taskrunner/template_hook.go b/client/allocrunner/taskrunner/template_hook.go index 74f264e281dc..184f7a2b4de0 100644 --- a/client/allocrunner/taskrunner/template_hook.go +++ b/client/allocrunner/taskrunner/template_hook.go @@ -44,7 +44,7 @@ type templateHookConfig struct { nomadNamespace string // handle is the driver handle that allows driver operations - handle *DriverHandle + handle ti.ScriptExecutor } type templateHook struct { @@ -118,6 +118,11 @@ func (h *templateHook) Prestart(ctx context.Context, req *interfaces.TaskPrestar return nil } +func (h *templateHook) Poststart(ctx context.Context, req *interfaces.TaskPoststartRequest, resp *interfaces.TaskPoststartResponse) error { + h.templateManager.SetDriverHandle(req.DriverExec) + return nil +} + func (h *templateHook) newManager() (unblock chan struct{}, err error) { unblock = make(chan struct{}) m, err := template.NewTaskTemplateManager(&template.TaskTemplateManagerConfig{ @@ -134,7 +139,6 @@ func (h *templateHook) newManager() (unblock chan struct{}, err error) { MaxTemplateEventRate: template.DefaultMaxTemplateEventRate, NomadNamespace: h.config.nomadNamespace, NomadToken: h.nomadToken, - Handle: h.config.handle, }) if err != nil { h.logger.Error("failed to create template manager", "error", err) From ce6f351d5b727ff14546badd0e5eb3d7136103a3 Mon Sep 17 00:00:00 2001 From: Peter Kazmierczak Date: Tue, 16 Aug 2022 10:31:09 +0200 Subject: [PATCH 39/68] nil check --- client/allocrunner/taskrunner/template_hook.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/client/allocrunner/taskrunner/template_hook.go b/client/allocrunner/taskrunner/template_hook.go index 184f7a2b4de0..c6d24c4001d1 100644 --- a/client/allocrunner/taskrunner/template_hook.go +++ b/client/allocrunner/taskrunner/template_hook.go @@ -119,7 +119,9 @@ func (h *templateHook) Prestart(ctx context.Context, req *interfaces.TaskPrestar } func (h *templateHook) Poststart(ctx context.Context, req *interfaces.TaskPoststartRequest, resp *interfaces.TaskPoststartResponse) error { - h.templateManager.SetDriverHandle(req.DriverExec) + if req.DriverExec != nil { + h.templateManager.SetDriverHandle(req.DriverExec) + } return nil } From 9c250954241cdc04743a64a2664c8cd77ab21e40 Mon Sep 17 00:00:00 2001 From: Peter Kazmierczak Date: Tue, 16 Aug 2022 09:59:51 +0000 Subject: [PATCH 40/68] fixed test --- client/allocrunner/taskrunner/template/template_test.go | 2 -- client/allocrunner/taskrunner/template_hook.go | 3 --- 2 files changed, 5 deletions(-) diff --git a/client/allocrunner/taskrunner/template/template_test.go b/client/allocrunner/taskrunner/template/template_test.go index 00c52ff24e62..245c5b5225dd 100644 --- a/client/allocrunner/taskrunner/template/template_test.go +++ b/client/allocrunner/taskrunner/template/template_test.go @@ -225,8 +225,6 @@ func (h *testHarness) startWithErr() error { EnvBuilder: h.envBuilder, MaxTemplateEventRate: h.emitRate, }) - h.manager.SetDriverHandle(h.driver) - return err } diff --git a/client/allocrunner/taskrunner/template_hook.go b/client/allocrunner/taskrunner/template_hook.go index c6d24c4001d1..bfad11bc5785 100644 --- a/client/allocrunner/taskrunner/template_hook.go +++ b/client/allocrunner/taskrunner/template_hook.go @@ -42,9 +42,6 @@ type templateHookConfig struct { // nomadNamespace is the job's Nomad namespace nomadNamespace string - - // handle is the driver handle that allows driver operations - handle ti.ScriptExecutor } type templateHook struct { From 13b6f7595a515a9fe06f65b8b0d842348f1441cd Mon Sep 17 00:00:00 2001 From: Peter Kazmierczak Date: Tue, 16 Aug 2022 17:02:17 +0000 Subject: [PATCH 41/68] fixed tests --- client/allocrunner/taskrunner/template/template.go | 10 +++++----- .../allocrunner/taskrunner/template/template_test.go | 12 +++++++----- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/client/allocrunner/taskrunner/template/template.go b/client/allocrunner/taskrunner/template/template.go index 515629fb5a66..7f62f7138073 100644 --- a/client/allocrunner/taskrunner/template/template.go +++ b/client/allocrunner/taskrunner/template/template.go @@ -69,11 +69,6 @@ type TaskTemplateManager struct { shutdownLock sync.Mutex } -// SetDriverHandle sets the executor -func (ttm *TaskTemplateManager) SetDriverHandle(executor interfaces.ScriptExecutor) { - ttm.handle = executor -} - // TaskTemplateManagerConfig is used to configure an instance of the // TaskTemplateManager type TaskTemplateManagerConfig struct { @@ -200,6 +195,11 @@ func (tm *TaskTemplateManager) Stop() { } } +// SetDriverHandle sets the executor +func (tm *TaskTemplateManager) SetDriverHandle(executor interfaces.ScriptExecutor) { + tm.handle = executor +} + // run is the long lived loop that handles errors and templates being rendered func (tm *TaskTemplateManager) run() { // Runner is nil if there are no templates diff --git a/client/allocrunner/taskrunner/template/template_test.go b/client/allocrunner/taskrunner/template/template_test.go index 245c5b5225dd..3c11a96ea98d 100644 --- a/client/allocrunner/taskrunner/template/template_test.go +++ b/client/allocrunner/taskrunner/template/template_test.go @@ -24,7 +24,6 @@ import ( ctestutil "github.com/hashicorp/consul/sdk/testutil" "github.com/hashicorp/nomad/ci" "github.com/hashicorp/nomad/client/allocdir" - "github.com/hashicorp/nomad/client/allocrunner/taskrunner/interfaces" "github.com/hashicorp/nomad/client/config" "github.com/hashicorp/nomad/client/taskenv" "github.com/hashicorp/nomad/helper" @@ -149,7 +148,6 @@ type testHarness struct { consul *ctestutil.TestServer emitRate time.Duration nomadNamespace string - driver interfaces.ScriptExecutor } // newTestHarness returns a harness starting a dev consul and vault server, @@ -1217,6 +1215,7 @@ func TestTaskTemplateManager_ScriptExecution(t *testing.T) { // Make a template that renders based on a key in Consul and triggers script key1 := "bam" + key2 := "bar" content1_1 := "cat" content1_2 := "dog" t1 := &structs.Template{ @@ -1248,10 +1247,10 @@ BAR={{key "bar"}} Envvars: true, } - me := mockExecutor{} + me := mockExecutor{DesiredExit: 0, DesiredErr: nil} harness := newTestHarness(t, []*structs.Template{t1, t2}, true, false) - harness.driver = &me harness.start(t) + harness.manager.SetDriverHandle(&me) defer harness.stop() // Ensure no unblock @@ -1263,6 +1262,7 @@ BAR={{key "bar"}} // Write the key to Consul harness.consul.SetKV(t, key1, []byte(content1_1)) + harness.consul.SetKV(t, key2, []byte(content1_1)) // Wait for the unblock select { @@ -1301,6 +1301,7 @@ func TestTaskTemplateManager_ScriptExecutionFailTask(t *testing.T) { // Make a template that renders based on a key in Consul and triggers script key1 := "bam" + key2 := "bar" content1_1 := "cat" content1_2 := "dog" t1 := &structs.Template{ @@ -1334,8 +1335,8 @@ BAR={{key "bar"}} me := mockExecutor{DesiredExit: 1, DesiredErr: fmt.Errorf("Script failed")} harness := newTestHarness(t, []*structs.Template{t1, t2}, true, false) - harness.driver = &me harness.start(t) + harness.manager.SetDriverHandle(&me) defer harness.stop() // Ensure no unblock @@ -1347,6 +1348,7 @@ BAR={{key "bar"}} // Write the key to Consul harness.consul.SetKV(t, key1, []byte(content1_1)) + harness.consul.SetKV(t, key2, []byte(content1_1)) // Wait for the unblock select { From 2266453a37f17b715d1451974f18a26532b1c353 Mon Sep 17 00:00:00 2001 From: Piotr Kazmierczak Date: Thu, 18 Aug 2022 13:37:22 +0200 Subject: [PATCH 42/68] Update website/content/api-docs/json-jobs.mdx Co-authored-by: Luiz Aoqui --- website/content/api-docs/json-jobs.mdx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/website/content/api-docs/json-jobs.mdx b/website/content/api-docs/json-jobs.mdx index 40eef6946b6a..1e227e0d6d36 100644 --- a/website/content/api-docs/json-jobs.mdx +++ b/website/content/api-docs/json-jobs.mdx @@ -1077,7 +1077,7 @@ README][ct]. - `fail_on_error` - If `true`, Nomad will kill the task if the script execution fails. If `false`, script failure will be logged but the task will - continue uninterrupted. + continue uninterrupted. Default value is `false`. - `DestPath` - Specifies the location where the resulting template should be rendered, relative to the task directory. From 010d8a1aea6c5f0b804947dfd205344ef96b3e69 Mon Sep 17 00:00:00 2001 From: Piotr Kazmierczak Date: Thu, 18 Aug 2022 13:37:49 +0200 Subject: [PATCH 43/68] Update website/content/api-docs/json-jobs.mdx Co-authored-by: Luiz Aoqui --- website/content/api-docs/json-jobs.mdx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/website/content/api-docs/json-jobs.mdx b/website/content/api-docs/json-jobs.mdx index 1e227e0d6d36..45e6be3238ab 100644 --- a/website/content/api-docs/json-jobs.mdx +++ b/website/content/api-docs/json-jobs.mdx @@ -1073,7 +1073,7 @@ README][ct]. executed on template change. - `timeout` - Timeout for script execution specified using a label suffix like - "30s" or "1h". + "30s" or "1h". Default value is `"5s"`. - `fail_on_error` - If `true`, Nomad will kill the task if the script execution fails. If `false`, script failure will be logged but the task will From ee50b9d3315bef607f688ce9500bcb92545d6363 Mon Sep 17 00:00:00 2001 From: Piotr Kazmierczak Date: Thu, 18 Aug 2022 13:38:14 +0200 Subject: [PATCH 44/68] Update website/content/api-docs/json-jobs.mdx Co-authored-by: Luiz Aoqui --- website/content/api-docs/json-jobs.mdx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/website/content/api-docs/json-jobs.mdx b/website/content/api-docs/json-jobs.mdx index 45e6be3238ab..4e0b6dffc776 100644 --- a/website/content/api-docs/json-jobs.mdx +++ b/website/content/api-docs/json-jobs.mdx @@ -1062,7 +1062,7 @@ README][ct]. `signal`. - `ChangeScriptConfig` - Configures the script triggered on template change. - This option is required if the `change_mode` is `script`. + This option is required if the `ChangeMode` is `script`. The `ChangeScriptConfig` object supports the following attributes: From de7156f3cda98eada64cf9667b09fd9104193252 Mon Sep 17 00:00:00 2001 From: Piotr Kazmierczak Date: Thu, 18 Aug 2022 14:05:13 +0200 Subject: [PATCH 45/68] "path" documentation --- website/content/api-docs/json-jobs.mdx | 4 +++- .../content/docs/job-specification/change_script_config.mdx | 5 +++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/website/content/api-docs/json-jobs.mdx b/website/content/api-docs/json-jobs.mdx index 4e0b6dffc776..e3114e699411 100644 --- a/website/content/api-docs/json-jobs.mdx +++ b/website/content/api-docs/json-jobs.mdx @@ -1067,7 +1067,9 @@ README][ct]. The `ChangeScriptConfig` object supports the following attributes: - `path` - Specifies the full path to a script that is to be executed on - template change. This option is required is the `change_mode` is `script`. + template change. Path is relative to the driver, e.g., if running with a + container driver the path must be existing in the container. This option is + required is the `change_mode` is `script`. - `args` - List of arguments that are passed to the script that is to be executed on template change. diff --git a/website/content/docs/job-specification/change_script_config.mdx b/website/content/docs/job-specification/change_script_config.mdx index bb482af2f02f..be3f2ecd5dfd 100644 --- a/website/content/docs/job-specification/change_script_config.mdx +++ b/website/content/docs/job-specification/change_script_config.mdx @@ -35,8 +35,9 @@ job "docs" { ## `change_script_config` Parameters - `path` `(string: "")` - Specifies the full path to a script that - is to be executed on template change. This option is required is the - `change_mode` is `script`. + is to be executed on template change. Path is relative to the driver, e.g., if + running with a container driver the path must be existing in the container. + This option is required is the `change_mode` is `script`. - `args` `(array: [])` - List of arguments that are passed to the script that is to be executed on template change. From 726397cf3987c9420e63e06fa5e1ab4d7d95871a Mon Sep 17 00:00:00 2001 From: Piotr Kazmierczak Date: Thu, 18 Aug 2022 14:16:47 +0200 Subject: [PATCH 46/68] fail task in the poststart if template change mode is script and driver can't exec --- client/allocrunner/taskrunner/template/template.go | 2 +- client/allocrunner/taskrunner/template_hook.go | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/client/allocrunner/taskrunner/template/template.go b/client/allocrunner/taskrunner/template/template.go index f8bec9e67949..076f39cb1e89 100644 --- a/client/allocrunner/taskrunner/template/template.go +++ b/client/allocrunner/taskrunner/template/template.go @@ -530,7 +530,7 @@ func (tm *TaskTemplateManager) onTemplateRendered(handledRenders map[string]time } } else { tm.config.Events.EmitEvent(structs.NewTaskEvent(structs.TaskHookFailed). - SetDisplayMessage("Template failed to run a script: task is lacking a driver"), + SetDisplayMessage("Template failed to run a change script because task driver doesn't support the exec operation"), ) if script.FailOnError { tm.config.Lifecycle.Kill(context.Background(), diff --git a/client/allocrunner/taskrunner/template_hook.go b/client/allocrunner/taskrunner/template_hook.go index bfad11bc5785..4e14fcc43f2a 100644 --- a/client/allocrunner/taskrunner/template_hook.go +++ b/client/allocrunner/taskrunner/template_hook.go @@ -118,6 +118,12 @@ func (h *templateHook) Prestart(ctx context.Context, req *interfaces.TaskPrestar func (h *templateHook) Poststart(ctx context.Context, req *interfaces.TaskPoststartRequest, resp *interfaces.TaskPoststartResponse) error { if req.DriverExec != nil { h.templateManager.SetDriverHandle(req.DriverExec) + } else { + for _, template := range h.config.templates { + if template.ChangeMode == structs.TemplateChangeModeScript { + return fmt.Errorf("template has change mode set to 'script' but the driver it uses does not provide exec capability") + } + } } return nil } From 67eda32c65cdd8d207f11882cc2272f54fced895 Mon Sep 17 00:00:00 2001 From: Piotr Kazmierczak Date: Thu, 18 Aug 2022 14:31:14 +0200 Subject: [PATCH 47/68] change script config -> change script --- api/tasks.go | 44 +++++++++---------- .../taskrunner/template/template.go | 4 +- .../taskrunner/template/template_test.go | 10 ++--- command/agent/job_endpoint.go | 44 +++++++++---------- command/agent/job_endpoint_test.go | 18 ++++---- jobspec/parse_task.go | 22 +++++----- jobspec/parse_test.go | 2 +- jobspec/test-fixtures/basic.hcl | 2 +- jobspec2/hcl_conversions.go | 3 +- jobspec2/parse_job.go | 43 +++++------------- jobspec2/parse_test.go | 31 ++++++------- nomad/structs/diff.go | 19 ++++---- nomad/structs/diff_test.go | 12 ++--- nomad/structs/structs.go | 10 ++--- website/content/api-docs/json-jobs.mdx | 6 +-- ...ge_script_config.mdx => change_script.mdx} | 14 +++--- .../docs/job-specification/template.mdx | 8 ++-- website/data/docs-nav-data.json | 4 +- 18 files changed, 139 insertions(+), 157 deletions(-) rename website/content/docs/job-specification/{change_script_config.mdx => change_script.mdx} (78%) diff --git a/api/tasks.go b/api/tasks.go index 8e34266643c1..5d47e9ad04d4 100644 --- a/api/tasks.go +++ b/api/tasks.go @@ -791,44 +791,44 @@ func (wc *WaitConfig) Copy() *WaitConfig { return nwc } -type ChangeScriptConfig struct { +type ChangeScript struct { Path *string `mapstructure:"path" hcl:"path"` Args []string `mapstructure:"args" hcl:"args,optional"` Timeout *time.Duration `mapstructure:"timeout" hcl:"timeout,optional"` FailOnError *bool `mapstructure:"fail_on_error" hcl:"fail_on_error"` } -func (ch *ChangeScriptConfig) Canonicalize() { +func (ch *ChangeScript) Canonicalize() { if ch.Path == nil { - ch.Path = stringToPtr("") + ch.Path = pointerOf("") } if ch.Args == nil { ch.Args = []string{} } if ch.Timeout == nil { - ch.Timeout = timeToPtr(5 * time.Second) + ch.Timeout = pointerOf(5 * time.Second) } if ch.FailOnError == nil { - ch.FailOnError = boolToPtr(false) + ch.FailOnError = pointerOf(false) } } type Template struct { - SourcePath *string `mapstructure:"source" hcl:"source,optional"` - DestPath *string `mapstructure:"destination" hcl:"destination,optional"` - EmbeddedTmpl *string `mapstructure:"data" hcl:"data,optional"` - ChangeMode *string `mapstructure:"change_mode" hcl:"change_mode,optional"` - ChangeScriptConfig *ChangeScriptConfig `mapstructure:"change_script_config" hcl:"change_script_config,block"` - ChangeSignal *string `mapstructure:"change_signal" hcl:"change_signal,optional"` - Splay *time.Duration `mapstructure:"splay" hcl:"splay,optional"` - Perms *string `mapstructure:"perms" hcl:"perms,optional"` - Uid *int `mapstructure:"uid" hcl:"uid,optional"` - Gid *int `mapstructure:"gid" hcl:"gid,optional"` - LeftDelim *string `mapstructure:"left_delimiter" hcl:"left_delimiter,optional"` - RightDelim *string `mapstructure:"right_delimiter" hcl:"right_delimiter,optional"` - Envvars *bool `mapstructure:"env" hcl:"env,optional"` - VaultGrace *time.Duration `mapstructure:"vault_grace" hcl:"vault_grace,optional"` - Wait *WaitConfig `mapstructure:"wait" hcl:"wait,block"` + SourcePath *string `mapstructure:"source" hcl:"source,optional"` + DestPath *string `mapstructure:"destination" hcl:"destination,optional"` + EmbeddedTmpl *string `mapstructure:"data" hcl:"data,optional"` + ChangeMode *string `mapstructure:"change_mode" hcl:"change_mode,optional"` + ChangeScript *ChangeScript `mapstructure:"change_script" hcl:"change_script,block"` + ChangeSignal *string `mapstructure:"change_signal" hcl:"change_signal,optional"` + Splay *time.Duration `mapstructure:"splay" hcl:"splay,optional"` + Perms *string `mapstructure:"perms" hcl:"perms,optional"` + Uid *int `mapstructure:"uid" hcl:"uid,optional"` + Gid *int `mapstructure:"gid" hcl:"gid,optional"` + LeftDelim *string `mapstructure:"left_delimiter" hcl:"left_delimiter,optional"` + RightDelim *string `mapstructure:"right_delimiter" hcl:"right_delimiter,optional"` + Envvars *bool `mapstructure:"env" hcl:"env,optional"` + VaultGrace *time.Duration `mapstructure:"vault_grace" hcl:"vault_grace,optional"` + Wait *WaitConfig `mapstructure:"wait" hcl:"wait,block"` } func (tmpl *Template) Canonicalize() { @@ -854,8 +854,8 @@ func (tmpl *Template) Canonicalize() { sig := *tmpl.ChangeSignal tmpl.ChangeSignal = pointerOf(strings.ToUpper(sig)) } - if tmpl.ChangeScriptConfig != nil { - tmpl.ChangeScriptConfig.Canonicalize() + if tmpl.ChangeScript != nil { + tmpl.ChangeScript.Canonicalize() } if tmpl.Splay == nil { tmpl.Splay = pointerOf(5 * time.Second) diff --git a/client/allocrunner/taskrunner/template/template.go b/client/allocrunner/taskrunner/template/template.go index 076f39cb1e89..a0b39fc86f74 100644 --- a/client/allocrunner/taskrunner/template/template.go +++ b/client/allocrunner/taskrunner/template/template.go @@ -400,7 +400,7 @@ func (tm *TaskTemplateManager) onTemplateRendered(handledRenders map[string]time var handling []string signals := make(map[string]struct{}) - scripts := []*structs.ChangeScriptConfig{} + scripts := []*structs.ChangeScript{} restart := false var splay time.Duration @@ -446,7 +446,7 @@ func (tm *TaskTemplateManager) onTemplateRendered(handledRenders map[string]time case structs.TemplateChangeModeRestart: restart = true case structs.TemplateChangeModeScript: - scripts = append(scripts, tmpl.ChangeScriptConfig) + scripts = append(scripts, tmpl.ChangeScript) case structs.TemplateChangeModeNoop: continue } diff --git a/client/allocrunner/taskrunner/template/template_test.go b/client/allocrunner/taskrunner/template/template_test.go index b5a8ddefea30..86cac38ba04a 100644 --- a/client/allocrunner/taskrunner/template/template_test.go +++ b/client/allocrunner/taskrunner/template/template_test.go @@ -1224,7 +1224,7 @@ FOO={{key "bam"}} `, DestPath: "test.env", ChangeMode: structs.TemplateChangeModeScript, - ChangeScriptConfig: &structs.ChangeScriptConfig{ + ChangeScript: &structs.ChangeScript{ Path: "/bin/foo", Args: []string{}, Timeout: 5 * time.Second, @@ -1238,7 +1238,7 @@ BAR={{key "bar"}} `, DestPath: "test2.env", ChangeMode: structs.TemplateChangeModeScript, - ChangeScriptConfig: &structs.ChangeScriptConfig{ + ChangeScript: &structs.ChangeScript{ Path: "/bin/foo", Args: []string{}, Timeout: 5 * time.Second, @@ -1282,7 +1282,7 @@ OUTER: case <-harness.mockHooks.RestartCh: require.Fail(t, "restart not expected") case ev := <-harness.mockHooks.EmitEventCh: - if strings.Contains(ev.DisplayMessage, t1.ChangeScriptConfig.Path) { + if strings.Contains(ev.DisplayMessage, t1.ChangeScript.Path) { break OUTER } case <-harness.mockHooks.SignalCh: @@ -1310,7 +1310,7 @@ FOO={{key "bam"}} `, DestPath: "test.env", ChangeMode: structs.TemplateChangeModeScript, - ChangeScriptConfig: &structs.ChangeScriptConfig{ + ChangeScript: &structs.ChangeScript{ Path: "/bin/foo", Args: []string{}, Timeout: 5 * time.Second, @@ -1324,7 +1324,7 @@ BAR={{key "bar"}} `, DestPath: "test2.env", ChangeMode: structs.TemplateChangeModeScript, - ChangeScriptConfig: &structs.ChangeScriptConfig{ + ChangeScript: &structs.ChangeScript{ Path: "/bin/foo", Args: []string{}, Timeout: 5 * time.Second, diff --git a/command/agent/job_endpoint.go b/command/agent/job_endpoint.go index 89c53c19f6e2..4f5bdba7c331 100644 --- a/command/agent/job_endpoint.go +++ b/command/agent/job_endpoint.go @@ -1219,21 +1219,21 @@ func ApiTaskToStructsTask(job *structs.Job, group *structs.TaskGroup, } structsTask.Templates = append(structsTask.Templates, &structs.Template{ - SourcePath: *template.SourcePath, - DestPath: *template.DestPath, - EmbeddedTmpl: *template.EmbeddedTmpl, - ChangeMode: *template.ChangeMode, - ChangeSignal: *template.ChangeSignal, - ChangeScriptConfig: apiChangeScriptConfigToStructsChangeScriptConfig(template.ChangeScriptConfig), - Splay: *template.Splay, - Perms: *template.Perms, - Uid: uid, - Gid: gid, - LeftDelim: *template.LeftDelim, - RightDelim: *template.RightDelim, - Envvars: *template.Envvars, - VaultGrace: *template.VaultGrace, - Wait: apiWaitConfigToStructsWaitConfig(template.Wait), + SourcePath: *template.SourcePath, + DestPath: *template.DestPath, + EmbeddedTmpl: *template.EmbeddedTmpl, + ChangeMode: *template.ChangeMode, + ChangeSignal: *template.ChangeSignal, + ChangeScript: apiChangeScriptToStructsChangeScript(template.ChangeScript), + Splay: *template.Splay, + Perms: *template.Perms, + Uid: uid, + Gid: gid, + LeftDelim: *template.LeftDelim, + RightDelim: *template.RightDelim, + Envvars: *template.Envvars, + VaultGrace: *template.VaultGrace, + Wait: apiWaitConfigToStructsWaitConfig(template.Wait), }) } } @@ -1265,16 +1265,16 @@ func apiWaitConfigToStructsWaitConfig(waitConfig *api.WaitConfig) *structs.WaitC } } -func apiChangeScriptConfigToStructsChangeScriptConfig(changeScriptConfig *api.ChangeScriptConfig) *structs.ChangeScriptConfig { - if changeScriptConfig == nil { +func apiChangeScriptToStructsChangeScript(changeScript *api.ChangeScript) *structs.ChangeScript { + if changeScript == nil { return nil } - return &structs.ChangeScriptConfig{ - Path: *changeScriptConfig.Path, - Args: changeScriptConfig.Args, - Timeout: *changeScriptConfig.Timeout, - FailOnError: *changeScriptConfig.FailOnError, + return &structs.ChangeScript{ + Path: *changeScript.Path, + Args: changeScript.Args, + Timeout: *changeScript.Timeout, + FailOnError: *changeScript.FailOnError, } } diff --git a/command/agent/job_endpoint_test.go b/command/agent/job_endpoint_test.go index b6c1a0fac0e2..c936421a597d 100644 --- a/command/agent/job_endpoint_test.go +++ b/command/agent/job_endpoint_test.go @@ -2731,19 +2731,19 @@ func TestJobs_ApiJobToStructsJob(t *testing.T) { EmbeddedTmpl: pointer.Of("embedded"), ChangeMode: pointer.Of("change"), ChangeSignal: pointer.Of("signal"), - ChangeScriptConfig: &api.ChangeScriptConfig{ + ChangeScript: &api.ChangeScript{ Path: pointer.Of("/bin/foo"), Args: []string{"-h"}, Timeout: pointer.Of(5 * time.Second), FailOnError: pointer.Of(false), }, - Splay: pointer.Of(1 * time.Minute), - Perms: pointer.Of("666"), - Uid: pointer.Of(1000), - Gid: pointer.Of(1000), - LeftDelim: pointer.Of("abc"), - RightDelim: pointer.Of("def"), - Envvars: pointer.Of(true), + Splay: pointer.Of(1 * time.Minute), + Perms: pointer.Of("666"), + Uid: pointer.Of(1000), + Gid: pointer.Of(1000), + LeftDelim: pointer.Of("abc"), + RightDelim: pointer.Of("def"), + Envvars: pointer.Of(true), Wait: &api.WaitConfig{ Min: pointer.Of(5 * time.Second), Max: pointer.Of(10 * time.Second), @@ -3144,7 +3144,7 @@ func TestJobs_ApiJobToStructsJob(t *testing.T) { EmbeddedTmpl: "embedded", ChangeMode: "change", ChangeSignal: "SIGNAL", - ChangeScriptConfig: &structs.ChangeScriptConfig{ + ChangeScript: &structs.ChangeScript{ Path: "/bin/foo", Args: []string{"-h"}, Timeout: 5 * time.Second, diff --git a/jobspec/parse_task.go b/jobspec/parse_task.go index 01eb65f5e418..f6933778a4b1 100644 --- a/jobspec/parse_task.go +++ b/jobspec/parse_task.go @@ -445,7 +445,7 @@ func parseTemplates(result *[]*api.Template, list *ast.ObjectList) error { valid := []string{ "change_mode", "change_signal", - "change_script_config", + "change_script", "data", "destination", "left_delimiter", @@ -466,7 +466,7 @@ func parseTemplates(result *[]*api.Template, list *ast.ObjectList) error { if err := hcl.DecodeObject(&m, o.Val); err != nil { return err } - delete(m, "change_script_config") // change_script_config is its own object + delete(m, "change_script") // change_script is its own object templ := &api.Template{ ChangeMode: stringToPtr("restart"), @@ -488,31 +488,31 @@ func parseTemplates(result *[]*api.Template, list *ast.ObjectList) error { return err } - // If we have change_script_config, parse it - if o := listVal.Filter("change_script_config"); len(o.Items) > 0 { + // If we have change_script, parse it + if o := listVal.Filter("change_script"); len(o.Items) > 0 { if len(o.Items) != 1 { return fmt.Errorf( - "change_script_config -> expected single stanza, got %d", len(o.Items), + "change_script -> expected single stanza, got %d", len(o.Items), ) } var m map[string]interface{} - changeScriptConfigBlock := o.Items[0] + changeScriptBlock := o.Items[0] // check for invalid fields valid := []string{"path", "args", "timeout", "fail_on_error"} - if err := checkHCLKeys(changeScriptConfigBlock.Val, valid); err != nil { - return multierror.Prefix(err, "change_script_config ->") + if err := checkHCLKeys(changeScriptBlock.Val, valid); err != nil { + return multierror.Prefix(err, "change_script ->") } - if err := hcl.DecodeObject(&m, changeScriptConfigBlock.Val); err != nil { + if err := hcl.DecodeObject(&m, changeScriptBlock.Val); err != nil { return err } - templ.ChangeScriptConfig = &api.ChangeScriptConfig{} + templ.ChangeScript = &api.ChangeScript{} dec, err := mapstructure.NewDecoder(&mapstructure.DecoderConfig{ DecodeHook: mapstructure.StringToTimeDurationHookFunc(), WeaklyTypedInput: true, - Result: templ.ChangeScriptConfig, + Result: templ.ChangeScript, }) if err != nil { return err diff --git a/jobspec/parse_test.go b/jobspec/parse_test.go index fa095e3dbffa..e335cc56701c 100644 --- a/jobspec/parse_test.go +++ b/jobspec/parse_test.go @@ -387,7 +387,7 @@ func TestParse(t *testing.T) { SourcePath: stringToPtr("bar"), DestPath: stringToPtr("bar"), ChangeMode: stringToPtr(templateChangeModeScript), - ChangeScriptConfig: &api.ChangeScriptConfig{ + ChangeScript: &api.ChangeScript{ Args: []string{"-debug", "-verbose"}, Path: stringToPtr("/bin/foo"), Timeout: timeToPtr(5 * time.Second), diff --git a/jobspec/test-fixtures/basic.hcl b/jobspec/test-fixtures/basic.hcl index aae539b69b46..50b21ce68eab 100644 --- a/jobspec/test-fixtures/basic.hcl +++ b/jobspec/test-fixtures/basic.hcl @@ -318,7 +318,7 @@ job "binstore-storagelocker" { source = "bar" destination = "bar" change_mode = "script" - change_script_config { + change_script { path = "/bin/foo" args = ["-debug", "-verbose"] timeout = "5s" diff --git a/jobspec2/hcl_conversions.go b/jobspec2/hcl_conversions.go index 423b30924da2..2afd71ed2b75 100644 --- a/jobspec2/hcl_conversions.go +++ b/jobspec2/hcl_conversions.go @@ -10,6 +10,7 @@ import ( "github.com/hashicorp/hcl/v2/gohcl" "github.com/hashicorp/hcl/v2/hcldec" "github.com/hashicorp/nomad/api" + "github.com/hashicorp/nomad/helper/pointer" "github.com/zclconf/go-cty/cty" "github.com/zclconf/go-cty/cty/gocty" ) @@ -116,7 +117,7 @@ func decodeAffinity(body hcl.Body, ctx *hcl.EvalContext, val interface{}) hcl.Di weight := v.GetAttr("weight") if !weight.IsNull() { w, _ := weight.AsBigFloat().Int64() - a.Weight = int8ToPtr(int8(w)) + a.Weight = pointer.Of(int8(w)) } // If "version" is provided, set the operand diff --git a/jobspec2/parse_job.go b/jobspec2/parse_job.go index 2fc0e5bdfb61..dcacfa457f65 100644 --- a/jobspec2/parse_job.go +++ b/jobspec2/parse_job.go @@ -4,6 +4,7 @@ import ( "time" "github.com/hashicorp/nomad/api" + "github.com/hashicorp/nomad/helper/pointer" ) func normalizeJob(jc *jobConfig) { @@ -59,10 +60,10 @@ func normalizeVault(v *api.Vault) { } if v.Env == nil { - v.Env = boolToPtr(true) + v.Env = pointer.Of(true) } if v.ChangeMode == nil { - v.ChangeMode = stringToPtr("restart") + v.ChangeMode = pointer.Of("restart") } } @@ -102,25 +103,25 @@ func normalizeTemplates(templates []*api.Template) { for _, t := range templates { if t.ChangeMode == nil { - t.ChangeMode = stringToPtr("restart") + t.ChangeMode = pointer.Of("restart") } if t.Perms == nil { - t.Perms = stringToPtr("0644") + t.Perms = pointer.Of("0644") } if t.Uid == nil { - t.Uid = intToPtr(-1) + t.Uid = pointer.Of(-1) } if t.Gid == nil { - t.Gid = intToPtr(-1) + t.Gid = pointer.Of(-1) } if t.Splay == nil { - t.Splay = durationToPtr(5 * time.Second) + t.Splay = pointer.Of(5 * time.Second) } - normalizeChangeScriptConfig(t.ChangeScriptConfig) + normalizeChangeScript(t.ChangeScript) } } -func normalizeChangeScriptConfig(ch *api.ChangeScriptConfig) { +func normalizeChangeScript(ch *api.ChangeScript) { if ch == nil { return } @@ -130,30 +131,10 @@ func normalizeChangeScriptConfig(ch *api.ChangeScriptConfig) { } if ch.Timeout == nil { - ch.Timeout = durationToPtr(5 * time.Second) + ch.Timeout = pointer.Of(5 * time.Second) } if ch.FailOnError == nil { - ch.FailOnError = boolToPtr(false) + ch.FailOnError = pointer.Of(false) } } - -func int8ToPtr(v int8) *int8 { - return &v -} - -func boolToPtr(v bool) *bool { - return &v -} - -func intToPtr(v int) *int { - return &v -} - -func stringToPtr(v string) *string { - return &v -} - -func durationToPtr(v time.Duration) *time.Duration { - return &v -} diff --git a/jobspec2/parse_test.go b/jobspec2/parse_test.go index 75c10f67e856..806412cad48a 100644 --- a/jobspec2/parse_test.go +++ b/jobspec2/parse_test.go @@ -9,6 +9,7 @@ import ( "github.com/hashicorp/nomad/api" "github.com/hashicorp/nomad/ci" + "github.com/hashicorp/nomad/helper/pointer" "github.com/hashicorp/nomad/jobspec" "github.com/stretchr/testify/require" ) @@ -644,13 +645,13 @@ job "job-webserver" { { "prod", &api.Job{ - ID: stringToPtr("job-webserver"), - Name: stringToPtr("job-webserver"), + ID: pointer.Of("job-webserver"), + Name: pointer.Of("job-webserver"), Datacenters: []string{"prod-dc1", "prod-dc2"}, TaskGroups: []*api.TaskGroup{ { - Name: stringToPtr("group-webserver"), - Count: intToPtr(20), + Name: pointer.Of("group-webserver"), + Count: pointer.Of(20), Tasks: []*api.Task{ { @@ -670,13 +671,13 @@ job "job-webserver" { { "staging", &api.Job{ - ID: stringToPtr("job-webserver"), - Name: stringToPtr("job-webserver"), + ID: pointer.Of("job-webserver"), + Name: pointer.Of("job-webserver"), Datacenters: []string{"dc1"}, TaskGroups: []*api.TaskGroup{ { - Name: stringToPtr("group-webserver"), - Count: intToPtr(3), + Name: pointer.Of("group-webserver"), + Count: pointer.Of(3), Tasks: []*api.Task{ { @@ -696,13 +697,13 @@ job "job-webserver" { { "unknown", &api.Job{ - ID: stringToPtr("job-webserver"), - Name: stringToPtr("job-webserver"), + ID: pointer.Of("job-webserver"), + Name: pointer.Of("job-webserver"), Datacenters: []string{}, TaskGroups: []*api.TaskGroup{ { - Name: stringToPtr("group-webserver"), - Count: intToPtr(0), + Name: pointer.Of("group-webserver"), + Count: pointer.Of(0), Tasks: []*api.Task{ { @@ -1005,11 +1006,11 @@ func TestParseServiceCheck(t *testing.T) { require.NoError(t, err) expectedJob := &api.Job{ - ID: stringToPtr("group_service_check_script"), - Name: stringToPtr("group_service_check_script"), + ID: pointer.Of("group_service_check_script"), + Name: pointer.Of("group_service_check_script"), TaskGroups: []*api.TaskGroup{ { - Name: stringToPtr("group"), + Name: pointer.Of("group"), Services: []*api.Service{ { Name: "foo-service", diff --git a/nomad/structs/diff.go b/nomad/structs/diff.go index 11ae5ca855ab..b5fecc4d29ef 100644 --- a/nomad/structs/diff.go +++ b/nomad/structs/diff.go @@ -1649,21 +1649,20 @@ func waitConfigDiff(old, new *WaitConfig, contextual bool) *ObjectDiff { return diff } -// changeScriptConfigDiff returns the diff of two ChangeScriptConfig objects. If -// contextual diff is enabled, all fields will be returned, even if no diff -// occurred. -func changeScriptConfigDiff(old, new *ChangeScriptConfig, contextual bool) *ObjectDiff { - diff := &ObjectDiff{Type: DiffTypeNone, Name: "ChangeScriptConfig"} +// changeScriptDiff returns the diff of two ChangeScript objects. If contextual +// diff is enabled, all fields will be returned, even if no diff occurred. +func changeScriptDiff(old, new *ChangeScript, contextual bool) *ObjectDiff { + diff := &ObjectDiff{Type: DiffTypeNone, Name: "ChangeScript"} var oldPrimitiveFlat, newPrimitiveFlat map[string]string if reflect.DeepEqual(old, new) { return nil } else if old == nil { - old = &ChangeScriptConfig{} + old = &ChangeScript{} diff.Type = DiffTypeAdded newPrimitiveFlat = flatmap.Flatten(new, nil, true) } else if new == nil { - new = &ChangeScriptConfig{} + new = &ChangeScript{} diff.Type = DiffTypeDeleted oldPrimitiveFlat = flatmap.Flatten(old, nil, true) } else { @@ -1713,9 +1712,9 @@ func templateDiff(old, new *Template, contextual bool) *ObjectDiff { diff.Objects = append(diff.Objects, waitDiffs) } - // ChangeScriptConfig diffs - if changeScriptDiffs := changeScriptConfigDiff( - old.ChangeScriptConfig, new.ChangeScriptConfig, contextual, + // ChangeScript diffs + if changeScriptDiffs := changeScriptDiff( + old.ChangeScript, new.ChangeScript, contextual, ); changeScriptDiffs != nil { diff.Objects = append(diff.Objects, changeScriptDiffs) } diff --git a/nomad/structs/diff_test.go b/nomad/structs/diff_test.go index a5e803c84565..5d7a0ca4d8fe 100644 --- a/nomad/structs/diff_test.go +++ b/nomad/structs/diff_test.go @@ -7042,7 +7042,7 @@ func TestTaskDiff(t *testing.T) { EmbeddedTmpl: "baz", ChangeMode: "bam", ChangeSignal: "SIGHUP", - ChangeScriptConfig: &ChangeScriptConfig{ + ChangeScript: &ChangeScript{ Path: "/bin/foo", Args: []string{"-debug"}, Timeout: 5, @@ -7063,7 +7063,7 @@ func TestTaskDiff(t *testing.T) { EmbeddedTmpl: "baz2", ChangeMode: "bam2", ChangeSignal: "SIGHUP2", - ChangeScriptConfig: &ChangeScriptConfig{ + ChangeScript: &ChangeScript{ Path: "/bin/foo2", Args: []string{"-debugs"}, Timeout: 6, @@ -7085,7 +7085,7 @@ func TestTaskDiff(t *testing.T) { EmbeddedTmpl: "baz new", ChangeMode: "bam", ChangeSignal: "SIGHUP", - ChangeScriptConfig: &ChangeScriptConfig{ + ChangeScript: &ChangeScript{ Path: "/bin/foo", Args: []string{"-debug"}, Timeout: 5, @@ -7106,7 +7106,7 @@ func TestTaskDiff(t *testing.T) { EmbeddedTmpl: "baz3", ChangeMode: "bam3", ChangeSignal: "SIGHUP3", - ChangeScriptConfig: &ChangeScriptConfig{ + ChangeScript: &ChangeScript{ Path: "/bin/foo3", Args: []string{"-debugss"}, Timeout: 7, @@ -7244,7 +7244,7 @@ func TestTaskDiff(t *testing.T) { }, { Type: DiffTypeAdded, - Name: "ChangeScriptConfig", + Name: "ChangeScript", Fields: []*FieldDiff{ { Type: DiffTypeAdded, @@ -7356,7 +7356,7 @@ func TestTaskDiff(t *testing.T) { Objects: []*ObjectDiff{ { Type: DiffTypeDeleted, - Name: "ChangeScriptConfig", + Name: "ChangeScript", Fields: []*FieldDiff{ { Type: DiffTypeDeleted, diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 1d5828e0c414..d1b864980a4d 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -7698,9 +7698,9 @@ var ( TemplateChangeModeInvalidError = errors.New("Invalid change mode. Must be one of the following: noop, signal, script, restart") ) -// ChangeScriptConfig holds the configuration for the script that is executed if +// ChangeScript holds the configuration for the script that is executed if // change mode is set to script -type ChangeScriptConfig struct { +type ChangeScript struct { // Path is the full path to the script Path string // Args is a slice of arguments passed to the script @@ -7731,9 +7731,9 @@ type Template struct { // requires it. ChangeSignal string - // ChangeScriptConfig is the configuration of the script. It's required if + // ChangeScript is the configuration of the script. It's required if // ChangeMode is set to script. - ChangeScriptConfig *ChangeScriptConfig + ChangeScript *ChangeScript // Splay is used to avoid coordinated restarts of processes by applying a // random wait between 0 and the given splay value before signalling the @@ -7834,7 +7834,7 @@ func (t *Template) Validate() error { _ = multierror.Append(&mErr, fmt.Errorf("cannot use signals with env var templates")) } case TemplateChangeModeScript: - if t.ChangeScriptConfig.Path == "" { + if t.ChangeScript.Path == "" { _ = multierror.Append(&mErr, fmt.Errorf("must specify script path value when change mode is script")) } default: diff --git a/website/content/api-docs/json-jobs.mdx b/website/content/api-docs/json-jobs.mdx index e3114e699411..cfe2408cab37 100644 --- a/website/content/api-docs/json-jobs.mdx +++ b/website/content/api-docs/json-jobs.mdx @@ -1061,10 +1061,10 @@ README][ct]. "SIGUSR1" or "SIGINT". This option is required if the `ChangeMode` is `signal`. -- `ChangeScriptConfig` - Configures the script triggered on template change. - This option is required if the `ChangeMode` is `script`. +- `ChangeScript` - Configures the script triggered on template change. This + option is required if the `ChangeMode` is `script`. - The `ChangeScriptConfig` object supports the following attributes: + The `ChangeScript` object supports the following attributes: - `path` - Specifies the full path to a script that is to be executed on template change. Path is relative to the driver, e.g., if running with a diff --git a/website/content/docs/job-specification/change_script_config.mdx b/website/content/docs/job-specification/change_script.mdx similarity index 78% rename from website/content/docs/job-specification/change_script_config.mdx rename to website/content/docs/job-specification/change_script.mdx index be3f2ecd5dfd..037356510f47 100644 --- a/website/content/docs/job-specification/change_script_config.mdx +++ b/website/content/docs/job-specification/change_script.mdx @@ -1,14 +1,14 @@ --- layout: docs -page_title: change_script_config Stanza - Job Specification -description: The "change_script_config" stanza configures a script to be run on template re-render. +page_title: change_script Stanza - Job Specification +description: The "change_script" stanza configures a script to be run on template re-render. --- -# `change_script_config` Stanza +# `change_script` Stanza - + -The `change_script_config` stanza allows operators to configure scripts that +The `change_script` stanza allows operators to configure scripts that will be executed on template change. This stanza is only used when template `change_mode` is set to `script`. @@ -20,7 +20,7 @@ job "docs" { source = "local/redis.conf.tpl" destination = "local/redis.conf" change_mode = "script" - change_script_config { + change_script { path = "/bin/foo" args = ["-verbose", "-debug"] timeout = "5s" @@ -32,7 +32,7 @@ job "docs" { } ``` -## `change_script_config` Parameters +## `change_script` Parameters - `path` `(string: "")` - Specifies the full path to a script that is to be executed on template change. Path is relative to the driver, e.g., if diff --git a/website/content/docs/job-specification/template.mdx b/website/content/docs/job-specification/template.mdx index 5a9b63527796..bdd20c27ad06 100644 --- a/website/content/docs/job-specification/template.mdx +++ b/website/content/docs/job-specification/template.mdx @@ -59,9 +59,9 @@ refer to the [Learn Go Template Syntax][gt_learn] Learn guide. string like `"SIGUSR1"` or `"SIGINT"`. This option is required if the `change_mode` is `signal`. -- `change_script_config` ([ChangeScriptConfig][]: nil) - Configures - the script triggered on template change. This option is required if the - `change_mode` is `script`. +- `change_script` ([ChangeScript][]: nil) - Configures the script + triggered on template change. This option is required if the `change_mode` is + `script`. - `data` `(string: "")` - Specifies the raw template to execute. One of `source` or `data` must be specified, but not both. This is useful for smaller @@ -565,7 +565,7 @@ options](/docs/configuration/client#options): files on the client host via the `file` function. By default templates can access files only within the [task working directory]. -[changescriptconfig]: /docs/job-specification/change_script_config 'Nomad change_script_config Job Specification' +[changescript]: /docs/job-specification/change_script 'Nomad change_script Job Specification' [ct]: https://github.com/hashicorp/consul-template 'Consul Template by HashiCorp' [ct_api]: https://github.com/hashicorp/consul-template/blob/master/docs/templating-language.md 'Consul Template API by HashiCorp' [ct_api_connect]: https://github.com/hashicorp/consul-template/blob/master/docs/templating-language.md#connect 'Consul Template API by HashiCorp - connect' diff --git a/website/data/docs-nav-data.json b/website/data/docs-nav-data.json index 92e907999f69..a721fbe78831 100644 --- a/website/data/docs-nav-data.json +++ b/website/data/docs-nav-data.json @@ -1359,8 +1359,8 @@ "path": "job-specification/affinity" }, { - "title": "change_script_config", - "path": "job-specification/change_script_config" + "title": "change_script", + "path": "job-specification/change_script" }, { "title": "check", From 0553d09635fa3e7b115b871bb7fd904a7f645089 Mon Sep 17 00:00:00 2001 From: Piotr Kazmierczak Date: Thu, 18 Aug 2022 14:40:46 +0200 Subject: [PATCH 48/68] removed obsolete ioutil dependency --- .../taskrunner/template/template_test.go | 51 +++++++++---------- 1 file changed, 25 insertions(+), 26 deletions(-) diff --git a/client/allocrunner/taskrunner/template/template_test.go b/client/allocrunner/taskrunner/template/template_test.go index 86cac38ba04a..925027e51b47 100644 --- a/client/allocrunner/taskrunner/template/template_test.go +++ b/client/allocrunner/taskrunner/template/template_test.go @@ -5,7 +5,6 @@ import ( "context" "fmt" "io" - "io/ioutil" "os" "os/user" "path/filepath" @@ -390,7 +389,7 @@ func TestTaskTemplateManager_InvalidConfig(t *testing.T) { func TestTaskTemplateManager_HostPath(t *testing.T) { ci.Parallel(t) // Make a template that will render immediately and write it to a tmp file - f, err := ioutil.TempFile("", "") + f, err := os.CreateTemp("", "") if err != nil { t.Fatalf("Bad: %v", err) } @@ -426,7 +425,7 @@ func TestTaskTemplateManager_HostPath(t *testing.T) { // Check the file is there path := filepath.Join(harness.taskDir, file) - raw, err := ioutil.ReadFile(path) + raw, err := os.ReadFile(path) if err != nil { t.Fatalf("Failed to read rendered template from %q: %v", path, err) } @@ -503,7 +502,7 @@ func TestTaskTemplateManager_Unblock_Static(t *testing.T) { // Check the file is there path := filepath.Join(harness.taskDir, file) - raw, err := ioutil.ReadFile(path) + raw, err := os.ReadFile(path) if err != nil { t.Fatalf("Failed to read rendered template from %q: %v", path, err) } @@ -582,7 +581,7 @@ func TestTaskTemplateManager_Unblock_Static_NomadEnv(t *testing.T) { // Check the file is there path := filepath.Join(harness.taskDir, file) - raw, err := ioutil.ReadFile(path) + raw, err := os.ReadFile(path) if err != nil { t.Fatalf("Failed to read rendered template from %q: %v", path, err) } @@ -607,7 +606,7 @@ func TestTaskTemplateManager_Unblock_Static_AlreadyRendered(t *testing.T) { // Write the contents path := filepath.Join(harness.taskDir, file) - if err := ioutil.WriteFile(path, []byte(content), 0777); err != nil { + if err := os.WriteFile(path, []byte(content), 0777); err != nil { t.Fatalf("Failed to write data: %v", err) } @@ -623,7 +622,7 @@ func TestTaskTemplateManager_Unblock_Static_AlreadyRendered(t *testing.T) { // Check the file is there path = filepath.Join(harness.taskDir, file) - raw, err := ioutil.ReadFile(path) + raw, err := os.ReadFile(path) if err != nil { t.Fatalf("Failed to read rendered template from %q: %v", path, err) } @@ -669,7 +668,7 @@ func TestTaskTemplateManager_Unblock_Consul(t *testing.T) { // Check the file is there path := filepath.Join(harness.taskDir, file) - raw, err := ioutil.ReadFile(path) + raw, err := os.ReadFile(path) if err != nil { t.Fatalf("Failed to read rendered template from %q: %v", path, err) } @@ -719,7 +718,7 @@ func TestTaskTemplateManager_Unblock_Vault(t *testing.T) { // Check the file is there path := filepath.Join(harness.taskDir, file) - raw, err := ioutil.ReadFile(path) + raw, err := os.ReadFile(path) if err != nil { t.Fatalf("Failed to read rendered template from %q: %v", path, err) } @@ -764,7 +763,7 @@ func TestTaskTemplateManager_Unblock_Multi_Template(t *testing.T) { // Check that the static file has been rendered path := filepath.Join(harness.taskDir, staticFile) - raw, err := ioutil.ReadFile(path) + raw, err := os.ReadFile(path) if err != nil { t.Fatalf("Failed to read rendered template from %q: %v", path, err) } @@ -785,7 +784,7 @@ func TestTaskTemplateManager_Unblock_Multi_Template(t *testing.T) { // Check the consul file is there path = filepath.Join(harness.taskDir, consulFile) - raw, err = ioutil.ReadFile(path) + raw, err = os.ReadFile(path) if err != nil { t.Fatalf("Failed to read rendered template from %q: %v", path, err) } @@ -837,7 +836,7 @@ func TestTaskTemplateManager_FirstRender_Restored(t *testing.T) { // Check the file is there path := filepath.Join(harness.taskDir, file) - raw, err := ioutil.ReadFile(path) + raw, err := os.ReadFile(path) require.NoError(err, "Failed to read rendered template from %q", path) require.Equal(content, string(raw), "Unexpected template data; got %s, want %q", raw, content) @@ -931,7 +930,7 @@ func TestTaskTemplateManager_Rerender_Noop(t *testing.T) { // Check the file is there path := filepath.Join(harness.taskDir, file) - raw, err := ioutil.ReadFile(path) + raw, err := os.ReadFile(path) if err != nil { t.Fatalf("Failed to read rendered template from %q: %v", path, err) } @@ -953,7 +952,7 @@ func TestTaskTemplateManager_Rerender_Noop(t *testing.T) { // Check the file has been updated path = filepath.Join(harness.taskDir, file) - raw, err = ioutil.ReadFile(path) + raw, err = os.ReadFile(path) if err != nil { t.Fatalf("Failed to read rendered template from %q: %v", path, err) } @@ -1043,7 +1042,7 @@ OUTER: // Check the files have been updated path := filepath.Join(harness.taskDir, file1) - raw, err := ioutil.ReadFile(path) + raw, err := os.ReadFile(path) if err != nil { t.Fatalf("Failed to read rendered template from %q: %v", path, err) } @@ -1053,7 +1052,7 @@ OUTER: } path = filepath.Join(harness.taskDir, file2) - raw, err = ioutil.ReadFile(path) + raw, err = os.ReadFile(path) if err != nil { t.Fatalf("Failed to read rendered template from %q: %v", path, err) } @@ -1117,7 +1116,7 @@ OUTER: // Check the files have been updated path := filepath.Join(harness.taskDir, file1) - raw, err := ioutil.ReadFile(path) + raw, err := os.ReadFile(path) if err != nil { t.Fatalf("Failed to read rendered template from %q: %v", path, err) } @@ -1152,7 +1151,7 @@ func TestTaskTemplateManager_Interpolate_Destination(t *testing.T) { // Check the file is there actual := fmt.Sprintf("%s.tmpl", harness.node.ID) path := filepath.Join(harness.taskDir, actual) - raw, err := ioutil.ReadFile(path) + raw, err := os.ReadFile(path) if err != nil { t.Fatalf("Failed to read rendered template from %q: %v", path, err) } @@ -1412,7 +1411,7 @@ TEST_ENV_NOT_FOUND: {{env "` + testenv + `_NOTFOUND" }}` // Check the file is there path := filepath.Join(harness.taskDir, file) - raw, err := ioutil.ReadFile(path) + raw, err := os.ReadFile(path) require.NoError(t, err) require.Equal(t, expected, string(raw)) @@ -1468,7 +1467,7 @@ func TestTaskTemplateManager_Env_Missing(t *testing.T) { d := t.TempDir() // Fake writing the file so we don't have to run the whole template manager - err := ioutil.WriteFile(filepath.Join(d, "exists.env"), []byte("FOO=bar\n"), 0644) + err := os.WriteFile(filepath.Join(d, "exists.env"), []byte("FOO=bar\n"), 0644) if err != nil { t.Fatalf("error writing template file: %v", err) } @@ -1501,7 +1500,7 @@ func TestTaskTemplateManager_Env_InterpolatedDest(t *testing.T) { d := t.TempDir() // Fake writing the file so we don't have to run the whole template manager - err := ioutil.WriteFile(filepath.Join(d, "exists.env"), []byte("FOO=bar\n"), 0644) + err := os.WriteFile(filepath.Join(d, "exists.env"), []byte("FOO=bar\n"), 0644) if err != nil { t.Fatalf("error writing template file: %v", err) } @@ -1536,11 +1535,11 @@ func TestTaskTemplateManager_Env_Multi(t *testing.T) { d := t.TempDir() // Fake writing the files so we don't have to run the whole template manager - err := ioutil.WriteFile(filepath.Join(d, "zzz.env"), []byte("FOO=bar\nSHARED=nope\n"), 0644) + err := os.WriteFile(filepath.Join(d, "zzz.env"), []byte("FOO=bar\nSHARED=nope\n"), 0644) if err != nil { t.Fatalf("error writing template file 1: %v", err) } - err = ioutil.WriteFile(filepath.Join(d, "aaa.env"), []byte("BAR=foo\nSHARED=yup\n"), 0644) + err = os.WriteFile(filepath.Join(d, "aaa.env"), []byte("BAR=foo\nSHARED=yup\n"), 0644) if err != nil { t.Fatalf("error writing template file 2: %v", err) } @@ -2380,7 +2379,7 @@ func TestTaskTemplateManager_writeToFile_Disabled(t *testing.T) { // Check the file is not there path := filepath.Join(harness.taskDir, file) - _, err := ioutil.ReadFile(path) + _, err := os.ReadFile(path) require.Error(t, err) } @@ -2433,13 +2432,13 @@ func TestTaskTemplateManager_writeToFile(t *testing.T) { // Check the templated file is there path := filepath.Join(harness.taskDir, file) - r, err := ioutil.ReadFile(path) + r, err := os.ReadFile(path) require.NoError(t, err) require.True(t, bytes.HasSuffix(r, []byte("...done\n")), string(r)) // Check that writeToFile was allowed path = filepath.Join(harness.taskDir, "writetofile.out") - r, err = ioutil.ReadFile(path) + r, err = os.ReadFile(path) require.NoError(t, err) require.Equal(t, "hello", string(r)) } From a63d77c460d5cbfe852ffbf95de515a3c10096c4 Mon Sep 17 00:00:00 2001 From: Piotr Kazmierczak Date: Thu, 18 Aug 2022 14:46:27 +0200 Subject: [PATCH 49/68] execute scripts concurrently --- .../taskrunner/template/template.go | 62 +++++++++++-------- 1 file changed, 35 insertions(+), 27 deletions(-) diff --git a/client/allocrunner/taskrunner/template/template.go b/client/allocrunner/taskrunner/template/template.go index a0b39fc86f74..41c9dc2fb40f 100644 --- a/client/allocrunner/taskrunner/template/template.go +++ b/client/allocrunner/taskrunner/template/template.go @@ -504,15 +504,40 @@ func (tm *TaskTemplateManager) onTemplateRendered(handledRenders map[string]time } } } + + // process script execution concurrently + var wg sync.WaitGroup for _, script := range scripts { - if tm.handle != nil { - _, exitCode, err := tm.handle.Exec(script.Timeout, script.Path, script.Args) - if err != nil { - structs.NewTaskEvent(structs.TaskHookFailed). - SetDisplayMessage( - fmt.Sprintf( - "Template failed to run script %v on change: %v Exit code: %v", script.Path, err, exitCode, - )) + wg.Add(1) + go func(script *structs.ChangeScript) { + defer wg.Done() + if tm.handle != nil { + _, exitCode, err := tm.handle.Exec(script.Timeout, script.Path, script.Args) + if err != nil { + structs.NewTaskEvent(structs.TaskHookFailed). + SetDisplayMessage( + fmt.Sprintf( + "Template failed to run script %v on change: %v Exit code: %v", script.Path, err, exitCode, + )) + if script.FailOnError { + tm.config.Lifecycle.Kill(context.Background(), + structs.NewTaskEvent(structs.TaskKilling). + SetFailsTask(). + SetDisplayMessage( + fmt.Sprintf("Template failed to run script %v and the task is being killed", script.Path), + )) + } + } else { + tm.config.Events.EmitEvent(structs.NewTaskEvent(structs.TaskHookMessage). + SetDisplayMessage( + fmt.Sprintf( + "Template successfully ran a script from %v with exit code: %v", script.Path, exitCode, + ))) + } + } else { + tm.config.Events.EmitEvent(structs.NewTaskEvent(structs.TaskHookFailed). + SetDisplayMessage("Template failed to run a change script because task driver doesn't support the exec operation"), + ) if script.FailOnError { tm.config.Lifecycle.Kill(context.Background(), structs.NewTaskEvent(structs.TaskKilling). @@ -521,27 +546,10 @@ func (tm *TaskTemplateManager) onTemplateRendered(handledRenders map[string]time fmt.Sprintf("Template failed to run script %v and the task is being killed", script.Path), )) } - } else { - tm.config.Events.EmitEvent(structs.NewTaskEvent(structs.TaskHookMessage). - SetDisplayMessage( - fmt.Sprintf( - "Template successfully ran a script from %v with exit code: %v", script.Path, exitCode, - ))) - } - } else { - tm.config.Events.EmitEvent(structs.NewTaskEvent(structs.TaskHookFailed). - SetDisplayMessage("Template failed to run a change script because task driver doesn't support the exec operation"), - ) - if script.FailOnError { - tm.config.Lifecycle.Kill(context.Background(), - structs.NewTaskEvent(structs.TaskKilling). - SetFailsTask(). - SetDisplayMessage( - fmt.Sprintf("Template failed to run script %v and the task is being killed", script.Path), - )) } - } + }(script) } + wg.Wait() } // allTemplatesNoop returns whether all the managed templates have change mode noop. From b4d4ec843aac43d753218c84012186b37014d1d1 Mon Sep 17 00:00:00 2001 From: Piotr Kazmierczak Date: Thu, 18 Aug 2022 19:19:25 +0200 Subject: [PATCH 50/68] mutex for driver handle --- client/allocrunner/taskrunner/template/template.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/client/allocrunner/taskrunner/template/template.go b/client/allocrunner/taskrunner/template/template.go index 41c9dc2fb40f..b51d7cfa09a5 100644 --- a/client/allocrunner/taskrunner/template/template.go +++ b/client/allocrunner/taskrunner/template/template.go @@ -55,7 +55,8 @@ type TaskTemplateManager struct { runner *manager.Runner // handle is used to execute scripts - handle interfaces.ScriptExecutor + handle interfaces.ScriptExecutor + handleLock sync.Mutex // signals is a lookup map from the string representation of a signal to its // actual signal @@ -197,7 +198,9 @@ func (tm *TaskTemplateManager) Stop() { // SetDriverHandle sets the executor func (tm *TaskTemplateManager) SetDriverHandle(executor interfaces.ScriptExecutor) { + tm.handleLock.Lock() tm.handle = executor + tm.handleLock.Unlock() } // run is the long lived loop that handles errors and templates being rendered From c97ea30b792caf79e83df7671d982a34df99689c Mon Sep 17 00:00:00 2001 From: Piotr Kazmierczak Date: Thu, 18 Aug 2022 19:31:18 +0200 Subject: [PATCH 51/68] refactored onTemplateRendered --- .../taskrunner/template/template.go | 51 ++++++++++--------- 1 file changed, 26 insertions(+), 25 deletions(-) diff --git a/client/allocrunner/taskrunner/template/template.go b/client/allocrunner/taskrunner/template/template.go index b51d7cfa09a5..723fb0ad86c0 100644 --- a/client/allocrunner/taskrunner/template/template.go +++ b/client/allocrunner/taskrunner/template/template.go @@ -514,33 +514,28 @@ func (tm *TaskTemplateManager) onTemplateRendered(handledRenders map[string]time wg.Add(1) go func(script *structs.ChangeScript) { defer wg.Done() - if tm.handle != nil { - _, exitCode, err := tm.handle.Exec(script.Timeout, script.Path, script.Args) - if err != nil { - structs.NewTaskEvent(structs.TaskHookFailed). - SetDisplayMessage( - fmt.Sprintf( - "Template failed to run script %v on change: %v Exit code: %v", script.Path, err, exitCode, + if tm.handle == nil { + tm.config.Events.EmitEvent(structs.NewTaskEvent(structs.TaskHookFailed). + SetDisplayMessage( + "Template failed to run a change script because task driver doesn't support the exec operation", + )) + if script.FailOnError { + tm.config.Lifecycle.Kill(context.Background(), + structs.NewTaskEvent(structs.TaskKilling). + SetFailsTask(). + SetDisplayMessage( + fmt.Sprintf("Template failed to run script %v and the task is being killed", script.Path), )) - if script.FailOnError { - tm.config.Lifecycle.Kill(context.Background(), - structs.NewTaskEvent(structs.TaskKilling). - SetFailsTask(). - SetDisplayMessage( - fmt.Sprintf("Template failed to run script %v and the task is being killed", script.Path), - )) - } - } else { - tm.config.Events.EmitEvent(structs.NewTaskEvent(structs.TaskHookMessage). - SetDisplayMessage( - fmt.Sprintf( - "Template successfully ran a script from %v with exit code: %v", script.Path, exitCode, - ))) } - } else { - tm.config.Events.EmitEvent(structs.NewTaskEvent(structs.TaskHookFailed). - SetDisplayMessage("Template failed to run a change script because task driver doesn't support the exec operation"), - ) + return + } + _, exitCode, err := tm.handle.Exec(script.Timeout, script.Path, script.Args) + if err != nil { + structs.NewTaskEvent(structs.TaskHookFailed). + SetDisplayMessage( + fmt.Sprintf( + "Template failed to run script %v on change: %v Exit code: %v", script.Path, err, exitCode, + )) if script.FailOnError { tm.config.Lifecycle.Kill(context.Background(), structs.NewTaskEvent(structs.TaskKilling). @@ -549,7 +544,13 @@ func (tm *TaskTemplateManager) onTemplateRendered(handledRenders map[string]time fmt.Sprintf("Template failed to run script %v and the task is being killed", script.Path), )) } + return } + tm.config.Events.EmitEvent(structs.NewTaskEvent(structs.TaskHookMessage). + SetDisplayMessage( + fmt.Sprintf( + "Template successfully ran a script from %v with exit code: %v", script.Path, exitCode, + ))) }(script) } wg.Wait() From 944eeee7771216085cbc37f8de96144fe9a905e8 Mon Sep 17 00:00:00 2001 From: Piotr Kazmierczak Date: Thu, 18 Aug 2022 20:19:25 +0200 Subject: [PATCH 52/68] path -> command --- api/tasks.go | 6 +++--- client/allocrunner/taskrunner/template/template.go | 10 +++++----- .../allocrunner/taskrunner/template/template_test.go | 10 +++++----- command/agent/job_endpoint.go | 2 +- command/agent/job_endpoint_test.go | 4 ++-- jobspec/parse_task.go | 2 +- jobspec/parse_test.go | 2 +- jobspec/test-fixtures/basic.hcl | 2 +- nomad/structs/diff_test.go | 8 ++++---- nomad/structs/structs.go | 6 +++--- website/content/api-docs/json-jobs.mdx | 8 ++++---- .../content/docs/job-specification/change_script.mdx | 8 ++++---- 12 files changed, 34 insertions(+), 34 deletions(-) diff --git a/api/tasks.go b/api/tasks.go index 5d47e9ad04d4..4fcf95a06f7b 100644 --- a/api/tasks.go +++ b/api/tasks.go @@ -792,15 +792,15 @@ func (wc *WaitConfig) Copy() *WaitConfig { } type ChangeScript struct { - Path *string `mapstructure:"path" hcl:"path"` + Command *string `mapstructure:"command" hcl:"command"` Args []string `mapstructure:"args" hcl:"args,optional"` Timeout *time.Duration `mapstructure:"timeout" hcl:"timeout,optional"` FailOnError *bool `mapstructure:"fail_on_error" hcl:"fail_on_error"` } func (ch *ChangeScript) Canonicalize() { - if ch.Path == nil { - ch.Path = pointerOf("") + if ch.Command == nil { + ch.Command = pointerOf("") } if ch.Args == nil { ch.Args = []string{} diff --git a/client/allocrunner/taskrunner/template/template.go b/client/allocrunner/taskrunner/template/template.go index 723fb0ad86c0..c1dd9c299764 100644 --- a/client/allocrunner/taskrunner/template/template.go +++ b/client/allocrunner/taskrunner/template/template.go @@ -524,24 +524,24 @@ func (tm *TaskTemplateManager) onTemplateRendered(handledRenders map[string]time structs.NewTaskEvent(structs.TaskKilling). SetFailsTask(). SetDisplayMessage( - fmt.Sprintf("Template failed to run script %v and the task is being killed", script.Path), + fmt.Sprintf("Template failed to run script %v and the task is being killed", script.Command), )) } return } - _, exitCode, err := tm.handle.Exec(script.Timeout, script.Path, script.Args) + _, exitCode, err := tm.handle.Exec(script.Timeout, script.Command, script.Args) if err != nil { structs.NewTaskEvent(structs.TaskHookFailed). SetDisplayMessage( fmt.Sprintf( - "Template failed to run script %v on change: %v Exit code: %v", script.Path, err, exitCode, + "Template failed to run script %v on change: %v Exit code: %v", script.Command, err, exitCode, )) if script.FailOnError { tm.config.Lifecycle.Kill(context.Background(), structs.NewTaskEvent(structs.TaskKilling). SetFailsTask(). SetDisplayMessage( - fmt.Sprintf("Template failed to run script %v and the task is being killed", script.Path), + fmt.Sprintf("Template failed to run script %v and the task is being killed", script.Command), )) } return @@ -549,7 +549,7 @@ func (tm *TaskTemplateManager) onTemplateRendered(handledRenders map[string]time tm.config.Events.EmitEvent(structs.NewTaskEvent(structs.TaskHookMessage). SetDisplayMessage( fmt.Sprintf( - "Template successfully ran a script from %v with exit code: %v", script.Path, exitCode, + "Template successfully ran a script from %v with exit code: %v", script.Command, exitCode, ))) }(script) } diff --git a/client/allocrunner/taskrunner/template/template_test.go b/client/allocrunner/taskrunner/template/template_test.go index 925027e51b47..722e2e23267c 100644 --- a/client/allocrunner/taskrunner/template/template_test.go +++ b/client/allocrunner/taskrunner/template/template_test.go @@ -1224,7 +1224,7 @@ FOO={{key "bam"}} DestPath: "test.env", ChangeMode: structs.TemplateChangeModeScript, ChangeScript: &structs.ChangeScript{ - Path: "/bin/foo", + Command: "/bin/foo", Args: []string{}, Timeout: 5 * time.Second, FailOnError: false, @@ -1238,7 +1238,7 @@ BAR={{key "bar"}} DestPath: "test2.env", ChangeMode: structs.TemplateChangeModeScript, ChangeScript: &structs.ChangeScript{ - Path: "/bin/foo", + Command: "/bin/foo", Args: []string{}, Timeout: 5 * time.Second, FailOnError: false, @@ -1281,7 +1281,7 @@ OUTER: case <-harness.mockHooks.RestartCh: require.Fail(t, "restart not expected") case ev := <-harness.mockHooks.EmitEventCh: - if strings.Contains(ev.DisplayMessage, t1.ChangeScript.Path) { + if strings.Contains(ev.DisplayMessage, t1.ChangeScript.Command) { break OUTER } case <-harness.mockHooks.SignalCh: @@ -1310,7 +1310,7 @@ FOO={{key "bam"}} DestPath: "test.env", ChangeMode: structs.TemplateChangeModeScript, ChangeScript: &structs.ChangeScript{ - Path: "/bin/foo", + Command: "/bin/foo", Args: []string{}, Timeout: 5 * time.Second, FailOnError: true, @@ -1324,7 +1324,7 @@ BAR={{key "bar"}} DestPath: "test2.env", ChangeMode: structs.TemplateChangeModeScript, ChangeScript: &structs.ChangeScript{ - Path: "/bin/foo", + Command: "/bin/foo", Args: []string{}, Timeout: 5 * time.Second, FailOnError: false, diff --git a/command/agent/job_endpoint.go b/command/agent/job_endpoint.go index 4f5bdba7c331..c09727556ac5 100644 --- a/command/agent/job_endpoint.go +++ b/command/agent/job_endpoint.go @@ -1271,7 +1271,7 @@ func apiChangeScriptToStructsChangeScript(changeScript *api.ChangeScript) *struc } return &structs.ChangeScript{ - Path: *changeScript.Path, + Command: *changeScript.Command, Args: changeScript.Args, Timeout: *changeScript.Timeout, FailOnError: *changeScript.FailOnError, diff --git a/command/agent/job_endpoint_test.go b/command/agent/job_endpoint_test.go index c936421a597d..f64094cd2fe0 100644 --- a/command/agent/job_endpoint_test.go +++ b/command/agent/job_endpoint_test.go @@ -2732,7 +2732,7 @@ func TestJobs_ApiJobToStructsJob(t *testing.T) { ChangeMode: pointer.Of("change"), ChangeSignal: pointer.Of("signal"), ChangeScript: &api.ChangeScript{ - Path: pointer.Of("/bin/foo"), + Command: pointer.Of("/bin/foo"), Args: []string{"-h"}, Timeout: pointer.Of(5 * time.Second), FailOnError: pointer.Of(false), @@ -3145,7 +3145,7 @@ func TestJobs_ApiJobToStructsJob(t *testing.T) { ChangeMode: "change", ChangeSignal: "SIGNAL", ChangeScript: &structs.ChangeScript{ - Path: "/bin/foo", + Command: "/bin/foo", Args: []string{"-h"}, Timeout: 5 * time.Second, FailOnError: false, diff --git a/jobspec/parse_task.go b/jobspec/parse_task.go index f6933778a4b1..1f0444b20382 100644 --- a/jobspec/parse_task.go +++ b/jobspec/parse_task.go @@ -499,7 +499,7 @@ func parseTemplates(result *[]*api.Template, list *ast.ObjectList) error { changeScriptBlock := o.Items[0] // check for invalid fields - valid := []string{"path", "args", "timeout", "fail_on_error"} + valid := []string{"command", "args", "timeout", "fail_on_error"} if err := checkHCLKeys(changeScriptBlock.Val, valid); err != nil { return multierror.Prefix(err, "change_script ->") } diff --git a/jobspec/parse_test.go b/jobspec/parse_test.go index e335cc56701c..fb2769e7fa3e 100644 --- a/jobspec/parse_test.go +++ b/jobspec/parse_test.go @@ -389,7 +389,7 @@ func TestParse(t *testing.T) { ChangeMode: stringToPtr(templateChangeModeScript), ChangeScript: &api.ChangeScript{ Args: []string{"-debug", "-verbose"}, - Path: stringToPtr("/bin/foo"), + Command: stringToPtr("/bin/foo"), Timeout: timeToPtr(5 * time.Second), FailOnError: boolToPtr(false), }, diff --git a/jobspec/test-fixtures/basic.hcl b/jobspec/test-fixtures/basic.hcl index 50b21ce68eab..a749bf91dda2 100644 --- a/jobspec/test-fixtures/basic.hcl +++ b/jobspec/test-fixtures/basic.hcl @@ -319,7 +319,7 @@ job "binstore-storagelocker" { destination = "bar" change_mode = "script" change_script { - path = "/bin/foo" + command = "/bin/foo" args = ["-debug", "-verbose"] timeout = "5s" fail_on_error = false diff --git a/nomad/structs/diff_test.go b/nomad/structs/diff_test.go index 5d7a0ca4d8fe..4fe336bcb4bc 100644 --- a/nomad/structs/diff_test.go +++ b/nomad/structs/diff_test.go @@ -7043,7 +7043,7 @@ func TestTaskDiff(t *testing.T) { ChangeMode: "bam", ChangeSignal: "SIGHUP", ChangeScript: &ChangeScript{ - Path: "/bin/foo", + Command: "/bin/foo", Args: []string{"-debug"}, Timeout: 5, FailOnError: false, @@ -7064,7 +7064,7 @@ func TestTaskDiff(t *testing.T) { ChangeMode: "bam2", ChangeSignal: "SIGHUP2", ChangeScript: &ChangeScript{ - Path: "/bin/foo2", + Command: "/bin/foo2", Args: []string{"-debugs"}, Timeout: 6, FailOnError: false, @@ -7086,7 +7086,7 @@ func TestTaskDiff(t *testing.T) { ChangeMode: "bam", ChangeSignal: "SIGHUP", ChangeScript: &ChangeScript{ - Path: "/bin/foo", + Command: "/bin/foo", Args: []string{"-debug"}, Timeout: 5, FailOnError: false, @@ -7107,7 +7107,7 @@ func TestTaskDiff(t *testing.T) { ChangeMode: "bam3", ChangeSignal: "SIGHUP3", ChangeScript: &ChangeScript{ - Path: "/bin/foo3", + Command: "/bin/foo3", Args: []string{"-debugss"}, Timeout: 7, FailOnError: false, diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index d1b864980a4d..739ff80794de 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -7701,8 +7701,8 @@ var ( // ChangeScript holds the configuration for the script that is executed if // change mode is set to script type ChangeScript struct { - // Path is the full path to the script - Path string + // Command is the full path to the script + Command string // Args is a slice of arguments passed to the script Args []string // Timeout is the amount of seconds we wait for the script to finish @@ -7834,7 +7834,7 @@ func (t *Template) Validate() error { _ = multierror.Append(&mErr, fmt.Errorf("cannot use signals with env var templates")) } case TemplateChangeModeScript: - if t.ChangeScript.Path == "" { + if t.ChangeScript.Command == "" { _ = multierror.Append(&mErr, fmt.Errorf("must specify script path value when change mode is script")) } default: diff --git a/website/content/api-docs/json-jobs.mdx b/website/content/api-docs/json-jobs.mdx index cfe2408cab37..4753406a997c 100644 --- a/website/content/api-docs/json-jobs.mdx +++ b/website/content/api-docs/json-jobs.mdx @@ -1066,10 +1066,10 @@ README][ct]. The `ChangeScript` object supports the following attributes: - - `path` - Specifies the full path to a script that is to be executed on - template change. Path is relative to the driver, e.g., if running with a - container driver the path must be existing in the container. This option is - required is the `change_mode` is `script`. + - `command` - Specifies the full path to a script or executable that is to be + executed on template change. Path is relative to the driver, e.g., if running + with a container driver the path must be existing in the container. This + option is required is the `change_mode` is `script`. - `args` - List of arguments that are passed to the script that is to be executed on template change. diff --git a/website/content/docs/job-specification/change_script.mdx b/website/content/docs/job-specification/change_script.mdx index 037356510f47..960288e52140 100644 --- a/website/content/docs/job-specification/change_script.mdx +++ b/website/content/docs/job-specification/change_script.mdx @@ -21,7 +21,7 @@ job "docs" { destination = "local/redis.conf" change_mode = "script" change_script { - path = "/bin/foo" + command = "/bin/foo" args = ["-verbose", "-debug"] timeout = "5s" fail_on_error = false @@ -34,9 +34,9 @@ job "docs" { ## `change_script` Parameters -- `path` `(string: "")` - Specifies the full path to a script that - is to be executed on template change. Path is relative to the driver, e.g., if - running with a container driver the path must be existing in the container. +- `command` `(string: "")` - Specifies the full path to a script or executable + that is to be executed on template change. Path is relative to the driver, e.g., + if running with a container driver the path must be existing in the container. This option is required is the `change_mode` is `script`. - `args` `(array: [])` - List of arguments that are passed to the script From 4b7b306932890a1387d60a5605219f2e247e85b9 Mon Sep 17 00:00:00 2001 From: Piotr Kazmierczak Date: Fri, 19 Aug 2022 10:23:07 +0200 Subject: [PATCH 53/68] another script example in the docs --- .../docs/job-specification/change_script.mdx | 36 ++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-) diff --git a/website/content/docs/job-specification/change_script.mdx b/website/content/docs/job-specification/change_script.mdx index 960288e52140..7b908790b913 100644 --- a/website/content/docs/job-specification/change_script.mdx +++ b/website/content/docs/job-specification/change_script.mdx @@ -47,4 +47,38 @@ job "docs" { - `fail_on_error` `(bool: false)` - If `true`, Nomad will kill the task if the script execution fails. If `false`, script failure will be logged but the task - will continue uninterrupted. \ No newline at end of file + will continue uninterrupted. + +### Template as a script example + +Below is an example of how a script can be embedded in a `data` block of another +`template` stanza: + +```hcl +job "docs" { + group "example" { + task "server" { + template { + data = "{{key \"abc\"}}" + destination = "local/test" + change_mode = "script" + + change_script { + path = "/local/script.sh" + } + } + + template { + data = < Date: Fri, 19 Aug 2022 10:49:38 +0200 Subject: [PATCH 54/68] simplified script execution processing --- .../taskrunner/template/template.go | 78 ++++++++++--------- 1 file changed, 40 insertions(+), 38 deletions(-) diff --git a/client/allocrunner/taskrunner/template/template.go b/client/allocrunner/taskrunner/template/template.go index c1dd9c299764..0d3ec6d0787a 100644 --- a/client/allocrunner/taskrunner/template/template.go +++ b/client/allocrunner/taskrunner/template/template.go @@ -512,48 +512,50 @@ func (tm *TaskTemplateManager) onTemplateRendered(handledRenders map[string]time var wg sync.WaitGroup for _, script := range scripts { wg.Add(1) - go func(script *structs.ChangeScript) { - defer wg.Done() - if tm.handle == nil { - tm.config.Events.EmitEvent(structs.NewTaskEvent(structs.TaskHookFailed). + go tm.processScript(script, &wg) + } + wg.Wait() +} + +func (tm *TaskTemplateManager) processScript(script *structs.ChangeScript, wg *sync.WaitGroup) { + defer wg.Done() + if tm.handle == nil { + tm.config.Events.EmitEvent(structs.NewTaskEvent(structs.TaskHookFailed). + SetDisplayMessage( + "Template failed to run a change script because task driver doesn't support the exec operation", + )) + if script.FailOnError { + tm.config.Lifecycle.Kill(context.Background(), + structs.NewTaskEvent(structs.TaskKilling). + SetFailsTask(). SetDisplayMessage( - "Template failed to run a change script because task driver doesn't support the exec operation", + fmt.Sprintf("Template failed to run script %v and the task is being killed", script.Command), )) - if script.FailOnError { - tm.config.Lifecycle.Kill(context.Background(), - structs.NewTaskEvent(structs.TaskKilling). - SetFailsTask(). - SetDisplayMessage( - fmt.Sprintf("Template failed to run script %v and the task is being killed", script.Command), - )) - } - return - } - _, exitCode, err := tm.handle.Exec(script.Timeout, script.Command, script.Args) - if err != nil { - structs.NewTaskEvent(structs.TaskHookFailed). + } + return + } + _, exitCode, err := tm.handle.Exec(script.Timeout, script.Command, script.Args) + if err != nil { + structs.NewTaskEvent(structs.TaskHookFailed). + SetDisplayMessage( + fmt.Sprintf( + "Template failed to run script %v on change: %v Exit code: %v", script.Command, err, exitCode, + )) + if script.FailOnError { + tm.config.Lifecycle.Kill(context.Background(), + structs.NewTaskEvent(structs.TaskKilling). + SetFailsTask(). SetDisplayMessage( - fmt.Sprintf( - "Template failed to run script %v on change: %v Exit code: %v", script.Command, err, exitCode, - )) - if script.FailOnError { - tm.config.Lifecycle.Kill(context.Background(), - structs.NewTaskEvent(structs.TaskKilling). - SetFailsTask(). - SetDisplayMessage( - fmt.Sprintf("Template failed to run script %v and the task is being killed", script.Command), - )) - } - return - } - tm.config.Events.EmitEvent(structs.NewTaskEvent(structs.TaskHookMessage). - SetDisplayMessage( - fmt.Sprintf( - "Template successfully ran a script from %v with exit code: %v", script.Command, exitCode, - ))) - }(script) + fmt.Sprintf("Template failed to run script %v and the task is being killed", script.Command), + )) + } + return } - wg.Wait() + tm.config.Events.EmitEvent(structs.NewTaskEvent(structs.TaskHookMessage). + SetDisplayMessage( + fmt.Sprintf( + "Template successfully ran a script from %v with exit code: %v", script.Command, exitCode, + ))) } // allTemplatesNoop returns whether all the managed templates have change mode noop. From f1050e190fd5d21bd7f7ddd79faaa9d9df013416 Mon Sep 17 00:00:00 2001 From: Piotr Kazmierczak Date: Fri, 19 Aug 2022 11:13:00 +0200 Subject: [PATCH 55/68] handle exit code --- .../taskrunner/template/template.go | 49 +++++++++++++------ 1 file changed, 33 insertions(+), 16 deletions(-) diff --git a/client/allocrunner/taskrunner/template/template.go b/client/allocrunner/taskrunner/template/template.go index 0d3ec6d0787a..2c90c6f1c25f 100644 --- a/client/allocrunner/taskrunner/template/template.go +++ b/client/allocrunner/taskrunner/template/template.go @@ -517,21 +517,30 @@ func (tm *TaskTemplateManager) onTemplateRendered(handledRenders map[string]time wg.Wait() } +// failOnError is a helper function that produces a TaskKilling event and emits +// a message +func (tm *TaskTemplateManager) failOnError(script *structs.ChangeScript, msg string) { + if script.FailOnError { + tm.config.Lifecycle.Kill(context.Background(), + structs.NewTaskEvent(structs.TaskKilling). + SetFailsTask(). + SetDisplayMessage(msg)) + } +} + +// processScript is used for executing change_mode script and handling errors func (tm *TaskTemplateManager) processScript(script *structs.ChangeScript, wg *sync.WaitGroup) { defer wg.Done() + if tm.handle == nil { tm.config.Events.EmitEvent(structs.NewTaskEvent(structs.TaskHookFailed). SetDisplayMessage( "Template failed to run a change script because task driver doesn't support the exec operation", )) - if script.FailOnError { - tm.config.Lifecycle.Kill(context.Background(), - structs.NewTaskEvent(structs.TaskKilling). - SetFailsTask(). - SetDisplayMessage( - fmt.Sprintf("Template failed to run script %v and the task is being killed", script.Command), - )) - } + tm.failOnError( + script, + fmt.Sprintf("Template failed to run script %v and the task is being killed", script.Command), + ) return } _, exitCode, err := tm.handle.Exec(script.Timeout, script.Command, script.Args) @@ -541,14 +550,22 @@ func (tm *TaskTemplateManager) processScript(script *structs.ChangeScript, wg *s fmt.Sprintf( "Template failed to run script %v on change: %v Exit code: %v", script.Command, err, exitCode, )) - if script.FailOnError { - tm.config.Lifecycle.Kill(context.Background(), - structs.NewTaskEvent(structs.TaskKilling). - SetFailsTask(). - SetDisplayMessage( - fmt.Sprintf("Template failed to run script %v and the task is being killed", script.Command), - )) - } + tm.failOnError( + script, + fmt.Sprintf("Template failed to run script %v and the task is being killed", script.Command), + ) + return + } + if exitCode != 0 { + structs.NewTaskEvent(structs.TaskHookFailed). + SetDisplayMessage( + fmt.Sprintf( + "Template ran script %v on change: %v Exit code: %v", script.Command, err, exitCode, + )) + tm.failOnError( + script, + fmt.Sprintf("Template ran script %v but it exited with code %v and the task is being killed", + script.Command, exitCode)) return } tm.config.Events.EmitEvent(structs.NewTaskEvent(structs.TaskHookMessage). From 6190282d268ae027798cd794061e8450041e6b3f Mon Sep 17 00:00:00 2001 From: Piotr Kazmierczak Date: Fri, 19 Aug 2022 16:35:35 +0200 Subject: [PATCH 56/68] fix diff_test --- nomad/structs/diff_test.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/nomad/structs/diff_test.go b/nomad/structs/diff_test.go index 4fe336bcb4bc..d624884bcb77 100644 --- a/nomad/structs/diff_test.go +++ b/nomad/structs/diff_test.go @@ -7248,15 +7248,15 @@ func TestTaskDiff(t *testing.T) { Fields: []*FieldDiff{ { Type: DiffTypeAdded, - Name: "FailOnError", + Name: "Command", Old: "", - New: "false", + New: "/bin/foo3", }, { Type: DiffTypeAdded, - Name: "Path", + Name: "FailOnError", Old: "", - New: "/bin/foo3", + New: "false", }, { Type: DiffTypeAdded, @@ -7360,14 +7360,14 @@ func TestTaskDiff(t *testing.T) { Fields: []*FieldDiff{ { Type: DiffTypeDeleted, - Name: "FailOnError", - Old: "false", + Name: "Command", + Old: "/bin/foo2", New: "", }, { Type: DiffTypeDeleted, - Name: "Path", - Old: "/bin/foo2", + Name: "FailOnError", + Old: "false", New: "", }, { From 953c091a7ccd7b150eacfd929e2a89300bb209c5 Mon Sep 17 00:00:00 2001 From: Piotr Kazmierczak Date: Fri, 19 Aug 2022 16:49:24 +0200 Subject: [PATCH 57/68] allowed_exit_codes --- api/tasks.go | 12 ++-- .../taskrunner/template/template.go | 3 +- .../taskrunner/template/template_test.go | 36 +++++----- command/agent/job_endpoint.go | 9 +-- command/agent/job_endpoint_test.go | 18 ++--- jobspec/parse_task.go | 8 ++- jobspec/parse_test.go | 9 +-- jobspec2/parse_job.go | 4 ++ nomad/structs/diff.go | 66 +++++++++++++++++++ nomad/structs/diff_test.go | 60 ++++++++++++----- nomad/structs/structs.go | 4 +- website/content/api-docs/json-jobs.mdx | 3 + .../docs/job-specification/change_script.mdx | 15 +++-- 13 files changed, 187 insertions(+), 60 deletions(-) diff --git a/api/tasks.go b/api/tasks.go index 4fcf95a06f7b..b590a36fec93 100644 --- a/api/tasks.go +++ b/api/tasks.go @@ -792,10 +792,11 @@ func (wc *WaitConfig) Copy() *WaitConfig { } type ChangeScript struct { - Command *string `mapstructure:"command" hcl:"command"` - Args []string `mapstructure:"args" hcl:"args,optional"` - Timeout *time.Duration `mapstructure:"timeout" hcl:"timeout,optional"` - FailOnError *bool `mapstructure:"fail_on_error" hcl:"fail_on_error"` + Command *string `mapstructure:"command" hcl:"command"` + Args []string `mapstructure:"args" hcl:"args,optional"` + Timeout *time.Duration `mapstructure:"timeout" hcl:"timeout,optional"` + FailOnError *bool `mapstructure:"fail_on_error" hcl:"fail_on_error"` + AllowedExitCodes []int `mapstructure:"allowed_exit_codes" hcl:"allowed_exit_codes"` } func (ch *ChangeScript) Canonicalize() { @@ -811,6 +812,9 @@ func (ch *ChangeScript) Canonicalize() { if ch.FailOnError == nil { ch.FailOnError = pointerOf(false) } + if ch.AllowedExitCodes == nil { + ch.AllowedExitCodes = []int{0} + } } type Template struct { diff --git a/client/allocrunner/taskrunner/template/template.go b/client/allocrunner/taskrunner/template/template.go index 2c90c6f1c25f..63df6e7d68c5 100644 --- a/client/allocrunner/taskrunner/template/template.go +++ b/client/allocrunner/taskrunner/template/template.go @@ -22,6 +22,7 @@ import ( "github.com/hashicorp/nomad/client/taskenv" "github.com/hashicorp/nomad/helper/pointer" "github.com/hashicorp/nomad/nomad/structs" + "golang.org/x/exp/slices" ) const ( @@ -556,7 +557,7 @@ func (tm *TaskTemplateManager) processScript(script *structs.ChangeScript, wg *s ) return } - if exitCode != 0 { + if !slices.Contains(script.AllowedExitCodes, exitCode) { structs.NewTaskEvent(structs.TaskHookFailed). SetDisplayMessage( fmt.Sprintf( diff --git a/client/allocrunner/taskrunner/template/template_test.go b/client/allocrunner/taskrunner/template/template_test.go index 722e2e23267c..b9514be76dd0 100644 --- a/client/allocrunner/taskrunner/template/template_test.go +++ b/client/allocrunner/taskrunner/template/template_test.go @@ -1224,10 +1224,11 @@ FOO={{key "bam"}} DestPath: "test.env", ChangeMode: structs.TemplateChangeModeScript, ChangeScript: &structs.ChangeScript{ - Command: "/bin/foo", - Args: []string{}, - Timeout: 5 * time.Second, - FailOnError: false, + Command: "/bin/foo", + Args: []string{}, + Timeout: 5 * time.Second, + FailOnError: false, + AllowedExitCodes: []int{0}, }, Envvars: true, } @@ -1238,10 +1239,11 @@ BAR={{key "bar"}} DestPath: "test2.env", ChangeMode: structs.TemplateChangeModeScript, ChangeScript: &structs.ChangeScript{ - Command: "/bin/foo", - Args: []string{}, - Timeout: 5 * time.Second, - FailOnError: false, + Command: "/bin/foo", + Args: []string{}, + Timeout: 5 * time.Second, + FailOnError: false, + AllowedExitCodes: []int{0}, }, Envvars: true, } @@ -1310,10 +1312,11 @@ FOO={{key "bam"}} DestPath: "test.env", ChangeMode: structs.TemplateChangeModeScript, ChangeScript: &structs.ChangeScript{ - Command: "/bin/foo", - Args: []string{}, - Timeout: 5 * time.Second, - FailOnError: true, + Command: "/bin/foo", + Args: []string{}, + Timeout: 5 * time.Second, + FailOnError: true, + AllowedExitCodes: []int{0}, }, Envvars: true, } @@ -1324,10 +1327,11 @@ BAR={{key "bar"}} DestPath: "test2.env", ChangeMode: structs.TemplateChangeModeScript, ChangeScript: &structs.ChangeScript{ - Command: "/bin/foo", - Args: []string{}, - Timeout: 5 * time.Second, - FailOnError: false, + Command: "/bin/foo", + Args: []string{}, + Timeout: 5 * time.Second, + FailOnError: false, + AllowedExitCodes: []int{0}, }, Envvars: true, } diff --git a/command/agent/job_endpoint.go b/command/agent/job_endpoint.go index c09727556ac5..b1d803a0e798 100644 --- a/command/agent/job_endpoint.go +++ b/command/agent/job_endpoint.go @@ -1271,10 +1271,11 @@ func apiChangeScriptToStructsChangeScript(changeScript *api.ChangeScript) *struc } return &structs.ChangeScript{ - Command: *changeScript.Command, - Args: changeScript.Args, - Timeout: *changeScript.Timeout, - FailOnError: *changeScript.FailOnError, + Command: *changeScript.Command, + Args: changeScript.Args, + Timeout: *changeScript.Timeout, + FailOnError: *changeScript.FailOnError, + AllowedExitCodes: changeScript.AllowedExitCodes, } } diff --git a/command/agent/job_endpoint_test.go b/command/agent/job_endpoint_test.go index f64094cd2fe0..fef27ba94ae8 100644 --- a/command/agent/job_endpoint_test.go +++ b/command/agent/job_endpoint_test.go @@ -2732,10 +2732,11 @@ func TestJobs_ApiJobToStructsJob(t *testing.T) { ChangeMode: pointer.Of("change"), ChangeSignal: pointer.Of("signal"), ChangeScript: &api.ChangeScript{ - Command: pointer.Of("/bin/foo"), - Args: []string{"-h"}, - Timeout: pointer.Of(5 * time.Second), - FailOnError: pointer.Of(false), + Command: pointer.Of("/bin/foo"), + Args: []string{"-h"}, + Timeout: pointer.Of(5 * time.Second), + FailOnError: pointer.Of(false), + AllowedExitCodes: []int{0}, }, Splay: pointer.Of(1 * time.Minute), Perms: pointer.Of("666"), @@ -3145,10 +3146,11 @@ func TestJobs_ApiJobToStructsJob(t *testing.T) { ChangeMode: "change", ChangeSignal: "SIGNAL", ChangeScript: &structs.ChangeScript{ - Command: "/bin/foo", - Args: []string{"-h"}, - Timeout: 5 * time.Second, - FailOnError: false, + Command: "/bin/foo", + Args: []string{"-h"}, + Timeout: 5 * time.Second, + FailOnError: false, + AllowedExitCodes: []int{0}, }, Splay: 1 * time.Minute, Perms: "666", diff --git a/jobspec/parse_task.go b/jobspec/parse_task.go index 1f0444b20382..09ceade0eefb 100644 --- a/jobspec/parse_task.go +++ b/jobspec/parse_task.go @@ -499,7 +499,13 @@ func parseTemplates(result *[]*api.Template, list *ast.ObjectList) error { changeScriptBlock := o.Items[0] // check for invalid fields - valid := []string{"command", "args", "timeout", "fail_on_error"} + valid := []string{ + "command", + "args", + "timeout", + "fail_on_error", + "allowed_exit_codes", + } if err := checkHCLKeys(changeScriptBlock.Val, valid); err != nil { return multierror.Prefix(err, "change_script ->") } diff --git a/jobspec/parse_test.go b/jobspec/parse_test.go index fb2769e7fa3e..32534b4a3499 100644 --- a/jobspec/parse_test.go +++ b/jobspec/parse_test.go @@ -388,10 +388,11 @@ func TestParse(t *testing.T) { DestPath: stringToPtr("bar"), ChangeMode: stringToPtr(templateChangeModeScript), ChangeScript: &api.ChangeScript{ - Args: []string{"-debug", "-verbose"}, - Command: stringToPtr("/bin/foo"), - Timeout: timeToPtr(5 * time.Second), - FailOnError: boolToPtr(false), + Args: []string{"-debug", "-verbose"}, + Command: stringToPtr("/bin/foo"), + Timeout: timeToPtr(5 * time.Second), + FailOnError: boolToPtr(false), + AllowedExitCodes: []int{0}, }, Splay: timeToPtr(5 * time.Second), Perms: stringToPtr("777"), diff --git a/jobspec2/parse_job.go b/jobspec2/parse_job.go index dcacfa457f65..053e603d73d0 100644 --- a/jobspec2/parse_job.go +++ b/jobspec2/parse_job.go @@ -137,4 +137,8 @@ func normalizeChangeScript(ch *api.ChangeScript) { if ch.FailOnError == nil { ch.FailOnError = pointer.Of(false) } + + if ch.AllowedExitCodes == nil { + ch.AllowedExitCodes = []int{0} + } } diff --git a/nomad/structs/diff.go b/nomad/structs/diff.go index b5fecc4d29ef..d2a065353920 100644 --- a/nomad/structs/diff.go +++ b/nomad/structs/diff.go @@ -4,6 +4,7 @@ import ( "fmt" "reflect" "sort" + "strconv" "strings" "github.com/hashicorp/nomad/helper/flatmap" @@ -1679,6 +1680,16 @@ func changeScriptDiff(old, new *ChangeScript, contextual bool) *ObjectDiff { diff.Objects = append(diff.Objects, setDiff) } + // AllowedExitCodes diffs + if setDiff := intSetDiff( + old.AllowedExitCodes, + new.AllowedExitCodes, + "AllowedExitCodes", + contextual, + ); setDiff != nil { + diff.Objects = append(diff.Objects, setDiff) + } + return diff } @@ -2573,6 +2584,61 @@ func stringSetDiff(old, new []string, name string, contextual bool) *ObjectDiff return diff } +// intSetDiff diffs two sets of ints with the given name. +func intSetDiff(old, new []int, name string, contextual bool) *ObjectDiff { + oldMap := make(map[int]struct{}, len(old)) + newMap := make(map[int]struct{}, len(new)) + for _, o := range old { + oldMap[o] = struct{}{} + } + for _, n := range new { + newMap[n] = struct{}{} + } + if reflect.DeepEqual(oldMap, newMap) && !contextual { + return nil + } + + diff := &ObjectDiff{Name: name} + var added, removed bool + for k := range oldMap { + kk := strconv.Itoa(k) + if _, ok := newMap[k]; !ok { + diff.Fields = append(diff.Fields, fieldDiff(kk, "", name, contextual)) + removed = true + } else if contextual { + diff.Fields = append(diff.Fields, fieldDiff(kk, kk, name, contextual)) + } + } + + for k := range newMap { + kk := strconv.Itoa(k) + if _, ok := oldMap[k]; !ok { + diff.Fields = append(diff.Fields, fieldDiff("", kk, name, contextual)) + added = true + } + } + + sort.Sort(FieldDiffs(diff.Fields)) + + // Determine the type + if added && removed { + diff.Type = DiffTypeEdited + } else if added { + diff.Type = DiffTypeAdded + } else if removed { + diff.Type = DiffTypeDeleted + } else { + // Diff of an empty set + if len(diff.Fields) == 0 { + return nil + } + + diff.Type = DiffTypeNone + } + + return diff +} + // primitiveObjectDiff returns a diff of the passed objects' primitive fields. // The filter field can be used to exclude fields from the diff. The name is the // name of the objects. If contextual is set, non-changed fields will also be diff --git a/nomad/structs/diff_test.go b/nomad/structs/diff_test.go index d624884bcb77..37d92458c47f 100644 --- a/nomad/structs/diff_test.go +++ b/nomad/structs/diff_test.go @@ -7043,10 +7043,11 @@ func TestTaskDiff(t *testing.T) { ChangeMode: "bam", ChangeSignal: "SIGHUP", ChangeScript: &ChangeScript{ - Command: "/bin/foo", - Args: []string{"-debug"}, - Timeout: 5, - FailOnError: false, + Command: "/bin/foo", + Args: []string{"-debug"}, + Timeout: 5, + FailOnError: false, + AllowedExitCodes: []int{0}, }, Splay: 1, Perms: "0644", @@ -7064,10 +7065,11 @@ func TestTaskDiff(t *testing.T) { ChangeMode: "bam2", ChangeSignal: "SIGHUP2", ChangeScript: &ChangeScript{ - Command: "/bin/foo2", - Args: []string{"-debugs"}, - Timeout: 6, - FailOnError: false, + Command: "/bin/foo2", + Args: []string{"-debugs"}, + Timeout: 6, + FailOnError: false, + AllowedExitCodes: []int{0}, }, Splay: 2, Perms: "0666", @@ -7086,10 +7088,11 @@ func TestTaskDiff(t *testing.T) { ChangeMode: "bam", ChangeSignal: "SIGHUP", ChangeScript: &ChangeScript{ - Command: "/bin/foo", - Args: []string{"-debug"}, - Timeout: 5, - FailOnError: false, + Command: "/bin/foo", + Args: []string{"-debug"}, + Timeout: 5, + FailOnError: false, + AllowedExitCodes: []int{0}, }, Splay: 1, Perms: "0644", @@ -7107,10 +7110,11 @@ func TestTaskDiff(t *testing.T) { ChangeMode: "bam3", ChangeSignal: "SIGHUP3", ChangeScript: &ChangeScript{ - Command: "/bin/foo3", - Args: []string{"-debugss"}, - Timeout: 7, - FailOnError: false, + Command: "/bin/foo3", + Args: []string{"-debugss"}, + Timeout: 7, + FailOnError: false, + AllowedExitCodes: []int{0}, }, Splay: 3, Perms: "0776", @@ -7266,6 +7270,18 @@ func TestTaskDiff(t *testing.T) { }, }, Objects: []*ObjectDiff{ + { + Type: DiffTypeAdded, + Name: "AllowedExitCodes", + Fields: []*FieldDiff{ + { + Type: DiffTypeAdded, + Name: "AllowedExitCodes", + Old: "", + New: "0", + }, + }, + }, { Type: DiffTypeAdded, Name: "Args", @@ -7378,6 +7394,18 @@ func TestTaskDiff(t *testing.T) { }, }, Objects: []*ObjectDiff{ + { + Type: DiffTypeDeleted, + Name: "AllowedExitCodes", + Fields: []*FieldDiff{ + { + Type: DiffTypeDeleted, + Name: "AllowedExitCodes", + Old: "0", + New: "", + }, + }, + }, { Type: DiffTypeDeleted, Name: "Args", diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 739ff80794de..703e350df9e3 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -7710,6 +7710,8 @@ type ChangeScript struct { // FailOnError indicates whether a task should fail in case script execution // fails or log script failure and don't interrupt the task FailOnError bool + // AllowedExitCodes lists which script exit codes we consider as clean + AllowedExitCodes []int } // Template represents a template configuration to be rendered for a given task @@ -7835,7 +7837,7 @@ func (t *Template) Validate() error { } case TemplateChangeModeScript: if t.ChangeScript.Command == "" { - _ = multierror.Append(&mErr, fmt.Errorf("must specify script path value when change mode is script")) + _ = multierror.Append(&mErr, fmt.Errorf("must specify command value when change mode is script")) } default: _ = multierror.Append(&mErr, TemplateChangeModeInvalidError) diff --git a/website/content/api-docs/json-jobs.mdx b/website/content/api-docs/json-jobs.mdx index 4753406a997c..c252f1cebb9e 100644 --- a/website/content/api-docs/json-jobs.mdx +++ b/website/content/api-docs/json-jobs.mdx @@ -1081,6 +1081,9 @@ README][ct]. execution fails. If `false`, script failure will be logged but the task will continue uninterrupted. Default value is `false`. + - `allowed_exit_codes` - List of integers that are considered clean exit codes + for the script. + - `DestPath` - Specifies the location where the resulting template should be rendered, relative to the task directory. diff --git a/website/content/docs/job-specification/change_script.mdx b/website/content/docs/job-specification/change_script.mdx index 7b908790b913..bdbda6e57cf7 100644 --- a/website/content/docs/job-specification/change_script.mdx +++ b/website/content/docs/job-specification/change_script.mdx @@ -21,10 +21,11 @@ job "docs" { destination = "local/redis.conf" change_mode = "script" change_script { - command = "/bin/foo" - args = ["-verbose", "-debug"] - timeout = "5s" - fail_on_error = false + command = "/bin/foo" + args = ["-verbose", "-debug"] + timeout = "5s" + fail_on_error = false + allowed_exit_codes = [0, 2] } } } @@ -49,6 +50,10 @@ job "docs" { script execution fails. If `false`, script failure will be logged but the task will continue uninterrupted. +- `allowed_exit_codes` `(array: [0])` - List of exit codes that are + considered clean, i.e., if a script exits with one of the codes in that list and + `fail_on_error` is true, Nomad will not kill the task. + ### Template as a script example Below is an example of how a script can be embedded in a `data` block of another @@ -64,7 +69,7 @@ job "docs" { change_mode = "script" change_script { - path = "/local/script.sh" + command = "/local/script.sh" } } From 4add2b0a69878b99f1616883afec6bf8afda7612 Mon Sep 17 00:00:00 2001 From: Piotr Kazmierczak Date: Fri, 19 Aug 2022 17:40:28 +0200 Subject: [PATCH 58/68] Revert "allowed_exit_codes" This reverts commit 953c091a7ccd7b150eacfd929e2a89300bb209c5. --- api/tasks.go | 12 ++-- .../taskrunner/template/template.go | 3 +- .../taskrunner/template/template_test.go | 36 +++++----- command/agent/job_endpoint.go | 9 ++- command/agent/job_endpoint_test.go | 18 +++-- jobspec/parse_task.go | 8 +-- jobspec/parse_test.go | 9 ++- jobspec2/parse_job.go | 4 -- nomad/structs/diff.go | 66 ------------------- nomad/structs/diff_test.go | 60 +++++------------ nomad/structs/structs.go | 4 +- website/content/api-docs/json-jobs.mdx | 3 - .../docs/job-specification/change_script.mdx | 15 ++--- 13 files changed, 60 insertions(+), 187 deletions(-) diff --git a/api/tasks.go b/api/tasks.go index b590a36fec93..4fcf95a06f7b 100644 --- a/api/tasks.go +++ b/api/tasks.go @@ -792,11 +792,10 @@ func (wc *WaitConfig) Copy() *WaitConfig { } type ChangeScript struct { - Command *string `mapstructure:"command" hcl:"command"` - Args []string `mapstructure:"args" hcl:"args,optional"` - Timeout *time.Duration `mapstructure:"timeout" hcl:"timeout,optional"` - FailOnError *bool `mapstructure:"fail_on_error" hcl:"fail_on_error"` - AllowedExitCodes []int `mapstructure:"allowed_exit_codes" hcl:"allowed_exit_codes"` + Command *string `mapstructure:"command" hcl:"command"` + Args []string `mapstructure:"args" hcl:"args,optional"` + Timeout *time.Duration `mapstructure:"timeout" hcl:"timeout,optional"` + FailOnError *bool `mapstructure:"fail_on_error" hcl:"fail_on_error"` } func (ch *ChangeScript) Canonicalize() { @@ -812,9 +811,6 @@ func (ch *ChangeScript) Canonicalize() { if ch.FailOnError == nil { ch.FailOnError = pointerOf(false) } - if ch.AllowedExitCodes == nil { - ch.AllowedExitCodes = []int{0} - } } type Template struct { diff --git a/client/allocrunner/taskrunner/template/template.go b/client/allocrunner/taskrunner/template/template.go index 63df6e7d68c5..2c90c6f1c25f 100644 --- a/client/allocrunner/taskrunner/template/template.go +++ b/client/allocrunner/taskrunner/template/template.go @@ -22,7 +22,6 @@ import ( "github.com/hashicorp/nomad/client/taskenv" "github.com/hashicorp/nomad/helper/pointer" "github.com/hashicorp/nomad/nomad/structs" - "golang.org/x/exp/slices" ) const ( @@ -557,7 +556,7 @@ func (tm *TaskTemplateManager) processScript(script *structs.ChangeScript, wg *s ) return } - if !slices.Contains(script.AllowedExitCodes, exitCode) { + if exitCode != 0 { structs.NewTaskEvent(structs.TaskHookFailed). SetDisplayMessage( fmt.Sprintf( diff --git a/client/allocrunner/taskrunner/template/template_test.go b/client/allocrunner/taskrunner/template/template_test.go index b9514be76dd0..722e2e23267c 100644 --- a/client/allocrunner/taskrunner/template/template_test.go +++ b/client/allocrunner/taskrunner/template/template_test.go @@ -1224,11 +1224,10 @@ FOO={{key "bam"}} DestPath: "test.env", ChangeMode: structs.TemplateChangeModeScript, ChangeScript: &structs.ChangeScript{ - Command: "/bin/foo", - Args: []string{}, - Timeout: 5 * time.Second, - FailOnError: false, - AllowedExitCodes: []int{0}, + Command: "/bin/foo", + Args: []string{}, + Timeout: 5 * time.Second, + FailOnError: false, }, Envvars: true, } @@ -1239,11 +1238,10 @@ BAR={{key "bar"}} DestPath: "test2.env", ChangeMode: structs.TemplateChangeModeScript, ChangeScript: &structs.ChangeScript{ - Command: "/bin/foo", - Args: []string{}, - Timeout: 5 * time.Second, - FailOnError: false, - AllowedExitCodes: []int{0}, + Command: "/bin/foo", + Args: []string{}, + Timeout: 5 * time.Second, + FailOnError: false, }, Envvars: true, } @@ -1312,11 +1310,10 @@ FOO={{key "bam"}} DestPath: "test.env", ChangeMode: structs.TemplateChangeModeScript, ChangeScript: &structs.ChangeScript{ - Command: "/bin/foo", - Args: []string{}, - Timeout: 5 * time.Second, - FailOnError: true, - AllowedExitCodes: []int{0}, + Command: "/bin/foo", + Args: []string{}, + Timeout: 5 * time.Second, + FailOnError: true, }, Envvars: true, } @@ -1327,11 +1324,10 @@ BAR={{key "bar"}} DestPath: "test2.env", ChangeMode: structs.TemplateChangeModeScript, ChangeScript: &structs.ChangeScript{ - Command: "/bin/foo", - Args: []string{}, - Timeout: 5 * time.Second, - FailOnError: false, - AllowedExitCodes: []int{0}, + Command: "/bin/foo", + Args: []string{}, + Timeout: 5 * time.Second, + FailOnError: false, }, Envvars: true, } diff --git a/command/agent/job_endpoint.go b/command/agent/job_endpoint.go index b1d803a0e798..c09727556ac5 100644 --- a/command/agent/job_endpoint.go +++ b/command/agent/job_endpoint.go @@ -1271,11 +1271,10 @@ func apiChangeScriptToStructsChangeScript(changeScript *api.ChangeScript) *struc } return &structs.ChangeScript{ - Command: *changeScript.Command, - Args: changeScript.Args, - Timeout: *changeScript.Timeout, - FailOnError: *changeScript.FailOnError, - AllowedExitCodes: changeScript.AllowedExitCodes, + Command: *changeScript.Command, + Args: changeScript.Args, + Timeout: *changeScript.Timeout, + FailOnError: *changeScript.FailOnError, } } diff --git a/command/agent/job_endpoint_test.go b/command/agent/job_endpoint_test.go index fef27ba94ae8..f64094cd2fe0 100644 --- a/command/agent/job_endpoint_test.go +++ b/command/agent/job_endpoint_test.go @@ -2732,11 +2732,10 @@ func TestJobs_ApiJobToStructsJob(t *testing.T) { ChangeMode: pointer.Of("change"), ChangeSignal: pointer.Of("signal"), ChangeScript: &api.ChangeScript{ - Command: pointer.Of("/bin/foo"), - Args: []string{"-h"}, - Timeout: pointer.Of(5 * time.Second), - FailOnError: pointer.Of(false), - AllowedExitCodes: []int{0}, + Command: pointer.Of("/bin/foo"), + Args: []string{"-h"}, + Timeout: pointer.Of(5 * time.Second), + FailOnError: pointer.Of(false), }, Splay: pointer.Of(1 * time.Minute), Perms: pointer.Of("666"), @@ -3146,11 +3145,10 @@ func TestJobs_ApiJobToStructsJob(t *testing.T) { ChangeMode: "change", ChangeSignal: "SIGNAL", ChangeScript: &structs.ChangeScript{ - Command: "/bin/foo", - Args: []string{"-h"}, - Timeout: 5 * time.Second, - FailOnError: false, - AllowedExitCodes: []int{0}, + Command: "/bin/foo", + Args: []string{"-h"}, + Timeout: 5 * time.Second, + FailOnError: false, }, Splay: 1 * time.Minute, Perms: "666", diff --git a/jobspec/parse_task.go b/jobspec/parse_task.go index 09ceade0eefb..1f0444b20382 100644 --- a/jobspec/parse_task.go +++ b/jobspec/parse_task.go @@ -499,13 +499,7 @@ func parseTemplates(result *[]*api.Template, list *ast.ObjectList) error { changeScriptBlock := o.Items[0] // check for invalid fields - valid := []string{ - "command", - "args", - "timeout", - "fail_on_error", - "allowed_exit_codes", - } + valid := []string{"command", "args", "timeout", "fail_on_error"} if err := checkHCLKeys(changeScriptBlock.Val, valid); err != nil { return multierror.Prefix(err, "change_script ->") } diff --git a/jobspec/parse_test.go b/jobspec/parse_test.go index 32534b4a3499..fb2769e7fa3e 100644 --- a/jobspec/parse_test.go +++ b/jobspec/parse_test.go @@ -388,11 +388,10 @@ func TestParse(t *testing.T) { DestPath: stringToPtr("bar"), ChangeMode: stringToPtr(templateChangeModeScript), ChangeScript: &api.ChangeScript{ - Args: []string{"-debug", "-verbose"}, - Command: stringToPtr("/bin/foo"), - Timeout: timeToPtr(5 * time.Second), - FailOnError: boolToPtr(false), - AllowedExitCodes: []int{0}, + Args: []string{"-debug", "-verbose"}, + Command: stringToPtr("/bin/foo"), + Timeout: timeToPtr(5 * time.Second), + FailOnError: boolToPtr(false), }, Splay: timeToPtr(5 * time.Second), Perms: stringToPtr("777"), diff --git a/jobspec2/parse_job.go b/jobspec2/parse_job.go index 053e603d73d0..dcacfa457f65 100644 --- a/jobspec2/parse_job.go +++ b/jobspec2/parse_job.go @@ -137,8 +137,4 @@ func normalizeChangeScript(ch *api.ChangeScript) { if ch.FailOnError == nil { ch.FailOnError = pointer.Of(false) } - - if ch.AllowedExitCodes == nil { - ch.AllowedExitCodes = []int{0} - } } diff --git a/nomad/structs/diff.go b/nomad/structs/diff.go index d2a065353920..b5fecc4d29ef 100644 --- a/nomad/structs/diff.go +++ b/nomad/structs/diff.go @@ -4,7 +4,6 @@ import ( "fmt" "reflect" "sort" - "strconv" "strings" "github.com/hashicorp/nomad/helper/flatmap" @@ -1680,16 +1679,6 @@ func changeScriptDiff(old, new *ChangeScript, contextual bool) *ObjectDiff { diff.Objects = append(diff.Objects, setDiff) } - // AllowedExitCodes diffs - if setDiff := intSetDiff( - old.AllowedExitCodes, - new.AllowedExitCodes, - "AllowedExitCodes", - contextual, - ); setDiff != nil { - diff.Objects = append(diff.Objects, setDiff) - } - return diff } @@ -2584,61 +2573,6 @@ func stringSetDiff(old, new []string, name string, contextual bool) *ObjectDiff return diff } -// intSetDiff diffs two sets of ints with the given name. -func intSetDiff(old, new []int, name string, contextual bool) *ObjectDiff { - oldMap := make(map[int]struct{}, len(old)) - newMap := make(map[int]struct{}, len(new)) - for _, o := range old { - oldMap[o] = struct{}{} - } - for _, n := range new { - newMap[n] = struct{}{} - } - if reflect.DeepEqual(oldMap, newMap) && !contextual { - return nil - } - - diff := &ObjectDiff{Name: name} - var added, removed bool - for k := range oldMap { - kk := strconv.Itoa(k) - if _, ok := newMap[k]; !ok { - diff.Fields = append(diff.Fields, fieldDiff(kk, "", name, contextual)) - removed = true - } else if contextual { - diff.Fields = append(diff.Fields, fieldDiff(kk, kk, name, contextual)) - } - } - - for k := range newMap { - kk := strconv.Itoa(k) - if _, ok := oldMap[k]; !ok { - diff.Fields = append(diff.Fields, fieldDiff("", kk, name, contextual)) - added = true - } - } - - sort.Sort(FieldDiffs(diff.Fields)) - - // Determine the type - if added && removed { - diff.Type = DiffTypeEdited - } else if added { - diff.Type = DiffTypeAdded - } else if removed { - diff.Type = DiffTypeDeleted - } else { - // Diff of an empty set - if len(diff.Fields) == 0 { - return nil - } - - diff.Type = DiffTypeNone - } - - return diff -} - // primitiveObjectDiff returns a diff of the passed objects' primitive fields. // The filter field can be used to exclude fields from the diff. The name is the // name of the objects. If contextual is set, non-changed fields will also be diff --git a/nomad/structs/diff_test.go b/nomad/structs/diff_test.go index 37d92458c47f..d624884bcb77 100644 --- a/nomad/structs/diff_test.go +++ b/nomad/structs/diff_test.go @@ -7043,11 +7043,10 @@ func TestTaskDiff(t *testing.T) { ChangeMode: "bam", ChangeSignal: "SIGHUP", ChangeScript: &ChangeScript{ - Command: "/bin/foo", - Args: []string{"-debug"}, - Timeout: 5, - FailOnError: false, - AllowedExitCodes: []int{0}, + Command: "/bin/foo", + Args: []string{"-debug"}, + Timeout: 5, + FailOnError: false, }, Splay: 1, Perms: "0644", @@ -7065,11 +7064,10 @@ func TestTaskDiff(t *testing.T) { ChangeMode: "bam2", ChangeSignal: "SIGHUP2", ChangeScript: &ChangeScript{ - Command: "/bin/foo2", - Args: []string{"-debugs"}, - Timeout: 6, - FailOnError: false, - AllowedExitCodes: []int{0}, + Command: "/bin/foo2", + Args: []string{"-debugs"}, + Timeout: 6, + FailOnError: false, }, Splay: 2, Perms: "0666", @@ -7088,11 +7086,10 @@ func TestTaskDiff(t *testing.T) { ChangeMode: "bam", ChangeSignal: "SIGHUP", ChangeScript: &ChangeScript{ - Command: "/bin/foo", - Args: []string{"-debug"}, - Timeout: 5, - FailOnError: false, - AllowedExitCodes: []int{0}, + Command: "/bin/foo", + Args: []string{"-debug"}, + Timeout: 5, + FailOnError: false, }, Splay: 1, Perms: "0644", @@ -7110,11 +7107,10 @@ func TestTaskDiff(t *testing.T) { ChangeMode: "bam3", ChangeSignal: "SIGHUP3", ChangeScript: &ChangeScript{ - Command: "/bin/foo3", - Args: []string{"-debugss"}, - Timeout: 7, - FailOnError: false, - AllowedExitCodes: []int{0}, + Command: "/bin/foo3", + Args: []string{"-debugss"}, + Timeout: 7, + FailOnError: false, }, Splay: 3, Perms: "0776", @@ -7270,18 +7266,6 @@ func TestTaskDiff(t *testing.T) { }, }, Objects: []*ObjectDiff{ - { - Type: DiffTypeAdded, - Name: "AllowedExitCodes", - Fields: []*FieldDiff{ - { - Type: DiffTypeAdded, - Name: "AllowedExitCodes", - Old: "", - New: "0", - }, - }, - }, { Type: DiffTypeAdded, Name: "Args", @@ -7394,18 +7378,6 @@ func TestTaskDiff(t *testing.T) { }, }, Objects: []*ObjectDiff{ - { - Type: DiffTypeDeleted, - Name: "AllowedExitCodes", - Fields: []*FieldDiff{ - { - Type: DiffTypeDeleted, - Name: "AllowedExitCodes", - Old: "0", - New: "", - }, - }, - }, { Type: DiffTypeDeleted, Name: "Args", diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 703e350df9e3..739ff80794de 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -7710,8 +7710,6 @@ type ChangeScript struct { // FailOnError indicates whether a task should fail in case script execution // fails or log script failure and don't interrupt the task FailOnError bool - // AllowedExitCodes lists which script exit codes we consider as clean - AllowedExitCodes []int } // Template represents a template configuration to be rendered for a given task @@ -7837,7 +7835,7 @@ func (t *Template) Validate() error { } case TemplateChangeModeScript: if t.ChangeScript.Command == "" { - _ = multierror.Append(&mErr, fmt.Errorf("must specify command value when change mode is script")) + _ = multierror.Append(&mErr, fmt.Errorf("must specify script path value when change mode is script")) } default: _ = multierror.Append(&mErr, TemplateChangeModeInvalidError) diff --git a/website/content/api-docs/json-jobs.mdx b/website/content/api-docs/json-jobs.mdx index c252f1cebb9e..4753406a997c 100644 --- a/website/content/api-docs/json-jobs.mdx +++ b/website/content/api-docs/json-jobs.mdx @@ -1081,9 +1081,6 @@ README][ct]. execution fails. If `false`, script failure will be logged but the task will continue uninterrupted. Default value is `false`. - - `allowed_exit_codes` - List of integers that are considered clean exit codes - for the script. - - `DestPath` - Specifies the location where the resulting template should be rendered, relative to the task directory. diff --git a/website/content/docs/job-specification/change_script.mdx b/website/content/docs/job-specification/change_script.mdx index bdbda6e57cf7..7b908790b913 100644 --- a/website/content/docs/job-specification/change_script.mdx +++ b/website/content/docs/job-specification/change_script.mdx @@ -21,11 +21,10 @@ job "docs" { destination = "local/redis.conf" change_mode = "script" change_script { - command = "/bin/foo" - args = ["-verbose", "-debug"] - timeout = "5s" - fail_on_error = false - allowed_exit_codes = [0, 2] + command = "/bin/foo" + args = ["-verbose", "-debug"] + timeout = "5s" + fail_on_error = false } } } @@ -50,10 +49,6 @@ job "docs" { script execution fails. If `false`, script failure will be logged but the task will continue uninterrupted. -- `allowed_exit_codes` `(array: [0])` - List of exit codes that are - considered clean, i.e., if a script exits with one of the codes in that list and - `fail_on_error` is true, Nomad will not kill the task. - ### Template as a script example Below is an example of how a script can be embedded in a `data` block of another @@ -69,7 +64,7 @@ job "docs" { change_mode = "script" change_script { - command = "/local/script.sh" + path = "/local/script.sh" } } From f9daf968e817077736b232042eadb961470310ab Mon Sep 17 00:00:00 2001 From: Piotr Kazmierczak Date: Fri, 19 Aug 2022 18:35:37 +0200 Subject: [PATCH 59/68] simplified processScript --- .../taskrunner/template/template.go | 51 ++++++++++--------- 1 file changed, 27 insertions(+), 24 deletions(-) diff --git a/client/allocrunner/taskrunner/template/template.go b/client/allocrunner/taskrunner/template/template.go index 2c90c6f1c25f..416a23f5f420 100644 --- a/client/allocrunner/taskrunner/template/template.go +++ b/client/allocrunner/taskrunner/template/template.go @@ -533,45 +533,48 @@ func (tm *TaskTemplateManager) processScript(script *structs.ChangeScript, wg *s defer wg.Done() if tm.handle == nil { - tm.config.Events.EmitEvent(structs.NewTaskEvent(structs.TaskHookFailed). - SetDisplayMessage( - "Template failed to run a change script because task driver doesn't support the exec operation", - )) - tm.failOnError( - script, - fmt.Sprintf("Template failed to run script %v and the task is being killed", script.Command), + failure_msg := fmt.Sprintf( + "Template failed to run script %v with arguments %v because task driver doesn't support the exec operation", + script.Command, + script.Args, ) + tm.config.Events.EmitEvent(structs.NewTaskEvent(structs.TaskHookFailed). + SetDisplayMessage(failure_msg)) + tm.failOnError(script, failure_msg+" Task is being killed.") return } _, exitCode, err := tm.handle.Exec(script.Timeout, script.Command, script.Args) if err != nil { - structs.NewTaskEvent(structs.TaskHookFailed). - SetDisplayMessage( - fmt.Sprintf( - "Template failed to run script %v on change: %v Exit code: %v", script.Command, err, exitCode, - )) - tm.failOnError( - script, - fmt.Sprintf("Template failed to run script %v and the task is being killed", script.Command), + failure_msg := fmt.Sprintf( + "Template failed to run script %v with arguments %v on change: %v Exit code: %v", + script.Command, + script.Args, + err, + exitCode, ) + structs.NewTaskEvent(structs.TaskHookFailed).SetDisplayMessage(failure_msg) + tm.failOnError(script, failure_msg+" Task is being killed. ") return } if exitCode != 0 { + failure_msg := fmt.Sprintf( + "Template ran script %v with arguments %v on change but it exited with code code: %v", + script.Command, + script.Args, + exitCode, + ) structs.NewTaskEvent(structs.TaskHookFailed). - SetDisplayMessage( - fmt.Sprintf( - "Template ran script %v on change: %v Exit code: %v", script.Command, err, exitCode, - )) - tm.failOnError( - script, - fmt.Sprintf("Template ran script %v but it exited with code %v and the task is being killed", - script.Command, exitCode)) + SetDisplayMessage(failure_msg) + tm.failOnError(script, failure_msg+" Task is being killed. ") return } tm.config.Events.EmitEvent(structs.NewTaskEvent(structs.TaskHookMessage). SetDisplayMessage( fmt.Sprintf( - "Template successfully ran a script from %v with exit code: %v", script.Command, exitCode, + "Template successfully ran a script from %v with arguments: %v. Exit code: %v", + script.Command, + script.Args, + exitCode, ))) } From b0e71818ac678515bff43a571a03b68f8209c7b6 Mon Sep 17 00:00:00 2001 From: Piotr Kazmierczak Date: Tue, 23 Aug 2022 10:13:49 +0200 Subject: [PATCH 60/68] Apply suggestions from code review Co-authored-by: Luiz Aoqui --- client/allocrunner/taskrunner/template/template.go | 7 ++++--- website/content/docs/job-specification/change_script.mdx | 8 ++++---- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/client/allocrunner/taskrunner/template/template.go b/client/allocrunner/taskrunner/template/template.go index 416a23f5f420..1e6746452e85 100644 --- a/client/allocrunner/taskrunner/template/template.go +++ b/client/allocrunner/taskrunner/template/template.go @@ -199,8 +199,9 @@ func (tm *TaskTemplateManager) Stop() { // SetDriverHandle sets the executor func (tm *TaskTemplateManager) SetDriverHandle(executor interfaces.ScriptExecutor) { tm.handleLock.Lock() + defer tm.handleLock.Unlock() tm.handle = executor - tm.handleLock.Unlock() + } // run is the long lived loop that handles errors and templates being rendered @@ -545,7 +546,7 @@ func (tm *TaskTemplateManager) processScript(script *structs.ChangeScript, wg *s } _, exitCode, err := tm.handle.Exec(script.Timeout, script.Command, script.Args) if err != nil { - failure_msg := fmt.Sprintf( + failureMsg := fmt.Sprintf( "Template failed to run script %v with arguments %v on change: %v Exit code: %v", script.Command, script.Args, @@ -571,7 +572,7 @@ func (tm *TaskTemplateManager) processScript(script *structs.ChangeScript, wg *s tm.config.Events.EmitEvent(structs.NewTaskEvent(structs.TaskHookMessage). SetDisplayMessage( fmt.Sprintf( - "Template successfully ran a script from %v with arguments: %v. Exit code: %v", + "Template successfully ran script %v with arguments: %v. Exit code: %v", script.Command, script.Args, exitCode, diff --git a/website/content/docs/job-specification/change_script.mdx b/website/content/docs/job-specification/change_script.mdx index 7b908790b913..e2dfd15106a2 100644 --- a/website/content/docs/job-specification/change_script.mdx +++ b/website/content/docs/job-specification/change_script.mdx @@ -35,15 +35,15 @@ job "docs" { ## `change_script` Parameters - `command` `(string: "")` - Specifies the full path to a script or executable - that is to be executed on template change. Path is relative to the driver, e.g., + that is to be executed on template change. The command must return exit code 0 to be considered successful. Path is relative to the driver, e.g., if running with a container driver the path must be existing in the container. - This option is required is the `change_mode` is `script`. + This option is required if `change_mode` is `script`. - `args` `(array: [])` - List of arguments that are passed to the script that is to be executed on template change. - `timeout` `(string: "5s")` - Timeout for script execution specified using a - label suffix like "30s" or "1h". + label suffix like `"30s"` or `"1h"`. - `fail_on_error` `(bool: false)` - If `true`, Nomad will kill the task if the script execution fails. If `false`, script failure will be logged but the task @@ -59,7 +59,7 @@ job "docs" { group "example" { task "server" { template { - data = "{{key \"abc\"}}" + data = "{{key \"my_key\"}}" destination = "local/test" change_mode = "script" From b9d0da2002f0383b1436caeb02bf3bd987a90fb2 Mon Sep 17 00:00:00 2001 From: Piotr Kazmierczak Date: Tue, 23 Aug 2022 10:43:41 +0200 Subject: [PATCH 61/68] refactored processScript --- .../taskrunner/template/template.go | 29 ++++++++++--------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/client/allocrunner/taskrunner/template/template.go b/client/allocrunner/taskrunner/template/template.go index 46e8a632abe6..85d6140ff611 100644 --- a/client/allocrunner/taskrunner/template/template.go +++ b/client/allocrunner/taskrunner/template/template.go @@ -518,14 +518,17 @@ func (tm *TaskTemplateManager) onTemplateRendered(handledRenders map[string]time wg.Wait() } -// failOnError is a helper function that produces a TaskKilling event and emits -// a message -func (tm *TaskTemplateManager) failOnError(script *structs.ChangeScript, msg string) { +// handleScriptError is a helper function that produces a TaskKilling event and +// emits a message +func (tm *TaskTemplateManager) handleScriptError(script *structs.ChangeScript, msg string) { + ev := structs.NewTaskEvent(structs.TaskHookFailed).SetDisplayMessage(msg) + tm.config.Events.EmitEvent(ev) + if script.FailOnError { tm.config.Lifecycle.Kill(context.Background(), structs.NewTaskEvent(structs.TaskKilling). SetFailsTask(). - SetDisplayMessage(msg)) + SetDisplayMessage("Template script failed, task is being killed")) } } @@ -534,14 +537,13 @@ func (tm *TaskTemplateManager) processScript(script *structs.ChangeScript, wg *s defer wg.Done() if tm.handle == nil { - failure_msg := fmt.Sprintf( + failureMsg := fmt.Sprintf( "Template failed to run script %v with arguments %v because task driver doesn't support the exec operation", script.Command, script.Args, ) - tm.config.Events.EmitEvent(structs.NewTaskEvent(structs.TaskHookFailed). - SetDisplayMessage(failure_msg)) - tm.failOnError(script, failure_msg+" Task is being killed.") + tm.config.Events.EmitEvent(structs.NewTaskEvent(structs.TaskHookFailed).SetDisplayMessage(failureMsg)) + tm.handleScriptError(script, failureMsg) return } _, exitCode, err := tm.handle.Exec(script.Timeout, script.Command, script.Args) @@ -553,20 +555,19 @@ func (tm *TaskTemplateManager) processScript(script *structs.ChangeScript, wg *s err, exitCode, ) - structs.NewTaskEvent(structs.TaskHookFailed).SetDisplayMessage(failure_msg) - tm.failOnError(script, failure_msg+" Task is being killed. ") + tm.config.Events.EmitEvent(structs.NewTaskEvent(structs.TaskHookFailed).SetDisplayMessage(failureMsg)) + tm.handleScriptError(script, failureMsg) return } if exitCode != 0 { - failure_msg := fmt.Sprintf( + failureMsg := fmt.Sprintf( "Template ran script %v with arguments %v on change but it exited with code code: %v", script.Command, script.Args, exitCode, ) - structs.NewTaskEvent(structs.TaskHookFailed). - SetDisplayMessage(failure_msg) - tm.failOnError(script, failure_msg+" Task is being killed. ") + tm.config.Events.EmitEvent(structs.NewTaskEvent(structs.TaskHookFailed).SetDisplayMessage(failureMsg)) + tm.handleScriptError(script, failureMsg) return } tm.config.Events.EmitEvent(structs.NewTaskEvent(structs.TaskHookMessage). From d8c4cdafe3ebc4cf55b71a2908be1c7570ea0507 Mon Sep 17 00:00:00 2001 From: Piotr Kazmierczak Date: Tue, 23 Aug 2022 11:25:06 +0200 Subject: [PATCH 62/68] test fix --- client/allocrunner/taskrunner/template/template_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/allocrunner/taskrunner/template/template_test.go b/client/allocrunner/taskrunner/template/template_test.go index 130fa9be6cc0..e4d7ca69ebd2 100644 --- a/client/allocrunner/taskrunner/template/template_test.go +++ b/client/allocrunner/taskrunner/template/template_test.go @@ -1368,7 +1368,7 @@ BAR={{key "bar"}} } require.NotNil(harness.mockHooks.KillEvent) - require.Contains(harness.mockHooks.KillEvent.DisplayMessage, "failed to run script") + require.Contains(harness.mockHooks.KillEvent.DisplayMessage, "task is being killed") } // TestTaskTemplateManager_FiltersProcessEnvVars asserts that we only render From a2a855e66e2c3cdfb28d12452fce13166b87ceba Mon Sep 17 00:00:00 2001 From: Piotr Kazmierczak Date: Tue, 23 Aug 2022 11:51:13 +0200 Subject: [PATCH 63/68] nolint --- client/allocrunner/tasklifecycle/doc.go | 1 + 1 file changed, 1 insertion(+) diff --git a/client/allocrunner/tasklifecycle/doc.go b/client/allocrunner/tasklifecycle/doc.go index a042c23e70ba..967dfabf1215 100644 --- a/client/allocrunner/tasklifecycle/doc.go +++ b/client/allocrunner/tasklifecycle/doc.go @@ -1,3 +1,4 @@ +//nolint /* Package tasklifecycle manages the execution order of tasks based on their lifecycle configuration. Its main structs are the Coordinator and the Gate. From 9629e69787edb8a4a7fa225aedd5b38e45cd7929 Mon Sep 17 00:00:00 2001 From: Piotr Kazmierczak Date: Tue, 23 Aug 2022 12:07:49 +0200 Subject: [PATCH 64/68] removed obsolete code --- jobspec2/parse_job.go | 8 -------- 1 file changed, 8 deletions(-) diff --git a/jobspec2/parse_job.go b/jobspec2/parse_job.go index 8bb7651ef7c5..a4ee50343666 100644 --- a/jobspec2/parse_job.go +++ b/jobspec2/parse_job.go @@ -132,11 +132,3 @@ func normalizeChangeScript(ch *api.ChangeScript) { ch.FailOnError = pointer.Of(false) } } - -func stringToPtr(v string) *string { - return &v -} - -func durationToPtr(v time.Duration) *time.Duration { - return &v -} From b77da29e3669b2372447c7bbe613dc8f72724269 Mon Sep 17 00:00:00 2001 From: Piotr Kazmierczak Date: Wed, 24 Aug 2022 10:10:51 +0200 Subject: [PATCH 65/68] Apply suggestions from code review Co-authored-by: Luiz Aoqui --- client/allocrunner/tasklifecycle/doc.go | 1 - client/allocrunner/taskrunner/template/template.go | 1 - 2 files changed, 2 deletions(-) diff --git a/client/allocrunner/tasklifecycle/doc.go b/client/allocrunner/tasklifecycle/doc.go index 967dfabf1215..a042c23e70ba 100644 --- a/client/allocrunner/tasklifecycle/doc.go +++ b/client/allocrunner/tasklifecycle/doc.go @@ -1,4 +1,3 @@ -//nolint /* Package tasklifecycle manages the execution order of tasks based on their lifecycle configuration. Its main structs are the Coordinator and the Gate. diff --git a/client/allocrunner/taskrunner/template/template.go b/client/allocrunner/taskrunner/template/template.go index 85d6140ff611..79a736f49f65 100644 --- a/client/allocrunner/taskrunner/template/template.go +++ b/client/allocrunner/taskrunner/template/template.go @@ -542,7 +542,6 @@ func (tm *TaskTemplateManager) processScript(script *structs.ChangeScript, wg *s script.Command, script.Args, ) - tm.config.Events.EmitEvent(structs.NewTaskEvent(structs.TaskHookFailed).SetDisplayMessage(failureMsg)) tm.handleScriptError(script, failureMsg) return } From 0a1b5d7f64b77502fe305154c92b4b5a5fbe2b53 Mon Sep 17 00:00:00 2001 From: Piotr Kazmierczak Date: Wed, 24 Aug 2022 10:18:50 +0200 Subject: [PATCH 66/68] Update website/content/api-docs/json-jobs.mdx Co-authored-by: Luiz Aoqui --- website/content/api-docs/json-jobs.mdx | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/website/content/api-docs/json-jobs.mdx b/website/content/api-docs/json-jobs.mdx index 4753406a997c..222201b1e64f 100644 --- a/website/content/api-docs/json-jobs.mdx +++ b/website/content/api-docs/json-jobs.mdx @@ -1066,18 +1066,18 @@ README][ct]. The `ChangeScript` object supports the following attributes: - - `command` - Specifies the full path to a script or executable that is to be + - `Command` - Specifies the full path to a script or executable that is to be executed on template change. Path is relative to the driver, e.g., if running with a container driver the path must be existing in the container. This option is required is the `change_mode` is `script`. - - `args` - List of arguments that are passed to the script that is to be + - `Args` - List of arguments that are passed to the script that is to be executed on template change. - - `timeout` - Timeout for script execution specified using a label suffix like + - `Timeout` - Timeout for script execution specified using a label suffix like "30s" or "1h". Default value is `"5s"`. - - `fail_on_error` - If `true`, Nomad will kill the task if the script + - `FailOnError` - If `true`, Nomad will kill the task if the script execution fails. If `false`, script failure will be logged but the task will continue uninterrupted. Default value is `false`. From 88993bb53903b21c1cba000c42da585ca8dfd7ae Mon Sep 17 00:00:00 2001 From: Piotr Kazmierczak Date: Wed, 24 Aug 2022 10:19:33 +0200 Subject: [PATCH 67/68] formatting --- website/content/api-docs/json-jobs.mdx | 6 +++--- .../docs/job-specification/change_script.mdx | 17 +++++++++-------- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/website/content/api-docs/json-jobs.mdx b/website/content/api-docs/json-jobs.mdx index 222201b1e64f..9bc6896a9472 100644 --- a/website/content/api-docs/json-jobs.mdx +++ b/website/content/api-docs/json-jobs.mdx @@ -1077,9 +1077,9 @@ README][ct]. - `Timeout` - Timeout for script execution specified using a label suffix like "30s" or "1h". Default value is `"5s"`. - - `FailOnError` - If `true`, Nomad will kill the task if the script - execution fails. If `false`, script failure will be logged but the task will - continue uninterrupted. Default value is `false`. + - `FailOnError` - If `true`, Nomad will kill the task if the script execution + fails. If `false`, script failure will be logged but the task will continue + uninterrupted. Default value is `false`. - `DestPath` - Specifies the location where the resulting template should be rendered, relative to the task directory. diff --git a/website/content/docs/job-specification/change_script.mdx b/website/content/docs/job-specification/change_script.mdx index e2dfd15106a2..1b10e36b7136 100644 --- a/website/content/docs/job-specification/change_script.mdx +++ b/website/content/docs/job-specification/change_script.mdx @@ -10,7 +10,7 @@ description: The "change_script" stanza configures a script to be run on templat The `change_script` stanza allows operators to configure scripts that will be executed on template change. This stanza is only used when template -`change_mode` is set to `script`. +`change_mode` is set to `script`. ```hcl job "docs" { @@ -35,19 +35,20 @@ job "docs" { ## `change_script` Parameters - `command` `(string: "")` - Specifies the full path to a script or executable - that is to be executed on template change. The command must return exit code 0 to be considered successful. Path is relative to the driver, e.g., - if running with a container driver the path must be existing in the container. - This option is required if `change_mode` is `script`. + that is to be executed on template change. The command must return exit code 0 + to be considered successful. Path is relative to the driver, e.g., if running + with a container driver the path must be existing in the container. This option + is required if `change_mode` is `script`. - `args` `(array: [])` - List of arguments that are passed to the script - that is to be executed on template change. + that is to be executed on template change. - `timeout` `(string: "5s")` - Timeout for script execution specified using a - label suffix like `"30s"` or `"1h"`. + label suffix like `"30s"` or `"1h"`. - `fail_on_error` `(bool: false)` - If `true`, Nomad will kill the task if the script execution fails. If `false`, script failure will be logged but the task - will continue uninterrupted. + will continue uninterrupted. ### Template as a script example @@ -81,4 +82,4 @@ EOF } } } -``` \ No newline at end of file +``` From 96856babe73d17ed8d43f3c99eed631e88d11a1d Mon Sep 17 00:00:00 2001 From: Piotr Kazmierczak Date: Wed, 24 Aug 2022 17:16:45 +0200 Subject: [PATCH 68/68] removed obsolete code --- client/allocrunner/taskrunner/template/template.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/client/allocrunner/taskrunner/template/template.go b/client/allocrunner/taskrunner/template/template.go index 79a736f49f65..10d45ccb73e3 100644 --- a/client/allocrunner/taskrunner/template/template.go +++ b/client/allocrunner/taskrunner/template/template.go @@ -554,7 +554,6 @@ func (tm *TaskTemplateManager) processScript(script *structs.ChangeScript, wg *s err, exitCode, ) - tm.config.Events.EmitEvent(structs.NewTaskEvent(structs.TaskHookFailed).SetDisplayMessage(failureMsg)) tm.handleScriptError(script, failureMsg) return } @@ -565,7 +564,6 @@ func (tm *TaskTemplateManager) processScript(script *structs.ChangeScript, wg *s script.Args, exitCode, ) - tm.config.Events.EmitEvent(structs.NewTaskEvent(structs.TaskHookFailed).SetDisplayMessage(failureMsg)) tm.handleScriptError(script, failureMsg) return }