Don't overwrite plugin configuration when removing/upgrading plugins #7890

dadoonet · 2014-09-26T09:11:47Z

When removing and installing again the plugin all configuration files will be removed in config/pluginname dir.
This is bad as users may have set and added specific configuration files.

We don't have yet an upgrade command which could handle this nicely.

So we remove that code for now and we will implement an upgrade method in another PR.

Related to #5064.

pickypg · 2014-09-26T15:49:56Z

src/main/java/org/elasticsearch/plugins/PluginManager.java

+                }
+                debug("Installed " + name + " into " + toLocation.getAbsolutePath());
+            } else {
+                debug("Ignoring config as previous config already exists in " + toLocation.getAbsolutePath());


Feel like this should be more significant than debug because it really indicates a form of failure in some scenarios.

agree... this one should have normal visibility

It'd be better, to only skip the config files that have the same name, not the whole directory. So for example, if currently es has a config dir that looks like this (for plugins foo):

/config/foo/bar.yml

and the new plugin that installs needs to copy two files there:

/config/foo/bar.yml /config/foo/baz.yml

we'll skip bar.yml but we'll still copy baz.yml. This will help us a lot when we guide the user on how to upgrade - instead of telling the user, to copy & modify a bunch of files, we'll only need to tell him to modify files under config/foo

Sure. I thought about it as well but did not want to complicate code as I thought the initial patch was "urgent". Going to implement that now and add more tests.

@uboness What about sub dirs?

If a plugin has for the first time:

/config/foo/bar/file1.yml

And

/config/foo/bar/file1.yml /config/foo/bar/file2.yml

Should I use Files.walkFileTree to go in each subdir/file or just take care of the first level of files?

I think it's both easier and cleaner. What do you think @uboness?

cleaner. For sure. Easier... Hmmm... Actually creating a /config/foo.backup/ would be easier. If everyone is OK with that, I'd prefer that solution.

I'm okay with foo.backup. It would also not be hidden from non-Windows users by default.

I still think skipping is better tbh... otherwise the users will be more confused on how to merge the old one with the new one. I don't think the file formats will change that much... but when they do, we'll simply be able to tell them what to change in the files they already configured in order to be compatible with the new version (not sure that will even be needed, we could perhaps support both formats in the code and make it transparent).

@dadoonet I don't think it complicates things that much.. it's just traversing the file tree...

and yes... sub-folders need to be supported as well. so if you see a folder with the same name/path in both places, recursively merge the two by adding the new files and skipping existing ones. I'd also argue that if in the es plugin config dir there's a file that doesn't exist in the new plugin dir structure, then rename it to "<original_file_name>.<original_extension>.old" (or something like that).

dadoonet · 2014-09-27T10:38:23Z

@pickypg PR updated based on your comments. So we now backup config/pluginname to config/pluginname.backup, then we remove config/pluginname and install the plugin file content.

It means that after removing and installing again the plugin, users will have to copy thei files from config/pluginname.backup to config/pluginname. Checking manually what they need to copy.

There still an issue. If you have already installed, remove and reinstall the plugin, you have created a config/pluginname.backup. If you remove and install again, it fails today as the backup directory already exists.
(A PR update is ready with a test which fails in that case).

I think we should remove the old backup directory before backup.

Also I was wondering if making a backup should not be part of the removal process instead of the install process. I think it makes more sense to backup data when we remove the plugin.

WDYT? cc @uboness

pickypg · 2014-09-27T19:19:38Z

I'm personally more fond of this approach because I think it's clearer how to get it working again.

I'm trying to think through the two proposed versions:

Add in place and do not change existing files
- Works best for users that do not change anything (no files floating around)
- Moderately annoying, but not necessarily confusing when the config changes and the updated plugin fails because of the existing config, which could be completely unchanged from the prior release
- Most confusing when changes have been made and other files are added
  - Particularly true if multiple plugin versions have been skipped and changes are not immediately obvious
- Works most similarly to Elasticsearch
Backup existing config and replace with new
- Works worst when nothing changes, but the existing config had been changed (forcing users to copy them back)
- No risk of failure if no changes were made and the update adds changes (wasted file copy).
- Less confusing when mixing updates with new files; still requires a copy if the once-existing file now has missing changes, but it makes it easier to merge them
- Easiest to merge
- Works differently from Elasticsearch, but does not presume that plugins are setup with proper defaults like Elasticsearch

I wonder if we should have some sort of a "force" where it skips any backups/checks and simply overwrites (the true shortcut to delete, then install).

There still an issue ... I think we should remove the old backup directory before backup.

My only fear with this approach is that users will think something got screwed up, then retry the upgrade. In doing so, they would delete the real backup.

Assuming we maintain this approach: if an existing backup still exists, then I wonder if we should force the user to delete it?

Also I was wondering if making a backup should not be part of the removal process instead of the install process. I think it makes more sense to backup data when we remove the plugin.

If someone goes through the process of removing a plugin, then I think it's their fault to not backup their config.

dadoonet · 2014-09-29T09:54:01Z

After talking with @spinscale, it's actually a bad idea to move old configuration files to an old place. Because after an update, plugins will fallback to default plugin configuration instead of keeping the right configuration in place.

So, let's do the opposite:

1st step: during an install, if we detect already existing config/pluginname directory, we copy plugin ZIP file content in config/pluginname.new.
2nd step: could be within the same PR or in a new one, we do the same thing but file by file; it means that we copy all non existing files from the plugin to config/pluginname but each time a file already exists, we simply copy the new file to the same dir but we append a .new at the end.

This 2nd step requires more work and more tests so it could be part of a second PR after 1.4.0.Beta1 has been released (target 1.4.0).

Comments?

pickypg · 2014-09-29T16:09:07Z

I completely agree. That makes perfect sense, and it should make it a lot easier to understand what has changed.

It does make me wonder if this is simple enough behavior to allow without user interaction or plugin interaction (enabled transformation of the configuration from within the plugin when upgrading, a la logstash transformation). As it stands -- no matter what -- some plugin will break from any of the three proposed solutions.

For 1.4.0.Beta1, we really need to be sure that we show red flags if we leave any .new files in the config directory. As long as we do that, then I am happy.

dadoonet · 2014-09-30T10:15:48Z

Small update on this. I will send in a couple of hours an update which basically only copy files when they does not exist in config/pluginname dir or append a .new before copying when another one already exist.

Working fine so far. Just need to update the test case.

dadoonet · 2014-09-30T11:57:54Z

It's now ready for review. @spinscale Wanna look?

spinscale · 2014-10-01T06:41:12Z

src/main/java/org/elasticsearch/common/io/FileSystemUtils.java

+     * @param source Source directory (for example /tmp/es/src)
+     * @param destination Destination directory (destination directory /tmp/es/dst)
+     */
+    public static void copyFilesWithoutOverwriting(File source, final File destination) throws IOException {


can you also add fast unit tests for this method?

the method name does not reveal what it does (leaving .new files around), maybe have a suffix parameter?

spinscale · 2014-10-01T06:51:30Z

did a quick review. Is there consensus now, that the .new suffix/impl is good for our purposes?
Still thinking that just returning a log line is valid as well... (you never need to worry, if someone changed the .new after plugin installation and doesnt want it to be overwritten, for example)

dadoonet · 2014-10-04T11:30:16Z

@spinscale I pushed a new commit. Could you check it please?

dadoonet · 2014-10-06T19:42:15Z

@pickypg Do you agree as well with what was done within this PR? Thanks for your feedback. cc @spinscale

pickypg · 2014-10-06T20:30:47Z

src/main/java/org/elasticsearch/common/io/FileSystemUtils.java

+                Files.copy(file, newFile);
+            } catch (FileAlreadyExistsException x) {
+                // We ignore this
+            } catch (IOException x) {


FileAlreadyExistsException is an IOException and this IOException block does the same thing as doing nothing -- return CONTINUE;.

I don't get it. If we have another exception than IOException, I guess it will be raised and not catch, right? Do you mean we should have only:

try { Files.copy(file, newFile); } catch (FileAlreadyExistsException x) { // We ignore this } return CONTINUE;

Yes, but replace FileAlreadyExistsException with IOException to maintain the same functionality. Sorry for saying it so confusingly before.

Indeed. Definitely better. Stupid me. :)

pickypg · 2014-10-06T21:08:00Z

Minor comment. Agree with the PR! LGTM

dadoonet · 2014-10-06T21:49:43Z

@pickypg @spinscale I updated based on latest comment, rebased my work on master and force pushed to my branch. My plan is to merge it tomorrow morning around 10AM CET after running all tests again.

Let me know if you think something is wrong with this PR.

Thanks!

When removing and installing again the plugin all configuration files will be removed in `config/pluginname` dir. This is bad as users may have set and added specific configuration files. During an install, if we detect already existing files in `config/pluginname` directory, we simply copy the new file to the same dir but we append a `.new` at the end. Related to elastic#5064. (cherry picked from commit 5da028f) (cherry picked from commit 4cb1f95)

dadoonet added the review label Sep 26, 2014

dadoonet self-assigned this Sep 26, 2014

dadoonet added the v2.0.0-beta1 label Sep 26, 2014

clintongormley added v1.4.0.Beta1 v1.5.0 >enhancement >breaking labels Sep 26, 2014

clintongormley changed the title ~~plugins: disable support for config dir~~ Plugins: Disable support for config dir Sep 26, 2014

clintongormley removed the review label Sep 26, 2014

dadoonet added the review label Sep 26, 2014

pickypg reviewed Sep 26, 2014
View reviewed changes

dadoonet removed the review label Sep 30, 2014

s1monw added v1.4.0 and removed v1.4.0.Beta1 labels Sep 30, 2014

dadoonet added review and removed review labels Sep 30, 2014

spinscale reviewed Oct 1, 2014
View reviewed changes

pickypg reviewed Oct 6, 2014
View reviewed changes

dadoonet force-pushed the pr/plugins-config-support branch from a004168 to 248f74d Compare October 6, 2014 21:47

dadoonet force-pushed the pr/plugins-config-support branch from 248f74d to 09ff372 Compare October 7, 2014 14:17

dadoonet merged commit 09ff372 into elastic:master Oct 7, 2014

dadoonet deleted the pr/plugins-config-support branch October 7, 2014 14:23

dadoonet mentioned this pull request Oct 7, 2014

Internal: Move plugin manager to new CLI #7339

Closed

jpountz removed the review label Oct 21, 2014

clintongormley changed the title ~~Plugins: Disable support for config dir~~ Plugins: Don't overwrite plugin configuration when removing/upgrading plugins Nov 3, 2014

clintongormley added the :Core/Infra/Plugins Plugin API and infrastructure label Mar 19, 2015

clintongormley changed the title ~~Plugins: Don't overwrite plugin configuration when removing/upgrading plugins~~ Don't overwrite plugin configuration when removing/upgrading plugins Jun 6, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't overwrite plugin configuration when removing/upgrading plugins #7890

Don't overwrite plugin configuration when removing/upgrading plugins #7890

dadoonet commented Sep 26, 2014

pickypg Sep 26, 2014

uboness Sep 26, 2014

uboness Sep 26, 2014

dadoonet Sep 26, 2014

dadoonet Sep 26, 2014

pickypg Sep 26, 2014

dadoonet Sep 26, 2014

pickypg Sep 26, 2014

uboness Sep 27, 2014

uboness Sep 27, 2014

dadoonet commented Sep 27, 2014

pickypg commented Sep 27, 2014

dadoonet commented Sep 29, 2014

pickypg commented Sep 29, 2014

dadoonet commented Sep 30, 2014

dadoonet commented Sep 30, 2014

spinscale Oct 1, 2014

spinscale Oct 1, 2014

spinscale commented Oct 1, 2014

dadoonet commented Oct 4, 2014

dadoonet commented Oct 6, 2014

pickypg Oct 6, 2014

dadoonet Oct 6, 2014

pickypg Oct 6, 2014

dadoonet Oct 6, 2014

pickypg commented Oct 6, 2014

dadoonet commented Oct 6, 2014

Don't overwrite plugin configuration when removing/upgrading plugins #7890

Don't overwrite plugin configuration when removing/upgrading plugins #7890

Conversation

dadoonet commented Sep 26, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dadoonet commented Sep 27, 2014

pickypg commented Sep 27, 2014

dadoonet commented Sep 29, 2014

pickypg commented Sep 29, 2014

dadoonet commented Sep 30, 2014

dadoonet commented Sep 30, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spinscale commented Oct 1, 2014

dadoonet commented Oct 4, 2014

dadoonet commented Oct 6, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pickypg commented Oct 6, 2014

dadoonet commented Oct 6, 2014