-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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.
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
Fix dispatched optional meta correctly #4403
Changes from 6 commits
8e453d0
e8f9b40
e325e85
81abf0a
304b752
e79d314
f00875a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ import ( | |
cstructs "github.com/hashicorp/nomad/client/structs" | ||
"github.com/hashicorp/nomad/nomad/mock" | ||
"github.com/hashicorp/nomad/nomad/structs" | ||
"github.com/stretchr/testify/require" | ||
) | ||
|
||
const ( | ||
|
@@ -372,3 +373,19 @@ func TestEnvironment_UpdateTask(t *testing.T) { | |
t.Errorf("Expected NOMAD_META_taskmeta to be unset but found: %q", v) | ||
} | ||
} | ||
|
||
// TestEnvironment_InterpolateEmptyOptionalMeta asserts that in a parameterized | ||
// job, if an optional meta field is not set, it will get interpolated as an | ||
// empty string. | ||
func TestEnvironment_InterpolateEmptyOptionalMeta(t *testing.T) { | ||
a := mock.Alloc() | ||
a.Job.ParameterizedJob = &structs.ParameterizedJobConfig{ | ||
MetaOptional: []string{"metaopt1", "metaopt2"}, | ||
} | ||
a.Job.Dispatched = true | ||
task := a.Job.TaskGroups[0].Tasks[0] | ||
task.Meta = map[string]string{"metaopt1": "metaopt1val"} | ||
env := NewBuilder(mock.Node(), a, task, "global").Build() | ||
require.Equal(t, "metaopt1val", env.ReplaceEnv("${NOMAD_META_metaopt1}")) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For consistency with other tests, initialize |
||
require.Empty(t, env.ReplaceEnv("${NOMAD_META_metaopt2}")) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -124,10 +124,8 @@ func (j *Job) Register(args *structs.JobRegisterRequest, reply *structs.JobRegis | |
} | ||
|
||
// Validate job transitions if its an update | ||
if existingJob != nil { | ||
if err := validateJobUpdate(existingJob, args.Job); err != nil { | ||
return err | ||
} | ||
if err := validateJobUpdate(existingJob, args.Job); err != nil { | ||
return err | ||
} | ||
|
||
// Ensure that the job has permissions for the requested Vault tokens | ||
|
@@ -1327,6 +1325,14 @@ func validateJob(job *structs.Job) (invalid, warnings error) { | |
|
||
// validateJobUpdate ensures updates to a job are valid. | ||
func validateJobUpdate(old, new *structs.Job) error { | ||
// Validate Dispatch not set on new Jobs | ||
if old == nil { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a comment above saying we are validating a new job |
||
if new.Dispatched { | ||
return fmt.Errorf("job can't be submitted with 'Dispatched' set") | ||
} | ||
return nil | ||
} | ||
|
||
// Type transitions are disallowed | ||
if old.Type != new.Type { | ||
return fmt.Errorf("cannot update job from type %q to %q", old.Type, new.Type) | ||
|
@@ -1348,6 +1354,10 @@ func validateJobUpdate(old, new *structs.Job) error { | |
return fmt.Errorf("cannot update parameterized job to being non-parameterized") | ||
} | ||
|
||
if old.Dispatched != new.Dispatched { | ||
return fmt.Errorf("field 'Dispatched' is read-only") | ||
} | ||
|
||
return nil | ||
} | ||
|
||
|
@@ -1398,11 +1408,11 @@ func (j *Job) Dispatch(args *structs.JobDispatchRequest, reply *structs.JobDispa | |
|
||
// Derive the child job and commit it via Raft | ||
dispatchJob := parameterizedJob.Copy() | ||
dispatchJob.ParameterizedJob = nil | ||
dispatchJob.ID = structs.DispatchedID(parameterizedJob.ID, time.Now()) | ||
dispatchJob.ParentID = parameterizedJob.ID | ||
dispatchJob.Name = dispatchJob.ID | ||
dispatchJob.SetSubmitTime() | ||
dispatchJob.Dispatched = true | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
// Merge in the meta data | ||
for k, v := range args.Meta { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -458,6 +458,33 @@ func TestJobEndpoint_Register_ParameterizedJob(t *testing.T) { | |
} | ||
} | ||
|
||
func TestJobEndpoint_Register_Dispatched(t *testing.T) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use |
||
t.Parallel() | ||
s1 := TestServer(t, func(c *Config) { | ||
c.NumSchedulers = 0 // Prevent automatic dequeue | ||
}) | ||
defer s1.Shutdown() | ||
codec := rpcClient(t, s1) | ||
testutil.WaitForLeader(t, s1.RPC) | ||
|
||
// Create the register request with a job with 'Dispatch' set to true | ||
job := mock.Job() | ||
job.Dispatched = true | ||
req := &structs.JobRegisterRequest{ | ||
Job: job, | ||
WriteRequest: structs.WriteRequest{ | ||
Region: "global", | ||
Namespace: job.Namespace, | ||
}, | ||
} | ||
|
||
// Fetch the response | ||
var resp structs.JobRegisterResponse | ||
err := msgpackrpc.CallWithCodec(codec, "Job.Register", req, &resp) | ||
if err == nil || !strings.Contains(err.Error(), "'Dispatched' is read-only") { | ||
t.Fatalf("expected dispatch read-only error: %v", err) | ||
} | ||
} | ||
func TestJobEndpoint_Register_EnforceIndex(t *testing.T) { | ||
t.Parallel() | ||
s1 := TestServer(t, func(c *Config) { | ||
|
@@ -3959,6 +3986,7 @@ func TestJobEndpoint_ValidateJob_KillSignal(t *testing.T) { | |
|
||
func TestJobEndpoint_ValidateJobUpdate(t *testing.T) { | ||
t.Parallel() | ||
require := require.New(t) | ||
old := mock.Job() | ||
new := mock.Job() | ||
|
||
|
@@ -3988,6 +4016,16 @@ func TestJobEndpoint_ValidateJobUpdate(t *testing.T) { | |
} else { | ||
t.Log(err) | ||
} | ||
|
||
new = mock.Job() | ||
new.Dispatched = true | ||
require.Error(validateJobUpdate(old, new), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Generally you want to assert the error you are expecting. require.Contains(err.Error(), "foo bar") |
||
"expected err when setting new job to dispatched") | ||
require.Error(validateJobUpdate(nil, new), | ||
"expected err when setting new job to dispatched") | ||
require.Error(validateJobUpdate(new, old), | ||
"expected err when setting dispatched to false") | ||
require.NoError(validateJobUpdate(nil, old)) | ||
} | ||
|
||
func TestJobEndpoint_ValidateJobUpdate_ACL(t *testing.T) { | ||
|
@@ -4343,6 +4381,15 @@ func TestJobEndpoint_Dispatch(t *testing.T) { | |
if out.ParentID != tc.parameterizedJob.ID { | ||
t.Fatalf("bad parent ID") | ||
} | ||
if !out.Dispatched { | ||
t.Fatal("expected dispatched job") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a test that the parameterized stanza is on the job There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
} | ||
if out.IsParameterized() { | ||
t.Fatal("dispatched job should not be parameterized") | ||
} | ||
if out.ParameterizedJob == nil { | ||
t.Fatal("parameter job config should exist") | ||
} | ||
|
||
if tc.noEval { | ||
return | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extract the size into a named variable so reading this is clearer.