From d19cf96d390ffcdba8b1f64a6dd3f3749ecc5872 Mon Sep 17 00:00:00 2001 From: Robert Varga Date: Tue, 21 Jan 2020 14:31:48 +0100 Subject: [PATCH] Force pruning during data migration Our current PruningDataTreeModification performs pruning only in two cases: 1) the root is being written (i.e. Snapshot recovery) 2) DataTree reports a validation issue This is not sufficient for uint-type migration, as we have to subject all writes and merges to pruning/translation irrespective of where they occur. Split up PruningDataTreeModification into two implementations, Proactive and Reactive and use them as appropriate based on migration requirements. JIRA: CONTROLLER-1923 Change-Id: I4184c56380b7f52a8d7af6f17346f9c5edd31b28 Signed-off-by: Robert Varga --- .../cluster/datastore/ShardDataTree.java | 34 +++-- .../utils/PruningDataTreeModification.java | 137 ++++++++++++------ .../cluster/datastore/ShardDataTreeTest.java | 90 ++++++++++-- .../PruningDataTreeModificationTest.java | 2 +- 4 files changed, 189 insertions(+), 74 deletions(-) diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardDataTree.java b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardDataTree.java index a097b68081..5a59b0d163 100644 --- a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardDataTree.java +++ b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/ShardDataTree.java @@ -311,32 +311,27 @@ public class ShardDataTree extends ShardDataTreeTransactionParent { */ void applyRecoverySnapshot(final @NonNull ShardSnapshotState snapshot) throws DataValidationFailedException { // TODO: we should be able to reuse the pruner, provided we are not reentrant - ReusableNormalizedNodePruner pruner = ReusableNormalizedNodePruner.forDataSchemaContext(dataSchemaContext); + final ReusableNormalizedNodePruner pruner = ReusableNormalizedNodePruner.forDataSchemaContext( + dataSchemaContext); if (snapshot.needsMigration()) { - pruner = pruner.withUintAdaption(); + final ReusableNormalizedNodePruner uintPruner = pruner.withUintAdaption(); + applySnapshot(snapshot.getSnapshot(), + delegate -> new PruningDataTreeModification.Proactive(delegate, dataTree, uintPruner)); + } else { + applySnapshot(snapshot.getSnapshot(), + delegate -> new PruningDataTreeModification.Reactive(delegate, dataTree, pruner)); } - - // For lambda below - final ReusableNormalizedNodePruner finalPruner = pruner; - applySnapshot(snapshot.getSnapshot(), - delegate -> new PruningDataTreeModification(delegate, dataTree, finalPruner)); } @SuppressWarnings("checkstyle:IllegalCatch") private void applyRecoveryCandidate(final CommitTransactionPayload payload) throws IOException { final Entry entry = payload.getCandidate(); final DataTreeModification unwrapped = dataTree.takeSnapshot().newModification(); + final PruningDataTreeModification mod = createPruningModification(unwrapped, + NormalizedNodeStreamVersion.MAGNESIUM.compareTo(entry.getValue().getVersion()) > 0); - // TODO: we should be able to reuse the pruner, provided we are not reentrant - ReusableNormalizedNodePruner pruner = ReusableNormalizedNodePruner.forDataSchemaContext(dataSchemaContext); - if (NormalizedNodeStreamVersion.MAGNESIUM.compareTo(entry.getValue().getVersion()) > 0) { - pruner = pruner.withUintAdaption(); - } - - final PruningDataTreeModification mod = new PruningDataTreeModification(unwrapped, dataTree, pruner); DataTreeCandidates.applyToModification(mod, entry.getValue().getCandidate()); mod.ready(); - LOG.trace("{}: Applying recovery modification {}", logContext, unwrapped); try { @@ -354,6 +349,15 @@ public class ShardDataTree extends ShardDataTreeTransactionParent { allMetadataCommittedTransaction(entry.getKey()); } + private PruningDataTreeModification createPruningModification(final DataTreeModification unwrapped, + final boolean uintAdapting) { + // TODO: we should be able to reuse the pruner, provided we are not reentrant + final ReusableNormalizedNodePruner pruner = ReusableNormalizedNodePruner.forDataSchemaContext( + dataSchemaContext); + return uintAdapting ? new PruningDataTreeModification.Proactive(unwrapped, dataTree, pruner.withUintAdaption()) + : new PruningDataTreeModification.Reactive(unwrapped, dataTree, pruner); + } + /** * Apply a payload coming from recovery. This method does not assume the SchemaContexts match and performs data * pruning in an attempt to adjust the state to our current SchemaContext. diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/utils/PruningDataTreeModification.java b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/utils/PruningDataTreeModification.java index 8b971327b7..8ce7119651 100644 --- a/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/utils/PruningDataTreeModification.java +++ b/opendaylight/md-sal/sal-distributed-datastore/src/main/java/org/opendaylight/controller/cluster/datastore/utils/PruningDataTreeModification.java @@ -31,7 +31,82 @@ import org.slf4j.LoggerFactory; * The PruningDataTreeModification first removes all entries from the data which do not belong in the schemaContext * before delegating it to the actual DataTreeModification. */ -public class PruningDataTreeModification extends ForwardingObject implements DataTreeModification { +public abstract class PruningDataTreeModification extends ForwardingObject implements DataTreeModification { + /** + * A PruningDataTreeModification which always performs pruning before attempting an operation. This sacrifices + * performance to ensure all data has passed through the pruner -- such that data adaptations are performed. + */ + public static final class Proactive extends PruningDataTreeModification { + public Proactive(final DataTreeModification delegate, final DataTree dataTree, + final ReusableNormalizedNodePruner pruner) { + super(delegate, dataTree, pruner); + } + + @Override + public void merge(final YangInstanceIdentifier path, final NormalizedNode data) { + pruneAndMergeNode(path, data); + } + + @Override + public void write(final YangInstanceIdentifier path, final NormalizedNode data) { + pruneAndWriteNode(path, data); + } + + @Override + PruningDataTreeModification createNew(final DataTreeModification delegate, final DataTree dataTree, + final ReusableNormalizedNodePruner pruner) { + return new Proactive(delegate, dataTree, pruner); + } + } + + /** + * A PruningDataTreeModification which performs pruning only when an operation results in an + * {@link SchemaValidationFailedException}. This offers superior performance in the normal case of not needing + * pruning. + */ + public static final class Reactive extends PruningDataTreeModification { + public Reactive(final DataTreeModification delegate, final DataTree dataTree, + final ReusableNormalizedNodePruner pruner) { + super(delegate, dataTree, pruner); + } + + @Override + public void merge(final YangInstanceIdentifier path, final NormalizedNode data) { + if (path.isEmpty()) { + pruneAndMergeNode(path, data); + return; + } + + try { + delegate().merge(path, data); + } catch (SchemaValidationFailedException e) { + LOG.warn("Node at path {} was pruned during merge due to validation error: {}", path, e.getMessage()); + pruneAndMergeNode(path, data); + } + } + + @Override + public void write(final YangInstanceIdentifier path, final NormalizedNode data) { + if (path.isEmpty()) { + pruneAndWriteNode(path, data); + return; + } + + try { + delegate().write(path, data); + } catch (SchemaValidationFailedException e) { + LOG.warn("Node at path : {} was pruned during write due to validation error: {}", path, e.getMessage()); + pruneAndWriteNode(path, data); + } + } + + @Override + PruningDataTreeModification createNew(final DataTreeModification delegate, final DataTree dataTree, + final ReusableNormalizedNodePruner pruner) { + return new Reactive(delegate, dataTree, pruner); + } + } + private static final Logger LOG = LoggerFactory.getLogger(PruningDataTreeModification.class); private final ReusableNormalizedNodePruner pruner; @@ -39,7 +114,7 @@ public class PruningDataTreeModification extends ForwardingObject implements Dat private DataTreeModification delegate; - public PruningDataTreeModification(final DataTreeModification delegate, final DataTree dataTree, + PruningDataTreeModification(final DataTreeModification delegate, final DataTree dataTree, final ReusableNormalizedNodePruner pruner) { this.delegate = requireNonNull(delegate); this.dataTree = requireNonNull(dataTree); @@ -47,17 +122,17 @@ public class PruningDataTreeModification extends ForwardingObject implements Dat } @Override - protected DataTreeModification delegate() { + protected final DataTreeModification delegate() { return delegate; } @Override - public SchemaContext getSchemaContext() { + public final SchemaContext getSchemaContext() { return delegate.getSchemaContext(); } @Override - public void delete(final YangInstanceIdentifier path) { + public final void delete(final YangInstanceIdentifier path) { try { delegate.delete(path); } catch (SchemaValidationFailedException e) { @@ -65,44 +140,14 @@ public class PruningDataTreeModification extends ForwardingObject implements Dat } } - @Override - public void merge(final YangInstanceIdentifier path, final NormalizedNode data) { - if (path.isEmpty()) { - pruneAndMergeNode(path, data); - return; - } - - try { - delegate.merge(path, data); - } catch (SchemaValidationFailedException e) { - LOG.warn("Node at path {} was pruned during merge due to validation error: {}", path, e.getMessage()); - pruneAndMergeNode(path, data); - } - } - - private void pruneAndMergeNode(final YangInstanceIdentifier path, final NormalizedNode data) { + final void pruneAndMergeNode(final YangInstanceIdentifier path, final NormalizedNode data) { final NormalizedNode pruned = pruneNormalizedNode(path, data); if (pruned != null) { delegate.merge(path, pruned); } } - @Override - public void write(final YangInstanceIdentifier path, final NormalizedNode data) { - if (path.isEmpty()) { - pruneAndWriteNode(path, data); - return; - } - - try { - delegate.write(path, data); - } catch (SchemaValidationFailedException e) { - LOG.warn("Node at path : {} was pruned during write due to validation error: {}", path, e.getMessage()); - pruneAndWriteNode(path, data); - } - } - - private void pruneAndWriteNode(final YangInstanceIdentifier path, final NormalizedNode data) { + final void pruneAndWriteNode(final YangInstanceIdentifier path, final NormalizedNode data) { final NormalizedNode pruned = pruneNormalizedNode(path, data); if (pruned != null) { delegate.write(path, pruned); @@ -110,7 +155,7 @@ public class PruningDataTreeModification extends ForwardingObject implements Dat } @Override - public void ready() { + public final void ready() { try { delegate.ready(); } catch (SchemaValidationFailedException e) { @@ -123,22 +168,23 @@ public class PruningDataTreeModification extends ForwardingObject implements Dat } @Override - public void applyToCursor(final DataTreeModificationCursor dataTreeModificationCursor) { + public final void applyToCursor(final DataTreeModificationCursor dataTreeModificationCursor) { delegate.applyToCursor(dataTreeModificationCursor); } @Override - public Optional> readNode(final YangInstanceIdentifier yangInstanceIdentifier) { + public final Optional> readNode(final YangInstanceIdentifier yangInstanceIdentifier) { return delegate.readNode(yangInstanceIdentifier); } @Override - public DataTreeModification newModification() { - return new PruningDataTreeModification(delegate.newModification(), dataTree, pruner.duplicate()); + public final DataTreeModification newModification() { + return createNew(delegate.newModification(), dataTree, pruner.duplicate()); } @VisibleForTesting - NormalizedNode pruneNormalizedNode(final YangInstanceIdentifier path, final NormalizedNode input) { + final NormalizedNode pruneNormalizedNode(final YangInstanceIdentifier path, + final NormalizedNode input) { pruner.initializeForPath(path); try { NormalizedNodeWriter.forStreamWriter(pruner).write(input); @@ -150,7 +196,10 @@ public class PruningDataTreeModification extends ForwardingObject implements Dat return pruner.getResult().orElse(null); } - private static class PruningDataTreeModificationCursor extends AbstractDataTreeModificationCursor { + abstract PruningDataTreeModification createNew(DataTreeModification delegate, DataTree dataTree, + ReusableNormalizedNodePruner pruner); + + private static final class PruningDataTreeModificationCursor extends AbstractDataTreeModificationCursor { private final DataTreeModification toModification; private final PruningDataTreeModification pruningModification; diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/ShardDataTreeTest.java b/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/ShardDataTreeTest.java index 43dd465037..abaefcb23b 100644 --- a/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/ShardDataTreeTest.java +++ b/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/ShardDataTreeTest.java @@ -65,8 +65,10 @@ import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier.NodeIdent import org.opendaylight.yangtools.yang.data.api.schema.ContainerNode; import org.opendaylight.yangtools.yang.data.api.schema.MapEntryNode; import org.opendaylight.yangtools.yang.data.api.schema.NormalizedNode; +import org.opendaylight.yangtools.yang.data.api.schema.tree.DataTree; import org.opendaylight.yangtools.yang.data.api.schema.tree.DataTreeCandidate; import org.opendaylight.yangtools.yang.data.api.schema.tree.DataTreeCandidates; +import org.opendaylight.yangtools.yang.data.api.schema.tree.DataTreeConfiguration; import org.opendaylight.yangtools.yang.data.api.schema.tree.DataTreeModification; import org.opendaylight.yangtools.yang.data.api.schema.tree.DataTreeSnapshot; import org.opendaylight.yangtools.yang.data.api.schema.tree.DataValidationFailedException; @@ -74,6 +76,7 @@ import org.opendaylight.yangtools.yang.data.api.schema.tree.ModificationType; import org.opendaylight.yangtools.yang.data.api.schema.tree.TreeType; import org.opendaylight.yangtools.yang.data.impl.schema.Builders; import org.opendaylight.yangtools.yang.data.impl.schema.ImmutableNodes; +import org.opendaylight.yangtools.yang.data.impl.schema.tree.InMemoryDataTreeFactory; import org.opendaylight.yangtools.yang.model.api.SchemaContext; public class ShardDataTreeTest extends AbstractTest { @@ -504,19 +507,74 @@ public class ShardDataTreeTest extends AbstractTest { assertCarsUint64(); } + @Test + public void testUintReplay() throws DataValidationFailedException, IOException { + // Commit two writes and one merge, saving the data tree candidate for each. + // write(foo=1) + // write(foo=2) + // merge(foo=3) + final DataTree dataTree = new InMemoryDataTreeFactory().create(DataTreeConfiguration.DEFAULT_OPERATIONAL, + fullSchema); + DataTreeModification mod = dataTree.takeSnapshot().newModification(); + mod.write(CarsModel.BASE_PATH, Builders.containerBuilder() + .withNodeIdentifier(new NodeIdentifier(CarsModel.BASE_QNAME)) + .withChild(Builders.mapBuilder() + .withNodeIdentifier(new NodeIdentifier(CarsModel.CAR_QNAME)) + .withChild(createCar("one", BigInteger.ONE)) + .build()) + .build()); + mod.ready(); + dataTree.validate(mod); + final DataTreeCandidate first = dataTree.prepare(mod); + dataTree.commit(first); + + mod = dataTree.takeSnapshot().newModification(); + mod.write(CarsModel.newCarPath("two"), createCar("two", BigInteger.TWO)); + mod.ready(); + dataTree.validate(mod); + final DataTreeCandidate second = dataTree.prepare(mod); + dataTree.commit(second); + + mod = dataTree.takeSnapshot().newModification(); + mod.merge(CarsModel.CAR_LIST_PATH, Builders.mapBuilder() + .withNodeIdentifier(new NodeIdentifier(CarsModel.CAR_QNAME)) + .withChild(createCar("three", BigInteger.TEN)) + .build()); + mod.ready(); + dataTree.validate(mod); + final DataTreeCandidate third = dataTree.prepare(mod); + dataTree.commit(third); + + // Apply first candidate as a snapshot + shardDataTree.applyRecoverySnapshot( + new ShardSnapshotState(new MetadataShardDataTreeSnapshot(first.getRootNode().getDataAfter().get()), true)); + // Apply the other two snapshots as transactions + shardDataTree.applyRecoveryPayload(CommitTransactionPayload.create(nextTransactionId(), second, + PayloadVersion.SODIUM_SR1)); + shardDataTree.applyRecoveryPayload(CommitTransactionPayload.create(nextTransactionId(), third, + PayloadVersion.SODIUM_SR1)); + + // Verify uint translation + final DataTreeSnapshot snapshot = shardDataTree.newReadOnlyTransaction(nextTransactionId()).getSnapshot(); + final NormalizedNode cars = snapshot.readNode(CarsModel.CAR_LIST_PATH).get(); + + assertEquals(Builders.mapBuilder() + .withNodeIdentifier(new NodeIdentifier(CarsModel.CAR_QNAME)) + // Note: Uint64 + .withChild(createCar("one", Uint64.ONE)) + .withChild(createCar("two", Uint64.TWO)) + .withChild(createCar("three", Uint64.TEN)) + .build(), cars); + } + private void assertCarsUint64() { final DataTreeSnapshot snapshot = shardDataTree.newReadOnlyTransaction(nextTransactionId()).getSnapshot(); final NormalizedNode cars = snapshot.readNode(CarsModel.CAR_LIST_PATH).get(); assertEquals(Builders.mapBuilder() .withNodeIdentifier(new NodeIdentifier(CarsModel.CAR_QNAME)) - .withChild(Builders.mapEntryBuilder() - .withNodeIdentifier(NodeIdentifierWithPredicates.of(CarsModel.CAR_QNAME, - CarsModel.CAR_NAME_QNAME, "foo")) - .withChild(ImmutableNodes.leafNode(CarsModel.CAR_NAME_QNAME, "foo")) - // Note: Uint64 - .withChild(ImmutableNodes.leafNode(CarsModel.CAR_PRICE_QNAME, Uint64.ONE)) - .build()) + // Note: Uint64 + .withChild(createCar("foo", Uint64.ONE)) .build(), cars); } @@ -527,18 +585,22 @@ public class ShardDataTreeTest extends AbstractTest { .withNodeIdentifier(new NodeIdentifier(CarsModel.CARS_QNAME)) .withChild(Builders.mapBuilder() .withNodeIdentifier(new NodeIdentifier(CarsModel.CAR_QNAME)) - .withChild(Builders.mapEntryBuilder() - .withNodeIdentifier(NodeIdentifierWithPredicates.of(CarsModel.CAR_QNAME, - CarsModel.CAR_NAME_QNAME, "foo")) - .withChild(ImmutableNodes.leafNode(CarsModel.CAR_NAME_QNAME, "foo")) - // Note: old BigInteger - .withChild(ImmutableNodes.leafNode(CarsModel.CAR_PRICE_QNAME, BigInteger.ONE)) - .build()) + // Note: BigInteger + .withChild(createCar("foo", BigInteger.ONE)) .build()) .build()) .build(); } + private static MapEntryNode createCar(final String name, final Object value) { + return Builders.mapEntryBuilder() + .withNodeIdentifier(NodeIdentifierWithPredicates.of(CarsModel.CAR_QNAME,CarsModel.CAR_NAME_QNAME, name)) + .withChild(ImmutableNodes.leafNode(CarsModel.CAR_NAME_QNAME, name)) + // Note: old BigInteger + .withChild(ImmutableNodes.leafNode(CarsModel.CAR_PRICE_QNAME, value)) + .build(); + } + private ShardDataTreeCohort newShardDataTreeCohort(final DataTreeOperation operation) { final ReadWriteShardDataTreeTransaction transaction = shardDataTree.newReadWriteTransaction(nextTransactionId()); diff --git a/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/utils/PruningDataTreeModificationTest.java b/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/utils/PruningDataTreeModificationTest.java index 9118d8f66f..6ec01cfbbf 100644 --- a/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/utils/PruningDataTreeModificationTest.java +++ b/opendaylight/md-sal/sal-distributed-datastore/src/test/java/org/opendaylight/controller/cluster/datastore/utils/PruningDataTreeModificationTest.java @@ -98,7 +98,7 @@ public class PruningDataTreeModificationTest { } }); - pruningDataTreeModification = new PruningDataTreeModification(proxyModification, dataTree, + pruningDataTreeModification = new PruningDataTreeModification.Reactive(proxyModification, dataTree, // Cannot reuse with parallel tests ReusableNormalizedNodePruner.forDataSchemaContext(CONTEXT_TREE)); } -- 2.36.6