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.

Sign up for GitHub

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

Jump to bottom

[UA] Use new _create_from ES API #207114

Merged
merged 3 commits into from
Jan 21, 2025
Merged

[UA] Use new _create_from ES API #207114

merged 3 commits into from
Jan 21, 2025

Conversation

afharo
Copy link
Member

@afharo afharo commented Jan 17, 2025 •
edited by kibanamachine
Loading

Summary

Resolves #205941.

Checklist

@afharo afharo added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc release_note:skip Skip the PR/issue when compiling release notes backport:prev-minor Backport to (9.0) the previous minor version (i.e. one version back from main) labels Jan 17, 2025
@afharo afharo self-assigned this Jan 17, 2025
@afharo
Copy link
Member Author

afharo commented Jan 20, 2025 •
edited
Loading

@jloleysens, we can't use this API because it copies settings such as write blocks and frozen flags.

We may need to stick to our current creation flow.

Nvm! I found out that ES sets a few known settings to null to remove them from the target index: https://github.com/elastic/elasticsearch/blob/9c0709f386fee4154e930cb61a02868adebe8572/x-pack/plugin/migrate/src/main/java/org/elasticsearch/xpack/migrate/action/ReindexDataStreamIndexTransportAction.java#L195-L210

I've applied the same approach here.

@rudolf
Copy link
Contributor

rudolf commented Jan 20, 2025

I think we should check with the ES team, I would have expected their API to take care of removing these settings

@rudolf rudolf mentioned this pull request Jan 20, 2025
Open
@afharo afharo marked this pull request as ready for review January 20, 2025 16:27
@afharo afharo requested a review from a team as a code owner January 20, 2025 16:27
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@afharo
Copy link
Member Author

afharo commented Jan 20, 2025

I think we should check with the ES team, I would have expected their API to take care of removing these settings

@rudolf, I asked via Slack. I wonder if we should move ahead for now (since ES does the same), and remove those 3 lines if the API changes.

WDYT?

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

cc @afharo

jloleysens
jloleysens approved these changes Jan 21, 2025
View reviewed changes
Copy link
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

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

Did not test locally, but the code changes LGTM. Approving to unblock progress.

We are still awaiting confirmation from ES (whether they will default remove write block settings), but, my 2c, I don't think we need to block this PR. We can remove our overrides/comments in a follow up if they turn out to be the defaults, otherwise we got confirmation that this is the right approach. In the meantime we can exercise the _create_from flow. Up to you though @afharo !

x-pack/platform/plugins/private/upgrade_assistant/server/lib/reindexing/reindex_service.ts
body: {
// Settings overrides copied from ES datastream reindex logic
// https://github.com/elastic/elasticsearch/blob/9c0709f386fee4154e930cb61a02868adebe8572/x-pack/plugin/migrate/src/main/java/org/elasticsearch/xpack/migrate/action/ReindexDataStreamIndexTransportAction.java#L195-L210
settings_override: {
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is a little less than ideal, the current design of this API does not allow another option. Thanks for making the link explicit in code!

x-pack/platform/plugins/private/upgrade_assistant/server/lib/reindexing/index_settings.ts
@@ -125,63 +113,3 @@ export const getReindexWarnings = (flatSettings: FlatSettings): ReindexWarning[]

return warnings;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not necessarily related to this PR, but I thought the above "deprecated setting detection" logic could also be avoided. Perhaps it's a holdover from 7 🤷🏻 ?

Copy link
Member Author

@afharo afharo Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

good point! I'll remove it as well.

EDIT: Actually... I was reading the logic wrong... in line 1118, we apply the filter of the current version being 8. If we backport this PR (which we should( and remove that piece of logic, we're removing a check in the current version.

I'd err on the conservative side here and keep it for now until we confirm that all the deprecated settings addressed in getDeprecatedSettingWarning are actually v7-specific.

@afharo afharo merged commit fb8a17b into elastic:main Jan 21, 2025
9 checks passed
@afharo afharo deleted the upgrade_assistant/use-create-from-api branch January 21, 2025 09:29
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/12883953447

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jan 21, 2025
@afharo
[UA] Use new _create_from ES API (elastic#207114)
fb38c7f 
(cherry picked from commit fb8a17b)
@kibanamachine kibanamachine mentioned this pull request Jan 21, 2025
Merged
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
✅ 8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Jan 21, 2025
@kibanamachine @afharo
[8.x] [UA] Use new `_create_from` ES API (#207114) (#207304)
1dfbcdb 
# Backport

This will backport the following commits from `main` to `8.x`:
- [[UA] Use new `_create_from` ES API
(#207114)](#207114)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Alejandro Fernández
Haro","email":"alejandro.haro@elastic.co"},"sourceCommit":{"committedDate":"2025-01-21T09:29:51Z","message":"[UA]
Use new `_create_from` ES API
(#207114)","sha":"fb8a17ba04f493cd5a0d0ba33c002750150fd0af","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Team:Core","release_note:skip","v9.0.0","backport:prev-minor"],"title":"[UA]
Use new `_create_from` ES
API","number":207114,"url":"https://github.com/elastic/kibana/pull/207114","mergeCommit":{"message":"[UA]
Use new `_create_from` ES API
(#207114)","sha":"fb8a17ba04f493cd5a0d0ba33c002750150fd0af"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/207114","number":207114,"mergeCommit":{"message":"[UA]
Use new `_create_from` ES API
(#207114)","sha":"fb8a17ba04f493cd5a0d0ba33c002750150fd0af"}}]}]
BACKPORT-->

Co-authored-by: Alejandro Fernández Haro <alejandro.haro@elastic.co>
cqliu1 pushed a commit to cqliu1/kibana that referenced this pull request Jan 21, 2025
@afharo @cqliu1
[UA] Use new _create_from ES API (elastic#207114)
435a3df
viduni94 pushed a commit to viduni94/kibana that referenced this pull request Jan 23, 2025
@afharo @viduni94
[UA] Use new _create_from ES API (elastic#207114)
78d630a
@afharo afharo mentioned this pull request Jan 24, 2025
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jloleysens jloleysens jloleysens approved these changes

Assignees

@afharo afharo

Labels
backport:prev-minor Backport to (9.0) the previous minor version (i.e. one version back from main) release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.18.0 v9.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[UA] Adopt new create index from source index API
5 participants
@afharo @rudolf @elasticmachine @kibanamachine @jloleysens