Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

job region defaults to node region #6064

Merged
merged 5 commits into from
Aug 5, 2019
Merged

job region defaults to node region #6064

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bde6fa0
job region defaults to node region
jazzyfresh Aug 2, 2019
274d938
set region defaults before job canonicalization
jazzyfresh Aug 2, 2019
6011d87
fix pointers
jazzyfresh Aug 3, 2019
d0ed978
use constant for magic 'global' region
jazzyfresh Aug 3, 2019
fcf7c0d
add better comments for GlobalRegion
jazzyfresh Aug 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion api/jobs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -704,7 +704,7 @@ func (j *Job) Canonicalize() {
j.Stop = boolToPtr(false)
}
if j.Region == nil {
j.Region = stringToPtr("global")
j.Region = stringToPtr("")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks all the canonicalize tests and seems like the wrong thing to do. Canonicalize should continue to use "global" as the region if its not set.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be set to a sentinel value that indicates region was not provided and canonicalized. Changing this means I will have to change all the tests tho

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we ultimately decided to leave canonicalization alone but changing the way jobUpdate sets defaults. Or do I still canonicalize to a sentienl value? cc @schmichael @angrycub

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. I'm leaning toward leaving canonicalization alone if for no other reason than backwards compatibility.

The job submission code can do the proper default for Job.Region and avoid this code taking effect entirely.

More detailed ramblings we can ignore for now:

I hate leaving intentionally-circumvented-cruft lying about, but I think if we change Canonicalize's behavior we should force it to accept a default region as an argument. This would break existing users of this code and force them to make an explicit region decision which I think is the ideal upgrade path. I don't think it's necessary to do that work today as it has implications for all of Canonicalize's callers.

}
if j.Namespace == nil {
j.Namespace = stringToPtr("default")
Expand Down
8 changes: 6 additions & 2 deletions command/agent/job_endpoint.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,14 +384,18 @@ func (s *HTTPServer) jobUpdate(resp http.ResponseWriter, req *http.Request,
return nil, CodedError(400, "Job ID does not match name")
}

// Http region takes precedence over hcl region
// Region in api request query param takes precedence over region in job config
if args.WriteRequest.Region != "" {
args.Job.Region = helper.StringToPtr(args.WriteRequest.Region)
}

jazzyfresh marked this conversation as resolved.
Show resolved Hide resolved
// If no region given, region is canonicalized to 'global'
sJob := ApiJobToStructJob(args.Job)

// If no region given, defaults to region of the node
if sJob.Region == "" {
sJob.Region = s.agent.config.Region
}

jazzyfresh marked this conversation as resolved.
Show resolved Hide resolved
regReq := structs.JobRegisterRequest{
Job: sJob,
EnforceIndex: args.EnforceIndex,
Expand Down
8 changes: 7 additions & 1 deletion command/agent/job_endpoint_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -493,11 +493,17 @@ func TestHTTP_JobUpdateRegion(t *testing.T) {
ExpectedRegion: "north-america",
},
{
Name: "falls back to default if no region is provided",
Name: "defaults to node region global if no region is provided",
ConfigRegion: "",
APIRegion: "",
ExpectedRegion: "global",
},
{
Name: "defaults to node region not-global if no region is provided",
ConfigRegion: "",
APIRegion: "",
ExpectedRegion: "not-global",
},
}

for _, tc := range cases {
Expand Down