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

Windows agent installation support #210

Merged
merged 17 commits into from Oct 22, 2015

Conversation

Projects
None yet
6 participants
@olivielpeau
Copy link
Member

commented Jun 10, 2015

Reworked commits from PR #188 and #162 to support agent installation (and upgrade) and deploy checks configuration files on Windows.

Tested successfully on Chef 12. On Chef 11, during an installation run, the agent gets stuck in the 'Stopping' state when requested to stop or restart. I'm still tying to figure out if there's a good workaround.

Many thanks to @EasyAsABC123 and @rlaveycal for their initial PRs.

Closes #142, #188 and #164

@olivielpeau olivielpeau added the feature label Jun 10, 2015

@olivielpeau olivielpeau added this to the Windows! milestone Jun 10, 2015

@miketheman

This comment has been minimized.

Copy link
Collaborator

commented Jun 10, 2015

Looking pretty awesome. I'll take a deeper review tomorrow and provide some feedback.

Gemfile Outdated
@@ -21,6 +21,6 @@ end

group :integration do
gem 'kitchen-vagrant'
gem 'test-kitchen', '~> 1.3.1'
gem 'test-kitchen'

This comment has been minimized.

Copy link
@miketheman

miketheman Jun 12, 2015

Collaborator

Why the removal of any version constraint?

This comment has been minimized.

Copy link
@olivielpeau

olivielpeau Jun 12, 2015

Author Member

We actually need ~> 1.4.0 for windows support, I'll change it.

rescue ArgumentError
Chef::Log.warn "could not parse python version string: #{node['languages']['python']['version']}"
end

# Agent Version
# Set to `nil` to install latest (will also upgrade to latest on Windows)

This comment has been minimized.

Copy link
@miketheman

miketheman Jun 12, 2015

Collaborator

The comment is a bit misleading - it prompts the user to set it to nil, but it's already nil.

"Default of `nil` will install latest version. On Windows, this will also upgrade to latest"
metadata.rb Outdated
@@ -21,6 +21,7 @@
depends 'apt' # We recommend '>= 2.1.0'. See CHANGELOG.md for details
depends 'chef_handler', '~> 1.1.0'
depends 'yum'
depends 'windows'

This comment has been minimized.

Copy link
@miketheman

miketheman Jun 12, 2015

Collaborator

The introduction of another cookbook dependency is something that is never "awesome" - are you certain the default providers in Chef 10, 11 and 12 cannot be used?

This comment has been minimized.

Copy link
@olivielpeau

olivielpeau Jun 12, 2015

Author Member

I wanted to use the default providers at first but ran into a lot of issues with the windows_package from core :

  • the resource is broken in chef 12.2 and 12.3 (fixed in 12.4), see chef/chef#3316
  • using the resource with action :remove to uninstall the agent makes the recipe fail if the agent is not installed, instead of simply being up-to-date
  • the resource can upgrade the agent but sometimes breaks it in the upgrade (for instance the upgrade from 5.3.0 to 5.3.1 breaks the agent)
  • the original msi needs to be provided to uninstall the agent (whereas the windows cookbook handles that nicely by using registry keys to find the uninstaller)

I haven't had any of these issues with the windows cookbook.
It seems that the windows cookbook is gradually getting merged in chef core, until then I think we should use the cookbook.

owner 'dd-agent'
mode 00600
template "#{node['datadog']['config_dir']}/conf.d/#{new_resource.name}.yaml" do
unless node['platform_family'] == 'windows'

This comment has been minimized.

Copy link
@miketheman

miketheman Jun 12, 2015

Collaborator

what ownership and permissions get set if on Windows?

@@ -0,0 +1,43 @@
#
# Cookbook Name:: datadog
# Recipe:: dd-agent-install-linux

This comment has been minimized.

Copy link
@miketheman

miketheman Jun 12, 2015

Collaborator

The recipe name here gets lengthy.

Please use something like _install-linux.rb so the final name will look like:

include_recipe 'datadog::_install-linux' unless platform_family? 'windows'
# Install the package
windows_package 'Datadog Agent' do
source temp_file
options "APIKEY=\"#{node['datadog']['api_key']}\" HOSTNAME=\"#{node['hostname']}\" TAGS=\"#{node['tags'].join(',')}\""

This comment has been minimized.

Copy link
@miketheman

miketheman Jun 12, 2015

Collaborator

You can use Ruby's interpolated string modifier to reduce the amount of quotes that you have to escape, like so:

options %(APIKEY="#{node['datadog']['api_key']}" HOSTNAME="#{node['hostname']}" TAGS="#{node['tags'].join(',')}")

For reference, %() is the same as %Q()

group 'root'
mode 0755
directory "#{node['datadog']['config_dir']}" do
unless node['platform_family'] == 'windows'

This comment has been minimized.

Copy link
@miketheman

miketheman Jun 12, 2015

Collaborator

Again, if on windows, what happens?

it_behaves_like 'datadog-agent service'

it 'ensures the Datadog config directory exists' do
expect(@chef_run).to create_directory "#{ENV['ProgramData']}/Datadog"

This comment has been minimized.

Copy link
@miketheman

miketheman Jun 12, 2015

Collaborator

Instead of determining and interpolating ENV during the test, set it to something realistic in the block where you construct the Windows object, and then compare with the static value here.
This will eliminate needing to look up an environment variable during testing that is likely not going to be there anyhow.

See here for an example of how you might stub an environment variable.

@miketheman

This comment has been minimized.

Copy link
Collaborator

commented Jun 12, 2015

@olivielpeau Looking good a bunch of comments.

This supplies updates to chefSpec - what about some test-kitchen tests for this behavior?

@olivielpeau

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2015

@miketheman Thanks a lot for your comments! I'll update the PR accordingly and add some test-kitchen tests.

@olivielpeau olivielpeau force-pushed the olivielpeau/windows-support branch from 9c5582f to 7a81e22 Jun 19, 2015

@olivielpeau

This comment has been minimized.

Copy link
Member Author

commented Jun 19, 2015

@miketheman I've updated the PR, it now has kitchen tests + solves the issues you pointed out.

@@ -4,6 +4,9 @@ driver:
customize:
memory: 1024

provisioner:
data_path: test/shared # directory containing shared helpers for integration tests

This comment has been minimized.

Copy link
@miketheman

miketheman Jul 4, 2015

Collaborator

This should not be needed - we already support shared directories via the test/integration/helpers subdirectory, so this should also simplify the path handling.

If there's a common spec helper, then let's move all the common requires and set path into there.


- name: <%= windows_platform %>-<%= chef_version %>
driver_config:
box: mwrock/Windows2012R2 # lightweight windows box

This comment has been minimized.

Copy link
@miketheman

miketheman Jul 4, 2015

Collaborator

"light"? Size: 3.3GB 😀

This comment has been minimized.

Copy link
@olivielpeau

olivielpeau Jul 6, 2015

Author Member

Most boxes are >4GB ; 3.3GB is as "light" as it gets 😃

excludes:
<% chef_versions.each do |chef_version| %>
- <%= windows_platform %>-<%= chef_version %>
<% end %>

This comment has been minimized.

Copy link
@miketheman

miketheman Jul 4, 2015

Collaborator

Build this array once and use YAML anchors to populate other recipes.

template "/etc/dd-agent/conf.d/#{new_resource.name}.yaml" do
owner 'dd-agent'
mode 00600
template "#{node['datadog']['config_dir']}/conf.d/#{new_resource.name}.yaml" do

This comment has been minimized.

Copy link
@miketheman

miketheman Jul 4, 2015

Collaborator

If we want to be fully cross-platform path-compliant, we should break elements into strings, and use File.join:

template File.join(node['datadog']['config_dir'], 'conf.d', "#{new_resource.name}.yaml") do

This will leverage the OS' native methods, instead of us relying on / working across platforms.

file "/etc/dd-agent/conf.d/#{new_resource.name}.yaml" do
if ::File.exist?("#{node['datadog']['config_dir']}/conf.d/#{new_resource.name}.yaml")
Chef::Log.debug "Removing #{new_resource.name} from #{node['datadog']['config_dir']}/conf.d/"
file "#{node['datadog']['config_dir']}/conf.d/#{new_resource.name}.yaml" do

This comment has been minimized.

Copy link
@miketheman

miketheman Jul 4, 2015

Collaborator

Same notes with regards to File.join

dd_agent_version = node['datadog']['agent_version']

# If version specified and lower than 5.x
if !dd_agent_version.nil? && dd_agent_version.split('.')[0].to_i < 5

This comment has been minimized.

Copy link
@miketheman

miketheman Jul 4, 2015

Collaborator

Let's add a Chef::Log.deprecation("Support for Agent pre 5.x will be removed in in datadog cookbook version 3.0") or something like that - so we can also remove anything related to datadog-agent-base in the future.

This comment has been minimized.

Copy link
@remh

remh Jul 6, 2015

Member

👍

template agent_config_file do
if node['platform_family'] == 'windows'
owner 'Administrators'
rights :read, 'Users'

This comment has been minimized.

Copy link
@miketheman

miketheman Jul 4, 2015

Collaborator

Should this be permissive? The file contains keys.

else
owner 'root'
group 'root'
mode 0644

This comment has been minimized.

Copy link
@miketheman

miketheman Jul 4, 2015

Collaborator

Maybe this is too permissive as well. I know it predates your change, but something to think about.

@miketheman

This comment has been minimized.

Copy link
Collaborator

commented Jul 4, 2015

@olivielpeau Awesome stuff! Ran through again, added some comments. It's looking better each pass.

@olivielpeau olivielpeau force-pushed the olivielpeau/windows-support branch 2 times, most recently from 8ea13ce to d3e3e1b Jul 7, 2015

@olivielpeau

This comment has been minimized.

Copy link
Member Author

commented Jul 7, 2015

Thanks @miketheman for the great review! I've addressed the issues you've found.

I've left the file permissions on Linux unchanged for now: we have a ticket in our backlog to review them on all of our supported installation methods.

@olivielpeau olivielpeau force-pushed the olivielpeau/windows-support branch from d3e3e1b to 280b5bf Jul 27, 2015

@miketheman miketheman referenced this pull request Aug 19, 2015

Closed

Pr/5 #188

@olivielpeau olivielpeau force-pushed the olivielpeau/windows-support branch from 280b5bf to fac5177 Oct 16, 2015

olivielpeau added some commits Jun 19, 2015

Update integration tests to support Windows
The bats tests on dd-agent are replaced by serverspec tests
Set more restrictive access rights to agent conf files on Windows
- Remove all rights to regular user as they should not need any
- Disable rights inheritance from parent directories
Move common helper of serverspec tests
- spec_helper.rb is moved to test/integration/helpers/serverspec
- move `require`s and `set :path` to the helper

@olivielpeau olivielpeau force-pushed the olivielpeau/windows-support branch from fac5177 to 6ced7e6 Oct 16, 2015

@olivielpeau

This comment has been minimized.

Copy link
Member Author

commented Oct 16, 2015

@miketheman I've fixed the issues you found, rebased the PR and re-run the kitchen tests so this is probably ready for a final review :)

@miketheman

This comment has been minimized.

Copy link
Collaborator

commented Oct 17, 2015

@olivielpeau Awesome! I think this is all ready to go, and will merge Monday. We can then start testing it internally, and hopefully find no bugs, then release to the world!

@miketheman miketheman self-assigned this Oct 17, 2015

miketheman added a commit that referenced this pull request Oct 22, 2015

Merge pull request #210 from DataDog/olivielpeau/windows-support
Windows agent installation support

@miketheman miketheman merged commit d653883 into master Oct 22, 2015

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details
@remh

This comment has been minimized.

Copy link
Member

commented Oct 22, 2015

🎉

@jboeshart

This comment has been minimized.

Copy link

commented Oct 22, 2015

Much thanks on getting this merged and making it available in a shorter timeline!

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.