enonic / xp

Enonic XP
https://enonic.com
GNU General Public License v3.0
201 stars 34 forks source link

com.enonic.xp.elasticsearch path.repo vs com.enonic.xp.repo snapshots.dir #7957

Closed rymsha closed 4 years ago

rymsha commented 4 years ago

path.repo and snapshots.dir both point to where snapshots should be stored, but if they mismatch snapshots cant't be lested or created.

warnings in logs are observed:

WARN  org.elasticsearch.repositories.fs - [b1c982d0-5306-41bc-aaac-144d31c85ce8] The specified location [C:\xp\bin\..\home\snapshots] doesn't start with any repository paths specified by the path.repo setting: [[C:\snap]]
2020-03-17 09:52:11,554 WARN  org.elasticsearch.repositories - [b1c982d0-5306-41bc-aaac-144d31c85ce8] failed to create repository [fs][enonic-xp-snapshot-repo]
org.elasticsearch.common.inject.CreationException: Guice creation errors:
...

It is still a question, why we have two properties for that. But it looks like snapshots.dir is an obsolete leftover (although documented one)

rymsha commented 4 years ago

Be aware of another confusion with these settings: path.repo actually specifies snaphots path (it is equally called in elasticsearch for filesystem snapshot repositories). But it is very easy to confuse with blobstore location (baseDir = ${xp.home}/repo/blob) and elasticsearch index location (path=${xp.home}/repo/index)

sigdestad commented 4 years ago

Where is the "path.repo" setting located? I have not heard of it before?

rymsha commented 4 years ago

in com.enonic.xp.elasticsearch Introduced during work on #6541

sigdestad commented 4 years ago

We need to sort this one out, which of these are actually being used? I guess only one of them will count if both are set?

rymsha commented 4 years ago

Both are in use. But it seems like a documentation bug, actually. Because elasticsearch supports multiple snapshot repositories. But only one of them should be used by for XP snapshoting.

rymsha commented 4 years ago

yet our path.repo supports only single value, while elasticsearch path.repo is an array. So specifying multiple repositories is not possible in XP, and reason to have two configuration parameters is still unclear.

sigdestad commented 4 years ago

I think we should remove (undocument) the elasticsearch path.repo, as it seems to have been introduced by mistake?

rymsha commented 4 years ago

It wasn't a mistake per se. It was unforeseen consequence of splitting up XP to more "independent" modules.

snapshots.dir is core-repo module implementation detail (this module does too much, including search implementation, node api imp, dump, vacuum, etc, etc.).

path.repo is core-elasticsearch module implementation detail (this module bootstraps elasticsearch).

core-elasticsearch does not depend on core-repo, nor does core-repo depend on core-elasticsearch. But both depend on "elasticsearch" module.

So, there is a chance that we can read path.repo property form elasticsearch itself and obsolete snapshots.dir, but this approach smells: it will be yet another complication when we migrate to ES7.

So, I don't see a simple solution.

I see it is too much work for now: it is simpler to update documentation and require users to make these two values in sync. And this is not "wrong" either: when one creates snapshot repository in elasticsearch she must provide one of the paths from path.repo static configuration. One of the reasons is, actually, security.

sigdestad commented 4 years ago

snapshots.dir is used by core to generate snapshot file path path.repo is a filter that specifies where ES is allowed to place snapshots (security setting)

anatol-sialitski commented 4 years ago

You are right, the path.repo property used for security more details here https://www.elastic.co/blog/elasticsearch-1-6-0-released#fs-config. So if we want to create a snaphost the path.repo property must be added to elasticsearch.yml file.

By default repo.path and snapshots.dir to pointer to the same destination.

Also, I would like notice, that this property already used before and we have notes in the documentation https://developer.enonic.com/docs/xp/stable/deployment/config#elasticsearch

path.repo
Allowed location for placing snapshots. NB! Use same value as for snapshots.dir as specified in com.enonic.xp.repo.cfg
rymsha commented 4 years ago

I'm closing this issue, because @sigdestad updated docs (and @anatol-sialitski even noticed that). We can't do much with this issue in XP7. But in XP8 it will be another story (because ES is external there #7656)