Clock went backwards corruption check #5131

sudiksha27 · 2020-11-25T10:17:30Z

Goals (and why):
TimeLock should validates the expected invariant for its timestamp bounds: they must increase with increasing sequence numbers.

Implementation Description (bullets):

LocalTimestampInvariantsVerifier - verifies timestamp bounds increase with increasing sequence numbers
The validation is done batch wise, e.g. [1, n], [n, 2 * n - 1] and so on. Two consecutive batches share
boundaries, this is done to catch inversion at batch end.
The verifier uses the existing corruption handling infra i.e. invariant violation will show up in the health check and (when enabled) is capable of killing TimeLock in the case of violation.

Testing (What was existing testing like? What have you done to improve it?):
Added unit tests

Concerns (what feedback would you like?):

Does the design seem reasonable?
Can we optimize the implementation?
Did I miss any edge cases?

Where should we start reviewing?:
HistoryAnalyzer.java

Priority (whenever / two weeks / yesterday):
End of week.

changelog-app · 2020-11-25T10:17:35Z

Generate changelog in `changelog/@unreleased`

Type

Description

TimeLock now validates the expected invariant for its timestamp bounds i.e. the timestamp bound must increase with increasing sequence numbers.

Check the box to generate changelog(s)

Generate changelog entry

gmaretic

Makes sense, though the analyzer itself need some polish

gmaretic · 2020-11-25T15:46:00Z

...tion-detection/src/main/java/com/palantir/timelock/corruption/detection/HistoryAnalyzer.java

+                .sorted(Comparator.comparingLong(SequenceAndTimestampPair::sequence))
+                .collect(Collectors.toList());
+
+        long greatestTimestampSoFar = -1L;


Technically, I don't think we guarantee non-negative timestamps, so this should be Long.MIN_VALUE

gmaretic · 2020-11-25T16:28:57Z

...tion-detection/src/main/java/com/palantir/timelock/corruption/detection/HistoryAnalyzer.java

+        List<SequenceAndTimestampPair> seqAndTimestampPairs = KeyedStream.stream(record.record())
+                .map(val -> getPaxosValueData(val.learnedValue()))
+                .filter(Predicates.notNull())
+                .mapEntries((sequence, timestamp) -> Maps.immutableEntry(
+                        sequence, ImmutableSequenceAndTimestampPair.of(sequence, PtBytes.toLong(timestamp))))
+                .values()
+                .sorted(Comparator.comparingLong(SequenceAndTimestampPair::sequence))
+                .collect(Collectors.toList());
+
+        long greatestTimestampSoFar = -1L;
+        long currentTimestamp;
+        for (SequenceAndTimestampPair sequenceAndTimestampPair : seqAndTimestampPairs) {
+            currentTimestamp = sequenceAndTimestampPair.timestamp();
+            if (currentTimestamp <= greatestTimestampSoFar) {
+                return true;
+            }
+            greatestTimestampSoFar = currentTimestamp;
+        }
+        return false;
+    }
+
+    @Value.Immutable
+    interface SequenceAndTimestampPair {
+        @Value.Parameter
+        long sequence();
+
+        @Value.Parameter
+        long timestamp();
+    }


If you want to be a bit fancy:

Suggested change

List<SequenceAndTimestampPair> seqAndTimestampPairs = KeyedStream.stream(record.record())

.map(val -> getPaxosValueData(val.learnedValue()))

.filter(Predicates.notNull())

.mapEntries((sequence, timestamp) -> Maps.immutableEntry(

sequence, ImmutableSequenceAndTimestampPair.of(sequence, PtBytes.toLong(timestamp))))

.values()

.sorted(Comparator.comparingLong(SequenceAndTimestampPair::sequence))

.collect(Collectors.toList());

long greatestTimestampSoFar = -1L;

long currentTimestamp;

for (SequenceAndTimestampPair sequenceAndTimestampPair : seqAndTimestampPairs) {

currentTimestamp = sequenceAndTimestampPair.timestamp();

if (currentTimestamp <= greatestTimestampSoFar) {

return true;

}

greatestTimestampSoFar = currentTimestamp;

}

return false;

}

@Value.Immutable

interface SequenceAndTimestampPair {

@Value.Parameter

long sequence();

@Value.Parameter

long timestamp();

}

Stream<Long> expectedSortedTimestamps = KeyedStream.stream(record.record())

.map(val -> getPaxosValueData(val.learnedValue()))

.filter(Predicates.notNull())

.mapEntries((sequence, timestamp) -> Maps.immutableEntry(sequence, PtBytes.toLong(timestamp)))

.entries()

.sorted(Comparator.comparingLong(Entry::getKey))

.map(Entry::getValue);

return StreamEx.of(expectedSortedTimestamps)

.pairMap((first, second) -> first >= second)

.anyMatch(x -> x);

If you don't want to use StreamEx, you can still avoid the immutable by checking expectedSortedTimestamps in a for loop, but just compare neighbours instead of maintaining a global maximum

sudiksha27 · 2020-11-25T18:08:28Z

This will need redesigning. Will be moving the check out of History analyzer and adding a separate thread to verify timestamp invariants.

This reverts commit 31b54ba.

...c/main/java/com/palantir/timelock/corruption/detection/LocalTimestampInvariantsVerifier.java

gmaretic

Mostly looks good, have some nits

gmaretic · 2020-12-01T12:34:37Z

...ection/src/main/java/com/palantir/timelock/corruption/detection/LocalCorruptionDetector.java

+                            .putAllViolatingStatusesToNamespaceAndUseCase(
+                                    paxosRoundCorruptionReport.violatingStatusesToNamespaceAndUseCase())


Suggested change

.putAllViolatingStatusesToNamespaceAndUseCase(

paxosRoundCorruptionReport.violatingStatusesToNamespaceAndUseCase())

.from(paxosRoundCorruptionReport)

gmaretic · 2020-12-01T12:38:36Z

...c/main/java/com/palantir/timelock/corruption/detection/LocalTimestampInvariantsVerifier.java

+    private CorruptionCheckViolation timestampInvariantsViolationLevel(NamespaceAndUseCase namespaceAndUseCase) {
+        Stream<Long> expectedSortedTimestamps = KeyedStream.stream(getLearnerLogs(namespaceAndUseCase))
+                .map(PaxosValue::getData)
+                .filter(Predicates.notNull())


Suggested change

.filter(Predicates.notNull())

.filter(Objects::nonNull)

gmaretic · 2020-12-01T12:41:26Z

...c/main/java/com/palantir/timelock/corruption/detection/LocalTimestampInvariantsVerifier.java

+    @VisibleForTesting
+    public static final int LEARNER_LOG_BATCH_SIZE_LIMIT = 250;
+
+    public static final long MIN_SEQUENCE_TO_BE_VERIFIED = -1L;


Suggested change

public static final long MIN_SEQUENCE_TO_BE_VERIFIED = -1L;

public static final long MIN_SEQUENCE_TO_BE_VERIFIED = Long.MIN_VALUE;

gmaretic · 2020-12-01T12:48:16Z

...st/java/com/palantir/timelock/corruption/detection/LocalTimestampInvariantsVerifierTest.java

+    @Test
+    public void detectCorruptionIfClockWentBackwardsOnNode() {
+        helper.writeLogsOnDefaultLocalServer(1, LEARNER_LOG_BATCH_SIZE_LIMIT - 1);
+        helper.forceTimestampToGoBackwards(5);


This method name is kinda misleading, it's actually forcing the timestamp to go forward / sequence to go backwards. It's only making the timestamp go backwards for the sequence after, right?

Ah yes, that makes sense. Basically what I mean is - create inversion b/w this sequence and the next. I have changed the name to createTimestampInversion now.

gmaretic · 2020-12-01T12:49:26Z

...st/java/com/palantir/timelock/corruption/detection/LocalTimestampInvariantsVerifierTest.java

+    @Test
+    public void detectIfClockWentBackwardsAtBatchEnd() {
+        helper.writeLogsOnDefaultLocalServer(1, LEARNER_LOG_BATCH_SIZE_LIMIT);
+        helper.forceTimestampToGoBackwards(LEARNER_LOG_BATCH_SIZE_LIMIT / 2);


Shouldn't this then be helper.forceTimestampToGoBackwards(LEARNER_LOG_BATCH_SIZE_LIMIT - 1);?

Good catch! yes, this is a copy pasting mistake.

gmaretic · 2020-12-01T12:51:38Z

...st/java/com/palantir/timelock/corruption/detection/LocalTimestampInvariantsVerifierTest.java

+        helper.assertLocalTimestampInvariantsStand();
+
+        helper.writeLogsOnDefaultLocalServer(2, LEARNER_LOG_BATCH_SIZE_LIMIT);
+        helper.forceTimestampToGoBackwards(LEARNER_LOG_BATCH_SIZE_LIMIT / 2);


Suggested change

helper.forceTimestampToGoBackwards(LEARNER_LOG_BATCH_SIZE_LIMIT / 2);

helper.forceTimestampToGoBackwards(1);

This way we test that we don't skip the one entry from first check. Also I guess do this before the first check to be consistent

gmaretic · 2020-12-01T12:53:00Z

.../test/java/com/palantir/timelock/corruption/detection/TimeLockCorruptionDetectionHelper.java

@@ -105,6 +105,31 @@ void assertViolationDetectedForNamespaceAndUseCases(
        return timeLockCorruptionTestSetup.createStatLogForNamespaceAndUseCase(namespaceAndUseCase);
    }

+    void assertClockWentBackwards() {


Maybe rename to assertClockWentBackwardsInNextBatch to be clearer? Same for the method below

gmaretic

LGTM

svc-autorelease · 2021-01-06T17:21:43Z

Released 0.280.1

This reverts commit 259908a.

This reverts commit 7f6c373.

* Revert "Revert "Clock went backwards corruption check (#5131)" (#5181)" This reverts commit 7f6c373. * Timestamps dont have to be strictly increasing * Add test * Add generated changelog entries

sudiksha27 added 2 commits November 25, 2020 10:00

Clock went backwards corruption check

31b54ba

Add generated changelog entries

ed2aa51

sudiksha27 marked this pull request as ready for review November 25, 2020 11:47

sudiksha27 added the pr: needs revision label Nov 25, 2020

gmaretic reviewed Nov 25, 2020

View reviewed changes

sudiksha27 added 3 commits November 25, 2020 19:21

Redesign - WIP

0cd816b

Revert "Clock went backwards corruption check"

d17e7fb

This reverts commit 31b54ba.

Keep required changes

0ed9dbc

sudiksha27 added the pr: work in progress label Nov 25, 2020

sudiksha27 added 4 commits November 30, 2020 10:26

Update check for edge cases + add tests

9d48095

Fix test

a195ecf

Bug fix - WIP

b62c445

Some more fixes

efcae68

sudiksha27 commented Nov 30, 2020

View reviewed changes

...c/main/java/com/palantir/timelock/corruption/detection/LocalTimestampInvariantsVerifier.java Show resolved Hide resolved

sudiksha27 added 2 commits December 1, 2020 00:59

Resign - II

c6a3cfd

Clean

59d1471

sudiksha27 removed the pr: work in progress label Dec 1, 2020

gmaretic reviewed Dec 1, 2020

View reviewed changes

Address comments

b25cf69

sudiksha27 added autorelease pr: needs review merge when ready and removed pr: needs revision labels Dec 11, 2020

gmaretic approved these changes Jan 6, 2021

View reviewed changes

bulldozer-bot bot merged commit 259908a into develop Jan 6, 2021

bulldozer-bot bot deleted the tl-corr-p0-flup branch January 6, 2021 17:21

sudiksha27 added a commit that referenced this pull request Jan 7, 2021

Revert "Clock went backwards corruption check (#5131)"

bcc42ad

This reverts commit 259908a.

changelog-app bot mentioned this pull request Jan 7, 2021

Revert "Clock went backwards corruption check (#5131)" #5181

Merged

sudiksha27 added a commit that referenced this pull request Jan 7, 2021

Revert "Clock went backwards corruption check (#5131)" (#5181)

7f6c373

This reverts commit 259908a.

sudiksha27 mentioned this pull request Jan 7, 2021

Revert revert 5131 #5182

Merged

sudiksha27 added a commit that referenced this pull request Jan 8, 2021

Revert "Revert "Clock went backwards corruption check (#5131)" (#5181)"

e0f589c

This reverts commit 7f6c373.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clock went backwards corruption check #5131

Clock went backwards corruption check #5131

sudiksha27 commented Nov 25, 2020 •

edited

Loading

changelog-app bot commented Nov 25, 2020 •

edited by sudiksha27

Loading

gmaretic left a comment

gmaretic Nov 25, 2020

gmaretic Nov 25, 2020

sudiksha27 commented Nov 25, 2020

gmaretic left a comment

gmaretic Dec 1, 2020

gmaretic Dec 1, 2020

gmaretic Dec 1, 2020

gmaretic Dec 1, 2020

sudiksha27 Dec 1, 2020

gmaretic Dec 1, 2020

sudiksha27 Dec 1, 2020

gmaretic Dec 1, 2020

sudiksha27 Dec 1, 2020

gmaretic Dec 1, 2020

gmaretic left a comment

svc-autorelease commented Jan 6, 2021

		.putAllViolatingStatusesToNamespaceAndUseCase(
		paxosRoundCorruptionReport.violatingStatusesToNamespaceAndUseCase())

	.putAllViolatingStatusesToNamespaceAndUseCase(
	paxosRoundCorruptionReport.violatingStatusesToNamespaceAndUseCase())
	.from(paxosRoundCorruptionReport)

	public static final long MIN_SEQUENCE_TO_BE_VERIFIED = -1L;
	public static final long MIN_SEQUENCE_TO_BE_VERIFIED = Long.MIN_VALUE;

	helper.forceTimestampToGoBackwards(LEARNER_LOG_BATCH_SIZE_LIMIT / 2);
	helper.forceTimestampToGoBackwards(1);

Clock went backwards corruption check #5131

Clock went backwards corruption check #5131

Conversation

sudiksha27 commented Nov 25, 2020 • edited Loading

changelog-app bot commented Nov 25, 2020 • edited by sudiksha27 Loading

Generate changelog in changelog/@unreleased

gmaretic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sudiksha27 commented Nov 25, 2020

gmaretic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmaretic left a comment

Choose a reason for hiding this comment

svc-autorelease commented Jan 6, 2021

sudiksha27 commented Nov 25, 2020 •

edited

Loading

changelog-app bot commented Nov 25, 2020 •

edited by sudiksha27

Loading

Generate changelog in `changelog/@unreleased`