Repository: aurora Updated Branches: refs/heads/master 7be7ad6f1 -> 07065b502
Move deprecated resource validations so they happen after the thrift backfill. As the validations for NumCpus, RamMb and DiskMb happened before the thrift backfill, those values needed to be set, even though they are deprecated. In the thrift backfill, if the Resources field is set, then NumCpus, RamMb and DiskMb are set accordingly. So by moving those validations, it is now possible to only set the Resources field instead of having to set the deprecated fields. As the validations are moved and not removed, the ckeck for the resource values being greater than 0 still happens. Furthermore, if the Resources field is set but there is no Resource for Ram in the set, the thrift backfill will throw an IllegalArgumentException. Some tests were slightly modified because of this, mostly by adding an unsetResources() operation. This is because as the validations now happen after the thrift backfill, during the thrift backfill the values in the deprecated fields are replaced by those in the Resources field (if it is set). There are also some new tests. Related Issue: AURORA-1707 Testing Done: src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh Reviewed at https://reviews.apache.org/r/55982/ Project: http://git-wip-us.apache.org/repos/asf/aurora/repo Commit: http://git-wip-us.apache.org/repos/asf/aurora/commit/07065b50 Tree: http://git-wip-us.apache.org/repos/asf/aurora/tree/07065b50 Diff: http://git-wip-us.apache.org/repos/asf/aurora/diff/07065b50 Branch: refs/heads/master Commit: 07065b502a40971d631811d0e5a2b1773fcf8814 Parents: 7be7ad6 Author: Nicolás Donatucci <[email protected]> Authored: Mon Jan 30 11:18:59 2017 -0800 Committer: Zameer Manji <[email protected]> Committed: Mon Jan 30 11:18:59 2017 -0800 ---------------------------------------------------------------------- .../configuration/ConfigurationManager.java | 9 ++- .../thrift/ReadOnlySchedulerImplTest.java | 4 +- .../thrift/SchedulerThriftInterfaceTest.java | 61 ++++++++++++++++++++ 3 files changed, 68 insertions(+), 6 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/aurora/blob/07065b50/src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java b/src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java index f96cd7a..80f0aeb 100644 --- a/src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java +++ b/src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java @@ -294,11 +294,6 @@ public class ConfigurationManager { + "' doesn't exist.")); } - // Maximize the usefulness of any thrown error message by checking required fields first. - for (RequiredFieldValidator<?> validator : REQUIRED_FIELDS_VALIDATORS) { - validator.validate(builder); - } - IConstraint constraint = getDedicatedConstraint(config); if (constraint != null) { if (!isValueConstraint(constraint.getConstraint())) { @@ -357,6 +352,10 @@ public class ConfigurationManager { thriftBackfill.backfillTask(builder); + for (RequiredFieldValidator<?> validator : REQUIRED_FIELDS_VALIDATORS) { + validator.validate(builder); + } + String types = config.getResources().stream() .collect(Collectors.groupingBy(e -> ResourceType.fromResource(e))) .entrySet().stream() http://git-wip-us.apache.org/repos/asf/aurora/blob/07065b50/src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java ---------------------------------------------------------------------- diff --git a/src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java b/src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java index 6d0e9bc..04f7829 100644 --- a/src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java +++ b/src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java @@ -856,9 +856,11 @@ public class ReadOnlySchedulerImplTest extends EasyMockTest { @Test public void testGetJobUpdateDiffInvalidConfig() throws Exception { control.replay(); + TaskConfig task = defaultTask(false).setNumCpus(-1); + task.unsetResources(); JobUpdateRequest request = - new JobUpdateRequest().setTaskConfig(defaultTask(false).setNumCpus(-1)); + new JobUpdateRequest().setTaskConfig(task); assertResponse(INVALID_REQUEST, thrift.getJobUpdateDiff(request)); } http://git-wip-us.apache.org/repos/asf/aurora/blob/07065b50/src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java ---------------------------------------------------------------------- diff --git a/src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java b/src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java index 5262e57..b7574e4 100644 --- a/src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java +++ b/src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java @@ -494,15 +494,27 @@ public class SchedulerThriftInterfaceTest extends EasyMockTest { task.setNumCpus(0); task.setRamMb(0); task.setDiskMb(0); + task.unsetResources(); assertResponse(INVALID_REQUEST, thrift.createJob(makeJob(task))); assertEquals(0L, statsProvider.getLongValue(CREATE_JOB)); } + @Test(expected = IllegalArgumentException.class) + public void testCreateJobMissingResources() throws Exception { + control.replay(); + TaskConfig task = productionTask(); + task.unsetResources(); + task.setResources(ImmutableSet.of(numCpus(1.0))); + + thrift.createJob(makeJob(task)); + } + @Test public void testCreateJobBadCpu() throws Exception { control.replay(); TaskConfig task = productionTask().setNumCpus(0.0); + task.unsetResources(); assertResponse(INVALID_REQUEST, thrift.createJob(makeJob(task))); assertEquals(0L, statsProvider.getLongValue(CREATE_JOB)); } @@ -512,6 +524,7 @@ public class SchedulerThriftInterfaceTest extends EasyMockTest { control.replay(); TaskConfig task = productionTask().setRamMb(-123); + task.unsetResources(); assertResponse(INVALID_REQUEST, thrift.createJob(makeJob(task))); assertEquals(0L, statsProvider.getLongValue(CREATE_JOB)); } @@ -521,11 +534,59 @@ public class SchedulerThriftInterfaceTest extends EasyMockTest { control.replay(); TaskConfig task = productionTask().setDiskMb(0); + task.unsetResources(); + assertResponse(INVALID_REQUEST, thrift.createJob(makeJob(task))); + } + + @Test + public void testCreateJobBadResources() throws Exception { + control.replay(); + + TaskConfig task = productionTask() + .setResources(ImmutableSet.of( + numCpus(-1), + ramMb(1024), + diskMb(1024))) + .setNumCpus(0) + .setRamMb(0) + .setDiskMb(0); + assertResponse(INVALID_REQUEST, thrift.createJob(makeJob(task))); assertEquals(0L, statsProvider.getLongValue(CREATE_JOB)); } @Test + public void testCreateJobGoodResources() throws Exception { + + TaskConfig task = productionTask() + .setResources(ImmutableSet.of( + numCpus(1.0), + ramMb(1024), + diskMb(1024))) + .setNumCpus(0) + .setRamMb(0) + .setDiskMb(0); + + IJobConfiguration job = IJobConfiguration.build(makeJob(task)); + SanitizedConfiguration sanitized = fromUnsanitized(job); + lockManager.assertNotLocked(LOCK_KEY); + storageUtil.expectTaskFetch(Query.jobScoped(JOB_KEY).active()); + expectNoCronJob(); + expect(taskIdGenerator.generate(sanitized.getJobConfig().getTaskConfig(), 1)) + .andReturn(TASK_ID); + expectInstanceQuotaCheck(sanitized, ENOUGH_QUOTA); + + stateManager.insertPendingTasks( + storageUtil.mutableStoreProvider, + sanitized.getJobConfig().getTaskConfig(), + sanitized.getInstanceIds()); + + control.replay(); + + assertOkResponse(thrift.createJob(job.newBuilder())); + } + + @Test public void testCreateJobPopulateDefaults() throws Exception { TaskConfig task = new TaskConfig() .setContactEmail("[email protected]")
