Make Timestamps Returned by Snapshot APIs Consistent #43148

original-brownbear · 2019-06-12T10:39:22Z

We don't have to calculate the start and end times form the shards for the status API, we have the start time available from the CS or the SnapshotInfo in the repo and can either take the end time form the SnapshotInfo or
take the most recent time from the shard stats for in progress snapshots
Had to add serialization of these time values since we can't compute them from the shards anymore
- Fall back to computing time values from shards in BwC scenario
Started a new IT class for this since the existing ones are huge already and we have a few more API inconsistencies with snapshots to fix and add tests for
Closes Snapshot status API returns different timing information than snapshot info API #43074

Admittedly the weird mutable data structures, SnapshotStats in particular becomes a little more magical than it already is with this change. I'd try to clean this up in a follow-up.

* We don't have to calculate the start and end times form the shards for the status API, we have the start time available from the CS or the `SnapshotInfo` in the repo and can either take the end time form the `SnapshotInfo` or take the most recent time from the shard stats for in progress snapshots * Closes elastic#43074

elasticmachine · 2019-06-12T10:39:24Z

Pinging @elastic/es-distributed

original-brownbear · 2019-06-12T11:19:12Z

Jenkins run elasticsearch-ci/2

tlrx

LGTM, thanks for taking care of this. I left some minor comments

I agree that SnapshotStats would benefit from a clean up

tlrx · 2019-07-04T08:37:45Z

server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/status/SnapshotStats.java

@@ -46,6 +46,11 @@
    SnapshotStats() {
    }

+    SnapshotStats(long startTime, long time) {


I don't think we need another constructor just for this, maybe reuse the existing one?

Sure, I guess I hated putting all those 0 but it's shorted this way for sure and horrible regardless :D

tlrx · 2019-07-04T08:40:04Z

...er/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/status/SnapshotStatus.java

        this.snapshot = Objects.requireNonNull(snapshot);
        this.state = Objects.requireNonNull(state);
        this.shards = Objects.requireNonNull(shards);
        this.includeGlobalState = includeGlobalState;
        shardsStats = new SnapshotShardsStats(shards);
-        updateShardStats();
+        updateShardStats(startTime, time);


Can we check that they are not < 0?

Actually, I don't think we need to I think :) We get these numbers from org.elasticsearch.index.snapshots.IndexShardSnapshotStatus which does all the checks internally so we should be good here?

tlrx · 2019-07-04T08:41:12Z

.../org/elasticsearch/action/admin/cluster/snapshots/status/TransportSnapshotsStatusAction.java

                    builder.add(new SnapshotStatus(new Snapshot(repositoryName, snapshotId), state,
-                        Collections.unmodifiableList(shardStatusBuilder), snapshotInfo.includeGlobalState()));
+                        Collections.unmodifiableList(shardStatusBuilder), snapshotInfo.includeGlobalState(),
+                        startTime, snapshotInfo.endTime() - startTime));


I don't think that SnapshotInfo guarantees that endTime > startTime, maybe we should be defensive here?

Well no it doesn't but this isn't a new issue is it? It just shows up in two places now instead of in one. If you have skewed clocks and a master failover during snapshotting the absolute timestamp stored in the CS won't work out. All that happens now is that we see the same issue in two places instead of one?

tlrx · 2019-07-04T08:42:21Z

server/src/test/java/org/elasticsearch/snapshots/SnapshotStatusApisIT.java

+import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.greaterThan;
+
+@ESIntegTestCase.ClusterScope(scope = ESIntegTestCase.Scope.TEST)


I don't think this is needed, the default scope SUITE should be fine

I guess for a single test it doesn't matter anyway, but in general I'm not a big fan of the suit scope when running tests that so heavily depend on the exact state of the cluster like here -> that's why I went with test scope. Not sure if that makes sense?

I still don't think there is a reason here to use TEST and not the default, unless you noticed test failures with the SUITE test scope and if so we need to investigate them.

Fine with me :) Removed it.

original-brownbear · 2019-07-04T14:00:06Z

After merging master into this branch (even after resolving the compile issue) this is actually failing tests. Looking into it, something is off now with the timestamps somewhere.

original-brownbear · 2019-07-04T14:54:49Z

@tlrx I have to admit, your initial instincts about the possibility of negative timestamps were correct after all. I had to push 6223466 after merging master. There were two issues here:

The code I changed from using relative to absolute time needed absolute time. Otherwise, it will not work in a BwC scenario (because in those we will still use the time stamp on the shards to calculate the end and start times) so I moved all the logic in the repo and snapshot handling that was relevant to the same absolute time method on the thread pool to get things consistent here
The logic of calculating the start and end time needs to be disabled when using timestamps from SnapshotInfo. This only worked fine when all the code was using System.currentTimeMillis but stopped working now with cached time. Since all the nodes in the test cluster use different thread pools that was simulating clock skew, which lead to the calculation not working out anymore (since due to skew a shard could look like it started before the snapshot itself or finished after).

Can you take another look please? :)

tlrx

LGTM. I left a comment about test scope which I think is important.

tlrx · 2019-07-04T15:11:51Z

...er/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/status/SnapshotStatus.java

        this.snapshot = Objects.requireNonNull(snapshot);
        this.state = Objects.requireNonNull(state);
        this.shards = Objects.requireNonNull(shards);
        this.includeGlobalState = includeGlobalState;
        shardsStats = new SnapshotShardsStats(shards);
-        updateShardStats();
+        updateShardStats(startTime, time);


tlrx · 2019-07-04T15:17:39Z

server/src/test/java/org/elasticsearch/snapshots/SnapshotStatusApisIT.java

+import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.greaterThan;
+
+@ESIntegTestCase.ClusterScope(scope = ESIntegTestCase.Scope.TEST)


I still don't think there is a reason here to use TEST and not the default, unless you noticed test failures with the SUITE test scope and if so we need to investigate them.

tlrx · 2019-07-04T15:19:17Z

.../org/elasticsearch/action/admin/cluster/snapshots/status/TransportSnapshotsStatusAction.java

                    builder.add(new SnapshotStatus(new Snapshot(repositoryName, snapshotId), state,
-                        Collections.unmodifiableList(shardStatusBuilder), snapshotInfo.includeGlobalState()));
+                        Collections.unmodifiableList(shardStatusBuilder), snapshotInfo.includeGlobalState(),
+                        startTime, snapshotInfo.endTime() - startTime));


original-brownbear · 2019-07-04T18:36:31Z

Jenkins run elasticsearch-ci/1

As a result of elastic#43148 neitehr of these apply: * First comment about relative time doesn't apply anymore since we use absolute time here again * The overall time in all snapshot APIs is now consistent and contains cleanup the time since it's not just calculated as the maximum of all shard upload times

As a result of #43148 neitehr of these apply: * First comment about relative time doesn't apply anymore since we use absolute time here again * The overall time in all snapshot APIs is now consistent and contains cleanup the time since it's not just calculated as the maximum of all shard upload times

* We don't have to calculate the start and end times form the shards for the status API, we have the start time available from the CS or the `SnapshotInfo` in the repo and can either take the end time form the `SnapshotInfo` or take the most recent time from the shard stats for in progress snapshots * Closes elastic#43074

* Since we're changing the way the snapshot status message is serialized in elastic#43148 we need to turn of BwC tests here

* We don't have to calculate the start and end times form the shards for the status API, we have the start time available from the CS or the `SnapshotInfo` in the repo and can either take the end time form the `SnapshotInfo` or take the most recent time from the shard stats for in progress snapshots * Closes #43074

* Since we're changing the way the snapshot status message is serialized in #43148 we need to turn of BwC tests here

* elastic#43148 has been backported to `7.4` -> we can reenable these tests after adjusting the version in teh serialization logic

* #43148 has been backported to `7.4` -> we can reenable these tests after adjusting the version in teh serialization logic

original-brownbear added >bug :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v8.0.0 v7.3.0 labels Jun 12, 2019

original-brownbear changed the title ~~Make Timestamps returned by Snapshot APIs Consistent~~ Make Timestamps Returned by Snapshot APIs Consistent Jun 12, 2019

original-brownbear requested review from ywelsch, andrershov and tlrx June 22, 2019 14:53

jpountz added v7.4.0 and removed v7.3.0 labels Jul 3, 2019

tlrx approved these changes Jul 4, 2019

View reviewed changes

original-brownbear added 3 commits July 4, 2019 12:46

Merge remote-tracking branch 'elastic/master' into 43074

9cae546

CR: remove redundant ctor

d4f95e4

Merge remote-tracking branch 'elastic/master' into 43074

cc995ee

original-brownbear added 2 commits July 4, 2019 16:33

Fix logic to work with clock skew

6223466

fix in progress reported timestamp

8aa1a5d

original-brownbear requested a review from tlrx July 4, 2019 14:54

tlrx approved these changes Jul 4, 2019

View reviewed changes

original-brownbear added 2 commits July 4, 2019 17:50

Merge remote-tracking branch 'elastic/master' into 43074

7f555a2

CR: remove test scope

dfbef32

original-brownbear merged commit deafb59 into elastic:master Jul 5, 2019

original-brownbear deleted the 43074 branch July 5, 2019 07:24

original-brownbear added the backport pending label Jul 5, 2019

original-brownbear mentioned this pull request Jul 5, 2019

Fix testGetSnapshotsMultipleRepos #43993

Merged

original-brownbear mentioned this pull request Jul 5, 2019

Remove Outdated Comments from BlobStoreRepository #44029

Merged

original-brownbear mentioned this pull request Jul 12, 2019

Make Timestamps Returned by Snapshot APIs Consistent (#43148) #44261

Merged

original-brownbear added a commit to original-brownbear/elasticsearch that referenced this pull request Jul 12, 2019

Disable BwC Tests for elastic#43148

d3b275f

* Since we're changing the way the snapshot status message is serialized in elastic#43148 we need to turn of BwC tests here

original-brownbear mentioned this pull request Jul 12, 2019

Disable BwC Tests for #43148 #44262

Merged

original-brownbear added a commit that referenced this pull request Jul 12, 2019

Disable BwC Tests for #43148 (#44262)

4fd4fdd

* Since we're changing the way the snapshot status message is serialized in #43148 we need to turn of BwC tests here

original-brownbear mentioned this pull request Jul 12, 2019

Enable BwC Tests after #43148 Backport #44270

Merged

original-brownbear removed the backport pending label Jul 12, 2019

original-brownbear added a commit that referenced this pull request Jul 12, 2019

Enable BwC Tests after #43148 Backport (#44270)

ac6b936

* #43148 has been backported to `7.4` -> we can reenable these tests after adjusting the version in teh serialization logic

codebrain mentioned this pull request Oct 14, 2019

7.4 meta ticket elastic/elasticsearch-net#4133

Closed

56 tasks

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Make Timestamps Returned by Snapshot APIs Consistent #43148

Make Timestamps Returned by Snapshot APIs Consistent #43148

Uh oh!

Conversation

original-brownbear commented Jun 12, 2019

Uh oh!

elasticmachine commented Jun 12, 2019

Uh oh!

original-brownbear commented Jun 12, 2019

Uh oh!

tlrx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

original-brownbear commented Jul 4, 2019

Uh oh!

original-brownbear commented Jul 4, 2019

Uh oh!

tlrx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

original-brownbear commented Jul 4, 2019

Uh oh!

Uh oh!