Add METADATA_READ and METADATA_WRITE blocks #9203

tlrx · 2015-01-08T17:32:57Z

While looking at #3703, it looks like the usage of 'read_only' is a bit confusing.

The documentation stipulates:

index.blocks.read_only
Set to true to have the index read only, false to allow writes and metadata changes.

Actually, it blocks all write operations (creating, indexing or deleting a document) but also all read/write operations on index metadata. This can lead to confusion because subsequent 'indices exists' requests will return an undocumented 403 Forbidden response code where one could expect a 200 OK (as reported in #3703). Same for 'type exists' and 'alias exists' requests.

Similar issues have been reported:

_status index.blocks.read_only prevents _status #2833 (deprecated in 1.2.0)
_stats Don't block stats for read-only indices #5876
/_cat/indices ClusterBlockException on /_cat/indices/ on readonly indices #5855

But several API calls returns a 403 when at least one index of the cluster has 'index.blocks.read_only' set to true:

DELETE /_all

POST /library/book/1
{
  "title": "Elasticsearch: the definitive guide"
}

POST /movies/dvd/1
{
  "title": "Elasticsearch: the movie"
}

PUT /library/_settings
{
  "index": {
    "blocks.read_only": true
  }
}

The following calls will return 403 / "ClusterBlockException[blocked by: [FORBIDDEN/5/index read-only (api)];]":

HEAD /library
HEAD /library/book
HEAD /_all/dvd

GET /library/_settings
GET /library/_mappings
GET /library/_warmers
GET /library/_aliases

GET /_settings
GET /_aliases
GET /_mappings
GET /_cat/indices

I agree to @imotov's comment (see #5876 (comment)) and I think we should split the METADATA blocks in METADATA_READ & METADATA_WRITE blocks. This way the 'read_only' setting will block all write operations and metadata updates but the metadata will still be available.

This is a first step before updating all other actions then removing METADATA block completely.

Closes #3703
Closes #10521
Closes #10522
Related to #8102

tlrx · 2015-01-19T10:29:47Z

@imotov @s1monw Can you have a look please? :)

…h#9203 is open

tlrx · 2015-01-26T08:24:56Z

For what I've seen:
index.blocks.read_only: if true, blocks index writes and metadata reads/writes
index.blocks.read: if true, blocks index reads and allows metadata reads/writes
index.blocks.write: if true, blocks index writes and allows metadata reads/writes

With this behavior, it's not possible to block index writes + metadata writes and allows index reads + metadata reads at the same time.

@kimchy can you have a look please? Specially regarding your comment #5876 (comment) . I find this change useful but maybe it's just me.

kimchy · 2015-02-24T16:06:43Z

looking at this change, won't it be confusing to have METADATA (which is deprecated) and METADATA_READ + METADATA_WRITE while METADATA is still used across the codebase?

I mean, I think we should either remove all usages of METADATA from the codebase, and only parse it for backward comp and resulting in 2 blocks MD_READ+MD_WRITE, and then go over all the places we use it, and use the equivalent MD_READ or MD_WRITE?

s1monw · 2015-03-24T09:33:01Z

@tlrx are you picking this up again?

tlrx · 2015-03-24T09:33:49Z

@s1monw yes, I asked @javanna to give me his opinion

javanna · 2015-03-24T09:34:10Z

I confirm, it's on my list :)

javanna · 2015-03-25T10:42:22Z

rest-api-spec/test/indices.exists/20_read_only_index.yaml

+      indices.exists:
+        index: test_index_ro
+
+  - is_true: ''


can you remind me what this assertion does? do we really need it here?

It's needed to check the result of the previous 'indices.exists' request.

From here:

is_true: '' means the response had no body but the client returned true (caused by 200)

alright thanks :)

javanna · 2015-03-25T10:56:10Z

looks good, I left a few comments. I do agree with @kimchy on moving the whole codebase to the new blocks and maintain the old one only for bw comp, where METADATA just gets translated to METADATA_READ and METADATA_WRITE. That would isolate the bw comp aspect even more.

javanna · 2015-03-26T08:06:27Z

src/main/java/org/elasticsearch/cluster/block/ClusterBlockLevel.java


-    public static final EnumSet<ClusterBlockLevel> ALL = EnumSet.of(READ, WRITE, METADATA);
+    /**
+     * After 1.5.0 METADATA has been splitted into two distincts cluster block levels


I would be more precise on the version, cause it's not clear if it is from 1.5.1 or 1.6.0.

imotov · 2015-04-10T23:50:53Z

src/main/java/org/elasticsearch/action/admin/indices/optimize/TransportOptimizeAction.java

@@ -121,11 +121,11 @@ protected GroupShardsIterator shards(ClusterState clusterState, OptimizeRequest

    @Override
    protected ClusterBlockException checkGlobalBlock(ClusterState state, OptimizeRequest request) {
-        return state.blocks().globalBlockedException(ClusterBlockLevel.METADATA);
+        return state.blocks().globalBlockedException(ClusterBlockLevel.METADATA_WRITE);


I am always getting confused by this one and refresh. Shouldn't this be WRITE and not METADATA_WRITE? We don't really change any metadata here.

I don't think optimize should be WRITE. It is an administrative type action, you aren't actually passing data, just telling the system to do something.

METADATA_READ then?

When I updated this block I supposed that users want to block refresh/optimize actions on a read only cluster so I set METADATA_WRITE. Anyway I don't have a strong opinion on this and METADATA_READ is fine to me since it will still be blockable with the index.blocks.metadata setting.

IMO it is write because the cluster is changing (an action is happening in the background), and metadata because it is not changing data (it is changing the underlying storage, but from the user perspective the data is not changing).

I don't have strong feelings about it. I just find it confusing.

Can we agree on letting METADATA_WRITE ?

+1 on keeping it METADATA_WRITE

I agree this is confusing. Given how we classified operations in this PR, I think this one is more under metadata_write than metadata_read. We might need to adress this confusion at some point though (same with snapshots). So +1 on leaving METADATA_WRITE.

imotov · 2015-04-10T23:56:28Z

Left a couple of comments. Otherwise LGTM.

tlrx · 2015-04-13T08:33:32Z

Thanks @imotov and @javanna for your reviews. I made few changes according to your last comments.

Concerning #8102 I think it should be the subject of a dedicated pull request... I'd like to know more how restoring an index works when blocks are enabled and moving the cluster state from metadata to custom is not so simple I guess? Since this pull request doesn't change the current behavior I think it could be merged as it.

javanna · 2015-04-16T08:08:48Z

src/main/java/org/elasticsearch/cluster/block/ClusterBlock.java

@@ -131,7 +131,7 @@ public void readFrom(StreamInput in) throws IOException {
        final int len = in.readVInt();
        ArrayList<ClusterBlockLevel> levels = new ArrayList<>();
        for (int i = 0; i < len; i++) {


Given that we changed the serialization code for this class, would it be possible to write a small unit test that verifies it? We already have similar *RequestTests#testSerialization tests that randomize the version and make sure that everything is fine.

imotov · 2015-04-16T09:01:04Z

@tlrx Sorry, missed the restore one the last time around. Otherwise, I think it's good.

tlrx · 2015-04-17T14:10:25Z

@javanna I just added the test, can you please have a look? @imotov no worries, thanks for your review.

I added another check on WRITE block on the empty "" index in the TransportRestoreSnapshotAction. I think that there's no need to check for WRITE block for the indices specified in the request as @javanna suggested in this comment because the indices are necessarily in closed state and so WRITE are de facto blocked.

javanna · 2015-04-21T11:13:11Z

src/test/java/org/elasticsearch/cluster/block/ClusterBlockTests.java

+            Version fromVersion = randomVersion();
+            Version toVersion = randomVersion();
+
+            ClusterBlockLevel[] blockLevels = ALL_CLUSTER_BLOCK_LEVELS;


ClusterBlockLevel.values() ? don't think we need the constant

javanna · 2015-04-21T11:34:19Z

The new serialization test looks good, left a couple of minor comments. When you backport to 1.x, can you double check that our existing bw comp tests already cover some potential bw comp problems here (e.g. ClusterBlocks gets serialized and everything is fine)? We didn't write a specific bw comp test but only a unit test for serialization, I think this is enough but I didn't verify it.

Other than that LGTM.

javanna · 2015-04-22T14:54:04Z

LGTM @tlrx thanks!!!

This commit splits the current ClusterBlockLevel.METADATA into two disctins ClusterBlockLevel.METADATA_READ and ClusterBlockLevel.METADATA_WRITE blocks. It allows to make a distinction between an operation that modifies the index or cluster metadata and an operation that does not change any metadata. Before this commit, many operations where blocked when the cluster was read-only: Cluster Stats, Get Mappings, Get Snapshot, Get Index Settings, etc. Now those operations are allowed even when the cluster or the index is read-only. Related to elastic#8102, elastic#2833 Closes elastic#3703 Closes elastic#5855 Closes elastic#10521 Closes elastic#10522

tlrx · 2015-04-23T13:26:56Z

This pr has been merged in master and 1.x. For this latest branch I added *BlocksTests for the IndicesStatus and DeleteMapping operations which are removed in master branch.

There's a bwc test for cluster state updates but it doesn't use any blocks. I'll add it one in a dedicated pull request.

Thanks again @javanna and @imotov for your very helpful reviews.

This commit removes the deprecated ClusterBlockLevel.METADATA, replaced in elastic#9203 with METADATA_READ and METADATA_WRITE.

tlrx added the review label Jan 8, 2015

jdeniau mentioned this pull request Jan 19, 2015

Elasticsearch 1.4.2 ruflin/Elastica#738

Merged

jdeniau added a commit to jdeniau/Elastica that referenced this pull request Jan 21, 2015

Hacky way to avoid block on metadata check while elastic/elasticsearc…

07b56f6

…h#9203 is open

tlrx added the v2.0.0-beta1 label Feb 9, 2015

imotov mentioned this pull request Feb 20, 2015

snapshot should work when cluster is in read_only mode. #8102

Closed

drewr force-pushed the master branch from dcc3da0 to 7c20a8a Compare February 20, 2015 16:48

s1monw assigned imotov Mar 24, 2015

s1monw assigned javanna and unassigned imotov Mar 24, 2015

javanna reviewed Mar 25, 2015
View reviewed changes

javanna removed the review label Mar 25, 2015

javanna reviewed Mar 26, 2015
View reviewed changes

javanna assigned imotov Apr 10, 2015

tlrx mentioned this pull request Apr 10, 2015

Internal: replace empty index block checks with global block checks #10530

Closed

imotov reviewed Apr 10, 2015
View reviewed changes

imotov removed their assignment Apr 10, 2015

javanna reviewed Apr 16, 2015
View reviewed changes

javanna reviewed Apr 21, 2015
View reviewed changes

tlrx force-pushed the blocks-metadata-read-write branch from 2d8f854 to adc0807 Compare April 23, 2015 13:20

tlrx merged commit adc0807 into elastic:master Apr 23, 2015

kevinkluge removed the review label Apr 23, 2015

tlrx mentioned this pull request Apr 23, 2015

Tests: Add backward compatibility test for cluster state with blocks #10754

Merged

tlrx added a commit to tlrx/elasticsearch that referenced this pull request Apr 24, 2015

Internal: Remove deprecated METADATA cluster block level

fd4ec43

This commit removes the deprecated ClusterBlockLevel.METADATA, replaced in elastic#9203 with METADATA_READ and METADATA_WRITE.

tlrx mentioned this pull request Apr 24, 2015

Remove deprecated METADATA cluster block level #10779

Merged

tlrx added a commit to tlrx/elasticsearch that referenced this pull request Apr 28, 2015

Internal: Remove deprecated METADATA cluster block level

2ce0ea1

This commit removes the deprecated ClusterBlockLevel.METADATA, replaced in elastic#9203 with METADATA_READ and METADATA_WRITE.

tlrx deleted the blocks-metadata-read-write branch April 28, 2015 12:58

clintongormley mentioned this pull request May 25, 2015

upgrading a read-only index causes ClusterBlockException #11286

Closed

clintongormley changed the title ~~Internal: Add METADATA_READ and METADATA_WRITE blocks~~ Add METADATA_READ and METADATA_WRITE blocks May 29, 2015

clintongormley added :Cluster and removed :Internal labels Jun 7, 2015

clintongormley added :Distributed Indexing/Distributed A catch all label for anything in the Distributed Area. Please avoid if you can. and removed :Cluster labels Feb 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add METADATA_READ and METADATA_WRITE blocks #9203

Add METADATA_READ and METADATA_WRITE blocks #9203

tlrx commented Jan 8, 2015

tlrx commented Jan 19, 2015

tlrx commented Jan 26, 2015

kimchy commented Feb 24, 2015

s1monw commented Mar 24, 2015

tlrx commented Mar 24, 2015

javanna commented Mar 24, 2015

javanna Mar 25, 2015

tlrx Mar 25, 2015

javanna Mar 25, 2015

javanna commented Mar 25, 2015

javanna Mar 26, 2015

imotov Apr 10, 2015

rjernst Apr 12, 2015

imotov Apr 13, 2015

tlrx Apr 13, 2015

rjernst Apr 13, 2015

imotov Apr 13, 2015

tlrx Apr 15, 2015

imotov Apr 15, 2015

javanna Apr 16, 2015

imotov commented Apr 10, 2015

tlrx commented Apr 13, 2015

javanna Apr 16, 2015

imotov commented Apr 16, 2015

tlrx commented Apr 17, 2015

javanna Apr 21, 2015

javanna commented Apr 21, 2015

javanna commented Apr 22, 2015

tlrx commented Apr 23, 2015

Add METADATA_READ and METADATA_WRITE blocks #9203

Add METADATA_READ and METADATA_WRITE blocks #9203

Conversation

tlrx commented Jan 8, 2015

tlrx commented Jan 19, 2015

tlrx commented Jan 26, 2015

kimchy commented Feb 24, 2015

s1monw commented Mar 24, 2015

tlrx commented Mar 24, 2015

javanna commented Mar 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna commented Mar 25, 2015

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

imotov commented Apr 10, 2015

tlrx commented Apr 13, 2015

Choose a reason for hiding this comment

imotov commented Apr 16, 2015

tlrx commented Apr 17, 2015

Choose a reason for hiding this comment

javanna commented Apr 21, 2015

javanna commented Apr 22, 2015

tlrx commented Apr 23, 2015