Introduce forget follower API #39718

jasontedor · 2019-03-05T20:25:40Z

This commit introduces the forget follower API. This API is needed in cases that unfollowing a following index fails to remove the shard history retention leases on the leader index. This can happen explicitly through user action, or implicitly through an index managed by ILM. When this occurs, history will be retained longer than necessary. While the retention lease will eventually expire, it can be expensive to allow history to persist for that long, and also prevent ILM from performing actions like shrink on the leader index. As such, we introduce an API to allow for manual removal of the shard history retention leases in this case.

Relates #37165

This commit introduces the forget follower API. This API is needed in cases that unfollowing a following index fails to remove the shard history retention leases on the leader index. This can happen explicitly through user action, or implicitly through an index managed by ILM. When this occurs, history will be retained longer than necessary. While the retention lease will eventually expire, it can be expensive to allow history to persist for that long, and also prevent ILM from performing actions like shrink on the leader index. As such, we introduce an API to allow for manual removal of the shard history retention leases in this case.

elasticmachine · 2019-03-05T20:26:05Z

Pinging @elastic/es-distributed

* master: Remove beta label from CCR (elastic#39722) Rename retention lease setting (elastic#39719)

martijnvg

This change looks good. I left some comments and questions.

martijnvg · 2019-03-06T08:57:35Z

.../plugin/core/src/main/java/org/elasticsearch/xpack/core/ccr/action/ForgetFollowerAction.java

+
+public class ForgetFollowerAction extends Action<BroadcastResponse> {
+
+    public static final String ACTION_NAME = "indices:admin/xpack/ccr/forget_follower";


should we not include /xpack/ in new actions?

I would rather to be consistent with all of CCR than introduce inconsistency there. We can address wholesale later, if needed.

.../plugin/core/src/main/java/org/elasticsearch/xpack/core/ccr/action/ForgetFollowerAction.java

martijnvg · 2019-03-06T09:02:32Z

client/rest-high-level/src/test/java/org/elasticsearch/client/core/BroadcastResponseTests.java

+public class BroadcastResponseTests extends ESTestCase {
+
+    public void testFromXContent() throws IOException {
+        final String index = randomAlphaOfLength(8);


Maybe use AbstractXContentTestCase#xContentTester() static method to test xcontent serialization?
I think it is a bit shorter and we have more test iterations by default.

I think that AbstractXContentTestCase here is a lie. This is because what we really want to test is that if the REST layer sends back an XContent serialized org.elasticsearch.action.support.broadcast.BroadcastResponse then org.elasticsearch.client.core.BroadcastResponse is able to de-serialize this XContent. AbstractXContentTestCase does not allow to test such a situation, instead only allowing you to go from concrete instance (of org.elasticsearch.client.core.BroadcastResponse) to XContent and then back to a concrete instance of (org.elasticsearch.client.core.BroadcastResponse), and then checks if the result is equal. That is not what actually happens in production though, what happens in production is what I outlined above: a concrete instance of org.elasticsearch.action.support.broadcast.BroadcastResponse is serialized on the REST layer which then has to be de-serialized on the HL client layer by org.elasticsearch.client.core.BroadcastResponse. Hence, this test reflects that.

Good point. I think we should open an issue, so that goes over the existing hlrc response serialization tests and change those tests to test way is tested here.

I opened #39745.

martijnvg · 2019-03-06T09:04:51Z

...rest-high-level/src/test/java/org/elasticsearch/client/documentation/CCRDocumentationIT.java

@@ -395,6 +401,98 @@ public void onFailure(Exception e) {
        assertTrue(latch.await(30L, TimeUnit.SECONDS));
    }

+    public void testForgetFollower() throws InterruptedException, IOException {


I think the hlrc docs itself are missing? Test looks good though.

I pushed 63721a4.

martijnvg · 2019-03-06T09:11:24Z

docs/reference/ccr/apis/follow/post-forget-follower.asciidoc

+
+==== Authorization
+
+If the {es} {security-features} are enabled, you must have `manage_follow_index`


The IndexPrivilege#MANAGE_FOLLOW_INDEX_AUTOMATON constant isn't updated to include the forget follower api. Also this text should say: index privileges for the leader index. instead of index privileges for the follower index..

I also I wonder whether manage_follow_index is the right privilege here. Because this is about a leases for the follower index in the leader index. I don't know a good alternative here. I just wanted to mention this.

I pushed e68b0c0.

martijnvg · 2019-03-06T09:12:51Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/doc/RestTestsFromSnippetsTask.groovy

@@ -104,7 +104,7 @@ public class RestTestsFromSnippetsTask extends SnippetsTask {
     * format of the response is incompatible i.e. it is not a JSON object.
     */
    static shouldAddShardFailureCheck(String path) {
-        return path.startsWith('_cat') == false &&  path.startsWith('_ml/datafeeds/') == false
+        return path.startsWith('_cat') == false && path.startsWith('_ml/datafeeds/') == false


why are the changes to the groovy files needed?

For the docs tests where we fire forget follower requests, it is entirely too hard to set up a request that is not going to fail. For example, to set up a request that is not going to fail, we have to know the index UUID of the follower. Obtaining that in a docs test is not possible to my knowledge, we do not have a way to set a value to use in a later docs test? Maybe I am missing something?

However, the docs infrastructure automatically adds is_false: _shards.failures to every docs test. Therefore, I had to add a hook that enables us to say skip that check for these tests where we expect failures.

Again, if there's a way to avoid the failures that I am not seeing, please let me know!

Got it, thanks for the explanation!

martijnvg · 2019-03-06T15:41:42Z

.../plugin/ccr/qa/security/src/test/java/org/elasticsearch/xpack/ccr/FollowIndexSecurityIT.java

+                @SuppressWarnings("unchecked") final Map<String, Object> shardStatsAsMap = (Map<String, Object>)shardsStats.get(0);
+                @SuppressWarnings("unchecked") final Map<String, Object> retentionLeasesStats =
+                        (Map<String, Object>) shardStatsAsMap.get("retention_leases");
+                @SuppressWarnings("unchecked") final ArrayList<Object> leases = (ArrayList<Object>)retentionLeasesStats.get("leases");


since no unfollow has happened, the shard follow task may add a new lease, right?

Not since it is paused.

right, I missed that pause follow index api call.

martijnvg · 2019-03-06T20:25:16Z

I made the above change, but the test is still failing, but now it fails because a lease is still returned, which isn't expected, as the leases should have been removed.

FAILURE 1.84s | FollowIndexSecurityIT.testForgetFollower <<< FAILURES!
   > Throwable #1: java.lang.AssertionError: 
   > Expected: an empty collection
   >      but: <[{id=follow-cluster/forget-follower/FgX6xx8PRdW-7Go66pjbQw-following-leader_cluster/forget-leader/tcYPeEYiRuCQoAVGQWFQLA, retaining_seq_no=0, timestamp=1551903399688, source=ccr}]>
   >    at __randomizedtesting.SeedInfo.seed([BD569197DFE90BBD:31C54051FDB9761D]:0)
   >    at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
   >    at org.elasticsearch.xpack.ccr.FollowIndexSecurityIT.testForgetFollower(FollowIndexSecurityIT.java:227)
   >    at java.lang.Thread.run(Thread.java:748)

I also noticed that the specified leader_remote_cluster was incorrect:

--- a/x-pack/plugin/ccr/qa/security/src/test/java/org/elasticsearch/xpack/ccr/FollowIndexSecurityIT.java
+++ b/x-pack/plugin/ccr/qa/security/src/test/java/org/elasticsearch/xpack/ccr/FollowIndexSecurityIT.java
@@ -208,9 +208,10 @@ public class FollowIndexSecurityIT extends ESCCRRestTestCase {
                         "\"follower_cluster\":\"follow-cluster\"," +
                         "\"follower_index\":\"" +  forgetFollower + "\"," +
                         "\"follower_index_uuid\":\"" + followerIndexUUID + "\"," +
-                        "\"leader_remote_cluster\":\"leader_cluster\"" +
+                        "\"leader_remote_cluster\":\"leader-cluster\"" +
                         "}";
                 request.setJsonEntity(requestBody);

But even with this change there is still a lease returned. I don't yet understand why that happens.

jasontedor · 2019-03-07T02:20:46Z

@martijnvg leader_cluster is correct because the name of the lease is based on the name of the remote cluster alias, not the actual name of the cluster.

jasontedor · 2019-03-07T02:48:20Z

@martijnvg The fix is 2e96e66.

jasontedor · 2019-03-07T04:25:14Z

@elasticmachine run elasticsearch-ci/bwc

martijnvg

LGTM - I've merged master into your branch, because that fixes the bwc test that was failing.

jasontedor · 2019-03-07T13:28:05Z

@martijnvg I pushed a commit that would have made the failure more apparent: 238c29b

This gives:

  2> REPRODUCE WITH: ./gradlew :x-pack:plugin:ccr:qa:security:followClusterTestRunner -Dtests.seed=A31A2C218548E5CC -Dtests.class=org.elasticsearch.xpack.ccr.FollowIndexSecurityIT -Dtests.method="testForgetFollower" -Dtests.security.manager=true -Dtests.locale=da -Dtests.timezone=Pacific/Bougainville -Dcompiler.java=11 -Druntime.java=8
FAILURE 1.77s | FollowIndexSecurityIT.testForgetFollower <<< FAILURES!
   > Throwable #1: java.lang.AssertionError: [{shard=0, index=forget-leader, status=INTERNAL_SERVER_ERROR, reason={type=failed_node_exception, reason=Failed node [ZQdq-2YiRkuZ8Rkwhg4IwQ], node_id=ZQdq-2YiRkuZ8Rkwhg4IwQ, caused_by={type=security_exception, reason=action [indices:admin/xpack/ccr/forget_follower[n]] is unauthorized for user [test_ccr]}}}]
   > Expected: <1>
   >      but: was <0>
   >    at __randomizedtesting.SeedInfo.seed([A31A2C218548E5CC:2F89FDE7A718986C]:0)
   >    at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
   >    at org.elasticsearch.xpack.ccr.FollowIndexSecurityIT.testForgetFollower(FollowIndexSecurityIT.java:219)
   >    at java.lang.Thread.run(Thread.java:748)

jasontedor · 2019-03-07T13:29:12Z

Thanks for the really helpful review @martijnvg.

This commit introduces the forget follower API. This API is needed in cases that unfollowing a following index fails to remove the shard history retention leases on the leader index. This can happen explicitly through user action, or implicitly through an index managed by ILM. When this occurs, history will be retained longer than necessary. While the retention lease will eventually expire, it can be expensive to allow history to persist for that long, and also prevent ILM from performing actions like shrink on the leader index. As such, we introduce an API to allow for manual removal of the shard history retention leases in this case.

* 6.7: Fix CCR HLRC docs Introduce forget follower API (elastic#39718) 6.6.2 release notes. Update release notes for 6.7.0 Add documentation for min_hash filter (elastic#39671) Unmute testIndividualActionsTimeout Unmute testFollowIndexAndCloseNode Use unwrapped cause to determine if node is closing (elastic#39723) Don’t ack if unable to remove failing replica (elastic#39584) Wipe Snapshots Before Indices in RestTests (elastic#39662) (elastic#39765) Bug fix for AnnotatedTextHighlighter (elastic#39525) Fix Snapshot BwC with Version 5.6.x (elastic#39737) Fix occasional SearchServiceTests failure (elastic#39697) Correct date in daterange-aggregation.asciidoc (elastic#39727) Add a note to purge the ingest-geoip plugin (elastic#39553)

jasontedor added 4 commits March 5, 2019 15:18

Missing newline

27c3b68

Fix precommit

b0be97e

Fix imports

fde0537

jasontedor requested review from martijnvg, ywelsch and dnhatn March 5, 2019 20:25

jasontedor added >enhancement v7.0.0 :Distributed Indexing/CCR Issues around the Cross Cluster State Replication features v6.7.0 v8.0.0 v7.2.0 labels Mar 5, 2019

jasontedor added 5 commits March 5, 2019 15:56

Fix docs

1a31b3d

Merge branch 'master' into forget-follower

7976f8b

* master: Remove beta label from CCR (elastic#39722) Rename retention lease setting (elastic#39719)

Remove beta

ca59bf7

Add javadocs

b2f2cf1

Fix imports

56ac397

jasontedor added the blocker label Mar 6, 2019

martijnvg reviewed Mar 6, 2019

View reviewed changes

Remove unused class

20b2038

jasontedor mentioned this pull request Mar 6, 2019

High level REST client XContent tests are misleading #39745

Closed

jasontedor added 2 commits March 6, 2019 10:04

Add permission

e68b0c0

Make the test more realistic

1564a7a

martijnvg reviewed Mar 6, 2019

View reviewed changes

jasontedor added 2 commits March 6, 2019 10:43

Pause follower before remove leases

3ab7cb7

Add HLRC docs

63721a4

jasontedor requested a review from martijnvg March 6, 2019 15:53

Fix imports

f6592f5

Fix method parameter

33edd22

Fix permissions

2e96e66

Simplify

b5f6150

jasontedor requested a review from martijnvg March 7, 2019 02:50

jasontedor added 2 commits March 6, 2019 21:58

Rename variable

b31f6d3

Fix imports

3d372b3

Merge remote-tracking branch 'es/master' into forget-follower

bbef603

martijnvg approved these changes Mar 7, 2019

View reviewed changes

jasontedor added 2 commits March 7, 2019 08:04

Fix failing test

2c3ef2e

Make failures more obvious

238c29b

Simplify

072e2e4

jasontedor merged commit 8c156ed into elastic:master Mar 7, 2019

jasontedor deleted the forget-follower branch March 7, 2019 16:33

This was referenced Mar 13, 2019

Add description of manage_leader_index privilege elastic/stack-docs#243

Merged

Add manage_leader_index privilege elastic/kibana#33154

Merged

jakelandis added v7.0.0-rc2 and removed v7.0.0 labels Apr 3, 2019

codebrain mentioned this pull request Jul 12, 2019

Implement Forget Follower API elastic/elasticsearch-net#3939

Merged

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


		public class ForgetFollowerAction extends Action<BroadcastResponse> {

		public static final String ACTION_NAME = "indices:admin/xpack/ccr/forget_follower";


		==== Authorization

		If the {es} {security-features} are enabled, you must have `manage_follow_index`

Introduce forget follower API #39718

Introduce forget follower API #39718

Uh oh!

Conversation

jasontedor commented Mar 5, 2019

Uh oh!

elasticmachine commented Mar 5, 2019

Uh oh!

martijnvg 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martijnvg commented Mar 6, 2019

Uh oh!

jasontedor commented Mar 7, 2019

Uh oh!

jasontedor commented Mar 7, 2019

Uh oh!

jasontedor commented Mar 7, 2019

Uh oh!

martijnvg left a comment

Choose a reason for hiding this comment

Uh oh!

jasontedor commented Mar 7, 2019

Uh oh!

jasontedor commented Mar 7, 2019

Uh oh!

Uh oh!