Repository: aurora Updated Branches: refs/heads/master 06518c86b -> 40d9d4dbe
Remove restriction on task id length. To work around an old Mesos bug (MESOS-691) we would reject jobs that resulted in Mesos task ids longer than 255 characters. This is because Mesos used to use the task id to generate the cgroup path. Now Mesos uses it's own id, we no longer need to work around this bug. This removes the restriction in the API layer. This is useful because some users may have very long role and service names that caused task ids to go over this limit. Bugs closed: AURORA-1897 Reviewed at https://reviews.apache.org/r/59957/ Project: http://git-wip-us.apache.org/repos/asf/aurora/repo Commit: http://git-wip-us.apache.org/repos/asf/aurora/commit/40d9d4db Tree: http://git-wip-us.apache.org/repos/asf/aurora/tree/40d9d4db Diff: http://git-wip-us.apache.org/repos/asf/aurora/diff/40d9d4db Branch: refs/heads/master Commit: 40d9d4dbec86cb4a17e281dc10ede25e83613eff Parents: 06518c8 Author: Zameer Manji <[email protected]> Authored: Mon Jun 12 13:14:18 2017 -0700 Committer: Zameer Manji <[email protected]> Committed: Mon Jun 12 13:14:18 2017 -0700 ---------------------------------------------------------------------- .../thrift/SchedulerThriftInterface.java | 22 ----- .../thrift/SchedulerThriftInterfaceTest.java | 99 -------------------- 2 files changed, 121 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/aurora/blob/40d9d4db/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java b/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java index 059fbb8..7ea6fc4 100644 --- a/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java +++ b/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java @@ -70,7 +70,6 @@ import org.apache.aurora.gen.ScheduleStatus; import org.apache.aurora.gen.StartJobUpdateResult; import org.apache.aurora.gen.StartMaintenanceResult; import org.apache.aurora.gen.TaskQuery; -import org.apache.aurora.scheduler.TaskIdGenerator; import org.apache.aurora.scheduler.base.JobKeys; import org.apache.aurora.scheduler.base.Numbers; import org.apache.aurora.scheduler.base.Query; @@ -155,11 +154,6 @@ import static org.apache.aurora.scheduler.thrift.Responses.ok; @DecoratedThrift class SchedulerThriftInterface implements AnnotatedAuroraAdmin { - // This number is derived from the maximum file name length limit on most UNIX systems, less - // the number of characters we've observed being added by mesos for the executor ID, prefix, and - // delimiters. - @VisibleForTesting - static final int MAX_TASK_ID_LENGTH = 255 - 90; @VisibleForTesting static final String STAT_PREFIX = "thrift_workload_"; @VisibleForTesting @@ -197,7 +191,6 @@ class SchedulerThriftInterface implements AnnotatedAuroraAdmin { private final CronJobManager cronJobManager; private final QuotaManager quotaManager; private final StateManager stateManager; - private final TaskIdGenerator taskIdGenerator; private final UUIDGenerator uuidGenerator; private final JobUpdateController jobUpdateController; private final ReadOnlyScheduler.Iface readOnlyScheduler; @@ -228,7 +221,6 @@ class SchedulerThriftInterface implements AnnotatedAuroraAdmin { MaintenanceController maintenance, QuotaManager quotaManager, StateManager stateManager, - TaskIdGenerator taskIdGenerator, UUIDGenerator uuidGenerator, JobUpdateController jobUpdateController, ReadOnlyScheduler.Iface readOnlyScheduler, @@ -246,7 +238,6 @@ class SchedulerThriftInterface implements AnnotatedAuroraAdmin { this.cronJobManager = requireNonNull(cronJobManager); this.quotaManager = requireNonNull(quotaManager); this.stateManager = requireNonNull(stateManager); - this.taskIdGenerator = requireNonNull(taskIdGenerator); this.uuidGenerator = requireNonNull(uuidGenerator); this.jobUpdateController = requireNonNull(jobUpdateController); this.readOnlyScheduler = requireNonNull(readOnlyScheduler); @@ -293,7 +284,6 @@ class SchedulerThriftInterface implements AnnotatedAuroraAdmin { int count = sanitized.getJobConfig().getInstanceCount(); validateTaskLimits( - template, count, quotaManager.checkInstanceAddition(template, count, storeProvider)); @@ -349,11 +339,9 @@ class SchedulerThriftInterface implements AnnotatedAuroraAdmin { try { lockManager.assertNotLocked(ILockKey.build(LockKey.job(jobKey.newBuilder()))); - ITaskConfig template = sanitized.getJobConfig().getTaskConfig(); int count = sanitized.getJobConfig().getInstanceCount(); validateTaskLimits( - template, count, quotaManager.checkCronUpdate(sanitized.getJobConfig(), storeProvider)); @@ -863,7 +851,6 @@ class SchedulerThriftInterface implements AnnotatedAuroraAdmin { ITaskConfig task = templateTask.get().getAssignedTask().getTask(); validateTaskLimits( - task, Iterables.size(currentTasks) + instanceIds.size(), quotaManager.checkInstanceAddition(task, instanceIds.size(), storeProvider)); @@ -891,7 +878,6 @@ class SchedulerThriftInterface implements AnnotatedAuroraAdmin { } private void validateTaskLimits( - ITaskConfig task, int totalInstances, QuotaCheckResult quotaCheck) throws TaskValidationException { @@ -900,13 +886,6 @@ class SchedulerThriftInterface implements AnnotatedAuroraAdmin { "Instance count must be between 1 and %d inclusive.", thresholds.getMaxTasksPerJob())); } - // TODO(maximk): This is a short-term hack to stop the bleeding from - // https://issues.apache.org/jira/browse/MESOS-691 - if (taskIdGenerator.generate(task, totalInstances).length() > MAX_TASK_ID_LENGTH) { - throw new TaskValidationException( - "Task ID is too long, please shorten your role or job name."); - } - if (quotaCheck.getResult() == INSUFFICIENT_QUOTA) { throw new TaskValidationException("Insufficient resource quota: " + quotaCheck.getDetails().or("")); @@ -1026,7 +1005,6 @@ class SchedulerThriftInterface implements AnnotatedAuroraAdmin { Response response = empty(); try { validateTaskLimits( - request.getTaskConfig(), request.getInstanceCount(), quotaManager.checkJobUpdate(update, storeProvider)); http://git-wip-us.apache.org/repos/asf/aurora/blob/40d9d4db/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 016859c..9c44668 100644 --- a/src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java +++ b/src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java @@ -76,7 +76,6 @@ import org.apache.aurora.gen.TaskConstraint; import org.apache.aurora.gen.TaskQuery; import org.apache.aurora.gen.ValueConstraint; import org.apache.aurora.gen.apiConstants; -import org.apache.aurora.scheduler.TaskIdGenerator; import org.apache.aurora.scheduler.base.JobKeys; import org.apache.aurora.scheduler.base.Query; import org.apache.aurora.scheduler.base.TaskTestUtil; @@ -164,7 +163,6 @@ import static org.apache.aurora.scheduler.thrift.SchedulerThriftInterface.DRAIN_ import static org.apache.aurora.scheduler.thrift.SchedulerThriftInterface.END_MAINTENANCE; import static org.apache.aurora.scheduler.thrift.SchedulerThriftInterface.KILL_TASKS; import static org.apache.aurora.scheduler.thrift.SchedulerThriftInterface.MAINTENANCE_STATUS; -import static org.apache.aurora.scheduler.thrift.SchedulerThriftInterface.MAX_TASK_ID_LENGTH; import static org.apache.aurora.scheduler.thrift.SchedulerThriftInterface.NOOP_JOB_UPDATE_MESSAGE; import static org.apache.aurora.scheduler.thrift.SchedulerThriftInterface.NO_CRON; import static org.apache.aurora.scheduler.thrift.SchedulerThriftInterface.RESTART_SHARDS; @@ -202,7 +200,6 @@ public class SchedulerThriftInterfaceTest extends EasyMockTest { private CronJobManager cronJobManager; private QuotaManager quotaManager; private StateManager stateManager; - private TaskIdGenerator taskIdGenerator; private UUIDGenerator uuidGenerator; private JobUpdateController jobUpdateController; private ReadOnlyScheduler.Iface readOnlyScheduler; @@ -221,7 +218,6 @@ public class SchedulerThriftInterfaceTest extends EasyMockTest { cronJobManager = createMock(CronJobManager.class); quotaManager = createMock(QuotaManager.class); stateManager = createMock(StateManager.class); - taskIdGenerator = createMock(TaskIdGenerator.class); uuidGenerator = createMock(UUIDGenerator.class); jobUpdateController = createMock(JobUpdateController.class); readOnlyScheduler = createMock(ReadOnlyScheduler.Iface.class); @@ -241,7 +237,6 @@ public class SchedulerThriftInterfaceTest extends EasyMockTest { maintenance, quotaManager, stateManager, - taskIdGenerator, uuidGenerator, jobUpdateController, readOnlyScheduler, @@ -289,8 +284,6 @@ public class SchedulerThriftInterfaceTest extends EasyMockTest { 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( @@ -311,8 +304,6 @@ public class SchedulerThriftInterfaceTest extends EasyMockTest { 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( @@ -402,35 +393,12 @@ public class SchedulerThriftInterfaceTest extends EasyMockTest { } @Test - public void testCreateJobFailsTaskIdLength() throws Exception { - IJobConfiguration job = IJobConfiguration.build(makeJob()); - SanitizedConfiguration sanitized = fromUnsanitized(job); - lockManager.assertNotLocked(LOCK_KEY); - storageUtil.expectTaskFetch(Query.jobScoped(JOB_KEY).active()); - expectNoCronJob(); - expect(quotaManager.checkInstanceAddition( - anyObject(ITaskConfig.class), - anyInt(), - eq(storageUtil.mutableStoreProvider))).andReturn(ENOUGH_QUOTA); - - expect(taskIdGenerator.generate(sanitized.getJobConfig().getTaskConfig(), 1)) - .andReturn(Strings.repeat("a", MAX_TASK_ID_LENGTH + 1)); - - control.replay(); - - assertResponse(INVALID_REQUEST, thrift.createJob(job.newBuilder())); - assertEquals(0L, statsProvider.getLongValue(CREATE_JOB)); - } - - @Test public void testCreateJobFailsQuotaCheck() throws Exception { IJobConfiguration job = IJobConfiguration.build(makeProdJob()); 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, NOT_ENOUGH_QUOTA); control.replay(); @@ -553,8 +521,6 @@ public class SchedulerThriftInterfaceTest extends EasyMockTest { 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( @@ -597,8 +563,6 @@ public class SchedulerThriftInterfaceTest extends EasyMockTest { lockManager.assertNotLocked(LOCK_KEY); storageUtil.expectTaskFetch(Query.jobScoped(JOB_KEY).active()); expectNoCronJob(); - expect(taskIdGenerator.generate(ITaskConfig.build(sanitized.getTaskConfig()), 1)) - .andReturn(TASK_ID); expectInstanceQuotaCheck(ITaskConfig.build(sanitized.getTaskConfig()), ENOUGH_QUOTA); stateManager.insertPendingTasks( storageUtil.mutableStoreProvider, @@ -988,8 +952,6 @@ public class SchedulerThriftInterfaceTest extends EasyMockTest { lockManager.assertNotLocked(LOCK_KEY); SanitizedConfiguration sanitized = fromUnsanitized(IJobConfiguration.build(CRON_JOB)); - expect(taskIdGenerator.generate(sanitized.getJobConfig().getTaskConfig(), 1)) - .andReturn(TASK_ID); expectCronQuotaCheck(sanitized.getJobConfig(), ENOUGH_QUOTA); cronJobManager.updateJob(anyObject(SanitizedCronJob.class)); control.replay(); @@ -1013,8 +975,6 @@ public class SchedulerThriftInterfaceTest extends EasyMockTest { lockManager.assertNotLocked(LOCK_KEY); SanitizedConfiguration sanitized = fromUnsanitized(IJobConfiguration.build(CRON_JOB)); - expect(taskIdGenerator.generate(sanitized.getJobConfig().getTaskConfig(), 1)) - .andReturn(TASK_ID); expectCronQuotaCheck(sanitized.getJobConfig(), ENOUGH_QUOTA); cronJobManager.updateJob(anyObject(SanitizedCronJob.class)); expectLastCall().andThrow(new CronException("Nope")); @@ -1045,8 +1005,6 @@ public class SchedulerThriftInterfaceTest extends EasyMockTest { lockManager.assertNotLocked(LOCK_KEY); SanitizedConfiguration sanitized = fromUnsanitized(IJobConfiguration.build(CRON_JOB)); - expect(taskIdGenerator.generate(sanitized.getJobConfig().getTaskConfig(), 1)) - .andReturn(TASK_ID); expectCronQuotaCheck(sanitized.getJobConfig(), ENOUGH_QUOTA); expectNoCronJob().times(2); @@ -1061,8 +1019,6 @@ public class SchedulerThriftInterfaceTest extends EasyMockTest { lockManager.assertNotLocked(LOCK_KEY); SanitizedConfiguration sanitized = fromUnsanitized(IJobConfiguration.build(CRON_JOB)); - expect(taskIdGenerator.generate(sanitized.getJobConfig().getTaskConfig(), 1)) - .andReturn(TASK_ID); expectCronQuotaCheck(sanitized.getJobConfig(), ENOUGH_QUOTA); expectNoCronJob(); @@ -1078,8 +1034,6 @@ public class SchedulerThriftInterfaceTest extends EasyMockTest { lockManager.assertNotLocked(LOCK_KEY); SanitizedConfiguration sanitized = fromUnsanitized(IJobConfiguration.build(CRON_JOB)); - expect(taskIdGenerator.generate(sanitized.getJobConfig().getTaskConfig(), 1)) - .andReturn(TASK_ID); expectCronQuotaCheck(sanitized.getJobConfig(), ENOUGH_QUOTA); expectCronJob(); @@ -1118,8 +1072,6 @@ public class SchedulerThriftInterfaceTest extends EasyMockTest { lockManager.assertNotLocked(LOCK_KEY); SanitizedConfiguration sanitized = fromUnsanitized(IJobConfiguration.build(CRON_JOB)); - expect(taskIdGenerator.generate(sanitized.getJobConfig().getTaskConfig(), 1)) - .andReturn(TASK_ID); expectCronQuotaCheck(sanitized.getJobConfig(), NOT_ENOUGH_QUOTA); control.replay(); @@ -1492,7 +1444,6 @@ public class SchedulerThriftInterfaceTest extends EasyMockTest { IScheduledTask activeTask = buildScheduledTask(); ITaskConfig task = activeTask.getAssignedTask().getTask(); storageUtil.expectTaskFetch(Query.jobScoped(JOB_KEY).active(), activeTask); - expect(taskIdGenerator.generate(task, 3)).andReturn(TASK_ID); expect(quotaManager.checkInstanceAddition( task, 2, @@ -1567,34 +1518,12 @@ public class SchedulerThriftInterfaceTest extends EasyMockTest { } @Test - public void testAddInstancesFailsTaskIdLength() throws Exception { - IScheduledTask activeTask = buildScheduledTask(); - ITaskConfig task = activeTask.getAssignedTask().getTask(); - expectNoCronJob(); - lockManager.assertNotLocked(LOCK_KEY); - storageUtil.expectTaskFetch(Query.jobScoped(JOB_KEY).active(), activeTask); - expect(quotaManager.checkInstanceAddition( - anyObject(ITaskConfig.class), - anyInt(), - eq(storageUtil.mutableStoreProvider))).andReturn(ENOUGH_QUOTA); - expect(taskIdGenerator.generate(task, 2)) - .andReturn(Strings.repeat("a", MAX_TASK_ID_LENGTH + 1)); - - control.replay(); - - assertResponse(INVALID_REQUEST, thrift.addInstances(INSTANCE_KEY, 1)); - assertEquals(0L, statsProvider.getLongValue(ADD_INSTANCES)); - } - - @Test public void testAddInstancesFailsQuotaCheck() throws Exception { IScheduledTask activeTask = buildScheduledTask(); ITaskConfig task = activeTask.getAssignedTask().getTask(); expectNoCronJob(); lockManager.assertNotLocked(LOCK_KEY); storageUtil.expectTaskFetch(Query.jobScoped(JOB_KEY).active(), activeTask); - expect(taskIdGenerator.generate(task, 2)) - .andReturn(TASK_ID); expectInstanceQuotaCheck(task, NOT_ENOUGH_QUOTA); control.replay(); @@ -1610,8 +1539,6 @@ public class SchedulerThriftInterfaceTest extends EasyMockTest { expectNoCronJob(); lockManager.assertNotLocked(LOCK_KEY); storageUtil.expectTaskFetch(Query.jobScoped(JOB_KEY).active(), activeTask); - expect(taskIdGenerator.generate(task, 2)) - .andReturn(TASK_ID); expectInstanceQuotaCheck(task, ENOUGH_QUOTA); stateManager.insertPendingTasks( storageUtil.mutableStoreProvider, @@ -1631,7 +1558,6 @@ public class SchedulerThriftInterfaceTest extends EasyMockTest { expectNoCronJob(); ITaskConfig newTask = buildTaskForJobUpdate(0).getAssignedTask().getTask(); - expect(taskIdGenerator.generate(newTask, 6)).andReturn(TASK_ID); IScheduledTask oldTask1 = buildTaskForJobUpdate(0, "old"); IScheduledTask oldTask2 = buildTaskForJobUpdate(1, "old"); @@ -1685,7 +1611,6 @@ public class SchedulerThriftInterfaceTest extends EasyMockTest { expectNoCronJob(); ITaskConfig newTask = buildTaskForJobUpdate(0).getAssignedTask().getTask(); - expect(taskIdGenerator.generate(newTask, 1)).andReturn(TASK_ID); IScheduledTask oldTask1 = buildTaskForJobUpdate(0); IScheduledTask oldTask2 = buildTaskForJobUpdate(1); @@ -1912,8 +1837,6 @@ public class SchedulerThriftInterfaceTest extends EasyMockTest { Query.unscoped().byJob(JOB_KEY).active(), newTask, oldTask1, oldTask2); expectJobUpdateQuotaCheck(ENOUGH_QUOTA); - expect(taskIdGenerator.generate(EasyMock.<ITaskConfig>anyObject(), anyInt())) - .andReturn(TASK_ID); jobUpdateController.start(EasyMock.<IJobUpdate>anyObject(), eq(AUDIT)); ITaskConfig newConfig = newTask.getAssignedTask().getTask(); @@ -1950,24 +1873,6 @@ public class SchedulerThriftInterfaceTest extends EasyMockTest { } @Test - public void testStartUpdateFailsTaskIdLength() throws Exception { - JobUpdateRequest request = buildServiceJobUpdateRequest(populatedTask()); - expectGetRemoteUser(); - expectNoCronJob(); - expect(uuidGenerator.createNew()).andReturn(UU_ID); - storageUtil.expectTaskFetch(Query.unscoped().byJob(JOB_KEY).active()); - expectJobUpdateQuotaCheck(ENOUGH_QUOTA); - - expect(taskIdGenerator.generate(ITaskConfig.build(request.getTaskConfig()), 6)) - .andReturn(Strings.repeat("a", MAX_TASK_ID_LENGTH + 1)); - - control.replay(); - - assertResponse(INVALID_REQUEST, thrift.startJobUpdate(request, AUDIT_MESSAGE)); - assertEquals(0L, statsProvider.getLongValue(START_JOB_UPDATE)); - } - - @Test public void testStartUpdateFailsQuotaCheck() throws Exception { JobUpdateRequest request = buildServiceJobUpdateRequest(populatedTask()); expectGetRemoteUser(); @@ -1976,8 +1881,6 @@ public class SchedulerThriftInterfaceTest extends EasyMockTest { IScheduledTask oldTask = buildTaskForJobUpdate(0); storageUtil.expectTaskFetch(Query.unscoped().byJob(JOB_KEY).active(), oldTask); - expect(taskIdGenerator.generate(ITaskConfig.build(request.getTaskConfig()), 6)) - .andReturn(TASK_ID); expectJobUpdateQuotaCheck(NOT_ENOUGH_QUOTA); @@ -2001,7 +1904,6 @@ public class SchedulerThriftInterfaceTest extends EasyMockTest { ImmutableMap.of(oldTask.getAssignedTask().getTask(), ImmutableSet.of(new Range(0, 0)))); expect(uuidGenerator.createNew()).andReturn(UU_ID); - expect(taskIdGenerator.generate(newTask, 1)).andReturn(TASK_ID); expectJobUpdateQuotaCheck(ENOUGH_QUOTA); storageUtil.expectTaskFetch(Query.unscoped().byJob(JOB_KEY).active(), oldTask); @@ -2030,7 +1932,6 @@ public class SchedulerThriftInterfaceTest extends EasyMockTest { ImmutableMap.of(oldTask.getAssignedTask().getTask(), ImmutableSet.of(new Range(0, 0)))); expect(uuidGenerator.createNew()).andReturn(UU_ID); - expect(taskIdGenerator.generate(newTask, 1)).andReturn(TASK_ID); expectJobUpdateQuotaCheck(ENOUGH_QUOTA); storageUtil.expectTaskFetch(Query.unscoped().byJob(JOB_KEY).active(), oldTask);
