Repository: aurora Updated Branches: refs/heads/master 7ab2a6c17 -> 28f976ce3
Don't reject update requests scoped to already-updated instances. Bugs closed: AURORA-1332 Reviewed at https://reviews.apache.org/r/36338/ Project: http://git-wip-us.apache.org/repos/asf/aurora/repo Commit: http://git-wip-us.apache.org/repos/asf/aurora/commit/28f976ce Tree: http://git-wip-us.apache.org/repos/asf/aurora/tree/28f976ce Diff: http://git-wip-us.apache.org/repos/asf/aurora/diff/28f976ce Branch: refs/heads/master Commit: 28f976ce3e61d85c56cdf17ee0d6d1b2a9c47086 Parents: 7ab2a6c Author: Bill Farner <[email protected]> Authored: Thu Jul 9 12:15:32 2015 -0700 Committer: Bill Farner <[email protected]> Committed: Thu Jul 9 12:15:32 2015 -0700 ---------------------------------------------------------------------- .../thrift/SchedulerThriftInterface.java | 3 +- .../aurora/scheduler/updater/JobDiff.java | 63 +++++++++++------ .../thrift/SchedulerThriftInterfaceTest.java | 70 ++++++++++++++----- .../aurora/scheduler/updater/JobDiffTest.java | 73 ++++++++++++++++---- 4 files changed, 154 insertions(+), 55 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/aurora/blob/28f976ce/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 12a1732..dc0cd2d 100644 --- a/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java +++ b/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java @@ -1155,7 +1155,8 @@ class SchedulerThriftInterface implements AnnotatedAuroraAdmin { Numbers.rangesToInstanceIds(settings.getUpdateOnlyTheseInstances())); if (!invalidScope.isEmpty()) { return invalidRequest( - "updateOnlyTheseInstances contains instances irrelevant to the update: " + "The update request attempted to update specific instances," + + " but some are irrelevant to the update and current job state: " + invalidScope); } http://git-wip-us.apache.org/repos/asf/aurora/blob/28f976ce/src/main/java/org/apache/aurora/scheduler/updater/JobDiff.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/aurora/scheduler/updater/JobDiff.java b/src/main/java/org/apache/aurora/scheduler/updater/JobDiff.java index bd31963..ab540a5 100644 --- a/src/main/java/org/apache/aurora/scheduler/updater/JobDiff.java +++ b/src/main/java/org/apache/aurora/scheduler/updater/JobDiff.java @@ -45,6 +45,7 @@ import static java.util.Objects.requireNonNull; public final class JobDiff { private final Map<Integer, ITaskConfig> replacedInstances; private final Set<Integer> replacementInstances; + private final Set<Integer> unchangedInstances; /** * Creates a job diff containing the instances to be replaced (original state), and instances @@ -54,9 +55,14 @@ public final class JobDiff { * @param replacedInstances Instances being replaced. * @param replacementInstances Instances to replace {@code replacedInstances}. */ - public JobDiff(Map<Integer, ITaskConfig> replacedInstances, Set<Integer> replacementInstances) { + public JobDiff( + Map<Integer, ITaskConfig> replacedInstances, + Set<Integer> replacementInstances, + Set<Integer> unchangedInstances) { + this.replacedInstances = requireNonNull(replacedInstances); this.replacementInstances = requireNonNull(replacementInstances); + this.unchangedInstances = requireNonNull(unchangedInstances); } public Map<Integer, ITaskConfig> getReplacedInstances() { @@ -67,6 +73,10 @@ public final class JobDiff { return replacementInstances; } + public Set<Integer> getUnchangedInstances() { + return unchangedInstances; + } + /** * Gets instances contained in a {@code scope} that are not a part of the job diff. * @@ -74,9 +84,12 @@ public final class JobDiff { * @return Instance IDs in {@code scope} that are not in this job diff. */ public Set<Integer> getOutOfScopeInstances(Set<Integer> scope) { - Set<Integer> allAlteredInstances = ImmutableSet.copyOf( - Sets.union(getReplacedInstances().keySet(), getReplacementInstances())); - return ImmutableSet.copyOf(Sets.difference(scope, allAlteredInstances)); + Set<Integer> allValidInstances = ImmutableSet.<Integer>builder() + .addAll(getReplacedInstances().keySet()) + .addAll(getReplacementInstances()) + .addAll(getUnchangedInstances()) + .build(); + return ImmutableSet.copyOf(Sets.difference(scope, allValidInstances)); } /** @@ -90,7 +103,7 @@ public final class JobDiff { @Override public int hashCode() { - return Objects.hash(replacedInstances, replacementInstances); + return Objects.hash(replacedInstances, replacementInstances, unchangedInstances); } @Override @@ -101,7 +114,8 @@ public final class JobDiff { JobDiff other = (JobDiff) o; return Objects.equals(getReplacedInstances(), other.getReplacedInstances()) - && Objects.equals(getReplacementInstances(), other.getReplacementInstances()); + && Objects.equals(getReplacementInstances(), other.getReplacementInstances()) + && Objects.equals(getUnchangedInstances(), other.getUnchangedInstances()); } @Override @@ -109,6 +123,7 @@ public final class JobDiff { return com.google.common.base.Objects.toStringHelper(this) .add("replacedInstances", getReplacedInstances()) .add("replacementInstances", getReplacementInstances()) + .add("unchangedInstances", getUnchangedInstances()) .toString(); } @@ -122,21 +137,13 @@ public final class JobDiff { } private static JobDiff computeUnscoped( - TaskStore taskStore, + Map<Integer, ITaskConfig> currentState, IJobKey job, Map<Integer, ITaskConfig> proposedState) { - requireNonNull(taskStore); requireNonNull(job); requireNonNull(proposedState); - Map<Integer, ITaskConfig> currentState = ImmutableMap.copyOf( - Maps.transformValues( - Maps.uniqueIndex( - taskStore.fetchTasks(Query.jobScoped(job).active()), - Tasks.SCHEDULED_TO_INSTANCE_ID), - Tasks.SCHEDULED_TO_INFO)); - MapDifference<Integer, ITaskConfig> diff = Maps.difference(currentState, proposedState); Map<Integer, ITaskConfig> removedInstances = ImmutableMap.<Integer, ITaskConfig>builder() @@ -149,7 +156,10 @@ public final class JobDiff { .addAll(diff.entriesDiffering().keySet()) .build(); - return new JobDiff(removedInstances, addedInstances); + return new JobDiff( + removedInstances, + addedInstances, + ImmutableSet.copyOf(diff.entriesInCommon().keySet())); } /** @@ -167,14 +177,27 @@ public final class JobDiff { Map<Integer, ITaskConfig> proposedState, Set<IRange> scope) { - JobDiff diff = computeUnscoped(taskStore, job, proposedState); + Map<Integer, ITaskConfig> currentState = ImmutableMap.copyOf( + Maps.transformValues( + Maps.uniqueIndex( + taskStore.fetchTasks(Query.jobScoped(job).active()), + Tasks.SCHEDULED_TO_INSTANCE_ID), + Tasks.SCHEDULED_TO_INFO)); + + JobDiff diff = computeUnscoped(currentState, job, proposedState); if (scope.isEmpty()) { return diff; } else { Set<Integer> limit = Numbers.rangesToInstanceIds(scope); - return new JobDiff( - ImmutableMap.copyOf(Maps.filterKeys(diff.getReplacedInstances(), Predicates.in(limit))), - ImmutableSet.copyOf(Sets.intersection(diff.getReplacementInstances(), limit))); + Map<Integer, ITaskConfig> replaced = + ImmutableMap.copyOf(Maps.filterKeys(diff.getReplacedInstances(), Predicates.in(limit))); + Set<Integer> replacements = + ImmutableSet.copyOf(Sets.intersection(diff.getReplacementInstances(), limit)); + Set<Integer> unchanged = ImmutableSet.copyOf( + Sets.difference( + ImmutableSet.copyOf(Sets.difference(currentState.keySet(), replaced.keySet())), + replacements)); + return new JobDiff(replaced, replacements, unchanged); } } http://git-wip-us.apache.org/repos/asf/aurora/blob/28f976ce/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 a5ca21b..d28baba 100644 --- a/src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java +++ b/src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java @@ -111,6 +111,7 @@ import org.apache.aurora.scheduler.updater.JobUpdateController; import org.apache.aurora.scheduler.updater.JobUpdateController.AuditData; import org.apache.aurora.scheduler.updater.UpdateStateException; import org.apache.thrift.TException; +import org.easymock.EasyMock; import org.easymock.IExpectationSetters; import org.junit.Before; import org.junit.Test; @@ -1828,6 +1829,12 @@ public class SchedulerThriftInterfaceTest extends EasyMockTest { response.getResult().getStartJobUpdateResult()); } + private void expectJobUpdateQuotaCheck(QuotaCheckResult result) { + expect(quotaManager.checkJobUpdate( + anyObject(IJobUpdate.class), + eq(storageUtil.mutableStoreProvider))).andReturn(result); + } + @Test public void testStartUpdateEmptyDesired() throws Exception { expectAuth(ROLE, true); @@ -1845,9 +1852,7 @@ public class SchedulerThriftInterfaceTest extends EasyMockTest { newTask, ImmutableMap.of(oldTask1.getAssignedTask().getTask(), ImmutableSet.of(new Range(0, 1)))); - expect(quotaManager.checkJobUpdate( - anyObject(IJobUpdate.class), - eq(storageUtil.mutableStoreProvider))).andReturn(ENOUGH_QUOTA); + expectJobUpdateQuotaCheck(ENOUGH_QUOTA); // Set diff-adjusted IJobUpdate expectations. JobUpdate expected = update.newBuilder(); @@ -2046,6 +2051,45 @@ public class SchedulerThriftInterfaceTest extends EasyMockTest { } @Test + public void testStartScopeIncludesNoop() throws Exception { + // Test for regression of AURORA-1332: a scoped update should be allowed when unchanged + // instances are included in the scope. + + expectAuth(ROLE, true); + expectNoCronJob(); + expect(uuidGenerator.createNew()).andReturn(UU_ID); + + ScheduledTask taskBuilder = buildTaskForJobUpdate(0).newBuilder(); + taskBuilder.getAssignedTask().getTask().setNumCpus(100); + IScheduledTask newTask = IScheduledTask.build(taskBuilder); + + IScheduledTask oldTask1 = buildTaskForJobUpdate(1); + IScheduledTask oldTask2 = buildTaskForJobUpdate(2); + storageUtil.expectTaskFetch( + 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(); + JobUpdate builder = buildJobUpdate( + 3, + newConfig, + ImmutableMap.of( + newConfig, ImmutableSet.of(new Range(0, 0)), + oldTask1.getAssignedTask().getTask(), ImmutableSet.of(new Range(1, 2)))) + .newBuilder(); + builder.getInstructions().getSettings() + .setUpdateOnlyTheseInstances(ImmutableSet.of(new Range(0, 2))); + + control.replay(); + JobUpdateRequest request = buildJobUpdateRequest(IJobUpdate.build(builder)); + assertResponse(OK, thrift.startJobUpdate(request, AUDIT_MESSAGE, SESSION)); + } + + @Test public void testStartUpdateFailsInstanceCountCheck() throws Exception { JobUpdateRequest request = buildServiceJobUpdateRequest(populatedTask()); request.setInstanceCount(4001); @@ -2053,9 +2097,7 @@ public class SchedulerThriftInterfaceTest extends EasyMockTest { expectNoCronJob(); expect(uuidGenerator.createNew()).andReturn(UU_ID); storageUtil.expectTaskFetch(Query.unscoped().byJob(JOB_KEY).active()); - expect(quotaManager.checkJobUpdate( - anyObject(IJobUpdate.class), - eq(storageUtil.mutableStoreProvider))).andReturn(ENOUGH_QUOTA); + expectJobUpdateQuotaCheck(ENOUGH_QUOTA); control.replay(); @@ -2069,9 +2111,7 @@ public class SchedulerThriftInterfaceTest extends EasyMockTest { expectNoCronJob(); expect(uuidGenerator.createNew()).andReturn(UU_ID); storageUtil.expectTaskFetch(Query.unscoped().byJob(JOB_KEY).active()); - expect(quotaManager.checkJobUpdate( - anyObject(IJobUpdate.class), - eq(storageUtil.mutableStoreProvider))).andReturn(ENOUGH_QUOTA); + expectJobUpdateQuotaCheck(ENOUGH_QUOTA); expect(taskIdGenerator.generate(ITaskConfig.build(request.getTaskConfig()), 6)) .andReturn(Strings.repeat("a", MAX_TASK_ID_LENGTH + 1)); @@ -2093,13 +2133,7 @@ public class SchedulerThriftInterfaceTest extends EasyMockTest { expect(taskIdGenerator.generate(ITaskConfig.build(request.getTaskConfig()), 6)) .andReturn(TASK_ID); - ITaskConfig config = ITaskConfig.build(request.getTaskConfig()); - IJobUpdate update = buildJobUpdate(6, config, ImmutableMap.of( - oldTask.getAssignedTask().getTask(), ImmutableSet.of(new Range(0, 0)))); - - expect(quotaManager.checkJobUpdate( - update, - storageUtil.mutableStoreProvider)).andReturn(NOT_ENOUGH_QUOTA); + expectJobUpdateQuotaCheck(NOT_ENOUGH_QUOTA); control.replay(); @@ -2121,9 +2155,7 @@ public class SchedulerThriftInterfaceTest extends EasyMockTest { expect(uuidGenerator.createNew()).andReturn(UU_ID); expect(taskIdGenerator.generate(newTask, 1)).andReturn(TASK_ID); - expect(quotaManager.checkJobUpdate( - update, - storageUtil.mutableStoreProvider)).andReturn(ENOUGH_QUOTA); + expectJobUpdateQuotaCheck(ENOUGH_QUOTA); storageUtil.expectTaskFetch(Query.unscoped().byJob(JOB_KEY).active(), oldTask); jobUpdateController.start(update, AUDIT); http://git-wip-us.apache.org/repos/asf/aurora/blob/28f976ce/src/test/java/org/apache/aurora/scheduler/updater/JobDiffTest.java ---------------------------------------------------------------------- diff --git a/src/test/java/org/apache/aurora/scheduler/updater/JobDiffTest.java b/src/test/java/org/apache/aurora/scheduler/updater/JobDiffTest.java index d0deada..a4c0702 100644 --- a/src/test/java/org/apache/aurora/scheduler/updater/JobDiffTest.java +++ b/src/test/java/org/apache/aurora/scheduler/updater/JobDiffTest.java @@ -45,7 +45,7 @@ public class JobDiffTest extends EasyMockTest { private static final IJobKey JOB = JobKeys.from("role", "env", "job"); private static final JobDiff NO_DIFF = - new JobDiff(ImmutableMap.of(), ImmutableSet.of()); + new JobDiff(ImmutableMap.of(), ImmutableSet.of(), ImmutableSet.of()); private static final Set<IRange> NO_SCOPE = ImmutableSet.of(); private static final Set<IRange> CANARY_SCOPE = ImmutableSet.of(IRange.build(new Range(0, 0))); @@ -64,8 +64,12 @@ public class JobDiffTest extends EasyMockTest { control.replay(); - assertEquals(NO_DIFF, JobDiff.compute(store, JOB, JobDiff.asMap(task, 2), NO_SCOPE)); - assertEquals(NO_DIFF, JobDiff.compute(store, JOB, JobDiff.asMap(task, 2), CANARY_SCOPE)); + assertEquals( + new JobDiff(ImmutableMap.of(), ImmutableSet.of(), ImmutableSet.of(0, 1)), + JobDiff.compute(store, JOB, JobDiff.asMap(task, 2), NO_SCOPE)); + assertEquals( + new JobDiff(ImmutableMap.of(), ImmutableSet.of(), ImmutableSet.of(0, 1)), + JobDiff.compute(store, JOB, JobDiff.asMap(task, 2), CANARY_SCOPE)); } @Test @@ -77,10 +81,10 @@ public class JobDiffTest extends EasyMockTest { control.replay(); assertEquals( - new JobDiff(ImmutableMap.of(), ImmutableSet.of(2, 3, 4)), + new JobDiff(ImmutableMap.of(), ImmutableSet.of(2, 3, 4), ImmutableSet.of(0, 1)), JobDiff.compute(store, JOB, JobDiff.asMap(task, 5), NO_SCOPE)); assertEquals( - NO_DIFF, + new JobDiff(ImmutableMap.of(), ImmutableSet.of(), ImmutableSet.of(0, 1)), JobDiff.compute(store, JOB, JobDiff.asMap(task, 5), CANARY_SCOPE)); } @@ -93,10 +97,10 @@ public class JobDiffTest extends EasyMockTest { control.replay(); assertEquals( - new JobDiff(ImmutableMap.of(1, task, 2, task), ImmutableSet.of()), + new JobDiff(ImmutableMap.of(1, task, 2, task), ImmutableSet.of(), ImmutableSet.of(0)), JobDiff.compute(store, JOB, JobDiff.asMap(task, 1), NO_SCOPE)); assertEquals( - NO_DIFF, + new JobDiff(ImmutableMap.of(), ImmutableSet.of(), ImmutableSet.of(0, 1, 2)), JobDiff.compute(store, JOB, JobDiff.asMap(task, 1), CANARY_SCOPE)); } @@ -110,10 +114,13 @@ public class JobDiffTest extends EasyMockTest { control.replay(); assertEquals( - new JobDiff(ImmutableMap.of(0, oldTask, 1, oldTask, 2, oldTask), ImmutableSet.of(0, 1, 2)), + new JobDiff( + ImmutableMap.of(0, oldTask, 1, oldTask, 2, oldTask), + ImmutableSet.of(0, 1, 2), + ImmutableSet.of()), JobDiff.compute(store, JOB, JobDiff.asMap(newTask, 3), NO_SCOPE)); assertEquals( - new JobDiff(ImmutableMap.of(0, oldTask), ImmutableSet.of(0)), + new JobDiff(ImmutableMap.of(0, oldTask), ImmutableSet.of(0), ImmutableSet.of(1, 2)), JobDiff.compute(store, JOB, JobDiff.asMap(newTask, 3), CANARY_SCOPE)); } @@ -132,21 +139,57 @@ public class JobDiffTest extends EasyMockTest { assertEquals( new JobDiff( ImmutableMap.of(0, oldTask, 3, oldTask2, 4, oldTask), - ImmutableSet.of(0, 2, 3, 4)), + ImmutableSet.of(0, 2, 3, 4), + ImmutableSet.of(1)), JobDiff.compute(store, JOB, JobDiff.asMap(newTask, 5), NO_SCOPE)); assertEquals( - new JobDiff(ImmutableMap.of(0, oldTask), ImmutableSet.of(0)), + new JobDiff(ImmutableMap.of(0, oldTask), ImmutableSet.of(0), ImmutableSet.of(1, 3, 4)), JobDiff.compute(store, JOB, JobDiff.asMap(newTask, 5), CANARY_SCOPE)); } @Test + public void testUnchangedInstances() { + ITaskConfig oldTask = makeTask("job", "echo"); + ITaskConfig newTask = makeTask("job", "echo2"); + + expectFetch(instance(oldTask, 0), instance(newTask, 1), instance(oldTask, 2)).times(3); + + control.replay(); + + assertEquals( + new JobDiff( + ImmutableMap.of(0, oldTask, 2, oldTask), + ImmutableSet.of(0, 2), + ImmutableSet.of(1)), + JobDiff.compute(store, JOB, JobDiff.asMap(newTask, 3), NO_SCOPE)); + assertEquals( + new JobDiff(ImmutableMap.of(0, oldTask), ImmutableSet.of(0), ImmutableSet.of(1, 2)), + JobDiff.compute(store, JOB, JobDiff.asMap(newTask, 3), CANARY_SCOPE)); + assertEquals( + new JobDiff(ImmutableMap.of(0, oldTask), ImmutableSet.of(0), ImmutableSet.of(1, 2)), + JobDiff.compute(store, JOB, JobDiff.asMap(newTask, 4), CANARY_SCOPE)); + } + + @Test public void testObjectOverrides() { control.replay(); - JobDiff a = new JobDiff(ImmutableMap.of(0, makeTask("job", "echo")), ImmutableSet.of(0)); - JobDiff b = new JobDiff(ImmutableMap.of(0, makeTask("job", "echo")), ImmutableSet.of(0)); - JobDiff c = new JobDiff(ImmutableMap.of(0, makeTask("job", "echo1")), ImmutableSet.of(0)); - JobDiff d = new JobDiff(ImmutableMap.of(0, makeTask("job", "echo")), ImmutableSet.of(1)); + JobDiff a = new JobDiff( + ImmutableMap.of(0, makeTask("job", "echo")), + ImmutableSet.of(0), + ImmutableSet.of()); + JobDiff b = new JobDiff( + ImmutableMap.of(0, makeTask("job", "echo")), + ImmutableSet.of(0), + ImmutableSet.of()); + JobDiff c = new JobDiff( + ImmutableMap.of(0, makeTask("job", "echo1")), + ImmutableSet.of(0), + ImmutableSet.of()); + JobDiff d = new JobDiff( + ImmutableMap.of(0, makeTask("job", "echo")), + ImmutableSet.of(1), + ImmutableSet.of()); assertEquals(a, b); assertEquals(ImmutableSet.of(a), ImmutableSet.of(a, b)); assertNotEquals(a, c);
