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

Adds a warning about manually mounting snapshots managed by ILM #111883

Merged

Conversation

kosabogi
Copy link
Contributor

@kosabogi kosabogi commented Aug 14, 2024

Overview

This PR introduces a warning to the documentation for the Mount Snapshot API.

Related issue: #105816

Preview

Searchable snapshots
Mount snapshot API

Copy link
Contributor

Documentation preview:

@elasticsearchmachine elasticsearchmachine added v8.16.0 needs:triage Requires assignment of a team area label external-contributor Pull request authored by a developer outside the Elasticsearch team labels Aug 14, 2024
@gbanasiak gbanasiak added the :Data Management/ILM+SLM Index and Snapshot lifecycle management label Aug 14, 2024
@elasticsearchmachine elasticsearchmachine added the Team:Data Management Meta label for data/management team label Aug 14, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-data-management (Team:Data Management)

@elasticsearchmachine elasticsearchmachine removed the needs:triage Requires assignment of a team area label label Aug 14, 2024
Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

I'd rather we didn't add warnings like this to the top of a page. They tend to accumulate, and it makes the docs very hostile to users who are looking for information about how to use the system rather than how not to do so.

Would it make sense to add this information to the other warnings on the page about searchable snapshots: https://www.elastic.co/guide/en/elasticsearch/reference/current/searchable-snapshots.html#searchable-snapshots-reliability

@DaveCTurner DaveCTurner added the >docs General docs changes label Aug 14, 2024
@elasticsearchmachine elasticsearchmachine added the Team:Docs Meta label for docs team label Aug 14, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-docs (Team:Docs)

Copy link
Contributor

@shainaraskas shainaraskas left a comment

Choose a reason for hiding this comment

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

quick drive-by review with some formatting and style recos :)

Comment on lines 10 to 12
WARNING: Manual Snapshot Mounting
====
When manually mounting a snapshot that was captured by an Index Lifecycle Management (ILM) policy, be aware of the following:
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you can set the title this way. it ends up just putting this sentence in the note block:

image

I think what you want is this

Suggested change
WARNING: Manual Snapshot Mounting
====
When manually mounting a snapshot that was captured by an Index Lifecycle Management (ILM) policy, be aware of the following:
[WARNING]
====
**Manual snapshot mounting**

Comment on lines 14 to 20
* **ILM Managed Snapshots:** Snapshots taken by ILM policies are handled automatically by ILM. If you manually mount these snapshots, it can interfere with ILM's automated processes, such as managing or deleting snapshots.

* **Potential Issues:** Manually mounting a snapshot can disrupt ILM actions, like deletions or phase transitions. This can lead to data loss or complications in managing your snapshots.

* **Best Practice:** It's best to let ILM manage your snapshots. If you must manually mount a snapshot, make sure you understand how this affects ILM and manage the snapshot lifecycle separately.

Always review the documentation and consider the impact on your data management before manually handling snapshots managed by ILM.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to see this formatted as prose rather than broken down like this. there also seems to be overlap between all of the paragraph content so you could slim this down. it will have an added benefit of making the warning much less scary looking.

consider also shrinking this to "don't use this for snapshots taken by ILM policies. [Learn more]" and linking out to the docs around how ILM snapshots are managed (and maybe adding these considerations there).

finally, we prefer sentence case headings over title case headings. see style guide :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the feedback!
I’ve shortened the warning to make it clearer and fixed the formatting issues. I encountered some difficulties with the local build but have resolved those issues now thanks to @szabosteve :)
Based on Dave's feedback, I’ve moved the warning to the section about Searchable Snapshots, where it complements the existing information. In the Mount API description, I’ve added a brief note that links to the warning for further details.

@szabosteve szabosteve added the WIP label Aug 15, 2024
@DaveCTurner DaveCTurner dismissed their stale review August 15, 2024 12:28

comments addressed

@kosabogi
Copy link
Contributor Author

@DaveCTurner Thank you for the feedback! I’ve moved the warning to the section about Searchable Snapshots, where it complements the existing information. In the Mount API description, I’ve added a brief note that links to the warning for further details.

Copy link
Contributor

@szabosteve szabosteve left a comment

Choose a reason for hiding this comment

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

LGTM! 🚢

@kosabogi kosabogi merged commit e7518fb into elastic:main Aug 15, 2024
5 checks passed
Copy link
Contributor

@shainaraskas shainaraskas left a comment

Choose a reason for hiding this comment

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

couple of comments for your consideration!

@@ -23,6 +23,8 @@ For more information, see <<security-privileges>>.
[[searchable-snapshots-api-mount-desc]]
==== {api-description-title}

This API mounts a snapshot as a searchable snapshot index.
Note that manually mounting {ilm-init}-managed snapshots can <<manually-mounting-snapshots,interfere>> with <<index-lifecycle-management,{ilm-init} processes>>.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd drop this in as a separate paragraph so it's more visible. Also, avoid "note that" - either it should be a callout, or it's just a sentence. technically all documentation is just noting things.

I think that because this can cause problems, and we want to discourage people from using this API in the context of ILM-managed snapshots, I'd say that a little more clearly as well.

Suggested change
Note that manually mounting {ilm-init}-managed snapshots can <<manually-mounting-snapshots,interfere>> with <<index-lifecycle-management,{ilm-init} processes>>.
Don't use this API for snapshots managed by {ilm-init}. Manually mounting {ilm-init}-managed snapshots can <<manually-mounting-snapshots,interfere>> with <<index-lifecycle-management,{ilm-init} processes>>.

Comment on lines +176 to +182
====
Manually mounting snapshots captured by an Index Lifecycle Management ({ilm-init}) policy can
interfere with {ilm-init}'s automatic management. This may lead to issues such as data loss
or complications with snapshot handling. For optimal results, allow {ilm-init} to manage
snapshots automatically. If manual mounting is necessary, be aware of its potential
impact on {ilm-init} processes. For more information, learn about <<index-lifecycle-management,{ilm-init} snapshot management>>.
====
Copy link
Contributor

Choose a reason for hiding this comment

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

consider breaking this into a couple of paragraphs. I'm also unclear on whether the complications you mentioned in your last draft ("can disrupt ILM actions, like deletions or phase transitions") is totally covered by this statement.

I'd avoid restating the problem, as you do in the "if you need to mount manually" sentence. it seems to imply that there are impacts not listed here. if it is meant to imply that, we should probably list the additional impacts as I don't think they're captured elsewhere.

finally, I don't think your link is ideal - there's no snapshot info on that top level ILM page. Consider a different link, or changing your link text to refer to just ILM.

Suggested change
====
Manually mounting snapshots captured by an Index Lifecycle Management ({ilm-init}) policy can
interfere with {ilm-init}'s automatic management. This may lead to issues such as data loss
or complications with snapshot handling. For optimal results, allow {ilm-init} to manage
snapshots automatically. If manual mounting is necessary, be aware of its potential
impact on {ilm-init} processes. For more information, learn about <<index-lifecycle-management,{ilm-init} snapshot management>>.
====
====
Manually mounting snapshots captured by an Index Lifecycle Management ({ilm-init}) policy can
interfere with {ilm-init}'s automatic snapshot management. This may lead to issues such as data loss
or complications with snapshot handling.
For optimal results, allow {ilm-init} to manage
snapshots automatically.
<<index-lifecycle-management,Learn more about {ilm-init} snapshot management>>.
====

weizijun added a commit to weizijun/elasticsearch that referenced this pull request Aug 16, 2024
* upstream/main: (91 commits)
  Mute org.elasticsearch.xpack.test.rest.XPackRestIT org.elasticsearch.xpack.test.rest.XPackRestIT elastic#111944
  Add audit_unenrolled_* attributes to fleet-agents template (elastic#111909)
  Fix windows memory locking (elastic#111866)
  Update OAuth2 OIDC SDK (elastic#108799)
  Adds a warning about manually mounting snapshots managed by ILM (elastic#111883)
  Update geoip fixture files and utility methods (elastic#111913)
  Updated Function Score Query Test with Explain Fixes for 8.15.1 (elastic#111929)
  Mute org.elasticsearch.xpack.sql.qa.security.JdbcCsvSpecIT org.elasticsearch.xpack.sql.qa.security.JdbcCsvSpecIT elastic#111923
  [ESQL] date nanos binary comparisons (elastic#111908)
  [DOCS] Documents output_field behavior after multiple inference runs (elastic#111875)
  Add additional BlobCacheMetrics, expose BlobCacheMetrics via SharedBlobCacheService (elastic#111730)
  Mute org.elasticsearch.xpack.sql.qa.multi_cluster_with_security.JdbcCsvSpecIT org.elasticsearch.xpack.sql.qa.multi_cluster_with_security.JdbcCsvSpecIT elastic#111923
  Mute org.elasticsearch.xpack.sql.qa.multi_cluster_with_security.JdbcCsvSpecIT test {agg-ordering.testHistogramDateTimeWithCountAndOrder_2} elastic#111919
  Mute org.elasticsearch.xpack.sql.qa.multi_cluster_with_security.JdbcCsvSpecIT test {date.testDateParseHaving} elastic#111921
  Mute org.elasticsearch.xpack.sql.qa.multi_cluster_with_security.JdbcCsvSpecIT test {agg-ordering.testHistogramDateTimeWithCountAndOrder_1} elastic#111918
  Mute org.elasticsearch.xpack.sql.qa.multi_cluster_with_security.JdbcCsvSpecIT test {datetime.testDateTimeParseHaving} elastic#111922
  Mute org.elasticsearch.xpack.sql.qa.single_node.JdbcCsvSpecIT org.elasticsearch.xpack.sql.qa.single_node.JdbcCsvSpecIT elastic#111923
  Mute org.elasticsearch.xpack.sql.qa.single_node.JdbcCsvSpecIT test {agg-ordering.testHistogramDateTimeWithCountAndOrder_1} elastic#111918
  Mute org.elasticsearch.xpack.sql.qa.single_node.JdbcCsvSpecIT test {datetime.testDateTimeParseHaving} elastic#111922
  Mute org.elasticsearch.xpack.sql.qa.single_node.JdbcCsvSpecIT test {date.testDateParseHaving} elastic#111921
  ...

# Conflicts:
#	server/src/main/java/org/elasticsearch/TransportVersions.java
cbuescher pushed a commit to cbuescher/elasticsearch that referenced this pull request Sep 4, 2024
…tic#111883)

* Adds a warning about manually mounting snapshots managed by ILM

* Shortens text and moves the warning to Searchable snapshots chapter
davidkyle pushed a commit to davidkyle/elasticsearch that referenced this pull request Sep 5, 2024
…tic#111883)

* Adds a warning about manually mounting snapshots managed by ILM

* Shortens text and moves the warning to Searchable snapshots chapter
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Data Management/ILM+SLM Index and Snapshot lifecycle management >docs General docs changes external-contributor Pull request authored by a developer outside the Elasticsearch team Team:Data Management Meta label for data/management team Team:Docs Meta label for docs team v8.16.0 WIP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants