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

Add systemd support for recursor and authoritative PowerDNS #66

Merged
merged 1 commit into from Jul 13, 2017

Conversation

Projects
None yet
2 participants
@jmauro
Copy link
Contributor

commented May 31, 2017

Hello @therobot @martinisoft,

Here is the code for using systemd in the recursor and the authoritative resource.

Let me know if you want me to change stuff.
Should fix:

Regards,
JM

cc: Cc. @aboten

@jmauro jmauro referenced this pull request May 31, 2017

Merged

Recursor systemd #61

@jmauro jmauro force-pushed the criteo-forks:power_systemd branch 5 times, most recently from 3b22604 to 78f27f0 May 31, 2017

@therobot

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2017

Can you push a branch with just the authoritative code? It will make my life easier in order to review it.

Many thanks @jmauro

@jmauro

This comment has been minimized.

Copy link
Contributor Author

commented Jun 2, 2017

thx @therobot will do, maybe tonight.

@jmauro jmauro force-pushed the criteo-forks:power_systemd branch 2 times, most recently from 96539bd to e4bbd4e Jun 2, 2017

@jmauro

This comment has been minimized.

Copy link
Contributor Author

commented Jun 2, 2017

@therobot the tests are failing with the following error: The job exceeded the maximum time limit for jobs, and has been terminated.

So will remove CentOS 6, what do you think?

@jmauro jmauro force-pushed the criteo-forks:power_systemd branch 4 times, most recently from b785857 to 5d05879 Jun 2, 2017

@jmauro

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2017

Hello @therobot, I have a time out issue, is that possible to increase the concurrency for the jobs: (ref: https://docs.travis-ci.com/user/customizing-the-build#Build-Timeouts) ?

@therobot

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2017

@jmauro let me check, I'll try to do it.

@therobot

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2017

I have limited builds to 1 to see if it helps.

@jmauro jmauro force-pushed the criteo-forks:power_systemd branch from 5d05879 to 5a0fd7b Jun 5, 2017

@jmauro

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2017

@therobot still the same issue, what do you want me to do?

@therobot therobot self-requested a review Jun 6, 2017

@therobot
Copy link
Contributor

left a comment

Hello @jmauro thanks a lot for your work. I just reviewed this PR and still needs some hammering in order to be merged.

Apart from all the comments in the reviews, unit test don't pass on my local machine.

Also am of the opinion that both authoritative resources and helpers and recursor resources and helpers should match each other as much as possible in terms of style, and concept. This will explain some of the comments here (consistency).

def default_authoritative_run_user
'pdns'
end
def sysvinit_name(name = nil)

This comment has been minimized.

Copy link
@therobot

therobot Jun 7, 2017

Contributor

I don't understand the change of the methods here, what is the reason you passing the chef node attributes as parameters? Is the namespacing? This goes further away chef conventions and makes the code more difficult to understand.

I prefer that this helper keeps the style it has on master.

This comment has been minimized.

Copy link
@jmauro

jmauro Jun 7, 2017

Author Contributor

In our company, we are not supposed to create any library without name spacing in order to ease the troubleshoot and avoid any collision, that's why. Maybe the namespacing schema does not fit your need, tell me how you see stuff I would be glad to change it.

return 'pdns-backend-opendbx' if node['platform_family'] == 'debian' && new_resource.instance_name == 'opendbx'
return 'pdns-backend-remote' if node['platform_family'] == 'debian' && new_resource.instance_name == 'remote'
return 'pdns-backend-tinydns' if node['platform_family'] == 'debian' && new_resource.instance_name == 'tinydns'
def backend_package_per_platform(name = nil)

This comment has been minimized.

Copy link
@therobot

therobot Jun 7, 2017

Contributor

Same as above, this code below is difficult to understand and not very chef-ish

This comment has been minimized.

Copy link
@jmauro

jmauro Jun 7, 2017

Author Contributor

This was also inspired from pattern we also enforce in Criteo (https://github.com/criteo-cookbooks/wsus-client/blob/master/libraries/helper.rb#L3). This allow people to monkey patch the code in case we are not able to publish the code as fast as other contributors would like and we tend to think just having a Hash map is easier to maintain, does that make sense?

end

module_function

This comment has been minimized.

Copy link
@therobot

therobot Jun 7, 2017

Contributor

Why is this required?

This comment has been minimized.

Copy link
@jmauro

jmauro Jun 7, 2017

Author Contributor

This is required otherwise I would have to include ::Chef::Recipe.send(:include, PdnsAuthoritativeResource::Helpers) in order to use default_authoritative_config_directory inside test/cookbooks/pdns_test/recipes/authoritative_install_multi.rb

By creating a module_function, calling ::PdnsAuthoritativeResource::Helpers.default_authoritative_config_directoryis good enough

action :install
end
end

action :uninstall do
apt_package backend_package_per_platform do
package backend_package_per_platform(new_resource.instance_name) do

This comment has been minimized.

Copy link
@therobot

therobot Jun 7, 2017

Contributor

Good catch, thanks.

@@ -34,7 +35,7 @@

property :instance_name, String, name_property: true
property :launch, Array, default: ['bind']
property :config_dir, String, default: lazy { default_authoritative_config_directory }
property :config_dir, String, default: lazy { default_authoritative_config_directory(node['platform_family']) }

This comment has been minimized.

Copy link
@therobot

therobot Jun 7, 2017

Contributor

As stated above I dislike passing node attributes as parameters to helper methods.

This comment has been minimized.

Copy link
@jmauro

jmauro Jun 7, 2017

Author Contributor

I don't see how this is dirtier than having the node['platform_family']inside the library?

end

if new_resource.source
template "/etc/init.d/pdns_authoritative-#{new_resource.instance_name}" do

This comment has been minimized.

Copy link
@therobot

therobot Jun 7, 2017

Contributor

This should call the helper method sysvinit_name for consistency.

This comment has been minimized.

Copy link
@jmauro

jmauro Jun 7, 2017

Author Contributor

true good catch


property :instance_name, String, name_property: true
property :cookbook, String, default: 'pdns'
property :source, %w(String NilClass)

This comment has been minimized.

Copy link
@therobot

therobot Jun 7, 2017

Contributor

Usually Chef prefers this syntax here: [String, NilClass]

This comment has been minimized.

Copy link
@therobot

therobot Jun 7, 2017

Contributor

I believe there should be a sane default which is the own template in the cookbook.

This comment has been minimized.

Copy link
@jmauro

jmauro Jun 7, 2017

Author Contributor

That would be the case if the templating is the default use which not, the link is, woudn't it?

PS: I will fix the property class definition

# Has specified in the PowerDNS documentation, a symlink to the init.d script
# "pdns" should be enough for setting up a Virtual instance:
# https://github.com/PowerDNS/pdns/blob/master/docs/markdown/authoritative/running.md#starting-virtual-instances-with-sysv-init-scripts
link "/etc/init.d/pdns_authoritative-#{new_resource.instance_name}" do

This comment has been minimized.

Copy link
@therobot

therobot Jun 7, 2017

Contributor

Same with the `sysvinit_name´ method.

This comment has been minimized.

Copy link
@jmauro

jmauro Jun 7, 2017

Author Contributor

True

it 'creates a specific init script' do
expect(chef_run).to create_template('/etc/init.d/pdns-authoritative-server-01')
it 'creates a specific init script (SysVinit)' do
mock_service_resource_providers(%i{debian upstart})

This comment has been minimized.

Copy link
@therobot

therobot Jun 7, 2017

Contributor

Why upstart?

This comment has been minimized.

Copy link
@jmauro

jmauro Jun 7, 2017

Author Contributor

Just a wrong copy paste.

cookbook new_resource.cookbook
action :create
end
else

This comment has been minimized.

Copy link
@therobot

therobot Jun 7, 2017

Contributor

I'd still like to offer the possibility of having a custom init script, this is the reason I copied the package script over. Yes a symlink is more efficient (and is what pdns advises) but provided an easy solution of running different installs and customization.

This comment has been minimized.

Copy link
@jmauro

jmauro Jun 7, 2017

Author Contributor

That's why by specifying a source the provider will create the service with any script.

describe processes('pdns_server-authoritative-server-01-instance') do
its ('users') { should eq [default_authoritative_run_user] }
end
check_process_name('server-01', default_authoritative_run_user)

This comment has been minimized.

Copy link
@therobot

therobot Jun 7, 2017

Contributor

Why this needs to go outside inspec and into a helper?

This comment has been minimized.

Copy link
@jmauro

jmauro Jun 7, 2017

Author Contributor

Like a said in the commit message, the partial search in inspec has some bugs inspec/inspec#1497 and inspec/inspec#1867, so just took the code from inspec changed it a bit in order to accomplish what we wanted and this allow the inspec test to work while chef is fixing the bugs.

This comment has been minimized.

Copy link
@therobot

therobot Jun 9, 2017

Contributor

OK, thanks!

@jmauro

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2017

@therobot, regarding the unit test, personally, I don't have any issue chefspec and all kitchen-dokken test passed on my branch. What kind of error have you?

For the consistency issue you are mentionnig, do you want me to be as close as possible from one function to the other? Same type of function name and so one? That's fine by me, just tell what name schema you want.

@jmauro jmauro force-pushed the criteo-forks:power_systemd branch 6 times, most recently from b7bf10c to dfea3cb Jun 7, 2017

resource_name :pdns_authoritative_service_sysvinit

provides :pdns_authoritative_service, os: 'linux' do |_node|
Chef::Platform::ServiceHelpers.service_resource_providers.include?(:debian) ||

This comment has been minimized.

Copy link
@therobot

therobot Jun 9, 2017

Contributor

This way of declaring �provides makes my local tests fail with this error:

  ChefSpec::Error::MayNeedToSpecifyPlatform:

I had to resort to the Chef DSL based way to make them work.

Would you mind to write them in the resource DSL way for the authoritative and recursor?

This comment has been minimized.

Copy link
@jmauro

jmauro Jun 9, 2017

Author Contributor

Sorry but I don't understand what you want me to do? What is the resource DSL way? I am just using what chef is also using in certain cookbooks. Do you want me to use the platform_family method?

This comment has been minimized.

Copy link
@therobot

therobot Jun 22, 2017

Contributor

I had to resort back to the Chef DSL to make my tests pass again. I needed to change in the recursor after merging your PR. I don't see any point of not using the Chef DSl:

https://github.com/dnsimple/chef-pdns/blob/master/resources/pdns_recursor_install_debian.rb#L23-L25

This comment has been minimized.

Copy link
@jmauro

jmauro Jun 22, 2017

Author Contributor

Ok, will fix it tonight.

@therobot

This comment has been minimized.

Copy link
Contributor

commented Jun 9, 2017

@jmauro thanks for your work on making the test pass, it looks great :)

I still have a few comments, specially regarding the implementation of the backend_package_per_platform helper and the implementation of the provides which is making my local ChefSpec tests fail.

Once we have this sorted out, and the question sorted we're ready to merge.

J.

@jmauro jmauro force-pushed the criteo-forks:power_systemd branch from dfea3cb to 81a7cb3 Jun 9, 2017

@jmauro

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2017

Hello @therobot,

I changed back the backend_package_per_platform, as you wanted.
Regarding the ChefSpec::Error::MayNeedToSpecifyPlatform: could you provide me with more context (The stack trace would be good) because it is quiet hard to reproduce.

Thx

@jmauro jmauro force-pushed the criteo-forks:power_systemd branch 10 times, most recently from a7b7b16 to 5b7f48f Jun 16, 2017

@martinisoft martinisoft requested review from martinisoft and onlyhavecans Jun 21, 2017

@jmauro jmauro force-pushed the criteo-forks:power_systemd branch 2 times, most recently from 03a0de6 to 97eaefd Jun 22, 2017

@jmauro

This comment has been minimized.

Copy link
Contributor Author

commented Jun 22, 2017

Hello @therobot ,
let me know if this is https://github.com/dnsimple/chef-pdns/pull/66/files#diff-692083a8321ce5c3233ff917b32eb87dR25 is enough for you or not.

Regards,
JM

@jmauro jmauro force-pushed the criteo-forks:power_systemd branch 3 times, most recently from 671a533 to 33fc25d Jun 27, 2017

Add systemd for pdns_authoritative
STATE:
- Currently, systemd is not handle for 'pdns'
- Kitchen-dokken test are failing for 'authoritative-postgres-centos-6'

CAUSE:
- systemd is not the current target for the cookbook mainteners
- The 'postgres' cookbook does fail the first run when used for CentOS 6
  Ref: sous-chefs/postgresql#421
- kitchen-dokken does not provide a correct 'retry' feature:
  PR has been filled: someara/kitchen-dokken#110
- Inspec 'process' resource has issue with partial match:
  inspec/inspec#1497
  inspec/inspec#1867

FIX:
- Add in '.kitchen.yml' a 'max_retries' value for handling 'posgresql'
  failure for CentOS 6
- Add helper: 'PdnsAuthoritativeResource'
- Convert 'default_authoritative_config_directory' to module_function to
  be used in the test cookbooks
- Change 'socket-dir' right to allow systemd to start
- Change 'pdns_authoritative_config' namespace to 'pdns-INSTANCE_NAME'
  and update rspec tests and cookbook pdns_test to reflect that
- Remove init scripts for sysvinit and use the default implementation
  of powerdns virtual use (link creation)
- Create 'pdns_authoritative_service_systemd' resource
- Create 'pdns_authoritative_service_sysvint' resource
- Add 'mock_service_resource_providers'
- Update rspec tests for handling 'sysvinit' and 'systemd' resources
- Remove 'pdns_authoritative_service_rhel_sysvinit' and
  'pdns_authoritative_service_debian_sysvinit' resources
- Fix recipe 'authoritative_install_single_postgres' (pdns_test)
- Update inspec tests
- Add new inspec helper function to determine if systemd is used or
  not ('systemd_is_init?')
- Add new inspec helper function to get powerdns process name according
  init system ('check_process_name')
- Update '.kitchen.dokken.yml' to be compliant with systemd

Change-Id: Id5ea3264391ab55774c51c1cff1325e9b0f58685

@jmauro jmauro force-pushed the criteo-forks:power_systemd branch from 33fc25d to 2b02912 Jul 3, 2017

@jmauro

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2017

Hello @therobot,

Any news? Still want me to do something?

Regards,
JM

@therobot

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2017

@jmauro I'm changing a few things on your branch that I disagree with before merging. Unfortunately tests don't pass locally with kitchen-dokken. I'd like to complete this today, but let's see if I can make tests pass.

@therobot therobot merged commit 2b02912 into dnsimple:master Jul 13, 2017

1 check passed

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

This comment has been minimized.

Copy link
Contributor

commented Jul 13, 2017

@jmauro I merged my checkout of this branch along with some changes and bug fixes.

@jmauro

This comment has been minimized.

Copy link
Contributor Author

commented Jul 13, 2017

Thx

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.