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

[WIP] Store should not update template on every restart. #28611

Conversation

njd5475
Copy link
Contributor

@njd5475 njd5475 commented Jan 11, 2019

Summary

This may be an alternative to trying to track version numbers for templates. Instead of trying to store, read, compare and track a version number. We can get the template properties, compare them to the codes properties with order agnostic hashes.

If anyone changes the templateProperties the template would be updated. As well as when it does not exist.

This would close #28540

Relevant details from that PR

This PR implements changes per feedback from the original Task Manager PR, that slipped through the cracks during review #24356 (comment):

@pickypg on Dec 7, 2018 Member
...
But also we should not be setting this unless we need to set it. If it already exists, then we're unnecessary causing a cluster state change on the ES side every time that Kibana starts up.

As an aside, the ES Template API supports version values and we should be using it based on the version of Kibana so that we do not replace a newer version of Kibana's template with our own. And, given a newer template, we should consider not marking the task manager as initialized.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@njd5475 njd5475 added the Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc label Jan 11, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform

@njd5475 njd5475 changed the title Store should not update template on every restart. [WIP] Store should not update template on every restart. Jan 11, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed


if (existingTemplate) {
shouldUpdate =
hash(existingTemplate[this.index].mappings._doc.properties) !== hash(templateProperties);
Copy link
Member

Choose a reason for hiding this comment

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

This does prevent overwriting it upon every restart, but it also means that if an older version happens to be restarted, then it will trample a newer version's template.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see this as an issue. Because index templates don't change existing indices. If you delete the index, it will then get created with the new mappings from the older templated version but before task manager would run and index a task it would update the template.

We could double check the mappings once per run before indexing or handle indexing error appropriately to determine the index was changed and doesn't match.

Copy link
Member

@tsullivan tsullivan Jan 14, 2019

Choose a reason for hiding this comment

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

I think @pickypg is right in that updates need to be controlled in that they must either install the template from scratch, upgrade it, or do nothing. If they can't upgrade it, they'll potentially break things in ways that will be hard to troubleshoot for the user: the outcome of the problem will be some kind of TypeError when an old instance of Kibana claims a new task and fields it depends on have moved.

Consider:

  1. Kibana 0.1.0 starts up, installs the template from scratch
  2. Kibana 1.0.0 starts up, overwrites the template
  3. Kibana 1.0.0 schedules a task
  4. Kibana 0.1.0 claims the task
  5. Boom.

Edit: to add more thoughts, the original PR we're working on handles this in two ways:

  1. Numeric versioning ensures old templates do not overwrite new templates, provides helpful log messaging to get users to upgrade
  2. API versioning ensures older instances are only able to claim tasks they understand

@njd5475 njd5475 closed this Feb 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v6.7.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants