From 60987ea0e1b86a2d9156c0b0e8e4dc24cf2ddaa0 Mon Sep 17 00:00:00 2001 From: Phil Renaud Date: Fri, 6 Sep 2024 11:03:23 -0400 Subject: [PATCH] Addressing some PR comments --- api/jobs.go | 38 ++++++++++++++++++--- api/jobs_test.go | 4 +-- command/agent/job_endpoint.go | 13 +++++++- command/job_history.go | 43 ++++++++++++++++-------- command/job_inspect.go | 2 +- command/job_plan.go | 36 +++++++++++++------- command/job_restart.go | 2 +- nomad/job_endpoint.go | 63 ++++++++++++----------------------- nomad/state/state_store.go | 15 +++++++++ nomad/structs/structs.go | 4 +-- 10 files changed, 142 insertions(+), 78 deletions(-) diff --git a/api/jobs.go b/api/jobs.go index 89c80c9191d2..32e69d7a829c 100644 --- a/api/jobs.go +++ b/api/jobs.go @@ -266,9 +266,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 } @@ -449,7 +474,7 @@ func (j *Jobs) PeriodicForce(jobID string, q *WriteOptions) (string, *WriteMeta, type PlanOptions struct { Diff bool PolicyOverride bool - DiffVersion string + DiffVersion *uint64 DiffTagName string } @@ -473,7 +498,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 } @@ -1483,7 +1511,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 438f15dfb5fa..85873e959a6d 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 b4b9af11fdd6..e6be2689f65a 100644 --- a/command/agent/job_endpoint.go +++ b/command/agent/job_endpoint.go @@ -183,6 +183,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, @@ -780,10 +781,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..52d6e0d7f1bd 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,13 @@ 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, + } + c.Ui.Info(fmt.Sprintf("VersionOptions", versionOptions)) + 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 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 0b3691df2eca..a12e49989c56 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 @@ -170,13 +171,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, "") @@ -266,13 +268,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 19cb454f027f..102cd9874b51 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 cb6f7fbaa747..593679eccca3 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 %q 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/state/state_store.go b/nomad/state/state_store.go index 6976999d1489..3581215ec08b 100644 --- a/nomad/state/state_store.go +++ b/nomad/state/state_store.go @@ -2288,6 +2288,21 @@ func (s *StateStore) JobVersionsByID(ws memdb.WatchSet, namespace, id string) ([ return s.jobVersionByID(txn, ws, namespace, id) } +// JobVersionByTagName returns a Job if it has a Tag with the passed name +func (s *StateStore) JobVersionByTagName(ws memdb.WatchSet, namespace, id string, tagName string) (*structs.Job, error) { + // First get all versions of the job + versions, err := s.JobVersionsByID(ws, namespace, id) + if err != nil { + return nil, err + } + for _, j := range versions { + if j.TaggedVersion != nil && j.TaggedVersion.Name == tagName { + return j, nil + } + } + return nil, nil +} + // jobVersionByID is the underlying implementation for retrieving all tracked // versions of a job and is called under an existing transaction. A watch set // can optionally be passed in to add the job histories to the watch set. diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 34da18dec934..fc8faa2761f2 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -846,7 +846,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 @@ -1654,7 +1654,7 @@ type JobListResponse struct { type JobVersionsRequest struct { JobID string Diffs bool - DiffVersion string + DiffVersion *uint64 DiffTagName string QueryOptions }