From a72e46b4258f3d4863696bc7393c0e1225a8a9c9 Mon Sep 17 00:00:00 2001 From: Phil Renaud Date: Wed, 28 Aug 2024 14:29:06 -0400 Subject: [PATCH 01/13] First pass at passing a tagname and/or diff version to plan/versions requests --- api/jobs.go | 6 +++++ command/agent/job_endpoint.go | 2 ++ command/job_plan.go | 10 ++++++++- nomad/job_endpoint.go | 41 ++++++++++++++++++++++++++++++++++- nomad/structs/structs.go | 6 +++-- 5 files changed, 61 insertions(+), 4 deletions(-) diff --git a/api/jobs.go b/api/jobs.go index da84df73c45f..bf79161c6f70 100644 --- a/api/jobs.go +++ b/api/jobs.go @@ -446,6 +446,8 @@ func (j *Jobs) PeriodicForce(jobID string, q *WriteOptions) (string, *WriteMeta, type PlanOptions struct { Diff bool PolicyOverride bool + DiffVersion string + DiffTagName string } func (j *Jobs) Plan(job *Job, diff bool, q *WriteOptions) (*JobPlanResponse, *WriteMeta, error) { @@ -468,6 +470,8 @@ func (j *Jobs) PlanOpts(job *Job, opts *PlanOptions, q *WriteOptions) (*JobPlanR if opts != nil { req.Diff = opts.Diff req.PolicyOverride = opts.PolicyOverride + req.DiffVersion = opts.DiffVersion + req.DiffTagName = opts.DiffTagName } var resp JobPlanResponse @@ -1477,6 +1481,8 @@ type JobPlanRequest struct { Job *Job Diff bool PolicyOverride bool + DiffVersion string + DiffTagName string WriteRequest } diff --git a/command/agent/job_endpoint.go b/command/agent/job_endpoint.go index 08e37a88f852..49a7a032d54c 100644 --- a/command/agent/job_endpoint.go +++ b/command/agent/job_endpoint.go @@ -188,6 +188,8 @@ func (s *HTTPServer) jobPlan(resp http.ResponseWriter, req *http.Request, planReq := structs.JobPlanRequest{ Job: sJob, Diff: args.Diff, + DiffVersion: args.DiffVersion, + DiffTagName: args.DiffTagName, PolicyOverride: args.PolicyOverride, WriteRequest: *writeReq, } diff --git a/command/job_plan.go b/command/job_plan.go index f28dea2ed72b..53433ea9889e 100644 --- a/command/job_plan.go +++ b/command/job_plan.go @@ -140,6 +140,8 @@ func (c *JobPlanCommand) AutocompleteFlags() complete.Flags { "-vault-token": complete.PredictAnything, "-vault-namespace": complete.PredictAnything, "-var": complete.PredictAnything, + "-tag": complete.PredictAnything, + "-version": complete.PredictAnything, "-var-file": complete.PredictFiles("*.var"), }) } @@ -155,11 +157,13 @@ func (c *JobPlanCommand) AutocompleteArgs() complete.Predictor { func (c *JobPlanCommand) Name() string { return "job plan" } func (c *JobPlanCommand) Run(args []string) int { var diff, policyOverride, verbose bool - var vaultToken, vaultNamespace string + var vaultToken, vaultNamespace, tag, version string flagSet := c.Meta.FlagSet(c.Name(), FlagSetClient) flagSet.Usage = func() { c.Ui.Output(c.Help()) } flagSet.BoolVar(&diff, "diff", true, "") + flagSet.StringVar(&tag, "tag", "", "") + flagSet.StringVar(&version, "version", "", "") flagSet.BoolVar(&policyOverride, "policy-override", false, "") flagSet.BoolVar(&verbose, "verbose", false, "") flagSet.BoolVar(&c.JobGetter.JSON, "json", false, "") @@ -244,6 +248,10 @@ func (c *JobPlanCommand) Run(args []string) int { return c.multiregionPlan(client, job, opts, diff, verbose) } + opts.DiffTagName = tag + opts.DiffVersion = version + // TODO: DiffTagName and DiffVersion are incongruous with one another, throw an error if they're both non-nil. + // Submit the job resp, _, err := client.Jobs().PlanOpts(job, opts, nil) if err != nil { diff --git a/nomad/job_endpoint.go b/nomad/job_endpoint.go index b355640d40f9..530b7beaf73a 100644 --- a/nomad/job_endpoint.go +++ b/nomad/job_endpoint.go @@ -9,6 +9,7 @@ import ( "fmt" "net/http" "sort" + "strconv" "strings" "time" @@ -1298,6 +1299,10 @@ func (j *Job) GetJobVersions(args *structs.JobVersionsRequest, // Setup the output reply.Versions = out if len(out) != 0 { + + compareVersionNumber := 0 + var compareVersion *structs.Job + // Note: a previous assumption here was that the 0th job was the latest, and that we don't modify "old" versions. // Adding version tags breaks this assumption (you can tag an old version, which should unblock /versions queries) so we now look for the highest ModifyIndex. var maxModifyIndex uint64 @@ -1305,12 +1310,21 @@ func (j *Job) GetJobVersions(args *structs.JobVersionsRequest, if job.ModifyIndex > maxModifyIndex { maxModifyIndex = job.ModifyIndex } + if job.Version == uint64(compareVersionNumber) { + compareVersion = job + } } reply.Index = maxModifyIndex + // Compute the diffs if args.Diffs { for i := 0; i < len(out)-1; i++ { - old, new := out[i+1], out[i] + // log out the version number and i + j.logger.Debug("diffing job versions", "version", out[i].Version, "i", i) + // old, new := out[i+1], out[i] + // old.Diff(new) always runs a diff against its previous version; + // let's have it run against version 0 always + old, new := compareVersion, out[i] d, err := old.Diff(new, true) if err != nil { return fmt.Errorf("failed to create job diff: %v", err) @@ -1821,6 +1835,30 @@ func (j *Job) Plan(args *structs.JobPlanRequest, reply *structs.JobPlanResponse) return err } + j.logger.Debug("===== Plan args for specific diffs", "diffVersion", args.DiffVersion, "diffTagName", args.DiffTagName) + + if args.DiffVersion != "" { + // Convert from string to uint64 + diffVersion, _ := strconv.ParseUint(args.DiffVersion, 10, 64) + existingJob, err = snap.JobByIDAndVersion(ws, args.RequestNamespace(), args.Job.ID, diffVersion) + if err != nil { + return err + } + } + + if args.DiffTagName != "" { + versions, err := snap.JobVersionsByID(ws, args.RequestNamespace(), args.Job.ID) + if err != nil { + return err + } + for _, version := range versions { + if version.TaggedVersion != nil && version.TaggedVersion.Name == args.DiffTagName { + existingJob = version + break + } + } + } + policyWarnings, err := j.enforceSubmitJob(args.PolicyOverride, args.Job, existingJob, nomadACLToken, ns) if err != nil { return err @@ -1906,6 +1944,7 @@ func (j *Job) Plan(args *structs.JobPlanRequest, reply *structs.JobPlanResponse) } annotations := planner.Plans[0].Annotations if args.Diff { + jobDiff, err := existingJob.Diff(args.Job, true) if err != nil { return fmt.Errorf("failed to create job diff: %v", err) diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index f9e3812d226b..45d67bd350f6 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -845,8 +845,10 @@ type JobStubFields struct { // JobPlanRequest is used for the Job.Plan endpoint to trigger a dry-run // evaluation of the Job. type JobPlanRequest struct { - Job *Job - Diff bool // Toggles an annotated diff + Job *Job + Diff bool // Toggles an annotated diff + DiffVersion string // shrug emoji + DiffTagName string // shrug emoji // PolicyOverride is set when the user is attempting to override any policies PolicyOverride bool WriteRequest From 379a2f87c6d572d422ade0a379341a7c640ced71 Mon Sep 17 00:00:00 2001 From: Phil Renaud Date: Thu, 29 Aug 2024 17:34:58 -0400 Subject: [PATCH 02/13] versions API now takes compare_to flags --- api/jobs.go | 4 +-- command/agent/job_endpoint.go | 9 ++++-- command/job_history.go | 3 +- command/job_inspect.go | 2 +- command/job_restart.go | 2 +- nomad/job_endpoint.go | 57 ++++++++++++++++++++++++++++++----- nomad/structs/structs.go | 12 +++++--- 7 files changed, 69 insertions(+), 20 deletions(-) diff --git a/api/jobs.go b/api/jobs.go index bf79161c6f70..34ff461b2a99 100644 --- a/api/jobs.go +++ b/api/jobs.go @@ -263,9 +263,9 @@ func (j *Jobs) ScaleStatus(jobID string, q *QueryOptions) (*JobScaleStatusRespon // Versions is used to retrieve all versions of a particular job given its // unique ID. -func (j *Jobs) Versions(jobID string, diffs bool, q *QueryOptions) ([]*Job, []*JobDiff, *QueryMeta, error) { +func (j *Jobs) Versions(jobID string, diffs bool, diffTag string, diffVersion string, q *QueryOptions) ([]*Job, []*JobDiff, *QueryMeta, error) { var resp JobVersionsResponse - qm, err := j.client.query(fmt.Sprintf("/v1/job/%s/versions?diffs=%v", url.PathEscape(jobID), diffs), &resp, q) + qm, err := j.client.query(fmt.Sprintf("/v1/job/%s/versions?diffs=%t&compare_to_tag=%s&compare_to_version=%d", url.PathEscape(jobID), diffs, diffTag, diffVersion), &resp, q) if err != nil { return nil, nil, nil, err } diff --git a/command/agent/job_endpoint.go b/command/agent/job_endpoint.go index 49a7a032d54c..cabccca0aa19 100644 --- a/command/agent/job_endpoint.go +++ b/command/agent/job_endpoint.go @@ -750,6 +750,9 @@ func (s *HTTPServer) jobScaleAction(resp http.ResponseWriter, req *http.Request, func (s *HTTPServer) jobVersions(resp http.ResponseWriter, req *http.Request, jobID string) (interface{}, error) { diffsStr := req.URL.Query().Get("diffs") + diffTagName := req.URL.Query().Get("compare_to_tag") + diffVersion := req.URL.Query().Get("compare_to_version") + var diffsBool bool if diffsStr != "" { var err error @@ -760,8 +763,10 @@ func (s *HTTPServer) jobVersions(resp http.ResponseWriter, req *http.Request, jo } args := structs.JobVersionsRequest{ - JobID: jobID, - Diffs: diffsBool, + JobID: jobID, + Diffs: diffsBool, + DiffVersion: diffVersion, + DiffTagName: diffTagName, } if s.parse(resp, req, &args.Region, &args.QueryOptions) { return nil, nil diff --git a/command/job_history.go b/command/job_history.go index 9b9e753f3adc..93ea4fd80295 100644 --- a/command/job_history.go +++ b/command/job_history.go @@ -136,7 +136,8 @@ func (c *JobHistoryCommand) Run(args []string) int { q := &api.QueryOptions{Namespace: namespace} // Prefix lookup matched a single job - versions, diffs, _, err := client.Jobs().Versions(jobID, diff, q) + // TODO: the empty string params here should probably be new DiffVersion/DiffTagName params. + versions, diffs, _, err := client.Jobs().Versions(jobID, diff, "", "", q) if err != nil { c.Ui.Error(fmt.Sprintf("Error retrieving job versions: %s", err)) return 1 diff --git a/command/job_inspect.go b/command/job_inspect.go index b35ac286c749..91de1df50571 100644 --- a/command/job_inspect.go +++ b/command/job_inspect.go @@ -227,7 +227,7 @@ func getJob(client *api.Client, namespace, jobID string, version *uint64) (*api. return job, err } - versions, _, _, err := client.Jobs().Versions(jobID, false, q) + versions, _, _, err := client.Jobs().Versions(jobID, false, "", "", q) if err != nil { return nil, err } diff --git a/command/job_restart.go b/command/job_restart.go index 30385b85c3a7..13916f7dfe2f 100644 --- a/command/job_restart.go +++ b/command/job_restart.go @@ -305,7 +305,7 @@ func (c *JobRestartCommand) Run(args []string) int { // Retrieve the job history so we can properly determine if a group or task // exists in the specific allocation job version. - jobVersions, _, _, err := c.client.Jobs().Versions(c.jobID, false, nil) + jobVersions, _, _, err := c.client.Jobs().Versions(c.jobID, false, "", "", nil) if err != nil { c.Ui.Error(fmt.Sprintf("Error retrieving versions of job %q: %s", c.jobID, err)) return 1 diff --git a/nomad/job_endpoint.go b/nomad/job_endpoint.go index 530b7beaf73a..48b795b3ca2e 100644 --- a/nomad/job_endpoint.go +++ b/nomad/job_endpoint.go @@ -1300,8 +1300,39 @@ func (j *Job) GetJobVersions(args *structs.JobVersionsRequest, reply.Versions = out if len(out) != 0 { - compareVersionNumber := 0 + var compareVersionNumber uint64 var compareVersion *structs.Job + var compareStatic bool + + if args.DiffTagName != "" { + compareStatic = true + tagFound := false + for _, version := range out { + j.logger.Debug("=== checking for tag version match: ", args.DiffTagName, args.DiffTagName) + if version.TaggedVersion != nil { + j.logger.Debug("=== checking for tag version match check: ", version.TaggedVersion.Name) + } + if version.TaggedVersion != nil && version.TaggedVersion.Name == args.DiffTagName { + j.logger.Debug("=== VERSION TAG MATCH FOUND", "version.Version", version.Version, "Version.Tag.Name", version.TaggedVersion.Name) + tagFound = true + compareVersionNumber = version.Version + break + } else { + j.logger.Debug("=== Version not a match", version.Version) + } + } + if !tagFound { + return fmt.Errorf("tag %q not found", args.DiffTagName) + } + } else if args.DiffVersion != "" { + compareStatic = true + compareVersionNumber, err = strconv.ParseUint(args.DiffVersion, 10, 64) + if err != nil { + return fmt.Errorf("failed to parse diff version: %v", err) + } + } + + j.logger.Debug("===== GetJobVersions and compareVersionNumber is", "compareVersionNumber", compareVersionNumber, "compareStatic", compareStatic, "compareVersionTag", args.DiffTagName) // Note: a previous assumption here was that the 0th job was the latest, and that we don't modify "old" versions. // Adding version tags breaks this assumption (you can tag an old version, which should unblock /versions queries) so we now look for the highest ModifyIndex. @@ -1310,20 +1341,27 @@ func (j *Job) GetJobVersions(args *structs.JobVersionsRequest, if job.ModifyIndex > maxModifyIndex { maxModifyIndex = job.ModifyIndex } - if job.Version == uint64(compareVersionNumber) { + if compareStatic && job.Version == uint64(compareVersionNumber) { compareVersion = job } } reply.Index = maxModifyIndex + if compareStatic && compareVersion == nil { + if args.DiffTagName != "" { + return fmt.Errorf("tag %q not found", args.DiffTagName) + } + return fmt.Errorf("version %q not found", args.DiffVersion) + } + // Compute the diffs if args.Diffs { for i := 0; i < len(out)-1; i++ { - // log out the version number and i - j.logger.Debug("diffing job versions", "version", out[i].Version, "i", i) - // old, new := out[i+1], out[i] - // old.Diff(new) always runs a diff against its previous version; - // let's have it run against version 0 always + if !compareStatic { + compareVersion = out[i+1] + } + j.logger.Debug("diffing job versions", "version", out[i].Version, "i", i, "compareStatic", compareStatic, "compare-version", compareVersion.Version) + old, new := compareVersion, out[i] d, err := old.Diff(new, true) if err != nil { @@ -1839,7 +1877,10 @@ func (j *Job) Plan(args *structs.JobPlanRequest, reply *structs.JobPlanResponse) if args.DiffVersion != "" { // Convert from string to uint64 - diffVersion, _ := strconv.ParseUint(args.DiffVersion, 10, 64) + diffVersion, err := strconv.ParseUint(args.DiffVersion, 10, 64) + if err != nil { + return err + } existingJob, err = snap.JobByIDAndVersion(ws, args.RequestNamespace(), args.Job.ID, diffVersion) if err != nil { return err diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 45d67bd350f6..8166ce400d9f 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -846,9 +846,9 @@ type JobStubFields struct { // evaluation of the Job. type JobPlanRequest struct { Job *Job - Diff bool // Toggles an annotated diff - DiffVersion string // shrug emoji - DiffTagName string // shrug emoji + Diff bool // Toggles an annotated diff + DiffVersion string + DiffTagName string // PolicyOverride is set when the user is attempting to override any policies PolicyOverride bool WriteRequest @@ -1653,8 +1653,10 @@ type JobListResponse struct { // JobVersionsRequest is used to get a jobs versions type JobVersionsRequest struct { - JobID string - Diffs bool + JobID string + Diffs bool + DiffVersion string + DiffTagName string QueryOptions } From 61eec16a3ec6107843ba63fadf23d302ee0b1817 Mon Sep 17 00:00:00 2001 From: Phil Renaud Date: Tue, 3 Sep 2024 10:11:39 -0400 Subject: [PATCH 03/13] Job history command output can have tag names and descriptions --- command/job_history.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/command/job_history.go b/command/job_history.go index 93ea4fd80295..a14a12e6c79b 100644 --- a/command/job_history.go +++ b/command/job_history.go @@ -258,6 +258,13 @@ func (c *JobHistoryCommand) formatJobVersion(job *api.Job, diff *api.JobDiff, ne fmt.Sprintf("Stable|%v", *job.Stable), fmt.Sprintf("Submit Date|%v", formatTime(time.Unix(0, *job.SubmitTime))), } + // if tagged version is not nil + if job.TaggedVersion != nil { + basic = append(basic, fmt.Sprintf("Tag Name|%v", *&job.TaggedVersion.Name)) + if job.TaggedVersion.Description != "" { + basic = append(basic, fmt.Sprintf("Tag Description|%v", *&job.TaggedVersion.Description)) + } + } if diff != nil { //diffStr := fmt.Sprintf("Difference between version %d and %d:", *job.Version, nextVersion) From a05b3c2babef6e857ef7291fb8ca4d234c710a52 Mon Sep 17 00:00:00 2001 From: Phil Renaud Date: Tue, 3 Sep 2024 15:09:02 -0400 Subject: [PATCH 04/13] compare_to to diff-tag and diff-version, plus adding flags to history command --- api/jobs.go | 2 +- command/agent/job_endpoint.go | 7 ++++-- command/job_history.go | 42 +++++++++++++++++++++++++++-------- command/job_plan.go | 28 ++++++++++++++++------- nomad/job_endpoint.go | 30 ++++++++++++++++++------- 5 files changed, 81 insertions(+), 28 deletions(-) diff --git a/api/jobs.go b/api/jobs.go index 34ff461b2a99..93d6651e3fd2 100644 --- a/api/jobs.go +++ b/api/jobs.go @@ -265,7 +265,7 @@ func (j *Jobs) ScaleStatus(jobID string, q *QueryOptions) (*JobScaleStatusRespon // unique ID. func (j *Jobs) Versions(jobID string, diffs bool, diffTag string, diffVersion string, q *QueryOptions) ([]*Job, []*JobDiff, *QueryMeta, error) { var resp JobVersionsResponse - qm, err := j.client.query(fmt.Sprintf("/v1/job/%s/versions?diffs=%t&compare_to_tag=%s&compare_to_version=%d", url.PathEscape(jobID), diffs, diffTag, diffVersion), &resp, q) + qm, err := j.client.query(fmt.Sprintf("/v1/job/%s/versions?diffs=%t&diff_tag=%s&diff_version=%s", url.PathEscape(jobID), diffs, diffTag, diffVersion), &resp, q) if err != nil { return nil, nil, nil, err } diff --git a/command/agent/job_endpoint.go b/command/agent/job_endpoint.go index cabccca0aa19..d1341441d3a5 100644 --- a/command/agent/job_endpoint.go +++ b/command/agent/job_endpoint.go @@ -750,8 +750,11 @@ func (s *HTTPServer) jobScaleAction(resp http.ResponseWriter, req *http.Request, func (s *HTTPServer) jobVersions(resp http.ResponseWriter, req *http.Request, jobID string) (interface{}, error) { diffsStr := req.URL.Query().Get("diffs") - diffTagName := req.URL.Query().Get("compare_to_tag") - diffVersion := req.URL.Query().Get("compare_to_version") + diffTagName := req.URL.Query().Get("diff_tag") + diffVersion := req.URL.Query().Get("diff_version") + + // log out + s.logger.Debug("++==jobVersions", "diffs", diffsStr, "diff_tag", diffTagName, "diff_version", diffVersion) var diffsBool bool if diffsStr != "" { diff --git a/command/job_history.go b/command/job_history.go index a14a12e6c79b..548c83187f53 100644 --- a/command/job_history.go +++ b/command/job_history.go @@ -40,7 +40,17 @@ General Options: History Options: -p - Display the difference between each job and its predecessor. + Display the difference between each version of the job and a reference + version. The reference version can be specified using the -diff-tag or + -diff-version flags. If neither flag is set, the preceding version is used. + + -diff-tag + Specifies the version of the job to compare against, referenced by + tag name (defaults to latest). This tag can be set using the + "nomad job tag" command. + + -diff-version + Specifies the version number of the job to compare against. -full Display the full job definition for each version. @@ -64,11 +74,13 @@ func (c *JobHistoryCommand) Synopsis() string { func (c *JobHistoryCommand) AutocompleteFlags() complete.Flags { return mergeAutocompleteFlags(c.Meta.AutocompleteFlags(FlagSetClient), complete.Flags{ - "-p": complete.PredictNothing, - "-full": complete.PredictNothing, - "-version": complete.PredictAnything, - "-json": complete.PredictNothing, - "-t": complete.PredictAnything, + "-p": complete.PredictNothing, + "-full": complete.PredictNothing, + "-version": complete.PredictAnything, + "-json": complete.PredictNothing, + "-t": complete.PredictAnything, + "-diff-tag": complete.PredictNothing, + "-diff-version": complete.PredictNothing, }) } @@ -91,7 +103,7 @@ func (c *JobHistoryCommand) Name() string { return "job history" } func (c *JobHistoryCommand) Run(args []string) int { var json, diff, full bool - var tmpl, versionStr string + var tmpl, versionStr, diffTag, diffVersion string flags := c.Meta.FlagSet(c.Name(), FlagSetClient) flags.Usage = func() { c.Ui.Output(c.Help()) } @@ -100,6 +112,8 @@ func (c *JobHistoryCommand) Run(args []string) int { flags.BoolVar(&json, "json", false, "") flags.StringVar(&versionStr, "version", "", "") flags.StringVar(&tmpl, "t", "", "") + flags.StringVar(&diffTag, "diff-tag", "", "") + flags.StringVar(&diffVersion, "diff-version", "", "") if err := flags.Parse(args); err != nil { return 1 @@ -118,6 +132,16 @@ func (c *JobHistoryCommand) Run(args []string) int { return 1 } + if (diffTag != "" && !diff) || (diffVersion != "" && !diff) { + c.Ui.Error("-diff-tag and -diff-version can only be used with -p") + return 1 + } + + if diffTag != "" && diffVersion != "" { + c.Ui.Error("-diff-tag and -diff-version are mutually exclusive") + return 1 + } + // Get the HTTP client client, err := c.Meta.Client() if err != nil { @@ -136,8 +160,8 @@ func (c *JobHistoryCommand) Run(args []string) int { q := &api.QueryOptions{Namespace: namespace} // Prefix lookup matched a single job - // TODO: the empty string params here should probably be new DiffVersion/DiffTagName params. - versions, diffs, _, err := client.Jobs().Versions(jobID, diff, "", "", q) + versions, diffs, _, err := client.Jobs().Versions(jobID, diff, diffTag, diffVersion, q) + // TODO: something about diffs isn't ever giving me something for the 0th version. Maybe in job_endpoint.go instead? if err != nil { c.Ui.Error(fmt.Sprintf("Error retrieving job versions: %s", err)) return 1 diff --git a/command/job_plan.go b/command/job_plan.go index 53433ea9889e..5931a0d3c8d2 100644 --- a/command/job_plan.go +++ b/command/job_plan.go @@ -84,6 +84,15 @@ Plan Options: Determines whether the diff between the remote job and planned job is shown. Defaults to true. + -diff-tag + Specifies the version of the remote job to compare against, referenced by + tag name (defaults to latest). This tag can be set using the + "nomad job tag" command. + + -diff-version + Specifies the version number of the remote job to compare against + (defaults to latest). + -json Parses the job file as JSON. If the outer object has a Job field, such as from "nomad job inspect" or "nomad run -output", the value of the field is @@ -140,8 +149,8 @@ func (c *JobPlanCommand) AutocompleteFlags() complete.Flags { "-vault-token": complete.PredictAnything, "-vault-namespace": complete.PredictAnything, "-var": complete.PredictAnything, - "-tag": complete.PredictAnything, - "-version": complete.PredictAnything, + "-diff-tag": complete.PredictAnything, + "-diff-version": complete.PredictAnything, "-var-file": complete.PredictFiles("*.var"), }) } @@ -157,13 +166,13 @@ func (c *JobPlanCommand) AutocompleteArgs() complete.Predictor { func (c *JobPlanCommand) Name() string { return "job plan" } func (c *JobPlanCommand) Run(args []string) int { var diff, policyOverride, verbose bool - var vaultToken, vaultNamespace, tag, version string + var vaultToken, vaultNamespace, diffTag, diffVersion string flagSet := c.Meta.FlagSet(c.Name(), FlagSetClient) flagSet.Usage = func() { c.Ui.Output(c.Help()) } flagSet.BoolVar(&diff, "diff", true, "") - flagSet.StringVar(&tag, "tag", "", "") - flagSet.StringVar(&version, "version", "", "") + flagSet.StringVar(&diffTag, "diff-tag", "", "") + flagSet.StringVar(&diffVersion, "diff-version", "", "") flagSet.BoolVar(&policyOverride, "policy-override", false, "") flagSet.BoolVar(&verbose, "verbose", false, "") flagSet.BoolVar(&c.JobGetter.JSON, "json", false, "") @@ -248,9 +257,12 @@ func (c *JobPlanCommand) Run(args []string) int { return c.multiregionPlan(client, job, opts, diff, verbose) } - opts.DiffTagName = tag - opts.DiffVersion = version - // TODO: DiffTagName and DiffVersion are incongruous with one another, throw an error if they're both non-nil. + opts.DiffTagName = diffTag + opts.DiffVersion = diffVersion + if diffTag != "" && diffVersion != "" { + c.Ui.Error("Cannot specify both -diff-tag and -diff-version") + return 255 + } // Submit the job resp, _, err := client.Jobs().PlanOpts(job, opts, nil) diff --git a/nomad/job_endpoint.go b/nomad/job_endpoint.go index 48b795b3ca2e..7264f5aff062 100644 --- a/nomad/job_endpoint.go +++ b/nomad/job_endpoint.go @@ -1268,6 +1268,8 @@ func (j *Job) GetJob(args *structs.JobSpecificRequest, // GetJobVersions is used to retrieve all tracked versions of a job. func (j *Job) GetJobVersions(args *structs.JobVersionsRequest, reply *structs.JobVersionsResponse) error { + // log + j.logger.Debug("GetJobVersions", "args.DiffVersion", args.DiffVersion, "args.DiffTagName", args.DiffTagName) authErr := j.srv.Authenticate(j.ctx, args) if done, err := j.srv.forward("Job.GetJobVersions", args, args, reply); done { return err @@ -1341,7 +1343,7 @@ func (j *Job) GetJobVersions(args *structs.JobVersionsRequest, if job.ModifyIndex > maxModifyIndex { maxModifyIndex = job.ModifyIndex } - if compareStatic && job.Version == uint64(compareVersionNumber) { + if compareStatic && job.Version == compareVersionNumber { compareVersion = job } } @@ -1868,10 +1870,13 @@ func (j *Job) Plan(args *structs.JobPlanRequest, reply *structs.JobPlanResponse) // Get the original job ws := memdb.NewWatchSet() - existingJob, err := snap.JobByID(ws, args.RequestNamespace(), args.Job.ID) - if err != nil { - return err - } + // existingJob, err := snap.JobByID(ws, args.RequestNamespace(), args.Job.ID) + // if err != nil { + // return err + // } + + // var existingJob interface{} + var existingJob *structs.Job j.logger.Debug("===== Plan args for specific diffs", "diffVersion", args.DiffVersion, "diffTagName", args.DiffTagName) @@ -1885,9 +1890,10 @@ func (j *Job) Plan(args *structs.JobPlanRequest, reply *structs.JobPlanResponse) if err != nil { return err } - } - - if args.DiffTagName != "" { + if existingJob == nil { + return fmt.Errorf("version %q not found", args.DiffVersion) + } + } else if args.DiffTagName != "" { versions, err := snap.JobVersionsByID(ws, args.RequestNamespace(), args.Job.ID) if err != nil { return err @@ -1898,6 +1904,14 @@ func (j *Job) Plan(args *structs.JobPlanRequest, reply *structs.JobPlanResponse) break } } + if existingJob == nil { + return fmt.Errorf("version tag %q not found", args.DiffTagName) + } + } else { + existingJob, err = snap.JobByID(ws, args.RequestNamespace(), args.Job.ID) + if err != nil { + return err + } } policyWarnings, err := j.enforceSubmitJob(args.PolicyOverride, args.Job, existingJob, nomadACLToken, ns) From 068fc15fe7338bfa026823b69ec7894a4e7cd42f Mon Sep 17 00:00:00 2001 From: Phil Renaud Date: Wed, 4 Sep 2024 10:13:27 -0400 Subject: [PATCH 05/13] 0th version now shows a diff if a specific diff target is requested --- api/jobs_test.go | 4 ++-- command/agent/job_endpoint.go | 3 --- command/job_history.go | 8 +++----- nomad/job_endpoint.go | 24 ++++++++---------------- nomad/state/state_store.go | 2 +- 5 files changed, 14 insertions(+), 27 deletions(-) diff --git a/api/jobs_test.go b/api/jobs_test.go index fa68dcf9ae7c..b614683609fb 100644 --- a/api/jobs_test.go +++ b/api/jobs_test.go @@ -1460,7 +1460,7 @@ func TestJobs_Versions(t *testing.T) { jobs := c.Jobs() // Trying to retrieve a job by ID before it exists returns an error - _, _, _, err := jobs.Versions("job1", false, nil) + _, _, _, err := jobs.Versions("job1", false, "", "", nil) must.ErrorContains(t, err, "not found") // Register the job @@ -1470,7 +1470,7 @@ func TestJobs_Versions(t *testing.T) { assertWriteMeta(t, wm) // Query the job again and ensure it exists - result, _, qm, err := jobs.Versions("job1", false, nil) + result, _, qm, err := jobs.Versions("job1", false, "", "", nil) must.NoError(t, err) assertQueryMeta(t, qm) diff --git a/command/agent/job_endpoint.go b/command/agent/job_endpoint.go index d1341441d3a5..cc934caaf6b4 100644 --- a/command/agent/job_endpoint.go +++ b/command/agent/job_endpoint.go @@ -753,9 +753,6 @@ func (s *HTTPServer) jobVersions(resp http.ResponseWriter, req *http.Request, jo diffTagName := req.URL.Query().Get("diff_tag") diffVersion := req.URL.Query().Get("diff_version") - // log out - s.logger.Debug("++==jobVersions", "diffs", diffsStr, "diff_tag", diffTagName, "diff_version", diffVersion) - var diffsBool bool if diffsStr != "" { var err error diff --git a/command/job_history.go b/command/job_history.go index 548c83187f53..498da902e3c2 100644 --- a/command/job_history.go +++ b/command/job_history.go @@ -161,7 +161,6 @@ func (c *JobHistoryCommand) Run(args []string) int { // Prefix lookup matched a single job versions, diffs, _, err := client.Jobs().Versions(jobID, diff, diffTag, diffVersion, q) - // TODO: something about diffs isn't ever giving me something for the 0th version. Maybe in job_endpoint.go instead? if err != nil { c.Ui.Error(fmt.Sprintf("Error retrieving job versions: %s", err)) return 1 @@ -247,15 +246,14 @@ func parseVersion(input string) (uint64, bool, error) { func (c *JobHistoryCommand) formatJobVersions(versions []*api.Job, diffs []*api.JobDiff, full bool) error { vLen := len(versions) dLen := len(diffs) - if dLen != 0 && vLen != dLen+1 { - return fmt.Errorf("Number of job versions %d doesn't match number of diffs %d", vLen, dLen) - } for i, version := range versions { var diff *api.JobDiff var nextVersion uint64 - if i+1 <= dLen { + if dLen > i && diffs[i] != nil { diff = diffs[i] + } + if i+1 < vLen { // if the current version is not the last version nextVersion = *versions[i+1].Version } diff --git a/nomad/job_endpoint.go b/nomad/job_endpoint.go index 7264f5aff062..6849637f8ed3 100644 --- a/nomad/job_endpoint.go +++ b/nomad/job_endpoint.go @@ -1268,8 +1268,6 @@ func (j *Job) GetJob(args *structs.JobSpecificRequest, // GetJobVersions is used to retrieve all tracked versions of a job. func (j *Job) GetJobVersions(args *structs.JobVersionsRequest, reply *structs.JobVersionsResponse) error { - // log - j.logger.Debug("GetJobVersions", "args.DiffVersion", args.DiffVersion, "args.DiffTagName", args.DiffTagName) authErr := j.srv.Authenticate(j.ctx, args) if done, err := j.srv.forward("Job.GetJobVersions", args, args, reply); done { return err @@ -1310,17 +1308,10 @@ func (j *Job) GetJobVersions(args *structs.JobVersionsRequest, compareStatic = true tagFound := false for _, version := range out { - j.logger.Debug("=== checking for tag version match: ", args.DiffTagName, args.DiffTagName) - if version.TaggedVersion != nil { - j.logger.Debug("=== checking for tag version match check: ", version.TaggedVersion.Name) - } if version.TaggedVersion != nil && version.TaggedVersion.Name == args.DiffTagName { - j.logger.Debug("=== VERSION TAG MATCH FOUND", "version.Version", version.Version, "Version.Tag.Name", version.TaggedVersion.Name) tagFound = true compareVersionNumber = version.Version break - } else { - j.logger.Debug("=== Version not a match", version.Version) } } if !tagFound { @@ -1334,8 +1325,6 @@ func (j *Job) GetJobVersions(args *structs.JobVersionsRequest, } } - j.logger.Debug("===== GetJobVersions and compareVersionNumber is", "compareVersionNumber", compareVersionNumber, "compareStatic", compareStatic, "compareVersionTag", args.DiffTagName) - // Note: a previous assumption here was that the 0th job was the latest, and that we don't modify "old" versions. // Adding version tags breaks this assumption (you can tag an old version, which should unblock /versions queries) so we now look for the highest ModifyIndex. var maxModifyIndex uint64 @@ -1358,11 +1347,16 @@ func (j *Job) GetJobVersions(args *structs.JobVersionsRequest, // Compute the diffs if args.Diffs { - for i := 0; i < len(out)-1; i++ { + for i := 0; i < len(out); i++ { + // For each of your job's versions, counting from highest version to lowest + // it should either be compared to the NEXT version in out[], or to the pre-specified version from compareStatic. if !compareStatic { - compareVersion = out[i+1] + if i+1 < len(out) && out[i+1] != nil { + compareVersion = out[i+1] + } else { + break + } } - j.logger.Debug("diffing job versions", "version", out[i].Version, "i", i, "compareStatic", compareStatic, "compare-version", compareVersion.Version) old, new := compareVersion, out[i] d, err := old.Diff(new, true) @@ -1878,8 +1872,6 @@ func (j *Job) Plan(args *structs.JobPlanRequest, reply *structs.JobPlanResponse) // var existingJob interface{} var existingJob *structs.Job - j.logger.Debug("===== Plan args for specific diffs", "diffVersion", args.DiffVersion, "diffTagName", args.DiffTagName) - if args.DiffVersion != "" { // Convert from string to uint64 diffVersion, err := strconv.ParseUint(args.DiffVersion, 10, 64) diff --git a/nomad/state/state_store.go b/nomad/state/state_store.go index 48f4ee4a8abb..bb416c48225b 100644 --- a/nomad/state/state_store.go +++ b/nomad/state/state_store.go @@ -2299,7 +2299,7 @@ func (s *StateStore) jobsByIDPrefixAllNamespaces(ws memdb.WatchSet, prefix strin return wrap, nil } -// JobVersionsByID returns all the tracked versions of a job. +// JobVersionsByID returns all the tracked versions of a job, sorted in from highest version to lowest. func (s *StateStore) JobVersionsByID(ws memdb.WatchSet, namespace, id string) ([]*structs.Job, error) { txn := s.db.ReadTxn() From e2552fffac42a15f4d9fcd9762a1391cb4609683 Mon Sep 17 00:00:00 2001 From: Phil Renaud Date: Fri, 6 Sep 2024 11:03:23 -0400 Subject: [PATCH 06/13] Addressing some PR comments --- api/jobs.go | 38 ++++++++++++++++++--- api/jobs_test.go | 4 +-- command/agent/job_endpoint.go | 13 +++++++- command/job_history.go | 44 ++++++++++++++++-------- command/job_inspect.go | 2 +- command/job_plan.go | 36 +++++++++++++------- command/job_restart.go | 2 +- nomad/job_endpoint.go | 63 ++++++++++++----------------------- nomad/structs/structs.go | 4 +-- 9 files changed, 127 insertions(+), 79 deletions(-) diff --git a/api/jobs.go b/api/jobs.go index 93d6651e3fd2..b3f9b7f7b3b0 100644 --- a/api/jobs.go +++ b/api/jobs.go @@ -263,9 +263,34 @@ func (j *Jobs) ScaleStatus(jobID string, q *QueryOptions) (*JobScaleStatusRespon // Versions is used to retrieve all versions of a particular job given its // unique ID. -func (j *Jobs) Versions(jobID string, diffs bool, diffTag string, diffVersion string, q *QueryOptions) ([]*Job, []*JobDiff, *QueryMeta, error) { +func (j *Jobs) Versions(jobID string, diffs bool, q *QueryOptions) ([]*Job, []*JobDiff, *QueryMeta, error) { + opts := &VersionsOptions{ + Diffs: diffs, + } + return j.VersionsOpts(jobID, opts, q) +} + +type VersionsOptions struct { + Diffs bool + DiffTag string + DiffVersion *uint64 +} + +func (j *Jobs) VersionsOpts(jobID string, opts *VersionsOptions, q *QueryOptions) ([]*Job, []*JobDiff, *QueryMeta, error) { var resp JobVersionsResponse - qm, err := j.client.query(fmt.Sprintf("/v1/job/%s/versions?diffs=%t&diff_tag=%s&diff_version=%s", url.PathEscape(jobID), diffs, diffTag, diffVersion), &resp, q) + + qp := url.Values{} + if opts != nil { + qp.Add("diffs", strconv.FormatBool(opts.Diffs)) + if opts.DiffTag != "" { + qp.Add("diff_tag", opts.DiffTag) + } + if opts.DiffVersion != nil { + qp.Add("diff_version", strconv.FormatUint(*opts.DiffVersion, 10)) + } + } + + qm, err := j.client.query(fmt.Sprintf("/v1/job/%s/versions?%s", url.PathEscape(jobID), qp.Encode()), &resp, q) if err != nil { return nil, nil, nil, err } @@ -446,7 +471,7 @@ func (j *Jobs) PeriodicForce(jobID string, q *WriteOptions) (string, *WriteMeta, type PlanOptions struct { Diff bool PolicyOverride bool - DiffVersion string + DiffVersion *uint64 DiffTagName string } @@ -470,7 +495,10 @@ func (j *Jobs) PlanOpts(job *Job, opts *PlanOptions, q *WriteOptions) (*JobPlanR if opts != nil { req.Diff = opts.Diff req.PolicyOverride = opts.PolicyOverride - req.DiffVersion = opts.DiffVersion + if opts.DiffVersion != nil { + req.DiffVersion = opts.DiffVersion + } + req.DiffTagName = opts.DiffTagName } @@ -1481,7 +1509,7 @@ type JobPlanRequest struct { Job *Job Diff bool PolicyOverride bool - DiffVersion string + DiffVersion *uint64 DiffTagName string WriteRequest } diff --git a/api/jobs_test.go b/api/jobs_test.go index b614683609fb..fa68dcf9ae7c 100644 --- a/api/jobs_test.go +++ b/api/jobs_test.go @@ -1460,7 +1460,7 @@ func TestJobs_Versions(t *testing.T) { jobs := c.Jobs() // Trying to retrieve a job by ID before it exists returns an error - _, _, _, err := jobs.Versions("job1", false, "", "", nil) + _, _, _, err := jobs.Versions("job1", false, nil) must.ErrorContains(t, err, "not found") // Register the job @@ -1470,7 +1470,7 @@ func TestJobs_Versions(t *testing.T) { assertWriteMeta(t, wm) // Query the job again and ensure it exists - result, _, qm, err := jobs.Versions("job1", false, "", "", nil) + result, _, qm, err := jobs.Versions("job1", false, nil) must.NoError(t, err) assertQueryMeta(t, qm) diff --git a/command/agent/job_endpoint.go b/command/agent/job_endpoint.go index cc934caaf6b4..60ff20eb0285 100644 --- a/command/agent/job_endpoint.go +++ b/command/agent/job_endpoint.go @@ -185,6 +185,7 @@ func (s *HTTPServer) jobPlan(resp http.ResponseWriter, req *http.Request, } sJob, writeReq := s.apiJobAndRequestToStructs(args.Job, req, args.WriteRequest) + planReq := structs.JobPlanRequest{ Job: sJob, Diff: args.Diff, @@ -762,10 +763,20 @@ func (s *HTTPServer) jobVersions(resp http.ResponseWriter, req *http.Request, jo } } + var diffVersionInt *uint64 + + if diffVersion != "" { + parsedDiffVersion, err := strconv.ParseUint(diffVersion, 10, 64) + if err != nil { + return nil, fmt.Errorf("Failed to parse value of %q (%v) as a uint64: %v", "diff_version", diffVersion, err) + } + diffVersionInt = &parsedDiffVersion + } + args := structs.JobVersionsRequest{ JobID: jobID, Diffs: diffsBool, - DiffVersion: diffVersion, + DiffVersion: diffVersionInt, DiffTagName: diffTagName, } if s.parse(resp, req, &args.Region, &args.QueryOptions) { diff --git a/command/job_history.go b/command/job_history.go index 498da902e3c2..a3c7ab31e863 100644 --- a/command/job_history.go +++ b/command/job_history.go @@ -41,16 +41,17 @@ History Options: -p Display the difference between each version of the job and a reference - version. The reference version can be specified using the -diff-tag or - -diff-version flags. If neither flag is set, the preceding version is used. + version. The reference version can be specified using the -diff-tag or + -diff-version flags. If neither flag is set, the most recent version is used. - -diff-tag - Specifies the version of the job to compare against, referenced by - tag name (defaults to latest). This tag can be set using the - "nomad job tag" command. + -diff-tag + Specifies the version of the job to compare against, referenced by + tag name (defaults to latest). Mutually exclusive with -diff-version. + This tag can be set using the "nomad job tag" command. - -diff-version - Specifies the version number of the job to compare against. + -diff-version + Specifies the version number of the job to compare against. + Mutually exclusive with -diff-tag. -full Display the full job definition for each version. @@ -103,7 +104,8 @@ func (c *JobHistoryCommand) Name() string { return "job history" } func (c *JobHistoryCommand) Run(args []string) int { var json, diff, full bool - var tmpl, versionStr, diffTag, diffVersion string + var tmpl, versionStr, diffTag, diffVersionFlag string + var diffVersion *uint64 flags := c.Meta.FlagSet(c.Name(), FlagSetClient) flags.Usage = func() { c.Ui.Output(c.Help()) } @@ -113,7 +115,7 @@ func (c *JobHistoryCommand) Run(args []string) int { flags.StringVar(&versionStr, "version", "", "") flags.StringVar(&tmpl, "t", "", "") flags.StringVar(&diffTag, "diff-tag", "", "") - flags.StringVar(&diffVersion, "diff-version", "", "") + flags.StringVar(&diffVersionFlag, "diff-version", "", "") if err := flags.Parse(args); err != nil { return 1 @@ -132,16 +134,25 @@ func (c *JobHistoryCommand) Run(args []string) int { return 1 } - if (diffTag != "" && !diff) || (diffVersion != "" && !diff) { + if (diffTag != "" && !diff) || (diffVersionFlag != "" && !diff) { c.Ui.Error("-diff-tag and -diff-version can only be used with -p") return 1 } - if diffTag != "" && diffVersion != "" { + if diffTag != "" && diffVersionFlag != "" { c.Ui.Error("-diff-tag and -diff-version are mutually exclusive") return 1 } + if diffVersionFlag != "" { + parsedDiffVersion, err := strconv.ParseUint(diffVersionFlag, 10, 64) + if err != nil { + c.Ui.Error(fmt.Sprintf("Error parsing diff version: %s", err)) + return 1 + } + diffVersion = &parsedDiffVersion + } + // Get the HTTP client client, err := c.Meta.Client() if err != nil { @@ -160,7 +171,12 @@ func (c *JobHistoryCommand) Run(args []string) int { q := &api.QueryOptions{Namespace: namespace} // Prefix lookup matched a single job - versions, diffs, _, err := client.Jobs().Versions(jobID, diff, diffTag, diffVersion, q) + versionOptions := &api.VersionsOptions{ + Diffs: diff, + DiffTag: diffTag, + DiffVersion: diffVersion, + } + versions, diffs, _, err := client.Jobs().VersionsOpts(jobID, versionOptions, q) if err != nil { c.Ui.Error(fmt.Sprintf("Error retrieving job versions: %s", err)) return 1 @@ -250,7 +266,7 @@ func (c *JobHistoryCommand) formatJobVersions(versions []*api.Job, diffs []*api. for i, version := range versions { var diff *api.JobDiff var nextVersion uint64 - if dLen > i && diffs[i] != nil { + if dLen > i { diff = diffs[i] } if i+1 < vLen { // if the current version is not the last version diff --git a/command/job_inspect.go b/command/job_inspect.go index 91de1df50571..b35ac286c749 100644 --- a/command/job_inspect.go +++ b/command/job_inspect.go @@ -227,7 +227,7 @@ func getJob(client *api.Client, namespace, jobID string, version *uint64) (*api. return job, err } - versions, _, _, err := client.Jobs().Versions(jobID, false, "", "", q) + versions, _, _, err := client.Jobs().Versions(jobID, false, q) if err != nil { return nil, err } diff --git a/command/job_plan.go b/command/job_plan.go index 5931a0d3c8d2..435d7f78a217 100644 --- a/command/job_plan.go +++ b/command/job_plan.go @@ -7,6 +7,7 @@ import ( "fmt" "os" "sort" + "strconv" "strings" "time" @@ -84,14 +85,14 @@ Plan Options: Determines whether the diff between the remote job and planned job is shown. Defaults to true. - -diff-tag - Specifies the version of the remote job to compare against, referenced by - tag name (defaults to latest). This tag can be set using the - "nomad job tag" command. + -diff-tag + Specifies the version of the remote job to compare against, referenced by + tag name (defaults to latest). This tag can be set using the + "nomad job tag" command. - -diff-version - Specifies the version number of the remote job to compare against - (defaults to latest). + -diff-version + Specifies the version number of the remote job to compare against + (defaults to latest). -json Parses the job file as JSON. If the outer object has a Job field, such as @@ -166,13 +167,14 @@ func (c *JobPlanCommand) AutocompleteArgs() complete.Predictor { func (c *JobPlanCommand) Name() string { return "job plan" } func (c *JobPlanCommand) Run(args []string) int { var diff, policyOverride, verbose bool - var vaultToken, vaultNamespace, diffTag, diffVersion string + var vaultToken, vaultNamespace, diffTag, diffVersionFlag string + var diffVersion *uint64 flagSet := c.Meta.FlagSet(c.Name(), FlagSetClient) flagSet.Usage = func() { c.Ui.Output(c.Help()) } flagSet.BoolVar(&diff, "diff", true, "") flagSet.StringVar(&diffTag, "diff-tag", "", "") - flagSet.StringVar(&diffVersion, "diff-version", "", "") + flagSet.StringVar(&diffVersionFlag, "diff-version", "", "") flagSet.BoolVar(&policyOverride, "policy-override", false, "") flagSet.BoolVar(&verbose, "verbose", false, "") flagSet.BoolVar(&c.JobGetter.JSON, "json", false, "") @@ -257,13 +259,23 @@ func (c *JobPlanCommand) Run(args []string) int { return c.multiregionPlan(client, job, opts, diff, verbose) } - opts.DiffTagName = diffTag - opts.DiffVersion = diffVersion - if diffTag != "" && diffVersion != "" { + if diffTag != "" && diffVersionFlag != "" { c.Ui.Error("Cannot specify both -diff-tag and -diff-version") return 255 } + if diffVersionFlag != "" { + parsedDiffVersion, err := strconv.ParseUint(diffVersionFlag, 10, 64) + if err != nil { + c.Ui.Error(fmt.Sprintf("Error parsing diff version: %s", err)) + return 1 + } + diffVersion = &parsedDiffVersion + } + + opts.DiffTagName = diffTag + opts.DiffVersion = diffVersion + // Submit the job resp, _, err := client.Jobs().PlanOpts(job, opts, nil) if err != nil { diff --git a/command/job_restart.go b/command/job_restart.go index 13916f7dfe2f..30385b85c3a7 100644 --- a/command/job_restart.go +++ b/command/job_restart.go @@ -305,7 +305,7 @@ func (c *JobRestartCommand) Run(args []string) int { // Retrieve the job history so we can properly determine if a group or task // exists in the specific allocation job version. - jobVersions, _, _, err := c.client.Jobs().Versions(c.jobID, false, "", "", nil) + jobVersions, _, _, err := c.client.Jobs().Versions(c.jobID, false, nil) if err != nil { c.Ui.Error(fmt.Sprintf("Error retrieving versions of job %q: %s", c.jobID, err)) return 1 diff --git a/nomad/job_endpoint.go b/nomad/job_endpoint.go index 6849637f8ed3..d027abc081f9 100644 --- a/nomad/job_endpoint.go +++ b/nomad/job_endpoint.go @@ -9,7 +9,6 @@ import ( "fmt" "net/http" "sort" - "strconv" "strings" "time" @@ -1304,24 +1303,23 @@ func (j *Job) GetJobVersions(args *structs.JobVersionsRequest, var compareVersion *structs.Job var compareStatic bool - if args.DiffTagName != "" { - compareStatic = true - tagFound := false - for _, version := range out { - if version.TaggedVersion != nil && version.TaggedVersion.Name == args.DiffTagName { - tagFound = true - compareVersionNumber = version.Version - break + if args.Diffs { + if args.DiffTagName != "" { + compareStatic = true + tagFound := false + for _, version := range out { + if version.TaggedVersion != nil && version.TaggedVersion.Name == args.DiffTagName { + tagFound = true + compareVersionNumber = version.Version + break + } } - } - if !tagFound { - return fmt.Errorf("tag %q not found", args.DiffTagName) - } - } else if args.DiffVersion != "" { - compareStatic = true - compareVersionNumber, err = strconv.ParseUint(args.DiffVersion, 10, 64) - if err != nil { - return fmt.Errorf("failed to parse diff version: %v", err) + if !tagFound { + return fmt.Errorf("tag %q not found", args.DiffTagName) + } + } else if args.DiffVersion != nil { + compareStatic = true + compareVersionNumber = *args.DiffVersion } } @@ -1342,7 +1340,7 @@ func (j *Job) GetJobVersions(args *structs.JobVersionsRequest, if args.DiffTagName != "" { return fmt.Errorf("tag %q not found", args.DiffTagName) } - return fmt.Errorf("version %q not found", args.DiffVersion) + return fmt.Errorf("version %d not found", *args.DiffVersion) } // Compute the diffs @@ -1810,6 +1808,7 @@ func (j *Job) Plan(args *structs.JobPlanRequest, reply *structs.JobPlanResponse) if done, err := j.srv.forward("Job.Plan", args, args, reply); done { return err } + j.srv.MeasureRPCRate("job", structs.RateMetricWrite, args) if authErr != nil { return structs.ErrPermissionDenied @@ -1864,38 +1863,21 @@ func (j *Job) Plan(args *structs.JobPlanRequest, reply *structs.JobPlanResponse) // Get the original job ws := memdb.NewWatchSet() - // existingJob, err := snap.JobByID(ws, args.RequestNamespace(), args.Job.ID) - // if err != nil { - // return err - // } - - // var existingJob interface{} var existingJob *structs.Job - if args.DiffVersion != "" { - // Convert from string to uint64 - diffVersion, err := strconv.ParseUint(args.DiffVersion, 10, 64) - if err != nil { - return err - } - existingJob, err = snap.JobByIDAndVersion(ws, args.RequestNamespace(), args.Job.ID, diffVersion) + if args.DiffVersion != nil { + existingJob, err = snap.JobByIDAndVersion(ws, args.RequestNamespace(), args.Job.ID, *args.DiffVersion) if err != nil { return err } if existingJob == nil { - return fmt.Errorf("version %q not found", args.DiffVersion) + return fmt.Errorf("version %d not found", *args.DiffVersion) } } else if args.DiffTagName != "" { - versions, err := snap.JobVersionsByID(ws, args.RequestNamespace(), args.Job.ID) + existingJob, err = snap.JobVersionByTagName(ws, args.RequestNamespace(), args.Job.ID, args.DiffTagName) if err != nil { return err } - for _, version := range versions { - if version.TaggedVersion != nil && version.TaggedVersion.Name == args.DiffTagName { - existingJob = version - break - } - } if existingJob == nil { return fmt.Errorf("version tag %q not found", args.DiffTagName) } @@ -1991,7 +1973,6 @@ func (j *Job) Plan(args *structs.JobPlanRequest, reply *structs.JobPlanResponse) } annotations := planner.Plans[0].Annotations if args.Diff { - jobDiff, err := existingJob.Diff(args.Job, true) if err != nil { return fmt.Errorf("failed to create job diff: %v", err) diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 8166ce400d9f..b0371fe99ade 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -847,7 +847,7 @@ type JobStubFields struct { type JobPlanRequest struct { Job *Job Diff bool // Toggles an annotated diff - DiffVersion string + DiffVersion *uint64 DiffTagName string // PolicyOverride is set when the user is attempting to override any policies PolicyOverride bool @@ -1655,7 +1655,7 @@ type JobListResponse struct { type JobVersionsRequest struct { JobID string Diffs bool - DiffVersion string + DiffVersion *uint64 DiffTagName string QueryOptions } From a0e4e5a602c5de3630b99ea5dd1c406e17240b44 Mon Sep 17 00:00:00 2001 From: Phil Renaud Date: Sat, 7 Sep 2024 09:40:32 -0400 Subject: [PATCH 07/13] Simplify the diff-appending part of jobVersions and hide None-type diffs from CLI --- api/jobs.go | 4 +--- command/job_history.go | 19 ++++++------------- nomad/job_endpoint.go | 33 ++++++++++++++++++++------------- 3 files changed, 27 insertions(+), 29 deletions(-) diff --git a/api/jobs.go b/api/jobs.go index b3f9b7f7b3b0..92be36e07c78 100644 --- a/api/jobs.go +++ b/api/jobs.go @@ -495,9 +495,7 @@ func (j *Jobs) PlanOpts(job *Job, opts *PlanOptions, q *WriteOptions) (*JobPlanR if opts != nil { req.Diff = opts.Diff req.PolicyOverride = opts.PolicyOverride - if opts.DiffVersion != nil { - req.DiffVersion = opts.DiffVersion - } + req.DiffVersion = opts.DiffVersion req.DiffTagName = opts.DiffTagName } diff --git a/command/job_history.go b/command/job_history.go index a3c7ab31e863..fb1ae2b80152 100644 --- a/command/job_history.go +++ b/command/job_history.go @@ -147,7 +147,7 @@ func (c *JobHistoryCommand) Run(args []string) int { if diffVersionFlag != "" { parsedDiffVersion, err := strconv.ParseUint(diffVersionFlag, 10, 64) if err != nil { - c.Ui.Error(fmt.Sprintf("Error parsing diff version: %s", err)) + c.Ui.Error(fmt.Sprintf("Error parsing -diff-version: %s", err)) return 1 } diffVersion = &parsedDiffVersion @@ -198,7 +198,6 @@ func (c *JobHistoryCommand) Run(args []string) int { var job *api.Job var diff *api.JobDiff - var nextVersion uint64 for i, v := range versions { if *v.Version != version { continue @@ -207,7 +206,6 @@ func (c *JobHistoryCommand) Run(args []string) int { job = v if i+1 <= len(diffs) { diff = diffs[i] - nextVersion = *versions[i+1].Version } } @@ -222,7 +220,7 @@ func (c *JobHistoryCommand) Run(args []string) int { return 0 } - if err := c.formatJobVersion(job, diff, nextVersion, full); err != nil { + if err := c.formatJobVersion(job, diff, full); err != nil { c.Ui.Error(err.Error()) return 1 } @@ -265,15 +263,11 @@ func (c *JobHistoryCommand) formatJobVersions(versions []*api.Job, diffs []*api. for i, version := range versions { var diff *api.JobDiff - var nextVersion uint64 - if dLen > i { + if i+1 <= dLen { diff = diffs[i] } - if i+1 < vLen { // if the current version is not the last version - nextVersion = *versions[i+1].Version - } - if err := c.formatJobVersion(version, diff, nextVersion, full); err != nil { + if err := c.formatJobVersion(version, diff, full); err != nil { return err } @@ -286,7 +280,7 @@ func (c *JobHistoryCommand) formatJobVersions(versions []*api.Job, diffs []*api. return nil } -func (c *JobHistoryCommand) formatJobVersion(job *api.Job, diff *api.JobDiff, nextVersion uint64, full bool) error { +func (c *JobHistoryCommand) formatJobVersion(job *api.Job, diff *api.JobDiff, full bool) error { if job == nil { return fmt.Errorf("Error printing job history for non-existing job or job version") } @@ -304,8 +298,7 @@ func (c *JobHistoryCommand) formatJobVersion(job *api.Job, diff *api.JobDiff, ne } } - if diff != nil { - //diffStr := fmt.Sprintf("Difference between version %d and %d:", *job.Version, nextVersion) + if diff != nil && diff.Type != "None" { basic = append(basic, fmt.Sprintf("Diff|\n%s", strings.TrimSpace(formatJobDiff(diff, false)))) } diff --git a/nomad/job_endpoint.go b/nomad/job_endpoint.go index d027abc081f9..fe17b258ef42 100644 --- a/nomad/job_endpoint.go +++ b/nomad/job_endpoint.go @@ -1301,11 +1301,11 @@ func (j *Job) GetJobVersions(args *structs.JobVersionsRequest, var compareVersionNumber uint64 var compareVersion *structs.Job - var compareStatic bool + var compareSpecificVersion bool if args.Diffs { if args.DiffTagName != "" { - compareStatic = true + compareSpecificVersion = true tagFound := false for _, version := range out { if version.TaggedVersion != nil && version.TaggedVersion.Name == args.DiffTagName { @@ -1318,7 +1318,7 @@ func (j *Job) GetJobVersions(args *structs.JobVersionsRequest, return fmt.Errorf("tag %q not found", args.DiffTagName) } } else if args.DiffVersion != nil { - compareStatic = true + compareSpecificVersion = true compareVersionNumber = *args.DiffVersion } } @@ -1330,13 +1330,13 @@ func (j *Job) GetJobVersions(args *structs.JobVersionsRequest, if job.ModifyIndex > maxModifyIndex { maxModifyIndex = job.ModifyIndex } - if compareStatic && job.Version == compareVersionNumber { + if compareSpecificVersion && job.Version == compareVersionNumber { compareVersion = job } } reply.Index = maxModifyIndex - if compareStatic && compareVersion == nil { + if compareSpecificVersion && compareVersion == nil { if args.DiffTagName != "" { return fmt.Errorf("tag %q not found", args.DiffTagName) } @@ -1344,19 +1344,26 @@ func (j *Job) GetJobVersions(args *structs.JobVersionsRequest, } // Compute the diffs + + // TODO: + // - tests + // - Remove job plan getting diffs and make it into a new command, `nomad job diff` + if args.Diffs { for i := 0; i < len(out); i++ { - // For each of your job's versions, counting from highest version to lowest - // it should either be compared to the NEXT version in out[], or to the pre-specified version from compareStatic. - if !compareStatic { - if i+1 < len(out) && out[i+1] != nil { - compareVersion = out[i+1] - } else { + var old, new *structs.Job + new = out[i] + + if compareSpecificVersion { + old = compareVersion + } else { + if i == len(out)-1 { + // Skip the last version if not comparing to a specific version break } + old = out[i+1] } - old, new := compareVersion, out[i] d, err := old.Diff(new, true) if err != nil { return fmt.Errorf("failed to create job diff: %v", err) @@ -1871,7 +1878,7 @@ func (j *Job) Plan(args *structs.JobPlanRequest, reply *structs.JobPlanResponse) return err } if existingJob == nil { - return fmt.Errorf("version %d not found", *args.DiffVersion) + return fmt.Errorf("version %q not found", *args.DiffVersion) } } else if args.DiffTagName != "" { existingJob, err = snap.JobVersionByTagName(ws, args.RequestNamespace(), args.Job.ID, args.DiffTagName) From 36240c5c1ba653072dd8642e419b77ef81171770 Mon Sep 17 00:00:00 2001 From: Phil Renaud Date: Sun, 8 Sep 2024 20:04:17 -0400 Subject: [PATCH 08/13] Remove the diff-tag and diff-version parts of nomad job plan, with an eye toward making them a new top-level CLI command soon --- api/jobs.go | 7 ------- command/agent/job_endpoint.go | 3 --- command/job_plan.go | 32 +------------------------------- nomad/job_endpoint.go | 24 +++--------------------- nomad/structs/structs.go | 6 ++---- 5 files changed, 6 insertions(+), 66 deletions(-) diff --git a/api/jobs.go b/api/jobs.go index 92be36e07c78..56293a90adbe 100644 --- a/api/jobs.go +++ b/api/jobs.go @@ -471,8 +471,6 @@ func (j *Jobs) PeriodicForce(jobID string, q *WriteOptions) (string, *WriteMeta, type PlanOptions struct { Diff bool PolicyOverride bool - DiffVersion *uint64 - DiffTagName string } func (j *Jobs) Plan(job *Job, diff bool, q *WriteOptions) (*JobPlanResponse, *WriteMeta, error) { @@ -495,9 +493,6 @@ func (j *Jobs) PlanOpts(job *Job, opts *PlanOptions, q *WriteOptions) (*JobPlanR if opts != nil { req.Diff = opts.Diff req.PolicyOverride = opts.PolicyOverride - req.DiffVersion = opts.DiffVersion - - req.DiffTagName = opts.DiffTagName } var resp JobPlanResponse @@ -1507,8 +1502,6 @@ type JobPlanRequest struct { Job *Job Diff bool PolicyOverride bool - DiffVersion *uint64 - DiffTagName string WriteRequest } diff --git a/command/agent/job_endpoint.go b/command/agent/job_endpoint.go index 60ff20eb0285..e60e1c38a101 100644 --- a/command/agent/job_endpoint.go +++ b/command/agent/job_endpoint.go @@ -185,12 +185,9 @@ func (s *HTTPServer) jobPlan(resp http.ResponseWriter, req *http.Request, } sJob, writeReq := s.apiJobAndRequestToStructs(args.Job, req, args.WriteRequest) - planReq := structs.JobPlanRequest{ Job: sJob, Diff: args.Diff, - DiffVersion: args.DiffVersion, - DiffTagName: args.DiffTagName, PolicyOverride: args.PolicyOverride, WriteRequest: *writeReq, } diff --git a/command/job_plan.go b/command/job_plan.go index 435d7f78a217..60d7f00dff0d 100644 --- a/command/job_plan.go +++ b/command/job_plan.go @@ -7,7 +7,6 @@ import ( "fmt" "os" "sort" - "strconv" "strings" "time" @@ -85,15 +84,6 @@ Plan Options: Determines whether the diff between the remote job and planned job is shown. Defaults to true. - -diff-tag - Specifies the version of the remote job to compare against, referenced by - tag name (defaults to latest). This tag can be set using the - "nomad job tag" command. - - -diff-version - Specifies the version number of the remote job to compare against - (defaults to latest). - -json Parses the job file as JSON. If the outer object has a Job field, such as from "nomad job inspect" or "nomad run -output", the value of the field is @@ -167,14 +157,11 @@ func (c *JobPlanCommand) AutocompleteArgs() complete.Predictor { func (c *JobPlanCommand) Name() string { return "job plan" } func (c *JobPlanCommand) Run(args []string) int { var diff, policyOverride, verbose bool - var vaultToken, vaultNamespace, diffTag, diffVersionFlag string - var diffVersion *uint64 + var vaultToken, vaultNamespace string flagSet := c.Meta.FlagSet(c.Name(), FlagSetClient) flagSet.Usage = func() { c.Ui.Output(c.Help()) } flagSet.BoolVar(&diff, "diff", true, "") - flagSet.StringVar(&diffTag, "diff-tag", "", "") - flagSet.StringVar(&diffVersionFlag, "diff-version", "", "") flagSet.BoolVar(&policyOverride, "policy-override", false, "") flagSet.BoolVar(&verbose, "verbose", false, "") flagSet.BoolVar(&c.JobGetter.JSON, "json", false, "") @@ -259,23 +246,6 @@ func (c *JobPlanCommand) Run(args []string) int { return c.multiregionPlan(client, job, opts, diff, verbose) } - if diffTag != "" && diffVersionFlag != "" { - c.Ui.Error("Cannot specify both -diff-tag and -diff-version") - return 255 - } - - if diffVersionFlag != "" { - parsedDiffVersion, err := strconv.ParseUint(diffVersionFlag, 10, 64) - if err != nil { - c.Ui.Error(fmt.Sprintf("Error parsing diff version: %s", err)) - return 1 - } - diffVersion = &parsedDiffVersion - } - - opts.DiffTagName = diffTag - opts.DiffVersion = diffVersion - // Submit the job resp, _, err := client.Jobs().PlanOpts(job, opts, nil) if err != nil { diff --git a/nomad/job_endpoint.go b/nomad/job_endpoint.go index fe17b258ef42..d4a865ec01d6 100644 --- a/nomad/job_endpoint.go +++ b/nomad/job_endpoint.go @@ -1872,27 +1872,9 @@ func (j *Job) Plan(args *structs.JobPlanRequest, reply *structs.JobPlanResponse) ws := memdb.NewWatchSet() var existingJob *structs.Job - if args.DiffVersion != nil { - existingJob, err = snap.JobByIDAndVersion(ws, args.RequestNamespace(), args.Job.ID, *args.DiffVersion) - if err != nil { - return err - } - if existingJob == nil { - return fmt.Errorf("version %q not found", *args.DiffVersion) - } - } else if args.DiffTagName != "" { - existingJob, err = snap.JobVersionByTagName(ws, args.RequestNamespace(), args.Job.ID, args.DiffTagName) - if err != nil { - return err - } - if existingJob == nil { - return fmt.Errorf("version tag %q not found", args.DiffTagName) - } - } else { - existingJob, err = snap.JobByID(ws, args.RequestNamespace(), args.Job.ID) - if err != nil { - return err - } + existingJob, err = snap.JobByID(ws, args.RequestNamespace(), args.Job.ID) + if err != nil { + return err } policyWarnings, err := j.enforceSubmitJob(args.PolicyOverride, args.Job, existingJob, nomadACLToken, ns) diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index b0371fe99ade..fbe79e268a00 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -845,10 +845,8 @@ type JobStubFields struct { // JobPlanRequest is used for the Job.Plan endpoint to trigger a dry-run // evaluation of the Job. type JobPlanRequest struct { - Job *Job - Diff bool // Toggles an annotated diff - DiffVersion *uint64 - DiffTagName string + Job *Job + Diff bool // Toggles an annotated diff // PolicyOverride is set when the user is attempting to override any policies PolicyOverride bool WriteRequest From 8a3cfeb4f82efda6a5c68e075787b7f29fd27ff5 Mon Sep 17 00:00:00 2001 From: Phil Renaud Date: Mon, 9 Sep 2024 15:42:08 -0400 Subject: [PATCH 09/13] Version diff tests --- command/job_history_test.go | 206 ++++++++++++++++++++++++++++++++++++ 1 file changed, 206 insertions(+) diff --git a/command/job_history_test.go b/command/job_history_test.go index d4be11e87901..3e1bcf8754c9 100644 --- a/command/job_history_test.go +++ b/command/job_history_test.go @@ -166,3 +166,209 @@ namespace "default" { }) } } + +func TestJobHistoryCommand_Diffs(t *testing.T) { + ci.Parallel(t) + + // Start test server + srv, _, url := testServer(t, true, nil) + defer srv.Shutdown() + state := srv.Agent.Server().State() + + // Create a job with multiple versions + job := mock.Job() + + job.ID = "test-job-history" + job.TaskGroups[0].Count = 1 + must.NoError(t, state.UpsertJob(structs.MsgTypeTestSetup, 1000, nil, job)) + + v2 := job.Copy() + v2.TaskGroups[0].Count = 2 + v2.TaggedVersion = &structs.JobTaggedVersion{ + Name: "example-tag", + Description: "example-description", + } + must.NoError(t, state.UpsertJob(structs.MsgTypeTestSetup, 1001, nil, v2)) + + v3 := job.Copy() + v3.TaskGroups[0].Count = 3 + must.NoError(t, state.UpsertJob(structs.MsgTypeTestSetup, 1002, nil, v3)) + + v4 := job.Copy() + v4.TaskGroups[0].Count = 4 + must.NoError(t, state.UpsertJob(structs.MsgTypeTestSetup, 1003, nil, v4)) + + t.Run("Without diffs", func(t *testing.T) { + ui := cli.NewMockUi() + cmd := &JobHistoryCommand{Meta: Meta{Ui: ui}} + + code := cmd.Run([]string{"-address", url, job.ID}) + must.Zero(t, code) + + out := ui.OutputWriter.String() + // There should be four outputs + must.Eq(t, 4, strings.Count(out, "Version")) + must.Eq(t, 0, strings.Count(out, "Diff")) + }) + t.Run("With diffs", func(t *testing.T) { + ui := cli.NewMockUi() + cmd := &JobHistoryCommand{Meta: Meta{Ui: ui}} + + code := cmd.Run([]string{"-p", "-address", url, job.ID}) + must.Zero(t, code) + + out := ui.OutputWriter.String() + rawBlocks := strings.Split(out, "Version") + // trim empty blocks from whitespace in the output + var blocks []string + for _, block := range rawBlocks { + trimmed := strings.TrimSpace(block) + if trimmed != "" { + blocks = append(blocks, trimmed) + } + } + + // Check that we have 4 versions + must.Eq(t, 4, len(blocks)) + must.Eq(t, 4, strings.Count(out, "Version")) + must.Eq(t, 3, strings.Count(out, "Diff")) + + // Diffs show up for all versions except the first one + must.True(t, strings.Contains(blocks[0], "Diff")) + must.True(t, strings.Contains(blocks[1], "Diff")) + must.True(t, strings.Contains(blocks[2], "Diff")) + must.False(t, strings.Contains(blocks[3], "Diff")) + + // Check that the diffs are specifically against their predecessor + must.True(t, strings.Contains(blocks[0], "\"3\" => \"4\"")) + must.True(t, strings.Contains(blocks[1], "\"2\" => \"3\"")) + must.True(t, strings.Contains(blocks[2], "\"1\" => \"2\"")) + }) + + t.Run("With diffs against a specific version that doesnt exist", func(t *testing.T) { + ui := cli.NewMockUi() + cmd := &JobHistoryCommand{Meta: Meta{Ui: ui}} + + code := cmd.Run([]string{"-p", "-diff-version", "4", "-address", url, job.ID}) + must.One(t, code) + // Error that version 4 doesnt exists + must.StrContains(t, ui.ErrorWriter.String(), "version 4 not found") + + }) + t.Run("With diffs against a specific version", func(t *testing.T) { + ui := cli.NewMockUi() + cmd := &JobHistoryCommand{Meta: Meta{Ui: ui}} + + code := cmd.Run([]string{"-p", "-diff-version", "3", "-address", url, job.ID}) + must.Zero(t, code) + + out := ui.OutputWriter.String() + rawBlocks := strings.Split(out, "Version") + // trim empty blocks from whitespace in the output + var blocks []string + for _, block := range rawBlocks { + trimmed := strings.TrimSpace(block) + if trimmed != "" { + blocks = append(blocks, trimmed) + } + } + + // Check that we have 4 versions + must.Eq(t, 4, len(blocks)) + must.Eq(t, 4, strings.Count(out, "Version")) + must.Eq(t, 3, strings.Count(out, "Diff")) + + // Diffs show up for all versions except the specified one + must.False(t, strings.Contains(blocks[0], "Diff")) + must.True(t, strings.Contains(blocks[1], "Diff")) + must.True(t, strings.Contains(blocks[2], "Diff")) + must.True(t, strings.Contains(blocks[3], "Diff")) + + // Check that the diffs are specifically against the tagged version (which has a count of 2) + must.True(t, strings.Contains(blocks[1], "\"4\" => \"3\"")) + must.True(t, strings.Contains(blocks[2], "\"4\" => \"2\"")) + must.True(t, strings.Contains(blocks[3], "\"4\" => \"1\"")) + + }) + + t.Run("With diffs against another specific version", func(t *testing.T) { + ui := cli.NewMockUi() + cmd := &JobHistoryCommand{Meta: Meta{Ui: ui}} + + // Diff against version 1 instead + code := cmd.Run([]string{"-p", "-diff-version", "2", "-address", url, job.ID}) + must.Zero(t, code) + + out := ui.OutputWriter.String() + rawBlocks := strings.Split(out, "Version") + // trim empty blocks from whitespace in the output + var blocks []string + for _, block := range rawBlocks { + trimmed := strings.TrimSpace(block) + if trimmed != "" { + blocks = append(blocks, trimmed) + } + } + + // Check that we have 4 versions + must.Eq(t, 4, len(blocks)) + must.Eq(t, 4, strings.Count(out, "Version")) + must.Eq(t, 3, strings.Count(out, "Diff")) + + // Diffs show up for all versions except the specified one + must.True(t, strings.Contains(blocks[0], "Diff")) + must.False(t, strings.Contains(blocks[1], "Diff")) + must.True(t, strings.Contains(blocks[2], "Diff")) + must.True(t, strings.Contains(blocks[3], "Diff")) + + // Check that the diffs are specifically against the tagged version (which has a count of 2) + must.True(t, strings.Contains(blocks[0], "\"3\" => \"4\"")) + must.True(t, strings.Contains(blocks[2], "\"3\" => \"2\"")) + must.True(t, strings.Contains(blocks[3], "\"3\" => \"1\"")) + }) + + t.Run("With diffs against a specific tag that doesnt exist", func(t *testing.T) { + ui := cli.NewMockUi() + cmd := &JobHistoryCommand{Meta: Meta{Ui: ui}} + + code := cmd.Run([]string{"-p", "-diff-tag", "nonexistent-tag", "-address", url, job.ID}) + must.One(t, code) + must.StrContains(t, ui.ErrorWriter.String(), "tag \"nonexistent-tag\" not found") + }) + + t.Run("With diffs against a specific tag", func(t *testing.T) { + ui := cli.NewMockUi() + + // Run history command with diff against the tag + cmd := &JobHistoryCommand{Meta: Meta{Ui: ui}} + code := cmd.Run([]string{"-p", "-diff-tag", "example-tag", "-address", url, job.ID}) + must.Zero(t, code) + + out := ui.OutputWriter.String() + rawBlocks := strings.Split(out, "Version") + // trim empty blocks from whitespace in the output + var blocks []string + for _, block := range rawBlocks { + trimmed := strings.TrimSpace(block) + if trimmed != "" { + blocks = append(blocks, trimmed) + } + } + + // Check that we have 4 versions + must.Eq(t, 4, len(blocks)) + must.Eq(t, 4, strings.Count(out, "Version")) + must.Eq(t, 3, strings.Count(out, "Diff")) + + // Check that the diff is present for versions other than the tagged version + must.True(t, strings.Contains(blocks[0], "Diff")) + must.True(t, strings.Contains(blocks[1], "Diff")) + must.False(t, strings.Contains(blocks[2], "Diff")) + must.True(t, strings.Contains(blocks[3], "Diff")) + + // Check that the diffs are specifically against the tagged version (which has a count of 2) + must.True(t, strings.Contains(blocks[0], "\"2\" => \"4\"")) + must.True(t, strings.Contains(blocks[1], "\"2\" => \"3\"")) + must.True(t, strings.Contains(blocks[3], "\"2\" => \"1\"")) + }) +} From c79f6e442345c4bfa70b2b416392445e4581d0ec Mon Sep 17 00:00:00 2001 From: Phil Renaud Date: Mon, 9 Sep 2024 16:42:38 -0400 Subject: [PATCH 10/13] re-implement JobVersionByTagName --- command/job_plan.go | 2 -- nomad/job_endpoint.go | 22 ++++++---------------- 2 files changed, 6 insertions(+), 18 deletions(-) diff --git a/command/job_plan.go b/command/job_plan.go index 60d7f00dff0d..f28dea2ed72b 100644 --- a/command/job_plan.go +++ b/command/job_plan.go @@ -140,8 +140,6 @@ func (c *JobPlanCommand) AutocompleteFlags() complete.Flags { "-vault-token": complete.PredictAnything, "-vault-namespace": complete.PredictAnything, "-var": complete.PredictAnything, - "-diff-tag": complete.PredictAnything, - "-diff-version": complete.PredictAnything, "-var-file": complete.PredictFiles("*.var"), }) } diff --git a/nomad/job_endpoint.go b/nomad/job_endpoint.go index d4a865ec01d6..2845fefc2f7a 100644 --- a/nomad/job_endpoint.go +++ b/nomad/job_endpoint.go @@ -1306,17 +1306,14 @@ func (j *Job) GetJobVersions(args *structs.JobVersionsRequest, if args.Diffs { if args.DiffTagName != "" { compareSpecificVersion = true - tagFound := false - for _, version := range out { - if version.TaggedVersion != nil && version.TaggedVersion.Name == args.DiffTagName { - tagFound = true - compareVersionNumber = version.Version - break - } + compareVersion, err = state.JobVersionByTagName(ws, args.RequestNamespace(), args.JobID, args.DiffTagName) + if err != nil { + return fmt.Errorf("error looking up job version by tag: %v", err) } - if !tagFound { + if compareVersion == nil { return fmt.Errorf("tag %q not found", args.DiffTagName) } + compareVersionNumber = compareVersion.Version } else if args.DiffVersion != nil { compareSpecificVersion = true compareVersionNumber = *args.DiffVersion @@ -1345,10 +1342,6 @@ func (j *Job) GetJobVersions(args *structs.JobVersionsRequest, // Compute the diffs - // TODO: - // - tests - // - Remove job plan getting diffs and make it into a new command, `nomad job diff` - if args.Diffs { for i := 0; i < len(out); i++ { var old, new *structs.Job @@ -1815,7 +1808,6 @@ func (j *Job) Plan(args *structs.JobPlanRequest, reply *structs.JobPlanResponse) if done, err := j.srv.forward("Job.Plan", args, args, reply); done { return err } - j.srv.MeasureRPCRate("job", structs.RateMetricWrite, args) if authErr != nil { return structs.ErrPermissionDenied @@ -1870,9 +1862,7 @@ func (j *Job) Plan(args *structs.JobPlanRequest, reply *structs.JobPlanResponse) // Get the original job ws := memdb.NewWatchSet() - var existingJob *structs.Job - - existingJob, err = snap.JobByID(ws, args.RequestNamespace(), args.Job.ID) + existingJob, err := snap.JobByID(ws, args.RequestNamespace(), args.Job.ID) if err != nil { return err } From 7cbb7b81cb4e94a8b708767ba8975edf940b8e57 Mon Sep 17 00:00:00 2001 From: Phil Renaud Date: Tue, 10 Sep 2024 10:06:47 -0400 Subject: [PATCH 11/13] Test mods and simplification --- command/job_history_test.go | 166 ++++++++++++++++-------------------- 1 file changed, 74 insertions(+), 92 deletions(-) diff --git a/command/job_history_test.go b/command/job_history_test.go index 3e1bcf8754c9..2170e44c2850 100644 --- a/command/job_history_test.go +++ b/command/job_history_test.go @@ -167,6 +167,20 @@ namespace "default" { } } +func blocksFromOutput(t *testing.T, out string) []string { + t.Helper() + rawBlocks := strings.Split(out, "Version") + // trim empty blocks from whitespace in the output + var blocks []string + for _, block := range rawBlocks { + trimmed := strings.TrimSpace(block) + if trimmed != "" { + blocks = append(blocks, trimmed) + } + } + return blocks +} + func TestJobHistoryCommand_Diffs(t *testing.T) { ci.Parallel(t) @@ -176,33 +190,33 @@ func TestJobHistoryCommand_Diffs(t *testing.T) { state := srv.Agent.Server().State() // Create a job with multiple versions - job := mock.Job() + v0 := mock.Job() - job.ID = "test-job-history" - job.TaskGroups[0].Count = 1 - must.NoError(t, state.UpsertJob(structs.MsgTypeTestSetup, 1000, nil, job)) + v0.ID = "test-job-history" + v0.TaskGroups[0].Count = 1 + must.NoError(t, state.UpsertJob(structs.MsgTypeTestSetup, 1000, nil, v0)) - v2 := job.Copy() - v2.TaskGroups[0].Count = 2 - v2.TaggedVersion = &structs.JobTaggedVersion{ + v1 := v0.Copy() + v1.TaskGroups[0].Count = 2 + v1.TaggedVersion = &structs.JobTaggedVersion{ Name: "example-tag", Description: "example-description", } - must.NoError(t, state.UpsertJob(structs.MsgTypeTestSetup, 1001, nil, v2)) + must.NoError(t, state.UpsertJob(structs.MsgTypeTestSetup, 1001, nil, v1)) - v3 := job.Copy() - v3.TaskGroups[0].Count = 3 - must.NoError(t, state.UpsertJob(structs.MsgTypeTestSetup, 1002, nil, v3)) + v2 := v0.Copy() + v2.TaskGroups[0].Count = 3 + must.NoError(t, state.UpsertJob(structs.MsgTypeTestSetup, 1002, nil, v2)) - v4 := job.Copy() - v4.TaskGroups[0].Count = 4 - must.NoError(t, state.UpsertJob(structs.MsgTypeTestSetup, 1003, nil, v4)) + v3 := v0.Copy() + v3.TaskGroups[0].Count = 4 + must.NoError(t, state.UpsertJob(structs.MsgTypeTestSetup, 1003, nil, v3)) t.Run("Without diffs", func(t *testing.T) { ui := cli.NewMockUi() cmd := &JobHistoryCommand{Meta: Meta{Ui: ui}} - code := cmd.Run([]string{"-address", url, job.ID}) + code := cmd.Run([]string{"-address", url, v0.ID}) must.Zero(t, code) out := ui.OutputWriter.String() @@ -214,42 +228,34 @@ func TestJobHistoryCommand_Diffs(t *testing.T) { ui := cli.NewMockUi() cmd := &JobHistoryCommand{Meta: Meta{Ui: ui}} - code := cmd.Run([]string{"-p", "-address", url, job.ID}) + code := cmd.Run([]string{"-p", "-address", url, v0.ID}) must.Zero(t, code) out := ui.OutputWriter.String() - rawBlocks := strings.Split(out, "Version") - // trim empty blocks from whitespace in the output - var blocks []string - for _, block := range rawBlocks { - trimmed := strings.TrimSpace(block) - if trimmed != "" { - blocks = append(blocks, trimmed) - } - } + blocks := blocksFromOutput(t, out) // Check that we have 4 versions - must.Eq(t, 4, len(blocks)) + must.Len(t, 4, blocks) must.Eq(t, 4, strings.Count(out, "Version")) must.Eq(t, 3, strings.Count(out, "Diff")) // Diffs show up for all versions except the first one - must.True(t, strings.Contains(blocks[0], "Diff")) - must.True(t, strings.Contains(blocks[1], "Diff")) - must.True(t, strings.Contains(blocks[2], "Diff")) - must.False(t, strings.Contains(blocks[3], "Diff")) + must.StrContains(t, blocks[0], "Diff") + must.StrContains(t, blocks[1], "Diff") + must.StrContains(t, blocks[2], "Diff") + must.StrNotContains(t, blocks[3], "Diff") // Check that the diffs are specifically against their predecessor - must.True(t, strings.Contains(blocks[0], "\"3\" => \"4\"")) - must.True(t, strings.Contains(blocks[1], "\"2\" => \"3\"")) - must.True(t, strings.Contains(blocks[2], "\"1\" => \"2\"")) + must.StrContains(t, blocks[0], "\"3\" => \"4\"") + must.StrContains(t, blocks[1], "\"2\" => \"3\"") + must.StrContains(t, blocks[2], "\"1\" => \"2\"") }) t.Run("With diffs against a specific version that doesnt exist", func(t *testing.T) { ui := cli.NewMockUi() cmd := &JobHistoryCommand{Meta: Meta{Ui: ui}} - code := cmd.Run([]string{"-p", "-diff-version", "4", "-address", url, job.ID}) + code := cmd.Run([]string{"-p", "-diff-version", "4", "-address", url, v0.ID}) must.One(t, code) // Error that version 4 doesnt exists must.StrContains(t, ui.ErrorWriter.String(), "version 4 not found") @@ -259,35 +265,27 @@ func TestJobHistoryCommand_Diffs(t *testing.T) { ui := cli.NewMockUi() cmd := &JobHistoryCommand{Meta: Meta{Ui: ui}} - code := cmd.Run([]string{"-p", "-diff-version", "3", "-address", url, job.ID}) + code := cmd.Run([]string{"-p", "-diff-version", "3", "-address", url, v0.ID}) must.Zero(t, code) out := ui.OutputWriter.String() - rawBlocks := strings.Split(out, "Version") - // trim empty blocks from whitespace in the output - var blocks []string - for _, block := range rawBlocks { - trimmed := strings.TrimSpace(block) - if trimmed != "" { - blocks = append(blocks, trimmed) - } - } + blocks := blocksFromOutput(t, out) // Check that we have 4 versions - must.Eq(t, 4, len(blocks)) + must.Len(t, 4, blocks) must.Eq(t, 4, strings.Count(out, "Version")) must.Eq(t, 3, strings.Count(out, "Diff")) // Diffs show up for all versions except the specified one - must.False(t, strings.Contains(blocks[0], "Diff")) - must.True(t, strings.Contains(blocks[1], "Diff")) - must.True(t, strings.Contains(blocks[2], "Diff")) - must.True(t, strings.Contains(blocks[3], "Diff")) + must.StrNotContains(t, blocks[0], "Diff") + must.StrContains(t, blocks[1], "Diff") + must.StrContains(t, blocks[2], "Diff") + must.StrContains(t, blocks[3], "Diff") - // Check that the diffs are specifically against the tagged version (which has a count of 2) - must.True(t, strings.Contains(blocks[1], "\"4\" => \"3\"")) - must.True(t, strings.Contains(blocks[2], "\"4\" => \"2\"")) - must.True(t, strings.Contains(blocks[3], "\"4\" => \"1\"")) + // Check that the diffs are specifically against the tagged version (which has a count of 4) + must.StrContains(t, blocks[1], "\"4\" => \"3\"") + must.StrContains(t, blocks[2], "\"4\" => \"2\"") + must.StrContains(t, blocks[3], "\"4\" => \"1\"") }) @@ -296,42 +294,34 @@ func TestJobHistoryCommand_Diffs(t *testing.T) { cmd := &JobHistoryCommand{Meta: Meta{Ui: ui}} // Diff against version 1 instead - code := cmd.Run([]string{"-p", "-diff-version", "2", "-address", url, job.ID}) + code := cmd.Run([]string{"-p", "-diff-version", "2", "-address", url, v0.ID}) must.Zero(t, code) out := ui.OutputWriter.String() - rawBlocks := strings.Split(out, "Version") - // trim empty blocks from whitespace in the output - var blocks []string - for _, block := range rawBlocks { - trimmed := strings.TrimSpace(block) - if trimmed != "" { - blocks = append(blocks, trimmed) - } - } + blocks := blocksFromOutput(t, out) // Check that we have 4 versions - must.Eq(t, 4, len(blocks)) + must.Len(t, 4, blocks) must.Eq(t, 4, strings.Count(out, "Version")) must.Eq(t, 3, strings.Count(out, "Diff")) // Diffs show up for all versions except the specified one - must.True(t, strings.Contains(blocks[0], "Diff")) - must.False(t, strings.Contains(blocks[1], "Diff")) - must.True(t, strings.Contains(blocks[2], "Diff")) - must.True(t, strings.Contains(blocks[3], "Diff")) - - // Check that the diffs are specifically against the tagged version (which has a count of 2) - must.True(t, strings.Contains(blocks[0], "\"3\" => \"4\"")) - must.True(t, strings.Contains(blocks[2], "\"3\" => \"2\"")) - must.True(t, strings.Contains(blocks[3], "\"3\" => \"1\"")) + must.StrContains(t, blocks[0], "Diff") + must.StrNotContains(t, blocks[1], "Diff") + must.StrContains(t, blocks[2], "Diff") + must.StrContains(t, blocks[3], "Diff") + + // Check that the diffs are specifically against the tagged version (which has a count of 3) + must.StrContains(t, blocks[0], "\"3\" => \"4\"") + must.StrContains(t, blocks[2], "\"3\" => \"2\"") + must.StrContains(t, blocks[3], "\"3\" => \"1\"") }) t.Run("With diffs against a specific tag that doesnt exist", func(t *testing.T) { ui := cli.NewMockUi() cmd := &JobHistoryCommand{Meta: Meta{Ui: ui}} - code := cmd.Run([]string{"-p", "-diff-tag", "nonexistent-tag", "-address", url, job.ID}) + code := cmd.Run([]string{"-p", "-diff-tag", "nonexistent-tag", "-address", url, v0.ID}) must.One(t, code) must.StrContains(t, ui.ErrorWriter.String(), "tag \"nonexistent-tag\" not found") }) @@ -341,34 +331,26 @@ func TestJobHistoryCommand_Diffs(t *testing.T) { // Run history command with diff against the tag cmd := &JobHistoryCommand{Meta: Meta{Ui: ui}} - code := cmd.Run([]string{"-p", "-diff-tag", "example-tag", "-address", url, job.ID}) + code := cmd.Run([]string{"-p", "-diff-tag", "example-tag", "-address", url, v0.ID}) must.Zero(t, code) out := ui.OutputWriter.String() - rawBlocks := strings.Split(out, "Version") - // trim empty blocks from whitespace in the output - var blocks []string - for _, block := range rawBlocks { - trimmed := strings.TrimSpace(block) - if trimmed != "" { - blocks = append(blocks, trimmed) - } - } + blocks := blocksFromOutput(t, out) // Check that we have 4 versions - must.Eq(t, 4, len(blocks)) + must.Len(t, 4, blocks) must.Eq(t, 4, strings.Count(out, "Version")) must.Eq(t, 3, strings.Count(out, "Diff")) // Check that the diff is present for versions other than the tagged version - must.True(t, strings.Contains(blocks[0], "Diff")) - must.True(t, strings.Contains(blocks[1], "Diff")) - must.False(t, strings.Contains(blocks[2], "Diff")) - must.True(t, strings.Contains(blocks[3], "Diff")) + must.StrContains(t, blocks[0], "Diff") + must.StrContains(t, blocks[1], "Diff") + must.StrNotContains(t, blocks[2], "Diff") + must.StrContains(t, blocks[3], "Diff") // Check that the diffs are specifically against the tagged version (which has a count of 2) - must.True(t, strings.Contains(blocks[0], "\"2\" => \"4\"")) - must.True(t, strings.Contains(blocks[1], "\"2\" => \"3\"")) - must.True(t, strings.Contains(blocks[3], "\"2\" => \"1\"")) + must.StrContains(t, blocks[0], "\"2\" => \"4\"") + must.StrContains(t, blocks[1], "\"2\" => \"3\"") + must.StrContains(t, blocks[3], "\"2\" => \"1\"") }) } From 81b3e448dd414e03d29b04c9be2cc0283c2f5b97 Mon Sep 17 00:00:00 2001 From: Phil Renaud Date: Tue, 10 Sep 2024 10:40:24 -0400 Subject: [PATCH 12/13] Documentation for nomad job history additions --- website/content/docs/commands/job/history.mdx | 60 +++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/website/content/docs/commands/job/history.mdx b/website/content/docs/commands/job/history.mdx index 9544f64140c9..3adf4e57a09b 100644 --- a/website/content/docs/commands/job/history.mdx +++ b/website/content/docs/commands/job/history.mdx @@ -36,6 +36,9 @@ run the command with a job prefix instead of the exact job ID. - `-version`: Display only the history for the given version. - `-json` : Output the job versions in its JSON format. - `-t` : Format and display the job versions using a Go template. +- `-diff-version`: Compare the job with a specific version. +- `-diff-tag`: Compare the job with a specific tag. + ## Examples @@ -81,3 +84,60 @@ v2: 512 v1: 256 v0: 256 ``` + +Compare the current job with a specific older version: + +```shell-session +$ nomad job history -version=3 -diff-version=1 example +Version = 3 +Stable = false +Submit Date = 07/25/17 20:35:43 UTC +Diff = ++/- Job: "example" ++/- Task Group: "cache" + +/- Task: "redis" + +/- Resources { + CPU: "500" + DiskMB: "0" + +/- MemoryMB: "256" => "512" + } +``` + +Compare all job versions with a specific version by tag name: + +```shell-session +$ nomad job history -diff-tag=low-energy-version example + +Version = 3 +Stable = false +Submit Date = 2024-09-09T16:41:53-04:00 +Diff = ++/- Job: "example" ++/- Task Group: "group" + +/- Count: "3" => "4" + Task: "task" + +Version = 2 +Stable = false +Submit Date = 2024-09-09T16:41:53-04:00 +Tag Name = low-energy-version +Tag Description = test description + +Version = 1 +Stable = false +Submit Date = 2024-09-09T16:41:53-04:00 +Diff = ++/- Job: "example" ++/- Task Group: "group" + +/- Count: "3" => "2" + Task: "task" + +Version = 0 +Stable = false +Submit Date = 2024-09-09T16:41:53-04:00 +Diff = ++/- Job: "example" ++/- Task Group: "group" + +/- Count: "3" => "1" + Task: "task" +``` From 50e4101bb9e7d0a530a6b9c98d8a637bbae22774 Mon Sep 17 00:00:00 2001 From: Phil Renaud Date: Wed, 25 Sep 2024 13:44:01 -0400 Subject: [PATCH 13/13] Prevent pruning and reaping of TaggedVersion jobs (#23983) tagged versions should not count against JobTrackedVersions i.e. new job versions being inserted should not evict tagged versions and GC should not delete a job if any of its versions are tagged Co-authored-by: Daniel Bennett --- nomad/core_sched.go | 11 +++ nomad/core_sched_test.go | 88 +++++++++++++++++++ nomad/state/schema.go | 5 ++ nomad/state/schema_test.go | 8 ++ nomad/state/state_store.go | 19 ++++- nomad/state/state_store_test.go | 146 ++++++++++++++++++++++++++++++++ 6 files changed, 273 insertions(+), 4 deletions(-) diff --git a/nomad/core_sched.go b/nomad/core_sched.go index 83999d064eb8..a4bb53a1e2d7 100644 --- a/nomad/core_sched.go +++ b/nomad/core_sched.go @@ -156,6 +156,17 @@ OUTER: // Job is eligible for garbage collection if allEvalsGC { + // if any version of the job is tagged, it should be kept + versions, err := c.snap.JobVersionsByID(ws, job.Namespace, job.ID) + if err != nil { + c.logger.Error("job GC failed to get versions for job", "job", job.ID, "error", err) + continue + } + for _, v := range versions { + if v.TaggedVersion != nil { + continue OUTER + } + } gcJob = append(gcJob, job) gcAlloc = append(gcAlloc, jobAlloc...) gcEval = append(gcEval, jobEval...) diff --git a/nomad/core_sched_test.go b/nomad/core_sched_test.go index b47d0ebdac90..f77da8944093 100644 --- a/nomad/core_sched_test.go +++ b/nomad/core_sched_test.go @@ -610,6 +610,94 @@ func TestCoreScheduler_EvalGC_Batch(t *testing.T) { }) } +// A job that has any of its versions tagged should not be GC-able. +func TestCoreScheduler_EvalGC_JobTaggedVersion(t *testing.T) { + ci.Parallel(t) + + s1, cleanupS1 := TestServer(t, nil) + defer cleanupS1() + testutil.WaitForLeader(t, s1.RPC) + + store := s1.fsm.State() + job := mock.MinJob() + job.Stop = true // to be GC-able + + // to be GC-able, the job needs an associated eval with a terminal Status, + // so that the job gets considered "dead" and not "pending" + // NOTE: this needs to come before UpsertJob for some Mystery Reason + // (otherwise job Status ends up as "pending" later) + eval := mock.Eval() + eval.JobID = job.ID + eval.Status = structs.EvalStatusComplete + must.NoError(t, store.UpsertEvals(structs.MsgTypeTestSetup, 999, []*structs.Evaluation{eval})) + // upsert a couple versions of the job, so the "jobs" table has one + // and the "job_version" table has two. + must.NoError(t, store.UpsertJob(structs.MsgTypeTestSetup, 1000, nil, job.Copy())) + must.NoError(t, store.UpsertJob(structs.MsgTypeTestSetup, 1001, nil, job.Copy())) + + jobExists := func(t *testing.T) bool { + t.Helper() + // any job at all + jobs, err := store.Jobs(nil, state.SortDefault) + must.NoError(t, err, must.Sprint("error getting jobs")) + return jobs.Next() != nil + } + forceGC := func(t *testing.T) { + t.Helper() + snap, err := store.Snapshot() + must.NoError(t, err) + core := NewCoreScheduler(s1, snap) + + idx, err := store.LatestIndex() + must.NoError(t, err) + gc := s1.coreJobEval(structs.CoreJobForceGC, idx+1) + + must.NoError(t, core.Process(gc)) + } + + applyTag := func(t *testing.T, idx, version uint64, name, desc string) { + t.Helper() + must.NoError(t, store.UpdateJobVersionTag(idx, job.Namespace, + &structs.JobApplyTagRequest{ + JobID: job.ID, + Name: name, + Tag: &structs.JobTaggedVersion{ + Name: name, + Description: desc, + }, + Version: version, + })) + } + unsetTag := func(t *testing.T, idx uint64, name string) { + t.Helper() + must.NoError(t, store.UpdateJobVersionTag(idx, job.Namespace, + &structs.JobApplyTagRequest{ + JobID: job.ID, + Name: name, + Tag: nil, // this triggers the deletion + })) + } + + // tagging the latest version (latest of the 2 jobs, 0 and 1, is 1) + // will tag the job in the "jobs" table, which should protect from GC + applyTag(t, 2000, 1, "v1", "version 1") + forceGC(t) + must.True(t, jobExists(t), must.Sprint("latest job version being tagged should protect from GC")) + + // untagging latest and tagging the oldest (only one in "job_version" table) + // should also protect from GC + unsetTag(t, 3000, "v1") + applyTag(t, 3001, 0, "v0", "version 0") + forceGC(t) + must.True(t, jobExists(t), must.Sprint("old job version being tagged should protect from GC")) + + //untagging v0 should leave no tags left, so GC should delete the job + //and all its versions + unsetTag(t, 4000, "v0") + forceGC(t) + must.False(t, jobExists(t), must.Sprint("all tags being removed should enable GC")) +} + func TestCoreScheduler_EvalGC_Partial(t *testing.T) { ci.Parallel(t) diff --git a/nomad/state/schema.go b/nomad/state/schema.go index 7aaafd5cfef0..6182caf5137b 100644 --- a/nomad/state/schema.go +++ b/nomad/state/schema.go @@ -372,6 +372,11 @@ func jobIsGCable(obj interface{}) (bool, error) { return false, fmt.Errorf("Unexpected type: %v", obj) } + // job versions that are tagged should be kept + if j.TaggedVersion != nil { + return false, nil + } + // If the job is periodic or parameterized it is only garbage collectable if // it is stopped. periodic := j.Periodic != nil && j.Periodic.Enabled diff --git a/nomad/state/schema_test.go b/nomad/state/schema_test.go index 657cc808bd0e..9ffdf78434b8 100644 --- a/nomad/state/schema_test.go +++ b/nomad/state/schema_test.go @@ -242,6 +242,14 @@ func Test_jobIsGCable(t *testing.T) { expectedOutput: true, expectedOutputError: nil, }, + { + name: "tagged", + inputObj: &structs.Job{ + TaggedVersion: &structs.JobTaggedVersion{Name: "any"}, + }, + expectedOutput: false, + expectedOutputError: nil, + }, } for _, tc := range testCases { diff --git a/nomad/state/state_store.go b/nomad/state/state_store.go index bb416c48225b..30f05ed9e8a0 100644 --- a/nomad/state/state_store.go +++ b/nomad/state/state_store.go @@ -2188,10 +2188,21 @@ func (s *StateStore) upsertJobVersion(index uint64, job *structs.Job, txn *txn) all[max-1], all[max] = all[max], all[max-1] } - // Delete the job outside of the set that are being kept. - d := all[max] - if err := txn.Delete("job_version", d); err != nil { - return fmt.Errorf("failed to delete job %v (%d) from job_version", d.ID, d.Version) + // Find the oldest non-tagged version to delete + deleteIdx := -1 + for i := len(all) - 1; i >= max; i-- { + if all[i].TaggedVersion == nil { + deleteIdx = i + break + } + } + + // If we found a non-tagged version to delete, delete it + if deleteIdx != -1 { + d := all[deleteIdx] + if err := txn.Delete("job_version", d); err != nil { + return fmt.Errorf("failed to delete job %v (%d) from job_version", d.ID, d.Version) + } } return nil diff --git a/nomad/state/state_store_test.go b/nomad/state/state_store_test.go index cbb81e50f1a3..5e44988e437c 100644 --- a/nomad/state/state_store_test.go +++ b/nomad/state/state_store_test.go @@ -2901,6 +2901,152 @@ func TestStateStore_DeleteJobTxn_BatchDeletes(t *testing.T) { require.Equal(t, deletionIndex, index) } +// TestStatestore_JobTaggedVersion tests that job versions which are tagged +// do not count against the configured server.job_tracked_versions count, +// do not get deleted when new versions are created, +// and *do* get deleted immediately when its tag is removed. +func TestStatestore_JobTaggedVersion(t *testing.T) { + ci.Parallel(t) + + state := testStateStore(t) + // tagged versions should be excluded from this limit + state.config.JobTrackedVersions = 5 + + job := mock.MinJob() + job.Stable = true + + // helpers for readability + upsertJob := func(t *testing.T) { + t.Helper() + must.NoError(t, state.UpsertJob(structs.MsgTypeTestSetup, nextIndex(state), nil, job.Copy())) + } + + applyTag := func(t *testing.T, version uint64) { + t.Helper() + name := fmt.Sprintf("v%d", version) + desc := fmt.Sprintf("version %d", version) + req := &structs.JobApplyTagRequest{ + JobID: job.ID, + Name: name, + Tag: &structs.JobTaggedVersion{ + Name: name, + Description: desc, + }, + Version: version, + } + must.NoError(t, state.UpdateJobVersionTag(nextIndex(state), job.Namespace, req)) + + // confirm + got, err := state.JobVersionByTagName(nil, job.Namespace, job.ID, name) + must.NoError(t, err) + must.Eq(t, version, got.Version) + must.Eq(t, name, got.TaggedVersion.Name) + must.Eq(t, desc, got.TaggedVersion.Description) + } + unsetTag := func(t *testing.T, name string) { + t.Helper() + req := &structs.JobApplyTagRequest{ + JobID: job.ID, + Name: name, + Tag: nil, // this triggers unset + } + must.NoError(t, state.UpdateJobVersionTag(nextIndex(state), job.Namespace, req)) + } + + assertVersions := func(t *testing.T, expect []uint64) { + t.Helper() + jobs, err := state.JobVersionsByID(nil, job.Namespace, job.ID) + must.NoError(t, err) + vs := make([]uint64, len(jobs)) + for i, j := range jobs { + vs[i] = j.Version + } + must.Eq(t, expect, vs) + } + + // we want to end up with JobTrackedVersions (5) versions, + // 0-2 tagged and 3-4 untagged, but also interleave the tagging + // to be somewhat true to normal behavior in reality. + { + // upsert 3 jobs + for range 3 { + upsertJob(t) + } + assertVersions(t, []uint64{2, 1, 0}) + + // tag 2 of them + applyTag(t, 0) + applyTag(t, 1) + // nothing should change + assertVersions(t, []uint64{2, 1, 0}) + + // add 2 more, up to JobTrackedVersions (5) + upsertJob(t) + upsertJob(t) + assertVersions(t, []uint64{4, 3, 2, 1, 0}) + + // tag one more + applyTag(t, 2) + // again nothing should change + assertVersions(t, []uint64{4, 3, 2, 1, 0}) + } + + // removing a tag at this point should leave the version in place + { + unsetTag(t, "v2") + assertVersions(t, []uint64{4, 3, 2, 1, 0}) + } + + // adding more versions should replace 2-4, + // and leave 0-1 in place because they are tagged + { + for range 10 { + upsertJob(t) + } + assertVersions(t, []uint64{14, 13, 12, 11, 10, 1, 0}) + } + + // untagging version 1 now should delete it immediately, + // since we now have more than JobTrackedVersions + { + unsetTag(t, "v1") + assertVersions(t, []uint64{14, 13, 12, 11, 10, 0}) + } + + // test some error conditions + { + // job does not exist + err := state.UpdateJobVersionTag(nextIndex(state), job.Namespace, &structs.JobApplyTagRequest{ + JobID: "non-existent-job", + Tag: &structs.JobTaggedVersion{Name: "tag name"}, + Version: 0, + }) + must.ErrorContains(t, err, `job "non-existent-job" version 0 not found`) + + // version does not exist + err = state.UpdateJobVersionTag(nextIndex(state), job.Namespace, &structs.JobApplyTagRequest{ + JobID: job.ID, + Tag: &structs.JobTaggedVersion{Name: "tag name"}, + Version: 999, + }) + must.ErrorContains(t, err, fmt.Sprintf("job %q version 999 not found", job.ID)) + + // tag name already exists + err = state.UpdateJobVersionTag(nextIndex(state), job.Namespace, &structs.JobApplyTagRequest{ + JobID: job.ID, + Tag: &structs.JobTaggedVersion{Name: "v0"}, + Version: 10, + }) + must.ErrorContains(t, err, fmt.Sprintf(`"v0" already exists on a different version of job %q`, job.ID)) + } + + // deleting all versions should also delete tagged versions + txn := state.db.WriteTxn(nextIndex(state)) + must.NoError(t, state.deleteJobVersions(nextIndex(state), job, txn)) + must.NoError(t, txn.Commit()) + assertVersions(t, []uint64{}) +} + func TestStateStore_DeleteJob_MultipleVersions(t *testing.T) { ci.Parallel(t)