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

Introduce provider and resource for configure #316

Merged
merged 1 commit into from Jun 8, 2015

Conversation

Projects
None yet
3 participants
@martinb3
Copy link
Contributor

commented Apr 29, 2015

  • Introduce provider and resource for configure (elasticsearch.yml and elasticsearch-env.sh).
  • Introduce provider and resource for service (elasticsearch.init.erb) and servide def'n
  • Add empty resource and provider for managing plugins

@martinb3 martinb3 force-pushed the martinb3-cookbooks:next_martinb3 branch 2 times, most recently from b3cd065 to 8150f79 Apr 29, 2015

@@ -31,5 +31,39 @@ def get_source_home_dir(new_resource, node)
def get_source_root_dir(new_resource, node)
new_resource.dir || node.ark[:prefix_root]
end

# This method takes a hash, but will convert to mash

This comment has been minimized.

Copy link
@karmi

karmi May 1, 2015

Member

I'd like to have a "regular" RDoc/YARD documentation for these Ruby methods, including at least one @example section. Do you think it's a good idea?

This comment has been minimized.

Copy link
@martinb3

martinb3 May 1, 2015

Author Contributor

👍 I do. :)

# Create data path directories
#
data_paths = new_resource.path_data.is_a?(Array) ? new_resource.path_data : new_resource.path_data.split(',')

This comment has been minimized.

Copy link
@karmi

karmi May 1, 2015

Member

What if we split these different features into separate Ruby methods, and then call them in the action_manage method, in a "composed method" pattern? Is it worthwile to do it? Would that be an antipattern in Chef? Would that make testing more transparent (ie. we can test separately thate create_data_path_directories creates the folders, and analogically with create_user, create_logging_configuration_file)?

This comment has been minimized.

Copy link
@martinb3

martinb3 May 1, 2015

Author Contributor

I think the chef pattern would be to reuse the directory resource where-ever possible, until we encounter features we need but the directory provider in chef doesn't have (and even then, we could call the provider in Ruby, instead of the recipe DSL). But the common chef pattern would also be to split out the logging configuration into a separate resource and provider. I stopped at this design, but I think it's probably better to split it out.

Basically anything that feels like a separate 'object' in the system should be it's own resource/provider. So probably we could take the logging file and move it out of the configure resource/provider. This would have the added benefit of making it optional.

This comment has been minimized.

Copy link
@karmi

karmi May 1, 2015

Member

That would be great -- let's strive for as much "separation" and modularity with the refactored cookbook as possible. I think my general approch is: we can save on implementing features, but invest into the codebase itself (modularity, testability, ...)

end
end

raise 'Could not determine the plugin directory. Please set plugin_dir on this resource.' unless new_resource.plugin_dir

This comment has been minimized.

Copy link
@karmi

karmi May 1, 2015

Member

Should we have a custom exception class for this?

This comment has been minimized.

Copy link
@martinb3

martinb3 May 1, 2015

Author Contributor

I don't think it will matter for chef when it's raised. I'd love to get rid of this logic entirely.

return if Dir.entries(new_resource.plugin_dir).any? do |plugin|
next if plugin =~ /^\./
name.include? plugin
end rescue false

This comment has been minimized.

Copy link
@karmi

karmi May 1, 2015

Member

This rescue false looks like a copy&paste leftover from the old code, I think we should be more careful with this knife this time :) I don't even remember why it's needed here.

This comment has been minimized.

Copy link
@martinb3

martinb3 May 1, 2015

Author Contributor

I'm assuming it is to make the chef code return/exit, vs. raise an exception. There's no nice (idempotent) way to ask bin/plugin to tell us if a plugin is already installed is there? It would be super if there was some way to say, "Install if we don't have it," but as far as I know, the plugin install binary that comes with ES doesn't do this.

This comment has been minimized.

Copy link
@karmi

karmi May 1, 2015

Member

No, I don't think the plugin script does that, but it supports plugin --list. That merely lists the contents of the directory, though, so it does the same thing. What kind of exception can occur here?


# Increase open file and memory limits
#
bash "enable user limits" do

This comment has been minimized.

Copy link
@karmi

karmi May 1, 2015

Member

We might need not to fiddle with system limits, if we do what the init scripts for deb/rpm packages do, and that is call ulimit during the startup process? What is best from the system operation / devops perspective?

This comment has been minimized.

Copy link
@martinb3

martinb3 May 1, 2015

Author Contributor

We should definitely try to model the deb/rpm scripts; I think that's the principle of least surprise.

This comment has been minimized.

Copy link
@karmi

karmi May 1, 2015

Member

Then let's get rid of the system limits entirely?

@@ -0,0 +1,26 @@
# JVM Configuration for ElasticSearch

This comment has been minimized.

Copy link
@karmi

karmi May 1, 2015

Member

I'm wondering if we need this file at all. What if we just rely on proper environment variables like ES_HEAP_SIZE being exported, and the init scripts either using them or setting them when they are not exported?

This comment has been minimized.

Copy link
@martinb3

martinb3 May 1, 2015

Author Contributor

👍 I'd love to make this simpler; I didn't make any assumptions about why this was needed, so I just brought it over. Let's get rid of it.

This comment has been minimized.

Copy link
@karmi

karmi May 1, 2015

Member

🤘 :)

@@ -0,0 +1,193 @@
#!/usr/bin/env bash

This comment has been minimized.

Copy link
@karmi

karmi May 1, 2015

Member

I would love to get rid of this file completely. I have couple of ideas:

  • Using the init scripts for Ubuntu and Redhat/Centos from the main repository (.deb and .rpm packages provide them)
  • Adding just a really minimal, stupid init script and encourage people provide their own (something really simple like Nginx has?

This comment has been minimized.

Copy link
@martinb3

martinb3 May 1, 2015

Author Contributor

It does seem like most cookbooks end up providing an init script, unfortunately.

Adding just a really minimal, stupid init script and encourage people provide their own (something really simple like Nginx has?

That seems like a good way to go.

This comment has been minimized.

Copy link
@karmi

karmi May 1, 2015

Member

Agreed. Let's keep it as simple and stupid as possible, please!


echo -en "\033[1mStarting Elasticsearch...\033[0m"
touch $PIDFILE && chown <%= @user %> $PIDFILE
<% if @platform_family == 'debian' %>

This comment has been minimized.

Copy link
@karmi

karmi May 1, 2015

Member

I always hated this: I think if we do provide an init script template, we need to put them to separate, platform specific directories?

This comment has been minimized.

Copy link
@martinb3

martinb3 May 1, 2015

Author Contributor

I think that makes sense as well. I think we should merge this, and then open an issue to work on splitting it out.

This comment has been minimized.

Copy link
@karmi

karmi May 1, 2015

Member

Yes, but let's trim the init script. We can of course split in another PR, but it would be fine to do it in this one for me,

# 'threadpool.index.size' => '2'
# // ...

################################### Cluster ###################################

This comment has been minimized.

Copy link
@karmi

karmi May 1, 2015

Member

Can we base the template on the following pruned version: https://github.com/elastic/elasticsearch/blob/trim-config/config/elasticsearch.yml ?

Also, since we support putting any key:value pair from @config file, we don't need all those lines? It will be included in the file automatically?

This comment has been minimized.

Copy link
@martinb3

martinb3 May 1, 2015

Author Contributor

Can we base the template on the following pruned version: https://github.com/elastic/elasticsearch/blob/trim-config/config/elasticsearch.yml ?

Sure!

Also, since we support putting any key:value pair from @config file, we don't need all those lines? It will be included in the file automatically?

That's also correct. Anything we want formatted in any fancy way, we can template out, otherwise the lines will just go in as-is :)

This comment has been minimized.

Copy link
@karmi

karmi May 1, 2015

Member

Great! Let's used the pruned version of the .yml file and leave everything else to the @config traversal...

@martinb3 martinb3 closed this May 1, 2015

@martinb3 martinb3 reopened this May 1, 2015

@martinb3

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2015

Closing and opening to force Travis to rebuild.

Introduce provider and resource for configure
- Introduce provider and resource for configure (elasticsearch.yml and elasticsearch-env.sh).
- Introduce provider and resource for service (elasticsearch.init.erb) and servide def'n
- Introduce provider and resource for plugin installation

@martinb3 martinb3 force-pushed the martinb3-cookbooks:next_martinb3 branch from 8150f79 to afa0b23 May 1, 2015

@lmunro

This comment has been minimized.

Copy link

commented Jun 4, 2015

👍 can't wait for this to be merged. nice work guys!

@martinb3

This comment has been minimized.

Copy link
Contributor Author

commented Jun 8, 2015

I've created issues for many of the items discussed here. I'm going to merge this, so others can build from it; hope that's okay.

martinb3 added a commit that referenced this pull request Jun 8, 2015

Merge pull request #316 from martinb3/next_martinb3
Introduce provider and resource for configure

@martinb3 martinb3 merged commit 90916d9 into elastic:next Jun 8, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@karmi

This comment has been minimized.

Copy link
Member

commented Jun 9, 2015

Hi Martin, great, this is a perfect approach.

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.