Tag and Untag job versions #23863

philrenaud · 2024-08-25T04:48:29Z

via `curl`:

Adds a tag to Version 3:

curl -X PUT localhost:4646/v1/job/difftester/versions/testabc/tag -H "Content-Type: application/json" -d '{"Version": "3", "Tag": {"Name": "Foo", "Description": "Bar"}}'

Removes the tag:

curl -X DELETE localhost:4646/v1/job/difftester/versions/testabc/tag -H "Content-Type: application/json"

via Nomad CLI:

Adds a tag to Version 0:

nomad job tag apply \
  -version=0 \
  -name="Foo" \
  -description="Bar baz" \
fails_every_10

Adds a tag to the latest/current version:

nomad job tag apply -name=foo -description=bar difftester

Removes a tag:

nomad job tag unset -name=foo fails_every_10

github-actions · 2024-08-25T05:02:54Z

Ember Test Audit comparison

	epic/23794-golden-versions	`b93441a`	change
passes	1578	1578	0
failures	0	0	0
flaky	0	0	0
duration	11m 41s 887ms	11m 37s 459ms	-04s 428ms

gulducat

looks pretty good! few comments for ye

api/jobs.go

gulducat · 2024-08-27T14:25:45Z

command/agent/job_endpoint.go

+	s.parseWriteRequest(req, &rpcArgs.WriteRequest)
+
+	var out structs.JobTagResponse
+	if err := s.agent.RPC("Job.TagVersion", &rpcArgs, &out); err != nil {


these two httpserver methods are almost identical except for the RPC that they call, so you could save a decent little bit of duplicate code by moving the logic into the main jobTagVersion method instead.

I ended up splitting these up a bit more — the differences started compounding (unset doesn't get version passed in now that we go by tag name; they both get their own structs for req/resp, etc.)

Not opposed to having a single method to handle these but they look to me like they should be separated currently

(This did in fact end up being a fence I straddled and continue to straddle — see "request for reviewers" code comment)

command/agent/job_endpoint.go

command/job_tag.go

nomad/structs/structs.go

website/content/docs/operations/metrics-reference.mdx

…sting struct

nomad/structs/structs.go

nomad/state/state_store.go

tgross · 2024-09-11T12:59:45Z

api/jobs.go

+	// If the version is not provided, get the "active" version of the job
+	// TODO: Request for reviewers: is this the right place to do this? I'd given some thought to doing it at state store level instead.
+	// And if this is the right place: does this invalidate my need for JobApplyTagRequest.Version to be a pointer / it could just be a uint64?
+	if version == "" {
+		job, _, err := j.Info(jobID, nil)
+		if err != nil {
+			return nil, err
+		}
+		version = strconv.FormatUint(*job.Version, 10)
+	}


Yes, if you require the argument in the request, then the JobApplyTagRequest.Version can just be uint64 and not *uint64. The only thing that you'd need to do to make that work with your untag logic in the state store is to ignore Version unless .Name == nil. Generally I think this would clean up a bunch of annoying logic and checking along the way, so that sounds like a good idea. That would be roughly in line with how arguments are handled most other places in the code base.

Either way I would definitely require this function take in a uint64 though. Consider that callers outside the CLI (ex. the Terraform provider) will likely already have a uint64 from the Job object so forcing them to use a string here just makes for unnecessary conversions. Use the type system to enforce correctness here rather than parsing.

tgross · 2024-09-11T13:02:33Z

command/agent/job_endpoint.go

+	case strings.HasSuffix(path, "/tag"):
+		parts := strings.Split(path, "/")
+		jobID := parts[0]
+		name := parts[2] // job/<jobID>/tag/<name>


You need a length check here, otherwise job/:jobid/tag will panic rather than 404.

tgross · 2024-09-11T13:10:51Z

nomad/job_endpoint.go

@@ -2391,3 +2398,19 @@ func (j *Job) GetServiceRegistrations(
 		},
 	})
 }
+
+func (j *Job) TagVersion(args *structs.JobApplyTagRequest, reply *structs.JobTagResponse) error {
+	// Apply time to the tag if it isn't null


Nitpick: this kind of comment doesn't really add anything to the code

tgross · 2024-09-11T13:11:37Z

nomad/job_endpoint.go

@@ -2391,3 +2398,19 @@ func (j *Job) GetServiceRegistrations(
 		},
 	})
 }
+
+func (j *Job) TagVersion(args *structs.JobApplyTagRequest, reply *structs.JobTagResponse) error {


I think you just haven't implemented it yet, but don't forget you've still got to implement auth, metrics, and RPC forwarding at the top of this handler.

tgross · 2024-09-11T13:16:22Z

command/agent/job_endpoint.go

+	}
+}
+
+// TODO: this is a copy of the function in command/job_history.go; any way to import it here?


That'd be a circular reference as command imports command/agent. You could make this function exported and then have command call it as agent.ParseVersion, but this code seems tiny enough that it's not worth bothering.

tgross · 2024-09-11T13:20:20Z

command/agent/job_endpoint.go

+	// TODO: Request for reviewers: I am splitting by method here, but you'll notice both
+	// methods now call the same RPC method, which later splits them based on presence of args.Tag.
+	// So far the benefit of a method-based split here is that I can more easily create a slimmed-down
+	// TagVersionRequest for the RPC method, but I'm not sure if that's enough to justify the split.
+	// ...is this the best and most obvious place to set the .Tag or not?


These HTTP endpoints are definitely the best place to set the .Tag, because this is the first point where we go from 2 APIs (PUT and DELETE) to a single API (the RPC handler).

Splitting the work into two methods seems fine to me. But if you didn't want to do that you could have each case here construct the args and then have the RPC call at the end of this jobTagVersion function after the switch statement. It makes it a little easier to read and to write tight little tests if you keep it the way you've got it here IMO, but it's a matter of taste.

tgross · 2024-09-11T13:21:21Z

command/job_tag.go

+
+  Example usage:
+
+    nomad job tag apply -name "My Golden Version" -description "The version of the job we can roll back to in the future if needed" <jobname>


Wrap command line help text at 80 cols if you can. For an example I'd add a \ to move the description into the line below (or just shorten the text)

tgross · 2024-09-11T13:22:55Z

nomad/state/state_store.go

+	for _, version := range versions {
+		// Allow for a tag to be updated (new description, for example) but otherwise don't allow a same-tagname to a different version.
+		if version.TaggedVersion != nil && version.TaggedVersion.Name == tag.Name && version.Version != *jobVersion {
+			duplicateVersionName = true


There's no more useful work to do at this point. So rather than setting a bool you check later, you can just bail out early and return the error here.

tgross · 2024-09-11T13:23:52Z

nomad/state/state_store.go

+		return fmt.Errorf("job %q version %d not found", jobID, *jobVersion)
+	}
+
+	copy := job.Copy()


copy is a Go built-in function, which you're shadowing here. That's not wrong but we try to avoid it because it looks funny.

tgross · 2024-09-11T13:24:59Z

nomad/state/state_store.go

+	versions, err := s.JobVersionsByID(ws, namespace, jobID)
+	if err != nil {
+		return err
+	}
+
+	var job *structs.Job
+
+	for _, version := range versions {
+		if version.TaggedVersion != nil && version.TaggedVersion.Name == name {
+			job = version
+			break
+		}
+	}
+
+	if job == nil {
+		return fmt.Errorf("tag %q not found on job %q", name, jobID)
+	}


Isn't this JobVersionByTagName?

It is, thanks for catching. I think I copied over the updateTag code here instead because it does duplicate-name checking (and this won't have to)

philrenaud self-assigned this Aug 25, 2024

vercel bot deployed to Preview – nomad August 25, 2024 04:52 View deployment

Base automatically changed from 23836-taggedversion-object-struct to epic/23794-golden-versions August 26, 2024 03:48

Tag and Untag at API level on down, but am I unblocking the wrong thing?

233f051

philrenaud force-pushed the 23846-nomad-job-tag-cli-command-scaffolding branch from 86153aa to 233f051 Compare August 26, 2024 03:58

vercel bot deployed to Preview – nomad-ui August 26, 2024 03:59 View deployment

vercel bot deployed to Preview – nomad August 26, 2024 04:04 View deployment

vercel bot deployed to Preview – nomad-ui August 26, 2024 21:15 View deployment

Code and comment cleanup

bb9ef81

philrenaud force-pushed the 23846-nomad-job-tag-cli-command-scaffolding branch from a3b6b5e to bb9ef81 Compare August 27, 2024 04:28

vercel bot deployed to Preview – nomad-ui August 27, 2024 04:30 View deployment

vercel bot deployed to Preview – nomad August 27, 2024 04:34 View deployment

msgtypes generated

1f8960b

vercel bot deployed to Preview – nomad-ui August 27, 2024 13:14 View deployment

gulducat reviewed Aug 27, 2024

View reviewed changes

Unset methods generally now I stare long into the namespace abyss

34b0d70

vercel bot deployed to Preview – nomad-ui August 28, 2024 05:25 View deployment

Namespace passes through with QueryOptions removed from a write reque…

0db9d4e

…sting struct

vercel bot deployed to Preview – nomad-ui August 28, 2024 15:01 View deployment

Comment and PR review cleanup

b40c036

vercel bot deployed to Preview – nomad-ui August 29, 2024 13:55 View deployment

vercel bot deployed to Preview – nomad August 29, 2024 14:00 View deployment

Version back to VersionStr

a6da85c

vercel bot deployed to Preview – nomad-ui August 29, 2024 21:37 View deployment

tgross reviewed Sep 3, 2024

View reviewed changes

nomad/structs/structs.go Outdated Show resolved Hide resolved

tgross reviewed Sep 10, 2024

View reviewed changes

nomad/state/state_store.go Outdated Show resolved Hide resolved

Generally consolidate unset logic into apply for version tagging

b93441a

philrenaud requested review from pkazmierczak and tgross September 11, 2024 03:41

philrenaud requested a review from gulducat September 11, 2024 03:41

vercel bot deployed to Preview – nomad-ui September 11, 2024 03:42 View deployment

tgross reviewed Sep 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tag and Untag job versions #23863

Tag and Untag job versions #23863

philrenaud commented Aug 25, 2024 •

edited

Loading

github-actions bot commented Aug 25, 2024 •

edited

Loading

gulducat left a comment

gulducat Aug 27, 2024

philrenaud Aug 29, 2024

philrenaud Sep 11, 2024

tgross Sep 11, 2024

tgross Sep 11, 2024

tgross Sep 11, 2024

tgross Sep 11, 2024

tgross Sep 11, 2024

tgross Sep 11, 2024

tgross Sep 11, 2024

tgross Sep 11, 2024

tgross Sep 11, 2024

tgross Sep 11, 2024

philrenaud Sep 11, 2024


		Example usage:

		nomad job tag apply -name "My Golden Version" -description "The version of the job we can roll back to in the future if needed" <jobname>

Tag and Untag job versions #23863

Are you sure you want to change the base?

Tag and Untag job versions #23863

Conversation

philrenaud commented Aug 25, 2024 • edited Loading

via curl:

via Nomad CLI:

github-actions bot commented Aug 25, 2024 • edited Loading

Ember Test Audit comparison

gulducat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philrenaud commented Aug 25, 2024 •

edited

Loading

via `curl`:

github-actions bot commented Aug 25, 2024 •

edited

Loading