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

Manage plugins to be installed offline #3404

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
7 participants
@purbon
Copy link
Contributor

commented Jun 9, 2015

The motivation of this PR is to enable users to dump a set of plugins, to later be managed in machines that have no internet connection.

This introduces a set of new commands that enable offline plugin management.

  • bin/plugin pack: let's you create a bundle with all plugins currently installed in a LS instance plus their dependencies. By default this package of plugins is created as zip files in windows and tar.gz in unix machines.
  • bin/plugin unpack: From a previously created package this command lets you install it in another logstash instance.
  • Add the --local flag option to the bin/plugin install command. When the new flag is passed the plugin manager will fetch the plugins from a local file system directory (created when doing the bin/plugin unpack).
  • Add the --local flag option to the bin/plugin update command. This command has the same behaviour and motivation as the one for the bin/plugin install

Expected workflow

The expected workflow would be:

  • a user created a package with the necessary plugins from a LS that has internet access.
  • with the generated bundle (zip/tgz) the user unpack this file into a LS that has no internet.
  • then using the --local flag when doing install or update he can install plugins from the local filesystem.

Other options added

bin/plugin pack:

option "--tgz", :flag, "compress package as a tar.gz file", :default => !LogStash::Environment.windows?   option "--zip", :flag, "compress package as a zip file", :default => LogStash::Environment.windows?
option "--[no-]clean", :flag, "clean up the generated dump of plugins", :default => true
option "--overwride", :flag, "Overwrite a previously generated package file", :default => false

bin/plugin unpack

+  option "--tgz", :flag, "unpack a packaged tar.gz file", :default => !LogStash::Environment.windows?
+  option "--zip", :flag, "unpack a packaged  zip file", :default => LogStash::Environment.windows?

Relates/Fixes to #2376

@purbon purbon added the enhancement label Jun 9, 2015

@purbon purbon changed the title [WiP] Manage plugins to be installed offline Manage plugins to be installed offline Jun 9, 2015

@purbon

This comment has been minimized.

Copy link
Contributor Author

commented Jun 9, 2015

@suyograo @jordansissel @colinsurprenant if would be to nice if one of you can review this.

@colinsurprenant

This comment has been minimized.

Copy link
Contributor

commented Jun 9, 2015

@purbon can you explain how you see the user workflow for packaging, transferring and installing local/cached gems, starting with someone who downloads a logstash package (tgz/zip/.deb/.rpm) ?

@purbon

This comment has been minimized.

Copy link
Contributor Author

commented Jun 9, 2015

@colinsurprenant thank you for taking your time to review, starting from a deployed package I see users:

  • Updating their plugins in a machine with internet connection (should have same architecture as the other ones, just to make sure deps are playing nice).
  • Dumping this into a vendor/cache directory, this can be done here by calling rake package:build-cache
  • Copying this to the others (offline) logstash deployments.
  • Running bin/plugin update --local

This is implemented as a bundler friendly option, basically leveraging that bundler package can do, and not excluding any other strategy. Even if with a bit of work, this would benefit users who face with offline setups.

Makes sense to you?

@colinsurprenant

This comment has been minimized.

Copy link
Contributor

commented Jun 9, 2015

@purbon a few notes:

  • machine architecture should not be an issue, unless some of the gem deps have FFI C extensions?
  • rake package:build-cache is not available on a deployed package, no? I mean I don't think the rakelib is included in the package nor can we assume the user has rake installed?
  • assuming the plugin dumping works, how do you suggest the user copy to target system? we tell them to tar, copy, untar?
@purbon

This comment has been minimized.

Copy link
Contributor Author

commented Jun 9, 2015

@colinsurprenant

  • rake package:build-cache rakefile should be added ot the package (this is a bug) thanks for noticing. I will fix.
  • You're right about architecture, it makes sense to me.

Speaking about the cache generation, I think it would be nice to have a zip/tar file being generated by the rake task, so then people can rsync that and script the change, makes sense ? do you have in mind another sharing strategy?

@colinsurprenant

This comment has been minimized.

Copy link
Contributor

commented Jun 9, 2015

@purbon I am not sure about the rakelib. That implies a dependency on rake (which is actually already included in the gems) but more importantly we'd have to expose a bin/rake. Do we want this? all rakelib stuff is development stuff...

instead of exposing the whole rakelib, why not make it a function of the plugin manager? like bin/plugin package or something? we can keep your work in the rakelib for the dev environment but expose a proper plugin manager interface for "production" packages?

@purbon purbon force-pushed the purbon:feature/plugins_offline branch 2 times, most recently from 7731c4a to b0bf18a Jun 9, 2015

@purbon

This comment has been minimized.

Copy link
Contributor Author

commented Jun 9, 2015

@colinsurprenant
makes complete sense to me, please see purbon@b0bf18a for details.

I'm thinking on adding an option to this command to pack the vendor/cache as zip/tar.gz, does this sounds good to you?

@colinsurprenant

This comment has been minimized.

Copy link
Contributor

commented Jun 9, 2015

@purbon yes! actually I think its a good idea so it's easier for the user to move this around. we'd just have to think about Windows and also provide the .zip options :P

Now one of the thing I noticed when I worked on this a bit was the cache directories duplication in Bundler. there is vendor/cache and there's vendor/bundle/jruby/1.9/cache - I'll lookup my code but I am wondering if we should try or not to unify both caches. In any case, since we are simply leveraging the Bundler package option maybe its better to just stick with the Bundler design for this :P

@suyograo suyograo added the v1.5.2 label Jun 10, 2015

@purbon purbon force-pushed the purbon:feature/plugins_offline branch from 990f597 to 0db5c8b Jun 10, 2015

@@ -15,6 +15,7 @@ module Environment
BUNDLE_DIR = ::File.join(LOGSTASH_HOME, "vendor", "bundle")
GEMFILE_PATH = ::File.join(LOGSTASH_HOME, "Gemfile")
LOCAL_GEM_PATH = ::File.join(LOGSTASH_HOME, 'vendor', 'local_gems')
CACHE_PATH = File.join(LogStash::Environment::LOGSTASH_HOME, "vendor", "cache")

This comment has been minimized.

Copy link
@colinsurprenant

colinsurprenant Jun 15, 2015

Contributor

in the previous 4 lines we use LOGSTASH_HOME but in this line it's LogStash::Environment::LOGSTASH_HOME please stay consistent unless there is a valid reason in which case they should all be updated.


def extract(source, target)
require "zip"
require "fileutils"

This comment has been minimized.

Copy link
@colinsurprenant

colinsurprenant Jun 15, 2015

Contributor

any reason why lazy require? in general this should be avoided. please bump these at the top. same for all require below.

This comment has been minimized.

Copy link
@purbon

purbon Jun 16, 2015

Author Contributor

I did the lazy require as we do it in so many places to load libraries, I tend to put everything on top. Can you educate me when is ok to be on top or when to do lazy require? thanks a lot.

This comment has been minimized.

Copy link
@colinsurprenant

colinsurprenant Jun 18, 2015

Contributor

I will answer below, in the main comments

require "zip"
require "fileutils"

puts "Extracting #{source} to #{target}"

This comment has been minimized.

Copy link
@colinsurprenant

colinsurprenant Jun 15, 2015

Contributor

I don't think these methods should puts, you cannot assume in what context they will be used: it could be used within logstash as some point where the @logger should be used for logging information. I think you should move all puts inside the plugin manager methods that uses these compress methods. same for all puts below.

end
puts "finished"
end

This comment has been minimized.

Copy link
@colinsurprenant

colinsurprenant Jun 15, 2015

Contributor

extra empty line

extend self

def extract(file, target)
raise Exception.new("Not using JRuby") unless LogStash::Environment.jruby?

This comment has been minimized.

Copy link
@colinsurprenant

colinsurprenant Jun 15, 2015

Contributor

this is not JRuby specific?


Zlib::GzipReader.open(file) do |gzip_file|
Gem::Package::TarReader.new(gzip_file) do |tar_file|
tar_file.each do |entry|

This comment has been minimized.

Copy link
@colinsurprenant

colinsurprenant Jun 15, 2015

Contributor

indentation

end

def compress(dir, target)
raise Exception.new("Not using JRuby") unless LogStash::Environment.jruby?

This comment has been minimized.

Copy link
@colinsurprenant

colinsurprenant Jun 15, 2015

Contributor

this is not JRuby specific?

gzip(target, tar_file)
puts "finished"

tar_file.close

This comment has been minimized.

Copy link
@colinsurprenant

colinsurprenant Jun 15, 2015

Contributor

the temporary tar_file is never deleted?


extend self

def extract(source, target)

This comment has been minimized.

Copy link
@colinsurprenant

colinsurprenant Jun 15, 2015

Contributor

I think it's a good practice to document each method, description, parameters, return value, etc. for example see https://github.com/elastic/logstash/blob/master/lib/logstash/util/accessors.rb#L87-L92

@@ -8,6 +8,7 @@ class LogStash::PluginManager::Install < LogStash::PluginManager::Command
option "--version", "VERSION", "version of the plugin to install"
option "--[no-]verify", :flag, "verify plugin validity before installation", :default => true
option "--development", :flag, "install all development dependencies of currently installed plugins", :default => false
option "--local", :flag, "update a plugin from a local cache", :default => false

This comment has been minimized.

Copy link
@colinsurprenant

colinsurprenant Jun 15, 2015

Contributor

shouldn't it be "install a plugin from ..." ?

@@ -85,8 +86,9 @@ def install_gems_list!(install_list)
gemfile.save

bundler_options = {:install => true}
bundler_options[:without] = [] if development?
bundler_options[:without] = [] if development? || local?

This comment has been minimized.

Copy link
@colinsurprenant

colinsurprenant Jun 15, 2015

Contributor

why clear :without if local?

This comment has been minimized.

Copy link
@purbon

purbon Jun 16, 2015

Author Contributor

no reason, leftover from research during the initial faces of this. Thanks a lot for noticing.

option "--clean", :flag, "clean up the generated dump of plugins", :default => false

def execute
signal_error("File #{LogStash::Environment::GEMFILE_PATH} does not exist or is not writable, aborting") unless File.writable?(LogStash::Environment::GEMFILE_PATH)

This comment has been minimized.

Copy link
@colinsurprenant

colinsurprenant Jun 15, 2015

Contributor

maybe we should just create the directory if the does not exist?

This comment has been minimized.

Copy link
@purbon

purbon Jun 16, 2015

Author Contributor

GEMFILE_PATH points to the Gemfile, I just used the regular checkup existence in the other plugin manager requests. Makes sense? or you were thinking about something else?

This comment has been minimized.

Copy link
@colinsurprenant

colinsurprenant Jun 18, 2015

Contributor

oh, yes I was confused.
but again, is this check necessary here? will this update the Gemfile?


def compress_cache(location)
require "logstash/util/compress"
target_file = File.join(LogStash::Environment::LOGSTASH_HOME, "plugins_cache")

This comment has been minimized.

Copy link
@colinsurprenant

colinsurprenant Jun 15, 2015

Contributor

what will happen if target_file already exists?

This comment has been minimized.

Copy link
@purbon

purbon Jun 16, 2015

Author Contributor

I added a few improvements in here, basically a check for this file plus options to override it from the command line or user interaction. Let me know if this makes sense to you.

@ph

This comment has been minimized.

Copy link
Member

commented Nov 12, 2015

I run this branch with the integration testing branch in #4166

All green, good job.

/e/logstash git:pr/3404 ❯❯❯ rake test:integration:local
[integration_spec] configuring local environment for running test in /Users/ph/es/logstash/integration_run, if you want to change this behavior delete the directory.
[integration_spec] configure environment
[integration_spec] Rsync source code into: /Users/ph/es/logstash/integration_run
[integration_spec] Finish rsync
[integration_spec] Running the test in /Users/ph/es/logstash/integration_run/logstash
[integration_spec] Running specs

bin/plugin install
  when the plugin doesn't exist
    fails to install
  with a local gem
    install the gem succesfully
  when the plugin exist
    sucessfully install
    allow to install a specific version

bin/plugin uninstall
  when the plugin isn't installed
    fails to uninstall it
  when the plugin is installed
    succesfully uninstall it

bin/logstash
  returns the logstash version

update
  update a specific plugin
    has executed succesfully
  update all the plugins
    has executed succesfully

File input to File output
  writes events to file

bin/plugin list
  without a specific plugin
    list the plugins with their versions
    display a list of plugins
    display a list of installed plugins
  with a specific plugin
    list the plugin and display the plugin name
    list the plugin with his version

Finished in 4 minutes 3.2 seconds (files took 3.79 seconds to load)
15 examples, 0 failures

Randomized with seed 14641
@@ -27,6 +27,12 @@ def set_key(key, value, hash, file)
value
end
end

::Bundler::Source::Rubygems.module_exec do

This comment has been minimized.

Copy link
@colinsurprenant

colinsurprenant Nov 16, 2015

Contributor

this patch is not documented and I don't know what it does and why it's need. please add documentation.

# @return [String, Exception] the installation captured output and any raised exception or nil if none
def invoke!(options = {})
options = {:max_tries => 10, :clean => false, :install => false, :update => false, :without => [:development]}.merge(options)
options = {:max_tries => 10, :clean => false, :install => false, :update => false, :local => false,
:all => false, :package => false, :without => [:development]}.merge(options)

This comment has been minimized.

Copy link
@colinsurprenant

colinsurprenant Nov 16, 2015

Contributor

the doc comment says that :all default is true but the options initialization sets it to false. ?

# @param target [String] Where you do want the file to be extracted
# @raise [IOError] If the target directory already exist
def extract(source, target)
raise IOError.new("Directory #{target} exist") if ::File.exist?(target)

This comment has been minimized.

Copy link
@colinsurprenant

colinsurprenant Nov 16, 2015

Contributor

the exceptions should be wrapped into as module/class-centric exception, for example see

class GemfileError < StandardError; end
or
class Error < StandardError; end

module LogStash
  def CompressError < StandardError; end
end
@@ -25,6 +25,16 @@ def set_key(key, value, hash, file)
end
end

## Patch to force bundler Rubygems to fetch data from the

This comment has been minimized.

Copy link
@colinsurprenant

colinsurprenant Nov 16, 2015

Contributor

in #4123 I got rid of lib/logstash/patches/bundler.rb is it was redundant and useless WRT lib/bootstrap/bundler.rb. In general if you need to duplicate code, there is something fishy and it should be addressed.


desc "Build a package with all the plugins, including dependencies, to be installed offline"
task "plugins-all" => ["test:install-all", "bundle"]

This comment has been minimized.

Copy link
@colinsurprenant

colinsurprenant Nov 16, 2015

Contributor

cosmetic: useless \n

expect(zip_file).to receive(:extract).exactly(3).times
subject.extract(source, target)
end

This comment has been minimized.

Copy link
@colinsurprenant

colinsurprenant Nov 16, 2015

Contributor

cosmetic: useless \n

expect(zip_file).to receive(:add).exactly(3).times
subject.compress(source, target)
end

This comment has been minimized.

Copy link
@colinsurprenant

colinsurprenant Nov 16, 2015

Contributor

cosmetic: useless \n

end

end

This comment has been minimized.

Copy link
@colinsurprenant

colinsurprenant Nov 16, 2015

Contributor

cosmetic: useless \n

expect(File).to receive(:open).exactly(3).times
subject.extract(source, target)
end

This comment has been minimized.

Copy link
@colinsurprenant

colinsurprenant Nov 16, 2015

Contributor

cosmetic: useless \n

expect(subject).to receive(:gzip).with(target, tar_file)
subject.compress(source, target)
end

This comment has been minimized.

Copy link
@colinsurprenant

colinsurprenant Nov 16, 2015

Contributor

cosmetic: useless \n

end

end

This comment has been minimized.

Copy link
@colinsurprenant

colinsurprenant Nov 16, 2015

Contributor

cosmetic: useless \n

@colinsurprenant

This comment has been minimized.

Copy link
Contributor

commented Nov 16, 2015

Just did a round of reviews. a few important concerns and some minor/cosmetic ones.

@colinsurprenant

This comment has been minimized.

Copy link
Contributor

commented Nov 17, 2015

LGTM

  • before merge I'd prefer that a final round to tests be performed to make sure no regression have been introduced with the new default in 49705a5
  • also, followup issues #4219 #4220
@purbon

This comment has been minimized.

Copy link
Contributor Author

commented Nov 17, 2015

@colinsurprenant this the test before committing the change. Thanks for your time, will squash (commit history is really screw up) and merge soon.

@purbon purbon removed the reviewing label Nov 17, 2015

@ph

This comment has been minimized.

Copy link
Member

commented Nov 17, 2015

❤️

Pere Urbon-Bayes
This adds a feature to let users dump all their installed plugins,
including dependencies, and reuse them in an offline installation by
providing an package for it. It adds two important commands to the
plugin manager, the pack and upack, to handle package creation and
installation and adds the --local flag to install and update to pull
plugins from the installed local packages.

Former commits:

add a task to create a bundle, under vendor/cache of the installed gems + their dependencies, this can be used later on to be installed offline

add an option to pass the --local flag to the bin/plugin update task, so it fetch plugins from a local cache, by default under vendor/cache

rename package:build to package:build-cache as is more meaningfull

add a --local flag to bin/plugin install to users can install plugins from the local cache, under the default directory of vendor/cache

add a plugin manager command to build the local cache of installed plugins using bundler package alike command

It adds code for compressing and extracting in zip and tar formats to
the utils module of logstash. The zip module is only jruby dependant as
it uses functions provided by java.
There is also code add in the plugin manager package command to handle
compression of plugins dumping.

Cleanup the custom java code to compress and extract zip files as it has
been known that using rubyzip is good and it has a more ruby like
features so the code is more clean and portable.

set of smallish improvement requested during review

added several options to handle situation when the package of plugins we want to generate is already there

clean up old code

applyed feedback from review, mostly changed in documentating behaviour plus better wording for messages

relocate the Environment.windows? check to the bootstrap side as it's used also in the plugin manager

add an unpack bin/plugin command so users can install packages of plugins throw the plugin manager

document override behaviour in the compress helpers (for zip and tar) plus add a fix for the tar extract when reading entries

made the unpack filename parameter a required one

add a force option to the bin/plugin unpack command

add a notice to that if using a local cache the --local flag should be passed with

Code cleanup and refactor introduced during review

add two wording suggestions comming from review

ammend more wording

skip the major version validation as in situation where there is no internet connection like when using the local vendor/cache to do the update

move compress to the bootstrap environment as being used in the plugin manager means not being loaded with logstash-core

Bring pack cached gems in the vendor directory so they can be used for bundler when packaging dependencies

Revert "Bring pack cached gems in the vendor directory so they can be used for bundler when packaging dependencies"

This reverts commit a9d7f46.

patch the Bundler::Source::Rubygems to fetch only gems from a remote  source

small changes in several parts of the plugin manager and the creation of a common pack command with shared code

change compress to read in chuncks

fix wrong var name in the bootstrap compress utils module

fix namespacing conflicts

add basic test for the compress utility module

ammend more namespace issues

add a comment to the rubygems mockey patch to rebuild the gem cache

apply cosmetic changes

make the compress module raise CompressError

remove vendor_path and pattern_path form environment as they where mix up during rebase

change the bin/pack force option to be force-delete-cache

rename force_delete_cache to overwrite

change force for overwrite in tha pack command

remove the override option for bin/plugin unpack

revert Gemfile and Genfile.lock wrong committed

@purbon purbon force-pushed the purbon:feature/plugins_offline branch from 794edd7 to 313684e Nov 18, 2015

@purbon

This comment has been minimized.

Copy link
Contributor Author

commented Nov 18, 2015

Merging this now to master and 2.x, backporting then manually to 2.1 as I expect there to have some conflicts.

@purbon

This comment has been minimized.

Copy link
Contributor Author

commented Nov 18, 2015

Merged to master in 2195f14, closing this and backporting.

@purbon

This comment has been minimized.

Copy link
Contributor Author

commented Nov 18, 2015

Merged and backported to master, 2.x and 2.1

@purbon purbon deleted the purbon:feature/plugins_offline branch May 10, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.