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

Added Windows Support #259

Merged
merged 5 commits into from Jan 20, 2016

Conversation

Projects
None yet
6 participants
@Ginja
Copy link
Collaborator

Ginja commented Dec 30, 2015

  • Fixes #236
  • Added Windows Support (binary install_method only) using NSSM
    to manage the service. NSSM runs consul as the local SYSTEM user.
    Support for running it as another user can/should be added later.
  • Prevented the firewall cookbook from creating rules for disabled
    ports
  • Added & Updated spec tests
  • Disabled Style/ModuleFunction cop
  • Simplified how attributes with paths for values are set
  • Added version check to windows binary installation so that it can
    be upgraded

@Ginja Ginja force-pushed the visioncritical:windows-support branch from 6bf825f to 8a8f23d Dec 30, 2015

@Ginja Ginja force-pushed the visioncritical:windows-support branch from 8a8f23d to c3871cc Dec 30, 2015

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 30, 2015

Current coverage is 55.20%

Merging #259 into master will increase coverage by +6.57% as of 5fa290d

@@            master   #259   diff @@
=====================================
  Files            6      7     +1
  Stmts          329    317    -12
  Branches         0      0       
  Methods          0      0       
=====================================
+ Hit            160    175    +15
  Partial          0      0       
+ Missed         169    142    -27

Review entire Coverage Diff as of 5fa290d

Powered by Codecov. Updated on successful CI builds.

default['consul']['config']['data_dir'] = '/var/lib/consul'
default['consul']['config']['ca_file'] = '/etc/consul/ssl/CA/ca.crt'
default['consul']['config']['cert_file'] = '/etc/consul/ssl/certs/consul.crt'
case node['os']

This comment has been minimized.

Copy link
@johnbellone

johnbellone Dec 30, 2015

Contributor

This can be simplified quite a bit. There are two methods I am thinking of, the first being simply creating a temporary value for prefix directory and passing that through to all of the attributes. The second would be a heavier modification that would use interpolation later in the code. The File class provides a platform-independent method for joining paths. I'll illustrate the latter (the former will require much more testing):

prefix_path = case node['os']
              when 'linux', 'freebsd', 'mac_os_x'
                '/etc/consul'
              when 'windows'
                program_files
              end
default['consul']['config']['path'] = File.join(prefix_path, 'consul.json')
default['consul']['config']['data_dir'] = File.join(prefix_path, 'data')
default['consul']['config']['ca_file'] = File.join(prefix_path, 'CA', 'ca.crt')

The more complex solution would be lazily performing string interpolation. A helper method would need to do this so that we're able to correctly modify the path separators. I think the first approach is the most sensible right now as I don't immediately have a thought on how to do it elegantly.

This comment has been minimized.

Copy link
@Ginja

Ginja Dec 30, 2015

Author Collaborator

This is a good idea. I'll update my PR.

owner new_resource.owner
group new_resource.group
mode '0755'
if node['os'].eql? 'linux'

This comment has been minimized.

Copy link
@johnbellone

johnbellone Dec 30, 2015

Contributor

I haven't used Chef with Windows yet. Is the mode the problem here?

This comment has been minimized.

Copy link
@Ginja

Ginja Dec 30, 2015

Author Collaborator

Only if you're not specifying an owner and/or group (which is what we're doing, for now, for Windows). Chef produces a warning otherwise.

}
end

def command(config_file, config_dir)

This comment has been minimized.

Copy link
@johnbellone

johnbellone Dec 30, 2015

Contributor

Is this needed? I imagine we can do something in the separate provider classes themselves?

This comment has been minimized.

Copy link
@Ginja

Ginja Dec 30, 2015

Author Collaborator

I created this file to try and keep everything DRY. Some methods are only used once (e.g. default_environment) but others are used in several places (e.g. windows?, program_files). I prefer to keep provider & resource classes as clean as possible.

@@ -5,38 +5,50 @@
# Copyright 2014, 2015 Bloomberg Finance L.P.
#

node.default['nssm']['install_location'] = '%WINDIR%'

This comment has been minimized.

Copy link
@johnbellone

johnbellone Dec 30, 2015

Contributor

What's nssm?

This comment has been minimized.

Copy link
@Ginja

Ginja Dec 30, 2015

Author Collaborator

NSSM is the Non-Sucking Service Manager. It's also what chocolatey used to run Consul on Windows. I removed chocolatey as we can't rely on the maintainers to keep Consul up-to-date (it's currently 0.5.2, and that was just published on Dec 20th). I also found using chef-nssm was a lot cleaner.

@Ginja Ginja force-pushed the visioncritical:windows-support branch from 25a8efe to 82dab18 Jan 4, 2016

@Ginja

This comment has been minimized.

Copy link
Collaborator Author

Ginja commented Jan 4, 2016

Rebased this PR to resolve conflicts.

@gdavison

This comment has been minimized.

Copy link
Contributor

gdavison commented Jan 4, 2016

@Ginja, this looks great. One problem I've found is that building all the file paths with prefix_path puts some things in the wrong place on Linux. For example, the Consul data should be under the /var/lib/consul hierarchy, rather than /etc/consul

@Ginja

This comment has been minimized.

Copy link
Collaborator Author

Ginja commented Jan 4, 2016

@gdavison you're right, and there was brief discussion to change it here.

In order to keep everything clean & standardized on both platforms (i.e. consul data lives within the data folder underneath the prefix folder), I changed the values. I'd like to see it stay, but I can simply enforce that for my environment in our wrapper cookbook.

@kamaradclimber

This comment has been minimized.

Copy link
Contributor

kamaradclimber commented Jan 13, 2016

@johnbellone how can we help this PR getting merged?

@gdavison

This comment has been minimized.

Copy link
Contributor

gdavison commented Jan 16, 2016

+1 on merging. It works great on our Windows boxes

@johnbellone

This comment has been minimized.

Copy link
Contributor

johnbellone commented Jan 19, 2016

Would you mind re-basing again?

@johnbellone johnbellone added this to the 1.4 milestone Jan 19, 2016

@@ -4,18 +4,23 @@
#
# Copyright 2014-2016, Bloomberg Finance L.P.
#
::Chef::Node.send(:include, ConsulCookbook::Helpers)

This comment has been minimized.

Copy link
@johnbellone

johnbellone Jan 19, 2016

Contributor

What's the purpose of this? The helpers should be injected automatically, no?

This comment has been minimized.

Copy link
@Ginja

Ginja Jan 19, 2016

Author Collaborator

They're not, unfortunately. I needed this to utilize the helper methods. I'll be rebasing shortly.

Ginja added some commits Dec 30, 2015

Added Windows Support
* Fixes #236
* Added Windows Support (binary install_method only) using NSSM
  to manage the service. NSSM runs consul as the local SYSTEM user.
  Support for running it as another user can/should be added later.
* Prevented the firewall cookbook from creating rules for disabled
  ports
* Added & Updated spec tests
* Disabled Style/ModuleFunction cop
Simplified the setting of paths
* Simplified how attributes with paths for values are set
* Changed default values for data_dir attribute to keep everything
  simple
* Added version check to windows binary installation so that it can
  be upgraded
Exposed nssm parameters
Added a Windows only attribute that allows
cookbook users to override and/or add their own
nssm parameters. Added logic for comparing
nssm parameters as the nssm resource doesn't
currently do this. Reverted the change to the
default path for the data directory. Borrowed
some of these ideas from @gdavison, thanks!

@Ginja Ginja force-pushed the visioncritical:windows-support branch from 0e7a3ac to 95c85ea Jan 19, 2016

Fixed an edge case, & changed default nssm parameters
Added exit code 1 as an acceptable return code within
`nssm_service_installed?`. If AppRotateOnline is not set
for nssm, it will only rotate the logs when the service
is restarted. Changed the default nssm parameters so
that logs are rotated when they reach 20 MB in size.

@Ginja Ginja force-pushed the visioncritical:windows-support branch from 95c85ea to 478a2e8 Jan 19, 2016

@gdavison

This comment has been minimized.

Copy link
Contributor

gdavison commented Jan 19, 2016

One issue: consul_definition resources have to have their path set manually on Windows. (And the Linux version doesn't use the config_dir parameter, but that's a separate issue).

Not a show-stopper, though

@Ginja

This comment has been minimized.

Copy link
Collaborator Author

Ginja commented Jan 20, 2016

I've got a PR incoming that'll fix that. Thanks for the head's up. I just noticed the nssm resource doesn't start services if they're stopped. So I added that check into the Windows provider. We should look at contributing some of these changes back to the nssm cookbook.

@Ginja Ginja force-pushed the visioncritical:windows-support branch from f1b9418 to 817c80e Jan 20, 2016

Added nssm running check & updated consul_definition
Added a check to start Consul via nssm if it's not running. And
updated consul_definition resource/provider with the new helper
methods.

@Ginja Ginja force-pushed the visioncritical:windows-support branch from 817c80e to cf8a9fb Jan 20, 2016

johnbellone added a commit that referenced this pull request Jan 20, 2016

@johnbellone johnbellone merged commit f3d3df5 into sous-chefs:master Jan 20, 2016

1 check passed

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

This comment has been minimized.

Copy link
Collaborator

legal90 commented Jan 29, 2016

Be aware - this PR introduces a backward incompatibility:

# Previous default value
default['consul']['config']['data_dir'] = '/var/lib/consul'

# New default value
default['consul']['config']['data_dir'] = join_path data_prefix_path, 'data'
# e.q. '/var/lib/consul/data'

cc: @johnbellone

@legal90

This comment has been minimized.

Copy link
Collaborator

legal90 commented on recipes/default.rb in ad2fcf5 Jun 1, 2017

@Ginja Could you please remind - why do we set this attribute here, not in an attribute file (attributes/default.rb)?

This comment has been minimized.

Copy link
Collaborator Author

Ginja replied Jun 1, 2017

No reason other than it was suggested awhile back to place overrides in a recipe. New advice says not to, so we can definitely move it to a file such as attributes/external.rb or the default.rb, I'm not picky.

@knightorc knightorc deleted the visioncritical:windows-support branch Jun 6, 2018

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.