bin/logstash-plugin prepare-offline-pack, improvements to install a pack and rats tests. #6404
Conversation
|
@jsvd you can try it out I've tested the following:
|
|
I've fixed the issue with wildcards you can now do this. bin/logstash-plugin prepare-offline-pack logstash-filter-* (you might need to quote the name) Also the install will now work if the plugin is already in the gemfile, it will get update with the right requirements and installed. |
ph
force-pushed
the
feature/prepare-offline
branch
2 times, most recently
from
433e03c to
6a8f88c
Compare
|
@jsvd this PR need some review love, will check if travis is green for the integration test. |
ph
changed the title
| mkdir tools | ||
| cp -r ../../tools/paquet tools/ | ||
| cd .. | ||
|
|
There was a problem hiding this comment.
Once we publish the new paquet we can remove this.
| mkdir tools | ||
| cp -r ../../tools/paquet tools/ | ||
| cd .. | ||
|
|
There was a problem hiding this comment.
Once we publish the new paquet we can remove this.
|
@jordansissel I confirm that your seccomp tricks does wonder, this PR now uses it for installing a pre-generated pack without any internet access. |
| definition.lock(lockfile_path) | ||
|
|
||
| return @new_deps | ||
| gemfile = LogStash::Gemfile.new(File.new(gemfile_path, "r+")).load |
There was a problem hiding this comment.
should a pointer for this FD be kept so it is closed on the ensure block?
There was a problem hiding this comment.
This is taken care by LogStash::Gemfile#save and LogStash::Gemfile#restore! that we call in the rescue block.
| @@ -20,7 +22,6 @@ def self.inject!(new_deps, options = { :gemfile => LogStash::Environment::GEMFIL | |||
| injector.inject(gemfile, lockfile) | |||
| end | |||
|
|
|||
|
|
|||
| # This class is pretty similar to what bundler's injector class is doing | |||
There was a problem hiding this comment.
we're doing different things, compared to the original implementation of the inject method, so this comment should be updated
| @@ -35,4 +35,8 @@ def relative_path(path) | |||
| require "pathname" | |||
| ::Pathname.new(path).relative_path_from(::Pathname.new(LogStash::Environment::LOGSTASH_HOME)).to_s | |||
| end | |||
|
|
|||
| def debug? | |||
| # │ └── stud-0.0.22.gem | ||
| # | ||
| # Right now this work fine, but I think we could also use Bundler's SourceList classes to handle the same thing | ||
| def update_in_memory_index(local_source) |
There was a problem hiding this comment.
not sure what's your preference, but since this is a very destructive method we could name it update_in_memory_index!
(only a suggestion)
| end | ||
|
|
||
| def self.transform_pattern_into_re(pattern) | ||
| Regexp.new(/^#{pattern.gsub(WILDCARD, WILDCARD_INTO_RE)}$/) |
There was a problem hiding this comment.
we're creating the regexp twice here, I suggest instead:
Regexp.new("^#{pattern.gsub(WILDCARD, WILDCARD_INTO_RE)}$")There was a problem hiding this comment.
good catch, will tiddy this up.
| ) | ||
|
|
||
| INVALID_PLUGINS_TO_EXPLICIT_PACK = IGNORE_GEMS_IN_PACK.collect { |name| Regexp.new(/^#{name}/) } + [ | ||
| /mixin/ |
There was a problem hiding this comment.
so you can't pack gems if they depend on mixins? or the pack will be broken because it doesn't include the mixin?
There was a problem hiding this comment.
ah..I think I understand, this list is used only to check the list of plugins manually stated with pack "plugin", right?
There was a problem hiding this comment.
The second comment, forbid plugins declared with the pack command.
| def validate_plugins! | ||
| @plugins_to_package.each do |plugin_name| | ||
| if INVALID_PLUGINS_TO_EXPLICIT_PACK.any? { |invalid_name| plugin_name =~ invalid_name } | ||
| raise UnpackablePluginError, "Cannot explicitely pack `#{plugin_name}` for offline installation" |
There was a problem hiding this comment.
"explicitly", same in method explicitely_declared_plugins_specs below
|
|
||
| def explicitely_declared_plugins_specs | ||
| @plugins_to_package.collect do |plugin_pattern| | ||
| specs = SpecificationHelpers.find_by_name_with_wildcards(plugin_pattern) |
There was a problem hiding this comment.
what is the purpose of supporting wildcards here? I think it's safer to support only 1 to 1 declaration of plugins to be packed. this would mean that specs.size has to be 1. If it's 2 or more it should generate some exception because you can have multiple plugin specs for different versions (it's easy for vendor/ to become littered with gem trash)
There was a problem hiding this comment.
I think its was mostly for convenience if someone want to pack all the filters without specifying all of them or all of the elasticsearch related plugins
If it's 2 or more it should generate some exception because you can have multiple plugin specs for different versions (it's easy for vendor/ to become littered with gem trash)
No this is impossible, because when you query the specification cache it can only return what is activated in your lock file, so only 1 version for 1 plugin will be returned.
| prepare_package(explicit_plugins_specs, temp_path) | ||
| LogStash::Util::Zip.compress(temp_path, @target) | ||
| ensure | ||
| FileUtils.rm_rf(temp_path) if Dir.exist?(temp_path) |
There was a problem hiding this comment.
I believe rm_rf tolerates the absence of the argument like rm -rf path does
| # We need to start bundler, dependencies so the plugins are available for the prepare | ||
| LogStash::Bundler.setup!({:without => [:build, :development]}) | ||
|
|
||
| # lazy require paquet since its an external dependency |
|
not sure if it is known, but currently running |
|
The artifact problems is know, I am copying the directory in the suite to make it run, see my comment on https://github.com/elastic/logstash/pull/6404/files#diff-9ea1854b834b68d347a8d758eda73309R25 |
| bin/logstash-plugin prepare-offline-pack logstash-filter-* | ||
| bin/logstash-plugin prepare-offline-pack logstash-filter-* logstash-input-beats | ||
|
|
||
| You can get a list of the installed plugin run `bin/logstash-plugin list` |
|
offline installation of a packed plugin worked correctly (I had to cp paquet to I confirm that direct .gem offline installation doesn't work, neither with full path or with a file:/// uri so, in conclusion: great work @ph, packing for offline and installation works perfectly 👍 |
ph
force-pushed
the
feature/prepare-offline
branch
from
83ba528 to
abb869c
Compare
ph
force-pushed
the
feature/prepare-offline
branch
from
abb869c to
02344aa
Compare
|
I've rebased this branch and also fixed the problem with the |
|
is there a way we can not have the fixture .gems like manticore commited? we could download them during bootstrap or install-development-dependencies? |
|
@jsvd That path should be in the git ignore which will solve the problem, will push a fix for it. |
|
@jsvd I've made sure we don't have any .gem. |
|
LGTM. |
|
LGTM |
This new command replace the old workflow of `pack`, `unpack` and the `install --local`, and wrap all the logic into one uniform way of installing plugins. The work is based on the flow developed for installing an x-pack inside Logstash, when you call prepare-offline-pack, the specified plugins and their dependencies will be packaged in a zip. And this zip can be installed with the same flow as the pack. Definition: Source Logstash: Where you run the prepare-offline-pack. Target Logstash: Where you install the offline package. PROS: - If you install a .gem in the source logstash, the .gem and his dependencies will be bundled. - The install flow doesn't need to have access to the internet. - Nothing special need to be setup in the target logstash environment. CONS: - The is one minor drawback, the plugins need to have their JARS bundled with them for this flow to work, this is currently the case for all the official plugins. - The source Logstash need to have access to the internet when you install plugins before packaging them. Usage examples: bin/logstash-plugin prepare-offline-pack logstash-input-beats bin/logstash-plugin prepare-offline-pack logstash-filter-jdbc logstash-input-beats bin/logstash-plugin prepare-offline-pack logstash-filter-* bin/logstash-plugin prepare-offline-pack logstash-filter-* logstash-input-beats How to install: bin/logstash-plugin install file:///tmp/logstash-offline-plugins-XXXX.zip
ph
force-pushed
the
feature/prepare-offline
branch
from
3931f9f to
1c1a463
Compare
This new command replace the old workflow of `pack`, `unpack` and the `install --local`, and wrap all the logic into one uniform way of installing plugins. The work is based on the flow developed for installing an x-pack inside Logstash, when you call prepare-offline-pack, the specified plugins and their dependencies will be packaged in a zip. And this zip can be installed with the same flow as the pack. Definition: Source Logstash: Where you run the prepare-offline-pack. Target Logstash: Where you install the offline package. PROS: - If you install a .gem in the source logstash, the .gem and his dependencies will be bundled. - The install flow doesn't need to have access to the internet. - Nothing special need to be setup in the target logstash environment. CONS: - The is one minor drawback, the plugins need to have their JARS bundled with them for this flow to work, this is currently the case for all the official plugins. - The source Logstash need to have access to the internet when you install plugins before packaging them. Usage examples: bin/logstash-plugin prepare-offline-pack logstash-input-beats bin/logstash-plugin prepare-offline-pack logstash-filter-jdbc logstash-input-beats bin/logstash-plugin prepare-offline-pack logstash-filter-* bin/logstash-plugin prepare-offline-pack logstash-filter-* logstash-input-beats How to install: bin/logstash-plugin install file:///tmp/logstash-offline-plugins-XXXX.zip Fixes #6404
|
Great work @ph. Users will rejoice. :) |
|
Not sure that this is the right spot to bring this up as I am new to ELK stack...
Validating file:
|
|
@greenie69 can you instead try the file:// syntax as specified in the docs? |
|
I have ran it with file:// and file:/// before, but ran them both again and the both give the same error .\logstash-plugin install file:///C:\ELK\logstash-offline-plugins-5.3.0.zip ERROR: Something went wrong when installing file:///C:\ELK\logstash-offline-plugins-5.3.0.zip, message: bad URI(is not URI?): https://artifacts.elastic.co/downloads/logstash-plugins/file:///c:\ELK\logstash-offline-plugins-5.3.0.zip/file:///C:\ELK\logstash-offline-plugins-5.3.0.zip-5.3.0.zip |
|
The only syntax that seems to work, but gives a different error is using the quotes: PS C:\ELK\logstash-5.3.0\bin> .\logstash-plugin install file: "C:\ELK\logstash-offline-plugins-5.3.0.zip" |
|
@greenie69 did you try this format? |
|
Yes...Finally...the confusing part is are the slashes..once I changed the slashes after the system drive letter that seemed to do the trick and it installed. Thanks ph |
|
It needs to be much clearer that the path needs to be a file:// URI. But yea, thanks. It works once I figured it out. :) |
|
The install from file use case is pretty clear in the docs. I can see some confusion when working on a Windows system though since backslashes may be the expected syntax. @dedemorton is this something you could help clarify in the docs? |
This new command replace the old workflow of
pack,unpackand theinstall --local, and wrap all the logic into one uniform way of installing plugins.The work is based on the flow developed for installing an x-pack inside Logstash, when you call prepare-offline-pack, the specified plugins and their dependencies will be packaged in a zip.
And this zip can be installed with the same flow as the pack.
Definition:
Source Logstash: Where you run the prepare-offline-pack.
Target Logstash: Where you install the offline package.
PROS:
CONS:
Usage examples:
bin/logstash-plugin prepare-offline-pack logstash-input-beats
bin/logstash-plugin prepare-offline-pack logstash-filter-jdbc logstash-input-beats
bin/logstash-plugin prepare-offline-pack logstash-filter-*
bin/logstash-plugin prepare-offline-pack logstash-filter-* logstash-input-beats
How to install:
bin/logstash-plugin install file:///tmp/logstash-offline-plugins-XXXX.zip
Merge TODO:
Fixes: #6393 #5906