From 68eacae7feb4cbca204545b61abf6ae4147a416b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maximilian=20Pa=C3=9F?= <22845248+mpass99@users.noreply.github.com> Date: Tue, 6 Jul 2021 10:09:36 +0200 Subject: [PATCH] Fix bug that config task group is not added to the template job (and the faulty tests) --- api/websocket_test.go | 34 ++++++++++++++++--- ci/demo-job.tpl.nomad | 5 ++- environment/template-environment-job.hcl | 5 ++- nomad/job.go | 43 ++++++++++++------------ nomad/job_test.go | 31 +++++------------ runner/manager_test.go | 2 +- tests/helpers/test_helpers.go | 42 +++++------------------ 7 files changed, 73 insertions(+), 89 deletions(-) diff --git a/api/websocket_test.go b/api/websocket_test.go index 48cb8ff..2df427b 100644 --- a/api/websocket_test.go +++ b/api/websocket_test.go @@ -391,7 +391,7 @@ var executionRequestLs = dto.ExecutionRequest{Command: "ls"} // mockApiExecuteLs mocks the ExecuteCommand of an ExecutorApi to act as if 'ls existing-file non-existing-file' was executed. func mockApiExecuteLs(api *nomad.ExecutorAPIMock) { - helpers.MockApiExecute(api, &executionRequestLs, + mockApiExecute(api, &executionRequestLs, func(_ string, _ context.Context, _ []string, _ bool, _ io.Reader, stdout, stderr io.Writer) (int, error) { _, _ = stdout.Write([]byte("existing-file\n")) _, _ = stderr.Write([]byte("ls: cannot access 'non-existing-file': No such file or directory\n")) @@ -403,7 +403,7 @@ var executionRequestHead = dto.ExecutionRequest{Command: "head -n 1"} // mockApiExecuteHead mocks the ExecuteCommand of an ExecutorApi to act as if 'head -n 1' was executed. func mockApiExecuteHead(api *nomad.ExecutorAPIMock) { - helpers.MockApiExecute(api, &executionRequestHead, + mockApiExecute(api, &executionRequestHead, func(_ string, _ context.Context, _ []string, _ bool, stdin io.Reader, stdout io.Writer, stderr io.Writer) (int, error) { scanner := bufio.NewScanner(stdin) for !scanner.Scan() { @@ -419,7 +419,7 @@ var executionRequestSleep = dto.ExecutionRequest{Command: "sleep infinity"} // mockApiExecuteSleep mocks the ExecuteCommand method of an ExecutorAPI to sleep until the execution is canceled. func mockApiExecuteSleep(api *nomad.ExecutorAPIMock) <-chan bool { canceled := make(chan bool, 1) - helpers.MockApiExecute(api, &executionRequestSleep, + mockApiExecute(api, &executionRequestSleep, func(_ string, ctx context.Context, _ []string, _ bool, stdin io.Reader, stdout io.Writer, stderr io.Writer) (int, error) { <-ctx.Done() close(canceled) @@ -432,7 +432,7 @@ var executionRequestError = dto.ExecutionRequest{Command: "error"} // mockApiExecuteError mocks the ExecuteCommand method of an ExecutorApi to return an error. func mockApiExecuteError(api *nomad.ExecutorAPIMock) { - helpers.MockApiExecute(api, &executionRequestError, + mockApiExecute(api, &executionRequestError, func(_ string, _ context.Context, _ []string, _ bool, _ io.Reader, _, _ io.Writer) (int, error) { return 0, errors.New("intended error") }) @@ -442,8 +442,32 @@ var executionRequestExitNonZero = dto.ExecutionRequest{Command: "exit 42"} // mockApiExecuteExitNonZero mocks the ExecuteCommand method of an ExecutorApi to exit with exit status 42. func mockApiExecuteExitNonZero(api *nomad.ExecutorAPIMock) { - helpers.MockApiExecute(api, &executionRequestExitNonZero, + mockApiExecute(api, &executionRequestExitNonZero, func(_ string, _ context.Context, _ []string, _ bool, _ io.Reader, _, _ io.Writer) (int, error) { return 42, nil }) } + +// mockApiExecute mocks the ExecuteCommand method of an ExecutorApi to call the given method run when the command +// corresponding to the given ExecutionRequest is called. +func mockApiExecute(api *nomad.ExecutorAPIMock, request *dto.ExecutionRequest, + run func(runnerId string, ctx context.Context, command []string, tty bool, stdin io.Reader, stdout, stderr io.Writer) (int, error)) { + call := api.On("ExecuteCommand", + mock.AnythingOfType("string"), + mock.Anything, + request.FullCommand(), + mock.AnythingOfType("bool"), + mock.Anything, + mock.Anything, + mock.Anything) + call.Run(func(args mock.Arguments) { + exit, err := run(args.Get(0).(string), + args.Get(1).(context.Context), + args.Get(2).([]string), + args.Get(3).(bool), + args.Get(4).(io.Reader), + args.Get(5).(io.Writer), + args.Get(6).(io.Writer)) + call.ReturnArguments = mock.Arguments{exit, err} + }) +} diff --git a/ci/demo-job.tpl.nomad b/ci/demo-job.tpl.nomad index e9983cb..9723bd2 100644 --- a/ci/demo-job.tpl.nomad +++ b/ci/demo-job.tpl.nomad @@ -57,7 +57,7 @@ job "template-0" { task "config" { driver = "exec" config { - command = "whoami" + command = "true" } logs { max_files = 1 @@ -70,9 +70,8 @@ job "template-0" { } } meta { - environment = "0" used = "false" - prewarmingPoolSize = "1" + prewarmingPoolSize = "0" } } } diff --git a/environment/template-environment-job.hcl b/environment/template-environment-job.hcl index 9c294b7..e1727eb 100644 --- a/environment/template-environment-job.hcl +++ b/environment/template-environment-job.hcl @@ -59,7 +59,7 @@ job "template-0" { task "config" { driver = "exec" config { - command = "whoami" + command = "true" } logs { max_files = 1 @@ -72,9 +72,8 @@ job "template-0" { } } meta { - environment = "0" used = "false" - prewarmingPoolSize = "1" + prewarmingPoolSize = "0" } } } diff --git a/nomad/job.go b/nomad/job.go index 5f3fa80..23a32ba 100644 --- a/nomad/job.go +++ b/nomad/job.go @@ -9,20 +9,19 @@ import ( ) const ( - TaskGroupName = "default-group" - TaskName = "default-task" - TemplateJobPrefix = "template" - ConfigTaskGroupName = "config" - DummyTaskName = "dummy" - DefaultTaskDriver = "docker" - DefaultDummyTaskDriver = "exec" - DefaultDummyTaskCommand = "true" - ConfigMetaEnvironmentKey = "environment" - ConfigMetaUsedKey = "used" - ConfigMetaUsedValue = "true" - ConfigMetaUnusedValue = "false" - ConfigMetaTimeoutKey = "timeout" - ConfigMetaPoolSizeKey = "prewarmingPoolSize" + TemplateJobPrefix = "template" + TaskGroupName = "default-group" + TaskName = "default-task" + TaskDriver = "docker" + ConfigTaskGroupName = "config" + ConfigTaskName = "config" + ConfigTaskDriver = "exec" + ConfigTaskCommand = "true" + ConfigMetaUsedKey = "used" + ConfigMetaUsedValue = "true" + ConfigMetaUnusedValue = "false" + ConfigMetaTimeoutKey = "timeout" + ConfigMetaPoolSizeKey = "prewarmingPoolSize" ) var ( @@ -165,7 +164,7 @@ func configureTask( exposedPorts []uint16) { var task *nomadApi.Task if len(taskGroup.Tasks) == 0 { - task = nomadApi.NewTask(name, DefaultTaskDriver) + task = nomadApi.NewTask(name, TaskDriver) taskGroup.Tasks = []*nomadApi.Task{task} } else { task = taskGroup.Tasks[0] @@ -173,10 +172,11 @@ func configureTask( } integerCPULimit := int(cpuLimit) integerMemoryLimit := int(memoryLimit) - task.Resources = &nomadApi.Resources{ - CPU: &integerCPULimit, - MemoryMB: &integerMemoryLimit, + if task.Resources == nil { + task.Resources = &nomadApi.Resources{} } + task.Resources.CPU = &integerCPULimit + task.Resources.MemoryMB = &integerMemoryLimit if task.Config == nil { task.Config = make(map[string]interface{}) @@ -204,6 +204,7 @@ func findOrCreateConfigTaskGroup(job *nomadApi.Job) *nomadApi.TaskGroup { taskGroup := FindConfigTaskGroup(job) if taskGroup == nil { taskGroup = nomadApi.NewTaskGroup(ConfigTaskGroupName, 0) + job.AddTaskGroup(taskGroup) } createDummyTaskIfNotPresent(taskGroup) return taskGroup @@ -213,19 +214,19 @@ func findOrCreateConfigTaskGroup(job *nomadApi.Job) *nomadApi.TaskGroup { func createDummyTaskIfNotPresent(taskGroup *nomadApi.TaskGroup) { var task *nomadApi.Task for _, t := range taskGroup.Tasks { - if t.Name == DummyTaskName { + if t.Name == ConfigTaskName { task = t break } } if task == nil { - task = nomadApi.NewTask(DummyTaskName, DefaultDummyTaskDriver) + task = nomadApi.NewTask(ConfigTaskName, ConfigTaskDriver) taskGroup.Tasks = append(taskGroup.Tasks, task) } if task.Config == nil { task.Config = make(map[string]interface{}) } - task.Config["command"] = DefaultDummyTaskCommand + task.Config["command"] = ConfigTaskCommand } diff --git a/nomad/job_test.go b/nomad/job_test.go index 2ca945a..f0488e7 100644 --- a/nomad/job_test.go +++ b/nomad/job_test.go @@ -9,6 +9,8 @@ import ( "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" "gitlab.hpi.de/codeocean/codemoon/poseidon/tests" + "gitlab.hpi.de/codeocean/codemoon/poseidon/tests/helpers" + "strconv" "testing" ) @@ -26,25 +28,6 @@ func createTestResources() *nomadApi.Resources { return &nomadApi.Resources{CPU: &expectedCPULimit, MemoryMB: &expectedMemoryLimit} } -func createTestJob() (job, base *nomadApi.Job) { - jobID := tests.DefaultJobID - base = nomadApi.NewBatchJob(jobID, jobID, "region-name", 100) - job = nomadApi.NewBatchJob(jobID, jobID, "region-name", 100) - task := createTestTask() - task.Name = TaskName - image := "python:latest" - task.Config = map[string]interface{}{"image": image} - task.Config["network_mode"] = "none" - task.Resources = createTestResources() - taskGroup := createTestTaskGroup() - taskGroupName := TaskGroupName - taskGroup.Name = &taskGroupName - taskGroup.Tasks = []*nomadApi.Task{task} - taskGroup.Networks = []*nomadApi.NetworkResource{} - job.TaskGroups = []*nomadApi.TaskGroup{taskGroup} - return job, base -} - func TestCreateTaskGroupCreatesNewTaskGroupWhenJobHasNoTaskGroup(t *testing.T) { job := nomadApi.NewBatchJob("test", "test", "test", 1) @@ -180,7 +163,7 @@ func TestConfigureTaskWhenNoTaskExists(t *testing.T) { expectedResources := createTestResources() expectedTaskGroup := *taskGroup - expectedTask := nomadApi.NewTask("task", DefaultTaskDriver) + expectedTask := nomadApi.NewTask("task", TaskDriver) expectedTask.Resources = expectedResources expectedImage := "python:latest" expectedTask.Config = map[string]interface{}{"image": expectedImage, "network_mode": "none"} @@ -220,11 +203,13 @@ func TestConfigureTaskWhenTaskExists(t *testing.T) { } func TestCreateTemplateJobSetsAllGivenArguments(t *testing.T) { - testJob, base := createTestJob() + testJob := helpers.CreateTemplateJob() + prewarmingPoolSize, err := strconv.Atoi(testJob.TaskGroups[1].Meta[ConfigMetaPoolSizeKey]) + require.NoError(t, err) job := CreateTemplateJob( - base, + helpers.CreateTemplateJob(), tests.DefaultJobID, - uint(*testJob.TaskGroups[0].Count), + uint(prewarmingPoolSize), uint(*testJob.TaskGroups[0].Tasks[0].Resources.CPU), uint(*testJob.TaskGroups[0].Tasks[0].Resources.MemoryMB), testJob.TaskGroups[0].Tasks[0].Config["image"].(string), diff --git a/runner/manager_test.go b/runner/manager_test.go index 953105b..5670893 100644 --- a/runner/manager_test.go +++ b/runner/manager_test.go @@ -257,7 +257,7 @@ func (s *ManagerTestSuite) TestUpdateRunnersRemovesIdleAndUsedRunner() { } func (s *ManagerTestSuite) TestUpdateEnvironmentRemovesIdleRunnersWhenScalingDown() { - job := helpers.CreateTestJob() + job := helpers.CreateTemplateJob() initialRunners := uint(40) updatedRunners := uint(10) err := s.nomadRunnerManager.registerEnvironment(anotherEnvironmentID, initialRunners, job, true) diff --git a/tests/helpers/test_helpers.go b/tests/helpers/test_helpers.go index 491bd08..0b0b259 100644 --- a/tests/helpers/test_helpers.go +++ b/tests/helpers/test_helpers.go @@ -4,16 +4,14 @@ package helpers import ( "bytes" - "context" "crypto/tls" "encoding/json" "github.com/gorilla/mux" "github.com/gorilla/websocket" nomadApi "github.com/hashicorp/nomad/api" - "github.com/stretchr/testify/mock" "gitlab.hpi.de/codeocean/codemoon/poseidon/api/dto" "gitlab.hpi.de/codeocean/codemoon/poseidon/config" - "gitlab.hpi.de/codeocean/codemoon/poseidon/nomad" + "gitlab.hpi.de/codeocean/codemoon/poseidon/tests" "io" "net/http" "net/http/httptest" @@ -113,30 +111,6 @@ func StartTLSServer(t *testing.T, router *mux.Router) (server *httptest.Server, return } -// MockApiExecute mocks the ExecuteCommand method of an ExecutorApi to call the given method run when the command -// corresponding to the given ExecutionRequest is called. -func MockApiExecute(api *nomad.ExecutorAPIMock, request *dto.ExecutionRequest, - run func(runnerId string, ctx context.Context, command []string, tty bool, stdin io.Reader, stdout, stderr io.Writer) (int, error)) { - call := api.On("ExecuteCommand", - mock.AnythingOfType("string"), - mock.Anything, - request.FullCommand(), - mock.AnythingOfType("bool"), - mock.Anything, - mock.Anything, - mock.Anything) - call.Run(func(args mock.Arguments) { - exit, err := run(args.Get(0).(string), - args.Get(1).(context.Context), - args.Get(2).([]string), - args.Get(3).(bool), - args.Get(4).(io.Reader), - args.Get(5).(io.Writer), - args.Get(6).(io.Writer)) - call.ReturnArguments = mock.Arguments{exit, err} - }) -} - // HttpDelete sends a Delete Http Request with body to the passed url. func HttpDelete(url string, body io.Reader) (response *http.Response, err error) { req, _ := http.NewRequest(http.MethodDelete, url, body) @@ -167,19 +141,21 @@ func HttpPutJSON(url string, body interface{}) (response *http.Response, err err return HttpPut(url, reader) } -func CreateTestJob() (job *nomadApi.Job) { - job = nomadApi.NewBatchJob("template-0", "template-0", "region-name", 100) +func CreateTemplateJob() (job *nomadApi.Job) { + job = nomadApi.NewBatchJob(tests.DefaultJobID, tests.DefaultJobID, "region-name", 100) configTaskGroup := nomadApi.NewTaskGroup("config", 0) configTaskGroup.Meta = make(map[string]string) - configTaskGroup.Meta["environment"] = "0" - configTaskGroup.Meta["used"] = "false" configTaskGroup.Meta["prewarmingPoolSize"] = "0" configTask := nomadApi.NewTask("config", "exec") - configTask.Config = map[string]interface{}{"command": "whoami"} + configTask.Config = map[string]interface{}{ + "command": "true", + "image": "python:latest", + } configTask.Resources = nomadApi.DefaultResources() configTaskGroup.AddTask(configTask) defaultTaskGroup := nomadApi.NewTaskGroup("default-group", 1) + defaultTaskGroup.Networks = []*nomadApi.NetworkResource{} defaultTask := nomadApi.NewTask("default-task", "docker") defaultTask.Config = map[string]interface{}{ "image": "python:latest", @@ -190,6 +166,6 @@ func CreateTestJob() (job *nomadApi.Job) { defaultTask.Resources = nomadApi.DefaultResources() defaultTaskGroup.AddTask(defaultTask) - job.TaskGroups = []*nomadApi.TaskGroup{configTaskGroup, defaultTaskGroup} + job.TaskGroups = []*nomadApi.TaskGroup{defaultTaskGroup, configTaskGroup} return job }