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

Quick Fix for version update issues #178

Closed
pivotal-casebook opened this issue Jan 22, 2014 · 10 comments
Closed

Quick Fix for version update issues #178

pivotal-casebook opened this issue Jan 22, 2014 · 10 comments
Labels
Bug Something isn't working

Comments

@pivotal-casebook
Copy link

The way that the default attributes are setup blocks the ability to change the version.

default.rb

# === VERSION AND LOCATION
#
default.elasticsearch[:version]       = "0.90.5"
default.elasticsearch[:host]          = "http://download.elasticsearch.org"
default.elasticsearch[:repository]    = "elasticsearch/elasticsearch"
default.elasticsearch[:filename]      = "elasticsearch-#{node.elasticsearch[:version]}.tar.gz"
default.elasticsearch[:download_url]  = [node.elasticsearch[:host], node.elasticsearch[:repository], node.elasticsearch[:filename]].join('/')

When this cookbook is compiled, it takes the version and then also sets the attributes for filename and download_url. If you only change the version, the pre-compiled filename and download_url are still stuck on 90.5.

This is dangerous for anyone who thinks they're upgrading their version by simply changing the number. All the directories will be created with the new version number, but the downloaded file is still 90.5. It also installs the 90.5 jar in the new version directory.

To fix this, you need to specifically set all three attributes. We've added these to our wrapper cookbook's default.rb file and the main cookbook now downloads and installs the correct version.

wrapper_cookbook/attributes/default.rb

default.elasticsearch[:version]       = "0.90.10"
default.elasticsearch[:filename]      = "elasticsearch-#{node.elasticsearch[:version]}.tar.gz"
default.elasticsearch[:download_url]  = [node.elasticsearch[:host], node.elasticsearch[:repository], node.elasticsearch[:filename]].join('/')

Helps/Fixes #100 #163 #165

A better fix would be to calculate the filename and download_url where they are used or by setting the node value inside the cookbook itself so it would pick up the correct version.

@karmi
Copy link
Contributor

karmi commented Jan 22, 2014

Thanks. There are many issues being reported about changing versions being counter-intuitive or downright buggy. I just haven't found the time to investigate, verify and potentially fix this.

As a side note, there are couple of related bugs and inconsistencies due to problems with the ark provider, which everybody seems to like being removed, in favour of plain remote_file and untarring/mv-ing/etc...

pivotal-casebook pushed a commit to Casecommons/cookbook-elasticsearch that referenced this issue Jan 22, 2014
The attributes are being pre-calculated so they don't pickup
a custom version set by the user.

Fixes sous-chefs#178
pivotal-casebook pushed a commit to Casecommons/cookbook-elasticsearch that referenced this issue Jan 22, 2014
The attributes are being pre-calculated so they don't pickup
the custom version set by the user which can lead to issues
with the wrong version being installed.

If a user needs to create a custom download_url, they can
set the host and repository attributes in their wrapper cookbook
or on the chef server. For example to create the url:
http://example/downloads/elasticsearch-0.90.10.tar.gz
you set the attributes as:

default.elasticsearch[:version] = '0.90.10'
default.elasticsearch[:host] = 'http://example'
default.elasticsearch[:repository] = 'downloads'

Fixes sous-chefs#178
@pivotal-casebook
Copy link
Author

It seems @weiner did the same thing in #165. We added a bit of an explanation on how you can still get a custom url without being able to directly set the attribute.

@brianz
Copy link

brianz commented Jan 23, 2014

Thanks for the workaround...that did the trick for me. It'd be great to see this fix merged.

@jsirex
Copy link

jsirex commented Sep 16, 2014

If I add my-elasticsearch::customize attribute, how it affects elasticsearch::customize?
How can I add elasticsearch::customize attribute file? where to put it? Modifying already existing cookbooks is bad idea.

I think doing such manipulations in attributes files is bad idea at all.
When chef runs, before all recipes evaluated, it evaluates attributes one by one.
elasticsearchs' attributes are loaded first with default values ALWAYS, then chef loads my customizations. No meter it is default/normal/set/override/force-override. Other values are already calculated. And yes, they are wrong.

I think best way is to move such manipulation on runtime (e.g. in recipe).

In attributes files we have only properties. On runtime all things are calculated in final url.

In our case what actually we have here? Actually we have 3 simple entity:

  1. download url
  2. target file where to save it
  3. version
    What can be calculated based on version? At most target file in form elasticsearch-#{version}.tar.gz
    Download url can be anything. For example: http://mymirror/only/one/and/the/best/elasticsearch-distro.tar.gz.

But If you want to preserve original functionality and runtime calculation then just:

# Attribute file
default.elasticsearch[:version]       = "0.90.12"
default.elasticsearch[:host]          = "http://download.elasticsearch.org"
default.elasticsearch[:repository]    = "elasticsearch/elasticsearch"
default.elasticsearch[:filename]      = nil
default.elasticsearch[:download_url]  = nil
# Recipe file
node.default['elasticsearch']['filename'] = "elasticsearch-#{node.elasticsearch[:version]}.tar.gz" unless node['elasticsearch']['filename']
node.default['elasticsearch']['download_url']  = [node.elasticsearch[:host], node.elasticsearch[:repository], node.elasticsearch[:filename]].join('/') unless node['elasticsearch']['download_url']

In a wrapper cookbook I can do

default['elasticsearch']['version'] = '1.1.0' # Yes, default. there is no need to use set or override. But you can
default['elasticsearch']['host'] = "http://mymirror.com" # Change host. No problem

# also works this
# version will be 1.1.0
# filename elasticsearch-1.1.0.tar.gz
# download url "http://other.com/es.tar.gz" 
default['elasticsearch']['download_url'] = "http://other.com/es.tar.gz" 

Want more flexibility? Ok - change logic on runtime in the same way

# wrapper-cookbook::recipe
# Do before including recipe elasticsearch
node.default['elasticsearch']['filename'] = "es-custom-#{node.elasticsearch[:version]}.tar.gz" unless node['elasticsearch']['filename']

include_recipe 'elasticsearch'

Of course, last example requires wrapper-cookbook to appears earlier than elasticsearch cookbook.

@jsirex
Copy link

jsirex commented Sep 16, 2014

UPD: Currently to hack this problem I'm using copy-paste and recalculation of attributes:

# My wrapper-cookbook/attributes/default.rb
default['elasticsearch']['version'] = '1.1.0'

# Hack of elastic attributes
default['elasticsearch']['filename']      = "elasticsearch-#{node['elasticsearch']['version']}.tar.gz"
default['elasticsearch']['download_url']  = [node['elasticsearch']['host'], node['elasticsearch']['repository'], node['elasticsearch']['filename']].join('/')

This also works as expected

@klamontagne
Copy link

Hi, I like this method to defer calculating derived attributes: https://coderanger.net/derived-attributes/

TL;DR

default['version'] = '1.0'
default['url'] = "http://example.com/%{version}.zip"

remote_file '/tmp/example.zip' do
  source node['url'] % {version: node['version']}
end

This should become standard in cookbooks IMO.

@jsirex
Copy link

jsirex commented Sep 16, 2014

Hm. Don't know about 'Delayed Interpolation' in ruby. Will experiment with this.
Anyway, general idea is leave attributes for settings some properties, move any logic and further actions on to runtime, e.g. in recipe

Anyway, thank you for link very much!

@martinb3
Copy link
Contributor

martinb3 commented Oct 3, 2014

This is simply the way Chef currently works. The only way to work around attributes like this is to convert this cookbook to a library-style cookbook and don't pre-compute any derived attributes -- see #180 for an issue proposing that (this is why converting to more of a library cookbook with LWRPs is so popular right now). Most users of library cookbooks will understand that they cannot simply override the version attribute -- they will need to override the version and any attributes derived using it.

I think this is a non-issue, as it's simply how Chef currently works for overriding multiple derived attributes.

karmi pushed a commit that referenced this issue Oct 13, 2014
This prevents a number of errors and surprises when setting the Elasticsearch version.

If the user overrode the node.elasticsearch[:version] property, the download directory and symlinks would be
created with the correctly overridden version number. However, the download URL would be computed with
the default version number, and thus the incorrect, default version of elasticsearch would be installed.

Since there's no need to compute the download filename and URL until node convergence, this patch moves that computation to the default.rb recipe. However, the node.elasticsearch[:filename] and node.elasticsearch[:download_url] attributes, if present, are respected and given preference upon node convergence, so that users still have the capability to override these attributes and thus configure downloads from a custom repository.

Related: #100, #180, #178, #240, #179, #245, #227, #245

Closes #242
@karmi
Copy link
Contributor

karmi commented Oct 13, 2014

Hi, I've merged #242, so this bug should be fixed now. I'm sorry for the inconvenience the bad behaviour caused you. Closing this issue for now, please ping me if you would like to discuss it further.

@modesolaa
Copy link

I have been having issues with elasticsearch cookbook. It tries to use yum which is not part of the cookbook. Can anyone help with resolving the issue?

elasticsearch_install 'elasticsearch_install' do
type 'tarball' # type of install
dir '/usr/local' # where to install

download_url "#{node['tgis_elk_config']['common_source_path']}/elasticsearch-6.2.2.tar.gz"
enable_repository_actions 'false'
action :install # could be :remove as well
end

elasticsearch_configure 'elasticsearch_configure' do
allocated_memory "#{node['tgis_elk_config']['allocated_memory']}"
configuration ({
'cluster.name' => "#{node['tgis_elk_config']['cluster_name']}",
'node.name' => "#{node['tgis_elk_config']['node_name']}"
})
end

elasticsearch_service 'elasticsearch_service' do
service_actions [:enable, :start]
end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants