Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable avoiding mmap bootstrap check #32421

Merged
merged 14 commits into from
Aug 21, 2018

Conversation

jasontedor
Copy link
Member

@jasontedor jasontedor commented Jul 27, 2018

The maximum map count boostrap check can be a hindrance to users that do not own the underlying platform on which they are executing Elasticsearch. This is because addressing it requires tuning the kernel and a platform provider might now allow this, especially on shared infrastructure. However, this bootstrap check is not needed if mmapfs is not in use. Today we do not have a way for the user to communicate that they are not going to use mmapfs. This commit therefore adds a setting that enables the user to disallow mmapfs. When mmapfs is disallowed, the maximum map count bootstrap check is not enforced. Additionally, we fallback to a different default index store and prevent the explicit use of mmapfs for an index.

Closes #32267

The maximum map count boostrap check can be a hindrance to users that do
not own the underlying platform on which they are executing
Elasticsearch. This is because addressing it requires tuning the kernel
and a platform provider might now allow this, especially on shared
infrastructure. However, this bootstrap check is not needed if mmapfs is
not in use. Today we do not have a way for the user to communicate that
they are not going to use mmapfs.  This commit therefore adds a setting
that enables the user to disallow mmapfs. When mmapfs is disallowed, the
maximum map count bootstrap check is not enforced.

This change presents an interesting problem though. The default store
type is mmapfs. If a user does not specify a store type, then mmapfs is
used. This means that a user should create all indices with a specific
store type, say niofs. Normally we would tell such users to add a global
index template that specifies a top-level match and sets
index.store.type. Now we run into a problem though if a user is using a
plugin that adds templates on startup (X-Pack is an example of
this). These templates do not (and will not) specify an
index.store.type. This means that they will fail template validation
because we create a dummy index service to ensure the template is
valid. However, we do not inherit other templates during validation so
even if the user has a global template that specifies a default
index.store.type, this will not be picked up during template
validation. To get around this, we do something unusual for us which is
add a node-level setting that controls a default index setting. Namely,
we add here node.default_index_store_type. It is a smell that we are
doing something that is unusual for us. If we had proper template
validation, we would not need to do this. An alternative would be to
have template validation skip validating index.store.type via a
hack. This felt too hacky to me so I elected to take the default index
store type route although I am open to other ideas.
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

* master:
  Fix ordering of bootstrap checks in docs (elastic#32417)
  [TEST] Mute failing InternalEngineTests#testSeqNoAndCheckpoints
  [TEST] Mute failing testConvertLongHexError
  bump lucene version after backport
  Upgrade to Lucene-7.5.0-snapshot-608f0277b0 (elastic#32390)
  [Kerberos] Avoid vagrant update on precommit (elastic#32416)
* master:
  Remove reference to non-existent store type (elastic#32418)
  [TEST] Mute failing FlushIT test
Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks good overall but I have some (minor yet existing) reluctance to solve this problem with two settings rather than 1, such as something like node.store.allow_mmapfs. If you like the 2-settings way better then let's do it, I mostly wanted to make sure that you considered doing it with one setting as well.

@jibbe
Copy link

jibbe commented Aug 2, 2018

@jasontedor @jpountz what about just having the single node.default_index_store_type config and make the mmap check remove mmap from the inbuilt list if it doesn't pass? this would auto configure that way.

@jasontedor
Copy link
Member Author

@jibbe An issue with only changing the default via a setting is that it would still allow someone to explicitly set the store type on indices to mmapfs which means that we would still want to enforce the bootstrap check.

@jibbe
Copy link

jibbe commented Aug 5, 2018

@jasontedor that makes sense!

…e-types

* elastic/master: (199 commits)
  Watcher: Remove unused hipchat render method (elastic#32211)
  Watcher: Remove extraneous auth classes (elastic#32300)
  Watcher: migrate PagerDuty v1 events API to v2 API (elastic#32285)
  [TEST] Select free port for Minio (elastic#32837)
  MINOR: Remove `IndexTemplateFilter` (elastic#32841)
  Core: Add java time version of rounding classes (elastic#32641)
  Aggregations/HL Rest client fix: missing scores (elastic#32774)
  HLRC: Add Delete License API (elastic#32586)
  INGEST: Create Index Before Pipeline Execute (elastic#32786)
  Fix NOOP bulk updates (elastic#32819)
  Remove client connections from TcpTransport (elastic#31886)
  Increase logging testRetentionPolicyChangeDuringRecovery
  AwaitsFix case-functions.sql-spec
  Mute security-cli tests in FIPS JVM (elastic#32812)
  SCRIPTING: Support BucketAggScript return null (elastic#32811)
  Unmute WildFly tests in FIPS JVM (elastic#32814)
  [TEST] Force a stop to save rollup state before continuing (elastic#32787)
  [test] disable packaging tests for suse boxes
  Mute IndicesRequestIT#testBulk
  [ML][DOCS] Refer to rules feature as custom rules (elastic#32785)
  ...
@jasontedor
Copy link
Member Author

@jpountz I have taken your suggestion and updated the PR. Would you review?

@jasontedor
Copy link
Member Author

@elasticmachine test this please

…e-types

* elastic/master: (89 commits)
  Fix assertion in AbstractSimpleTransportTestCase (elastic#32991)
  [DOC] Splits role mapping APIs into separate pages (elastic#32797)
  HLRC: ML Close Job (elastic#32943)
  Generalize remote license checker (elastic#32971)
  Trim translog when safe commit advanced (elastic#32967)
  Fix an inaccuracy in the dynamic templates documentation. (elastic#32890)
  Logging: Use settings when building daemon threads (elastic#32751)
  All Translog inner closes should happen after tragedy exception is set (elastic#32674)
  HLREST: AwaitsFix ML Test
  Pass DiscoveryNode to initiateChannel (elastic#32958)
  Add mzn and dz to unsupported locales (elastic#32957)
  Use settings from the context in BootstrapChecks (elastic#32908)
  Update docs for node specifications (elastic#30468)
  HLRC: Forbid all Elasticsearch logging infra (elastic#32784)
  Only configure publishing if it's applied externally (elastic#32351)
  Fixes libs:dissect when in eclipse
  Protect ScriptedMetricIT test cases against failures on 0-doc shards (elastic#32959) (elastic#32968)
  [Kerberos] Add documentation for Kerberos realm (elastic#32662)
  Watcher: Properly find next valid date in cron expressions (elastic#32734)
  Fix some small issues in the getting started docs (elastic#30346)
  ...
Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need the node.allowed_index_store_types setting?

public static final Setting<Boolean> NODE_STORE_ALLOW_MMAPFS =
Setting.boolSetting("node.store.allow_mmapfs", true, Property.NodeScope);

public static final Setting<List<String>> NODE_ALLOWED_INDEX_STORE_TYPES_SETTING =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was expecting that this setting would go away?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, that is leftover. It is indeed not used anywhere. Sorry for that. I will remove shortly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jpountz I pushed bb9f1f2.

@@ -228,6 +228,14 @@ public void onRemoval(ShardId shardId, String fieldName, boolean wasEvicted, lon
this.cacheCleaner = new CacheCleaner(indicesFieldDataCache, indicesRequestCache, logger, threadPool, this.cleanInterval);
this.metaStateService = metaStateService;
this.engineFactoryProviders = engineFactoryProviders;

// do not allow any plugin-provided index store type to conflict with a built-in type
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @jasontedor !

@jasontedor jasontedor merged commit bdfcc32 into elastic:master Aug 21, 2018
jasontedor added a commit that referenced this pull request Aug 21, 2018
The maximum map count boostrap check can be a hindrance to users that do
not own the underlying platform on which they are executing
Elasticsearch. This is because addressing it requires tuning the kernel
and a platform provider might now allow this, especially on shared
infrastructure. However, this bootstrap check is not needed if mmapfs is
not in use. Today we do not have a way for the user to communicate that
they are not going to use mmapfs. This commit therefore adds a setting
that enables the user to disallow mmapfs. When mmapfs is disallowed, the
maximum map count bootstrap check is not enforced. Additionally, we
fallback to a different default index store and prevent the explicit use
of mmapfs for an index.
@jasontedor jasontedor deleted the allowed-index-store-types branch August 21, 2018 15:27
@jasontedor
Copy link
Member Author

Thanks a lot for reviewing @jpountz.

@rubin55
Copy link

rubin55 commented Nov 8, 2018

@jasontedor is this feature in (released) Elasticsearch 6.4.3?

@jasontedor
Copy link
Member Author

@rubin55 No, we intend that it will be available in 6.5.0, but that is subject to change (please check the release notes when 6.5.0 is available). Anticipating your next question: sorry, but we do not provide any indication of release dates. I know that can be frustrating, but I hope that you understand. 😇

@rubin55
Copy link

rubin55 commented Nov 8, 2018

@jasontedor Alright, are builds made available of pre-release versions for testing purposes? Since the change is merged to master and 6.5 branch (I think) I would expect a build of master or the 6.5 branch to contain this, would be nice to test!

@jasontedor
Copy link
Member Author

jasontedor commented Nov 8, 2018

@rubin55 You can use a nightly snapshot: use the Docker image docker.elastic.co/elasticsearch/elasticsearch:6.5.0-SNAPSHOT that includes this functionality. Thank you for your interest, and I wish for successful testing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants