Repository: aurora Updated Branches: refs/heads/master 318b40d71 -> 7a536a0c3
Fix DB constraint violation when updating host attributes. Bugs closed: AURORA-1379 Reviewed at https://reviews.apache.org/r/36096/ Project: http://git-wip-us.apache.org/repos/asf/aurora/repo Commit: http://git-wip-us.apache.org/repos/asf/aurora/commit/7a536a0c Tree: http://git-wip-us.apache.org/repos/asf/aurora/tree/7a536a0c Diff: http://git-wip-us.apache.org/repos/asf/aurora/diff/7a536a0c Branch: refs/heads/master Commit: 7a536a0c3cf13f7d19c1c5f8f5716fe6c15339fb Parents: 318b40d Author: Bill Farner <[email protected]> Authored: Mon Jul 6 14:21:18 2015 -0700 Committer: Bill Farner <[email protected]> Committed: Mon Jul 6 14:21:18 2015 -0700 ---------------------------------------------------------------------- .../scheduler/storage/AttributeStore.java | 7 +++++ .../scheduler/storage/db/AttributeMapper.java | 20 ++++++++++-- .../scheduler/storage/db/DbAttributeStore.java | 33 +++++++++++--------- .../scheduler/storage/db/AttributeMapper.xml | 15 ++++++--- .../storage/db/DbAttributeStoreTest.java | 32 +++++++++++++++++++ 5 files changed, 85 insertions(+), 22 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/aurora/blob/7a536a0c/src/main/java/org/apache/aurora/scheduler/storage/AttributeStore.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/aurora/scheduler/storage/AttributeStore.java b/src/main/java/org/apache/aurora/scheduler/storage/AttributeStore.java index eef758b..36bdc81 100644 --- a/src/main/java/org/apache/aurora/scheduler/storage/AttributeStore.java +++ b/src/main/java/org/apache/aurora/scheduler/storage/AttributeStore.java @@ -32,6 +32,13 @@ import org.apache.mesos.Protos; public interface AttributeStore { /** * Fetches all host attributes given by the host. + * <p> + * TODO(wfarner): We need to transition this API to key off slave ID instead of host name, as + * the host is not guaranteed to be unique (it's possible, and sometimes desirable to have + * multiple slaves on a machine). The current API ripples down into storage, since the store + * is not in the position to dictate behavior when host names collide. Most callers seem to have + * sufficient context to use slave ID instead, with the exception of those related to host + * maintenance. * * @param host host name. * @return attributes associated with {@code host}, if the host is known. http://git-wip-us.apache.org/repos/asf/aurora/blob/7a536a0c/src/main/java/org/apache/aurora/scheduler/storage/db/AttributeMapper.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/aurora/scheduler/storage/db/AttributeMapper.java b/src/main/java/org/apache/aurora/scheduler/storage/db/AttributeMapper.java index 3763f4d..a454887 100644 --- a/src/main/java/org/apache/aurora/scheduler/storage/db/AttributeMapper.java +++ b/src/main/java/org/apache/aurora/scheduler/storage/db/AttributeMapper.java @@ -18,7 +18,9 @@ import java.util.List; import javax.annotation.Nullable; import org.apache.aurora.gen.HostAttributes; +import org.apache.aurora.gen.MaintenanceMode; import org.apache.aurora.scheduler.storage.entities.IHostAttributes; +import org.apache.ibatis.annotations.Param; /** * MyBatis mapper interface for Attribute.xml. @@ -34,9 +36,21 @@ interface AttributeMapper { /** * Deletes all attributes and attribute values associated with a slave. * - * @param slaveId Slave ID to delete associated values from. + * @param host Host to delete associated values from. */ - void deleteAttributesAndValues(String slaveId); + void deleteAttributeValues(@Param("host") String host); + + /** + * Updates the mode and slave ID associated with a host. + * + * @param host Host to update. + * @param mode New host maintenance mode. + * @param slaveId New host slave ID. + */ + void updateHostModeAndSlaveId( + @Param("host") String host, + @Param("mode") MaintenanceMode mode, + @Param("slaveId") String slaveId); /** * Inserts values in {@link IHostAttributes#getAttributes()}, associating them with @@ -53,7 +67,7 @@ interface AttributeMapper { * @return Attributes associated with {@code host}, or {@code null} if no association exists. */ @Nullable - HostAttributes select(String host); + HostAttributes select(@Param("host") String host); /** * Retrieves all stored host attributes. http://git-wip-us.apache.org/repos/asf/aurora/blob/7a536a0c/src/main/java/org/apache/aurora/scheduler/storage/db/DbAttributeStore.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/aurora/scheduler/storage/db/DbAttributeStore.java b/src/main/java/org/apache/aurora/scheduler/storage/db/DbAttributeStore.java index 5f1cd2b..1b274c8 100644 --- a/src/main/java/org/apache/aurora/scheduler/storage/db/DbAttributeStore.java +++ b/src/main/java/org/apache/aurora/scheduler/storage/db/DbAttributeStore.java @@ -53,13 +53,29 @@ class DbAttributeStore implements AttributeStore.Mutable { checkArgument(hostAttributes.isSetAttributes()); checkArgument(hostAttributes.isSetMode()); + if (Iterables.any(hostAttributes.getAttributes(), EMPTY_VALUES)) { + throw new IllegalArgumentException( + "Host attributes contains empty values: " + hostAttributes); + } + Optional<IHostAttributes> existing = getHostAttributes(hostAttributes.getHost()); if (existing.equals(Optional.of(hostAttributes))) { return false; + } else if (existing.isPresent()) { + mapper.updateHostModeAndSlaveId( + hostAttributes.getHost(), + hostAttributes.getMode(), + hostAttributes.getSlaveId()); } else { - merge(hostAttributes); - return true; + mapper.insert(hostAttributes); } + + mapper.deleteAttributeValues(hostAttributes.getHost()); + if (!hostAttributes.getAttributes().isEmpty()) { + mapper.insertAttributeValues(hostAttributes); + } + + return true; } private static final Predicate<IAttribute> EMPTY_VALUES = new Predicate<IAttribute>() { @@ -69,19 +85,6 @@ class DbAttributeStore implements AttributeStore.Mutable { } }; - private void merge(IHostAttributes hostAttributes) { - if (Iterables.any(hostAttributes.getAttributes(), EMPTY_VALUES)) { - throw new IllegalArgumentException( - "Host attributes contains empty values: " + hostAttributes); - } - - mapper.deleteAttributesAndValues(hostAttributes.getHost()); - mapper.insert(hostAttributes); - if (!hostAttributes.getAttributes().isEmpty()) { - mapper.insertAttributeValues(hostAttributes); - } - } - @Timed("attribute_store_fetch_one") @Override public Optional<IHostAttributes> getHostAttributes(String host) { http://git-wip-us.apache.org/repos/asf/aurora/blob/7a536a0c/src/main/resources/org/apache/aurora/scheduler/storage/db/AttributeMapper.xml ---------------------------------------------------------------------- diff --git a/src/main/resources/org/apache/aurora/scheduler/storage/db/AttributeMapper.xml b/src/main/resources/org/apache/aurora/scheduler/storage/db/AttributeMapper.xml index d36b42a..41519de 100644 --- a/src/main/resources/org/apache/aurora/scheduler/storage/db/AttributeMapper.xml +++ b/src/main/resources/org/apache/aurora/scheduler/storage/db/AttributeMapper.xml @@ -31,12 +31,19 @@ </foreach> </insert> - <delete id="deleteAttributesAndValues"> + <delete id="deleteAttributeValues"> <!-- This assumes the schema enables cascading deletes in the values table. --> - DELETE FROM host_attributes - WHERE host = #{id} + DELETE FROM host_attribute_values + WHERE host_attribute_id IN (SELECT id FROM host_attributes WHERE host = #{host}) </delete> + <update id="updateHostModeAndSlaveId"> + UPDATE host_attributes SET + mode = #{mode, typeHandler=org.apache.aurora.scheduler.storage.db.typehandlers.MaintenanceModeTypeHandler}, + slave_id = #{slaveId} + WHERE host = #{host} + </update> + <resultMap id="hostAttributeResultMap" type="org.apache.aurora.gen.HostAttributes"> <id column="a_id" /> <result property="mode" @@ -69,7 +76,7 @@ <select id="select" resultMap="hostAttributeResultMap"> <include refid="unscoped_select"/> - WHERE host = #{id} + WHERE host = #{host} </select> <select id="selectAll" resultMap="hostAttributeResultMap"> http://git-wip-us.apache.org/repos/asf/aurora/blob/7a536a0c/src/test/java/org/apache/aurora/scheduler/storage/db/DbAttributeStoreTest.java ---------------------------------------------------------------------- diff --git a/src/test/java/org/apache/aurora/scheduler/storage/db/DbAttributeStoreTest.java b/src/test/java/org/apache/aurora/scheduler/storage/db/DbAttributeStoreTest.java index e19d15d..a7de023 100644 --- a/src/test/java/org/apache/aurora/scheduler/storage/db/DbAttributeStoreTest.java +++ b/src/test/java/org/apache/aurora/scheduler/storage/db/DbAttributeStoreTest.java @@ -22,12 +22,16 @@ import com.google.common.collect.ImmutableSet; import org.apache.aurora.gen.Attribute; import org.apache.aurora.gen.HostAttributes; import org.apache.aurora.gen.MaintenanceMode; +import org.apache.aurora.gen.ScheduledTask; +import org.apache.aurora.scheduler.base.JobKeys; +import org.apache.aurora.scheduler.base.TaskTestUtil; import org.apache.aurora.scheduler.storage.Storage; import org.apache.aurora.scheduler.storage.Storage.MutableStoreProvider; import org.apache.aurora.scheduler.storage.Storage.MutateWork; import org.apache.aurora.scheduler.storage.Storage.StoreProvider; import org.apache.aurora.scheduler.storage.Storage.Work; import org.apache.aurora.scheduler.storage.entities.IHostAttributes; +import org.apache.aurora.scheduler.storage.entities.IScheduledTask; import org.junit.Before; import org.junit.Test; @@ -130,6 +134,34 @@ public class DbAttributeStoreTest { assertEquals(Optional.of(updated), read(HOST_A)); } + @Test + public void testUpdateAttributesWithRelations() { + // Test for regression of AURORA-1379, where host attribute mutation performed a delete, + // violating foreign key constraints. + insert(HOST_A_ATTRS); + + ScheduledTask builder = TaskTestUtil.makeTask("a", JobKeys.from("role", "env", "job")) + .newBuilder(); + builder.getAssignedTask() + .setSlaveHost(HOST_A_ATTRS.getHost()) + .setSlaveId(HOST_A_ATTRS.getSlaveId()); + final IScheduledTask taskA = IScheduledTask.build(builder); + + storage.write(new MutateWork.NoResult.Quiet() { + @Override + protected void execute(MutableStoreProvider storeProvider) { + storeProvider.getUnsafeTaskStore().saveTasks(ImmutableSet.of(taskA)); + } + }); + + HostAttributes attributeBuilder = HOST_A_ATTRS.newBuilder() + .setMode(MaintenanceMode.DRAINED); + attributeBuilder.addToAttributes(new Attribute("newAttr", ImmutableSet.of("a", "b"))); + IHostAttributes hostAUpdated = IHostAttributes.build(attributeBuilder); + insert(hostAUpdated); + assertEquals(Optional.of(hostAUpdated), read(HOST_A)); + } + private void insert(final IHostAttributes attributes) { storage.write(new MutateWork.NoResult.Quiet() { @Override
