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

[Upgrade Assistant] Removes unnecessary mappings for reindexing #150878

Merged
merged 5 commits into from
Feb 21, 2023

Conversation

yuliacech
Copy link
Contributor

Summary

Partially addresses #64547

This PR removes unnecessary mappings from the saved objects type upgrade-assistant-reindex-operation that holds information about reindex processes.
Currently, the reindexing is disabled via a config for Upgrade Assistant plugin xpack.upgrade_assistant.featureSet.reindexCorrectiveActions. ES also doesn't log any deprecations for indices created since 7.0 and indices created before 7.0 are incompatible with versions 8.0+. That means that when reindexing will be enabled in UA, it needs to be configured for the specific upgrade scenario, for example for 9.0.

How to test

Even though reindexing is currently disabled, I was able to do some testing for the changes. Here is how to test reindexing locally:

  1. Add xpack.upgrade_assistant.featureSet.reindexCorrectiveActions: true to your kibana.dev.yml file
  2. Start ES and Kibana with yarn es snapshot --license=trial and yarn start
  3. Create a test user with the role system_indices_superuser (in Stack Management -> Security -> Users). Log in with that user for further steps.
  4. Create an index via Dev Tools
POST /old_index/_doc
{
  "test": "test"
}
  1. replace deprecations on line 22 in the file /server/lib/es_deprecations_status.ts with
  const deprecations = {
    cluster_settings: [],
    node_settings: [],
    ml_settings: [],
    index_settings: {
      old_index: [
        {
          level: 'critical',
          message: 'Index created before 7.0',
          url: 'https: //www.elastic.co/guide/en/elasticsearch/reference/master/breaking-changes-8.0.html',
          details: 'This index was created using version: 6.8.13',
          resolve_during_rolling_upgrade: false,
        },
      ],
    },
  };
  1. open UA and test the reindexing of old_index

Follow up work

A follow up PR if needed will be opened for saved objects types upgrade-assistant-ml-upgrade-operation and upgrade-assistant-telemetry

@yuliacech yuliacech added Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more release_note:skip Skip the PR/issue when compiling release notes Feature:Upgrade Assistant v8.8.0 labels Feb 10, 2023
@yuliacech yuliacech requested a review from a team as a code owner February 10, 2023 13:33
@elasticmachine
Copy link
Contributor

Pinging @elastic/platform-deployment-management (Team:Deployment Management)

@yuliacech yuliacech requested a review from a team as a code owner February 13, 2023 10:53
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Saved Objects .kibana field count

Every field in each saved object type adds overhead to Elasticsearch. Kibana needs to keep the total field count below Elasticsearch's default limit of 1000 fields. Only specify field mappings for the fields you wish to search on or query. See https://www.elastic.co/guide/en/kibana/master/saved-objects-service.html#_mappings

id before after diff
upgrade-assistant-reindex-operation 18 3 -15

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Contributor

@ElenaStoeva ElenaStoeva left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @yuliacech! Checked out the PR locally and tested reindexing - works fine for me. 👍

Regarding the code changes, I see that some of the deleted fields are used in multiple test files and other files like x-pack/plugins/upgrade_assistant/server/lib/reindexing/reindex_actions.ts for example:

    async createReindexOp(indexName: string, opts?: ReindexOptions) {
      return client.create<ReindexOperation>(REINDEX_OP_TYPE, {
        indexName,
        newIndexName: generateNewIndexName(indexName),
        status: ReindexStatus.inProgress,
        lastCompletedStep: ReindexStep.created,
        locked: null,
        reindexTaskId: null,
        reindexTaskPercComplete: null,
        errorMessage: null,
        runningReindexCount: null,
        reindexOptions: opts,
      });
    },

Should these files be updated as well?

@yuliacech
Copy link
Contributor Author

Thanks a lot for your review, @ElenaStoeva!
The fields this PR removes are only in the mappings, that means the code won't be able to query or search saved objects by this fields. For example, as it's done in the linked file on line 174:

findReindexOperations(indexName: string) {
      return client.find<ReindexOperation>({
        type: REINDEX_OP_TYPE,
        search: `"${indexName}"`,
        searchFields: ['indexName'],
      });
    },

For this code to work, we need to keep the field indexName in the mappings for reindex operation saved objects. The same is true for the field status (line 185).
The rest of the properties will still be stored in saved objects but only as "not indexed" content so to say. The Core team recommends to consider if the fields are being searched, sorted or aggregated over. Be sure to consider queries from browser side, server side, public HTTP APIs and telemetry collection.

@ElenaStoeva
Copy link
Contributor

Thanks a lot for your review, @ElenaStoeva! The fields this PR removes are only in the mappings, that means the code won't be able to query or search saved objects by this fields. For example, as it's done in the linked file on line 174:

findReindexOperations(indexName: string) {
      return client.find<ReindexOperation>({
        type: REINDEX_OP_TYPE,
        search: `"${indexName}"`,
        searchFields: ['indexName'],
      });
    },

For this code to work, we need to keep the field indexName in the mappings for reindex operation saved objects. The same is true for the field status (line 185). The rest of the properties will still be stored in saved objects but only as "not indexed" content so to say. The Core team recommends to consider if the fields are being searched, sorted or aggregated over. Be sure to consider queries from browser side, server side, public HTTP APIs and telemetry collection.

Thanks for the clarification @yuliacech! Reviewed the code changes again and given what you explained, everything looks good to me. 👍

@yuliacech yuliacech merged commit eaa0884 into elastic:main Feb 21, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Feb 21, 2023
yuliacech added a commit that referenced this pull request Feb 21, 2023
…1014)

## Summary
Partially addresses #64547
Follow up to #150878

This PR removes unnecessary mappings from the saved objects type
`upgrade-assistant-ml-upgrade-operation. This saved objects are used to
store the resolution progress of ml snapshot deprecations.

Currently, the ml deprecations are disabled via a config for Upgrade
Assistant plugin `xpack.upgrade_assistant.featureSet.mlSnapshots`. And
ES will probably not let us use any old ml snapshots in the 8.x version.

### How to test
1. Add `xpack.upgrade_assistant.featureSet.mlSnapshots: true` to your
`kibana.dev.yml` file
2. Start ES and Kibana with `yarn es snapshot --license=trial` and `yarn
start`
3. Add a sample data set, for example `kibana_sample_data_flights`.
4. Navigate to Analytics -> Machine Learning -> Anomaly Detection ->
Jobs.
5. Create a job for a single metric, chose a time range with some data
in it. Memorize the value you input for job ID.
6. Start and stop the job's datafeed a couple of times, so that you have
more than 1 snapshot. Memorize the value of the snapshot ID.
7. replace `deprecations` on line 22 in the file
[`/server/lib/es_deprecations_status.ts`](https://github.com/elastic/kibana/blob/main/x-pack/plugins/upgrade_assistant/server/lib/es_deprecations_status.ts#L22)
with
```
  const deprecations = {
    cluster_settings: [],
    node_settings: [],
    ml_settings: [
      {
        level: 'critical',
        message:
          'model snapshot [1676291073] for job [test_123] needs to be deleted or upgraded',
        url: '',
        details: 'details',
        _meta: { snapshot_id: '1676291073', job_id: 'test_123' },
        resolve_during_rolling_upgrade: false,
      },
    ],
    index_settings: {},
  };
```
where `job_id` and `snapshot_id` are values from step 5 and 6.
8. open UA and test the resolution of the ml snapshot deprecation.
yuliacech added a commit that referenced this pull request Feb 27, 2023
## Summary
Fixes #64547 
Follow up to #150878 and
#151014

This PR removes the saved object type `upgrade-assistant-telemetry` from
the Upgrade Assistant plugin because it seems to not be used for
telemetry or UA functionality. There is a telemetry object to track UA
deprecation logging in this
[file](https://github.com/elastic/kibana/blob/main/x-pack/plugins/telemetry_collection_xpack/schema/xpack_plugins.json#L13628),
I don't think that a saved object is needed for that.

---------

Co-authored-by: kibanamachine <[email protected]>
bmorelli25 pushed a commit to bmorelli25/kibana that referenced this pull request Mar 10, 2023
## Summary
Fixes elastic#64547 
Follow up to elastic#150878 and
elastic#151014

This PR removes the saved object type `upgrade-assistant-telemetry` from
the Upgrade Assistant plugin because it seems to not be used for
telemetry or UA functionality. There is a telemetry object to track UA
deprecation logging in this
[file](https://github.com/elastic/kibana/blob/main/x-pack/plugins/telemetry_collection_xpack/schema/xpack_plugins.json#L13628),
I don't think that a saved object is needed for that.

---------

Co-authored-by: kibanamachine <[email protected]>
yuliacech added a commit that referenced this pull request Apr 19, 2023
## Summary
Fixes #154037

This PR updates the saved objects types used in Upgrade Assistant for
reindexing operations and ML snapshots. This is needed in preparation
for serverless and should not have any affect on the UI.

For testing this PR I would suggest using the same steps as in these 2
PRs for [reindexing](#150878) and
[ml snapshots](#151014).

---------

Co-authored-by: kibanamachine <[email protected]>
@yuliacech yuliacech deleted the 8.8/ua_so_mappings branch February 15, 2024 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Upgrade Assistant release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v8.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants