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

Reduce performance impact of deleting legacy url aliases #141136

Open
TinaHeiligers opened this issue Sep 20, 2022 · 11 comments
Open

Reduce performance impact of deleting legacy url aliases #141136

TinaHeiligers opened this issue Sep 20, 2022 · 11 comments
Assignees
Labels
enhancement New value added to drive a business result Feature:Security/Sharing Saved Objects Platform Security - Sharing Saved Objects feature performance Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! triage_needed

Comments

@TinaHeiligers
Copy link
Contributor

TinaHeiligers commented Sep 20, 2022

With the addition of a bulk delete API, we run the risk of performance degradation when objects being deleted have many legacy URL aliases.

The API that handles legacy URL alias cleanup after a write operation uses an updateByQuery with a simple script to delete one alias at a time.

For the API to handle many objects (a pseudo-bulk operation), the script would need to be adapted to handle different namespaces and delete behavior for each saved object that no longer exists.

At the moment, there are two bulk operations that include cleaning up legacy URL aliases:

To avoid potentially serious performance-related issues, the saved objects repository needs a performant API to delete legacy URL aliases in bulk.

Originally posted by @TinaHeiligers in #139680 (comment)

@botelastic botelastic bot added the needs-team Issues missing a team label label Sep 20, 2022
@TinaHeiligers TinaHeiligers added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:Security/Sharing Saved Objects Platform Security - Sharing Saved Objects feature labels Sep 20, 2022
@elasticmachine
Copy link
Contributor

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

@botelastic botelastic bot removed the needs-team Issues missing a team label label Sep 20, 2022
@TinaHeiligers TinaHeiligers added Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! needs-team Issues missing a team label labels Sep 20, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-security (Team:Security)

@botelastic botelastic bot removed the needs-team Issues missing a team label label Sep 20, 2022
@TinaHeiligers TinaHeiligers added performance needs-team Issues missing a team label labels Sep 20, 2022
@botelastic botelastic bot removed the needs-team Issues missing a team label label Sep 20, 2022
@TinaHeiligers TinaHeiligers added the enhancement New value added to drive a business result label Sep 20, 2022
@TinaHeiligers
Copy link
Contributor Author

We should also consider making MAX_CONCURRENT_ALIAS_DELETIONS configurable at either the global level or per API that implements the limit.

@rudolf
Copy link
Contributor

rudolf commented Oct 11, 2022

Because the majority of saved object types are of namespaceType: 'multiple-isolated' this performance bug could potentially impact most saved object types that existed before 8.0.

Some use cases that might attempt to do large bulk deletes where legacy url aliases might cause performance problems:

@LeeDr
Copy link

LeeDr commented Oct 18, 2022

I had an idea I wanted to suggest for these saved objects. They don't contain very many fields (see example below). What if instead of creating a saved object for every one of them we had a single saved object containing a list of all the sourceId, targetId, etc from each.

  • Kibana would have to fetch it from the index at start up and cache it in Kibana memory.
  • looking up a targetId from a sourceId (or whichever way that works) would be super-fast and wouldn't require a round-trip to Elasticsearch.
  • Any changes would have to get persisted back to that saved object with an update.

This would reduce the number of SO docs Kibana has to read and write during migration.

There's obviously some risk to this idea. It's putting more eggs in one basket. If this one doc were accidentally deleted or corrupted it would be bad. Might even be necessary to keep a few documents as old versions as updates are made.
It could be one SO which tells Kibana the ID of the list. When a new revision of the list is written successfully that pointer doc gets updated with the new ID.

But how large might this one doc get? And how much memory in Kibana would it potentially consume while cached? Maybe the new file service could chunk it if it was too large.

Example doc;

{
  "_index": ".kibana_8.6.0_001",
  "_id": "legacy-url-alias:automation:index-pattern:ff959d40-b880-11e8-a6d9-e546fe2bba5f",
  "_version": 2,
  "_score": 0,
  "_source": {
    "legacy-url-alias": {
      "sourceId": "ff959d40-b880-11e8-a6d9-e546fe2bba5f",
      "targetNamespace": "automation",
      "targetType": "index-pattern",
      "targetId": "261c3a4b-d344-572c-bd88-7f484776534d",
      "purpose": "savedObjectConversion"
    },
    "type": "legacy-url-alias",
    "references": [],
    "migrationVersion": {
      "legacy-url-alias": "8.2.0"
    },
    "coreMigrationVersion": "8.6.0"
  },
  "fields": {
    "migrationVersion.legacy-url-alias.keyword": [
      "8.2.0"
    ],
    "legacy-url-alias.purpose": [
      "savedObjectConversion"
    ],
    "legacy-url-alias.targetId": [
      "261c3a4b-d344-572c-bd88-7f484776534d"
    ],
    "coreMigrationVersion": [
      "8.6.0"
    ],
    "type": [
      "legacy-url-alias"
    ],
    "migrationVersion.legacy-url-alias": [
      "8.2.0"
    ],
    "legacy-url-alias.targetNamespace": [
      "automation"
    ],
    "legacy-url-alias.targetType": [
      "index-pattern"
    ],
    "legacy-url-alias.sourceId": [
      "ff959d40-b880-11e8-a6d9-e546fe2bba5f"
    ]
  }
}

@TinaHeiligers
Copy link
Contributor Author

@elastic/kibana-security Would the suggestion in #141136 (comment) be feasible?

@TinaHeiligers
Copy link
Contributor Author

@rudolf Considering we've seen real instances of many legacy_url_aliases making the problem of too many saved objects even worse and causing upgrade migration timeouts, we may need to reconsider the priority on this issue.
Had bulk_delete been used to clean up the issues we saw with a lot of 'stale' case-user-actions and case-comments saved objects, we probably would've seen a serious performance impact.

@legrego
Copy link
Member

legrego commented Oct 20, 2022

Some of the history escapes me at the moment, but one of the reasons we create an alias-document-per-object is to prevent multiple aliases from referencing the same object. Document IDs are the only uniqueness guarantee that we have in ES today, and so leveraging this ID for uniqueness prevents another class of unresolvable conflict errors.
There's a note in the original implementation which says:

Aliases will be stored as separate saved objects.
Benefits of this approach:

  • ...
  • Alias raw ID ensures that there will never be multiple aliases with the same target namespace+type+ID
  • ...

Based on what I've been able to reconstruct, I don't think it would be infeasible, but a change like this would require careful planning, design, and consideration before moving forward with implementation.

@rudolf
Copy link
Contributor

rudolf commented Oct 27, 2022

Agree with @legrego that this would be a risky change to make.

If we put all the aliases in one document we could enforce the uniqueness constraint in Kibana itself. But I would be concerned about the potential size of this document. We could probably easily test this by seeing how big the JSON of a object with 2m legacy url aliases inside an array would get.

Another option could be disable legacy url aliases for some SO types. I don't know the details, but I could imagine that we maybe have URL's to a case but not to a case-user-action. This would not solve the problem completely but it can reduce the impact somewhat.

@jeramysoucy
Copy link
Contributor

Yes to @legrego and @rudolf - Definitely something to approach carefully and spend some time to test design feasibility.

One other idea to mitigate having one large alias doc would be to create one alias doc per type and/or per space. We could also create a secondary lookup system if this still proves to be too large in some deployments (have a max doc size, multiple alias docs, and a lookup doc), but I think that may be more complicated than justified by the benefits. Not sure what our most dense deployments look like IRL.

@cnasikas
Copy link
Member

cnasikas commented Nov 1, 2022

Another option could be disable legacy url aliases for some SO types. I don't know the details, but I could imagine that we maybe have URL's to a case but not to a case-user-action. This would not solve the problem completely but it can reduce the impact somewhat.

Unfortunately, we have URLs that point to user actions and comments. Each comment and user action has a "Copy reference link" button (see screenshot) with which the user can get a URL pointing to this particular user action or comment (the UI will scroll to this position). The format of the URL is <kibana>/cases/<case_id>/<case_comment_id> or <user_action_id>

Screenshot 2022-11-01 at 11 01 39 AM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New value added to drive a business result Feature:Security/Sharing Saved Objects Platform Security - Sharing Saved Objects feature performance Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! triage_needed
Projects
None yet
Development

No branches or pull requests

7 participants