Mailing-List: contact commits-help@aurora.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@aurora.apache.org
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
From: zmanji@apache.org
To: commits@aurora.apache.org
Message-Id: <72479ac3b60046fc8bf756874ec211f0@git.apache.org>
Subject: aurora git commit: Move deprecated resource validations so they
 happen after the thrift backfill.
Date: Mon, 30 Jan 2017 19:19:09 +0000 (UTC)
archived-at: Mon, 30 Jan 2017 19:19:12 -0000

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 <ndonatucci@medallia.com>
Authored: Mon Jan 30 11:18:59 2017 -0800
Committer: Zameer Manji <zmanji@apache.org>
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("testing@twitter.com")