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

Allow plugins to upgrade templates and index metadata on startup #24379

Merged
merged 6 commits into from May 3, 2017

Conversation

Projects
None yet
6 participants
@imotov
Member

imotov commented Apr 28, 2017

The UpgraderPlugin adds two additional extension points called during cluster upgrade and when old indices are introduced into the cluster state during initial recovery, restore from a snapshot or as a dangling index. One extension point allows plugin to update old templates and another extension points allows to update/check index metadata.

This change should simplify upgrade process for plugins that are using special indices to store their data, and should prevent situations when, for example, an accidental restore of an obsolete index would break the plugin.

Add UpgraderPlugin
The UpgraderPlugin adds two additional extension points called during cluster upgrade and when old indices are introduced into the cluster state during initial recovery, restore from a snapshot or as a dangling index. One extension point allows plugin to update old templates and another extension points allows to update/check index metadata.

This change should simplify upgrade process for plugins that are using special indices to store their data, and should prevent situations when, for example, an accidental restore of an obsolete index would break the plugin.
@areek

areek suggested changes Apr 28, 2017 edited

thanks @imotov! This looks great, I would recommend adding a unit test to ensure that UpgraderPlugin.getIndexTemplateMetaDataUpgrader works as expected when an ISE is thrown. (similar to GatewayMetaStateTests#testCustomMetaDataValidation for UpgraderPlugin.getCustomMetaDataUpgrader)

changed = true;
}
return changed ? upgradedMetaData.build() : metaData;
}
private static <Data> boolean applyPluginUpraders(ImmutableOpenMap<String, Data> existingData,

This comment has been minimized.

@areek

areek Apr 28, 2017

Contributor

+1

This comment has been minimized.

@archanid

archanid Apr 29, 2017

Contributor

This is misspelled. applyPluginUpraders missing a g

This comment has been minimized.

@imotov

imotov Apr 29, 2017

Member

Great catch! Thanks!

* @throws IllegalStateException if the node should not start because at least one {@code MetaData.Custom}
* is unsupported
*/
public UnaryOperator<Map<String, MetaData.Custom>> getCustomMetaDataUpgrader() {

This comment has been minimized.

@areek

areek Apr 28, 2017

Contributor

+1, removing this from the Plugin extension

@imotov

This comment has been minimized.

Member

imotov commented Apr 28, 2017

@areek good point. I have added the test.

@areek

areek approved these changes Apr 28, 2017

Thanks @imotov, this LGTM! Can we add a test for multiple UpgraderPlugins with index template meta data upgrader? similar to GatewayMetaStateTests#testMultipleCustomMetaDataUpgrade for multiple custom meta data upgrader.

@rjernst

This comment has been minimized.

Member

rjernst commented Apr 30, 2017

Does this really need to be it's own UpgraderPlugin interface? We have been creating those for "categories" of plugins, for things they might implement, like a mapper or scripting. But this is general purpose, that is why the custom metadata method was on Plugin already, so I think maybe this should all stay there. Any plugin may have clusterstate/indexes.

@imotov

This comment has been minimized.

Member

imotov commented Apr 30, 2017

Does this really need to be it's own UpgraderPlugin interface?

@rjernst It doesn't need to be its own interface, it just seemed to be cleaner this way. Any plugin may expose actions as well, and yet we separated them into the ActionPlugin interface. I expect that only a small percentage of plugins will actually implement upgrade functionality, because not many plugins store data in clusterstate or dedicated indices, so it felt like a pretty specialized functionality to me. However, if you feel strongly about it and @areek is fine with it, I will move these methods back to the Plugin interface.

@nik9000

This comment has been minimized.

Contributor

nik9000 commented May 1, 2017

I feel like we'd be better off putting the "upgrade" plugin behavior next to the builtin plugin behavior. That way those methods are right there when you first write the plugin so you know what hooks are available on upgrade. So wherever we register custom cluster state, custom templates, and custom index metadata. But it looks like we don't have a place for any of those in plugins. Custom cluster state is just a matter of registering writeable readers, right? Custom templates are a thing that'd be ok in a plugin but they are implemented in the plugins that need it. And indexes are things you make intentionally while running the plugin. In that case, I like keeping all of these methods in Plugin as well.

@imotov imotov changed the title from Add UpgraderPlugin to Allow plugins to upgrade templates and index metadata on startup May 1, 2017

@imotov

This comment has been minimized.

Member

imotov commented May 1, 2017

@nik9000, I agree. That makes sense. Going to push the change soon.

this.customMetaDataUpgraders = customs -> {
Map<String, MetaData.Custom> upgradedCustoms = new HashMap<>(customs);
for (UnaryOperator<Map<String, MetaData.Custom>> customMetaDataUpgrader : customMetaDataUpgraders) {

This comment has been minimized.

@rjernst

rjernst May 1, 2017

Member

Why is this now tightly coupled with the Plugin api? It was decoupled before by using generic UnaryOperator. I think it should stay that way. In other places we have tried keeping any references to the Plugin api and associated interfaces in Node initialization. This makes it much easier to test (don't need to create subclasses of Plugin), and also ensures we pull concrete references to plugin defined classes/methods/objects at Node initialization, instead of lazily which may result in the Plugin impl being able to change this on the fly (which could have weird and unintended semantics).

This comment has been minimized.

@imotov
@rjernst

This comment has been minimized.

Member

rjernst commented May 1, 2017

Thanks @imotov! The changes look good, but I have one final concern and I am not sure how/if it can/needs to be fixed.

The existing cluster state upgrade code assumed that any plugin would only be touching keys within the cluster state that are "owned" by that plugin, such that the order of application of upgrading does not matter. The code in this PR does nothing with ordering, so has this same assumption but for templates and index metadata. Can this assumption be safely made? At a minimum, I think the assumption should be documented in the javadocs for the methods on the Plugin api.

@imotov

This comment has been minimized.

Member

imotov commented May 1, 2017

@rjernst that's a really good point. Indeed, we don't maintain any "ownership" relationships between indices, templates, custom metadata and plugins. And I cannot think of a reasonable way to enforce the convention of plugins only upgrading their own stuff without maintaining such relationship.

I don't think I can do anything more than adding javadoc explaining the assumption here but I think it might be interesting to explore this "ownership" idea outside this PR. I can see several benefits in preventing external interference with plugin's data even outside of the upgrade scenario. Maybe we should require all plugin to create objects with names that start with plugin name?

@rjernst

This comment has been minimized.

Member

rjernst commented May 1, 2017

I don't think I can do anything more than adding javadoc explaining the assumption here but I think it might be interesting to explore this "ownership" idea outside this PR.

Sounds good. Create a separate discuss issue? One possible way might be for plugins to register the elements they will add to cluster state, templates, whatever (and along with that registration would be how to upgrade).

@imotov imotov merged commit cacba6b into elastic:master May 3, 2017

2 checks passed

CLA Commit author is a member of Elasticsearch
Details
elasticsearch-ci Build finished.
Details

imotov added a commit that referenced this pull request May 3, 2017

Allow plugins to upgrade templates and index metadata on startup (#24379
)

The UpgraderPlugin adds two additional extension points called during cluster upgrade and when old indices are introduced into the cluster state during initial recovery, restore from a snapshot or as a dangling index. One extension point allows plugin to update old templates and another extension points allows to update/check index metadata.

@clintongormley clintongormley removed the v6.0.0 label May 9, 2017

debadair added a commit to debadair/elasticsearch that referenced this pull request May 16, 2017

Allow plugins to upgrade templates and index metadata on startup (ela…
…stic#24379)

The UpgraderPlugin adds two additional extension points called during cluster upgrade and when old indices are introduced into the cluster state during initial recovery, restore from a snapshot or as a dangling index. One extension point allows plugin to update old templates and another extension points allows to update/check index metadata.

imotov added a commit to imotov/elasticsearch that referenced this pull request Jun 15, 2017

TemplateUpgraders should be called during rolling restart
In elastic#24379 we added ability to upgrade templates on full cluster startup. This PR invokes the same update procedure also when a new node first joins the cluster allowing to update templates on a full cluster restart.

Closes elastic#24680

imotov added a commit that referenced this pull request Jun 22, 2017

TemplateUpgraders should be called during rolling restart (#25263)
In #24379 we added ability to upgrade templates on full cluster startup. This PR invokes the same update procedure also when a new node first joins the cluster allowing to update templates on a rolling cluster restart as well.

Closes #24680

imotov added a commit that referenced this pull request Jun 23, 2017

TemplateUpgraders should be called during rolling restart (#25263)
In #24379 we added ability to upgrade templates on full cluster startup. This PR invokes the same update procedure also when a new node first joins the cluster allowing to update templates on a rolling cluster restart as well.

Closes #24680
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment