Fix Issue-96 Add AIX Support #97

TJM · 2018-02-22T22:41:55Z

This fixes Issue-96, providing AIX support. NOTE that we enabled module level hiera to provide OSFAMILY specific data. We also put in a basic workaround for the "reload" of automount. Currently, at least on AIX, the service provider ignores the custom restart command.

NOTE: I did my best on the tests. I was not able to use PDK to validate the tests because your gemfile has binary requirements, which I cannot compile on my "work" computer. Let me know if they need fixed.

Thanks in advance,
Tommy

bastelfreak · 2018-02-22T23:21:28Z

Hi @TJM, thanks for the PR. You can click on the travis link, it will show you all issues that the testsuite found.

TJM · 2018-02-23T01:44:15Z

Will fix stuff tomorrow, thanks

TJM · 2018-02-23T15:52:45Z

Hi there,

I think I may need some rspec help. https://travis-ci.org/voxpupuli/puppet-autofs/jobs/345296736

I know why this test is failing, because I added the "correct" filename for /etc/auto_master on Solaris. This brings up two issues for me.

Why did I not get the same failure for AIX? (maybe I am missing where to specify that AIX should be tested)
Sometimes, certain os's, such as AIX and Solaris will have a different filename or different file ownership for certrain resources. What is customarily the way to handle differences like that in rspec? Also, in AIX the file will have different group ownership (root:system). I am trying to add it in the most efficient/effective way.

I am researching, but any pointers in the right direction are appreciated ;)
Thanks in advance,
Tommy

TJM · 2018-02-23T18:36:57Z

That was kinda painful... sorry about all the noise. It is still not testing AIX. I am not sure why, as I think I updated metadata.json correctly (its lint is not balking). I am guessing its just soo obscure that its not in the factdb. I have done some testing on live systems and I think its good.

bastelfreak · 2018-02-24T00:39:31Z

I haven't digged through the travis output, but I think it emulates facter 2.5 or 2.4. Facts come from https://github.com/camptocamp/facterdb/tree/master/facts/2.5. Do you have access to an AIX box and can provide those facts? The procedure is described in the README.md

bastelfreak · 2018-02-24T00:39:50Z

hiera.yaml

@@ -0,0 +1,9 @@
+version: 5


hiera 5 \o/

Yea, I just couldn't bring myself to do the params.pp thing. :)

that's totally fine.

bastelfreak · 2018-02-24T00:40:01Z

manifests/init.pp

+  Variant[String, Array[String]] $package_name,
+  Optional[String] $package_source,
+  Enum[ 'stopped', 'running' ] $service_ensure,
+  Boolean $service_enable,


more datatypes \o/

bastelfreak · 2018-02-24T00:40:10Z

manifests/init.pp

+# @param service_name Determine the name of the service for cross platform compatibility
+# @param auto_master_map Filename of the auto.master map for cross platform compatiblity
+# @param map_file_owner owner of the automount map files for cross platform compatiblity
+# @param map_file_group group of the automount map files for cross platform compatiblity


puppet-strings \o/

aye, I did not generate the docs, I assume that is part of the release process?

Strings generation are a part of the release process, yes

bastelfreak · 2018-02-24T00:41:02Z

manifests/service.pp

+
+  exec { 'automount':
+    path        => '/sbin:/usr/sbin',
+    command     => 'automount -v',


what does this do? Is this always needed?

This tells the automount (daemon) to reload its config, without restarting (which causes automount to attempt to unmount all of its mounted filesystems).

Can't we do a notify to the service? CC: @dhollinger

Puppet seems pretty intent on restarting it on AIX at least, potentially other OS's as well, it doesn't seem to handle the concept of a "soft" reload.

On AIX It makes a decision on whether to send it a "signal" (HUP?) based on lsssrc -Ss automountd output. Basically the 11th(?) flag is a -S vs a -K, see provider for gory details. Long story short, its out of our hands. The src provider (used by AIX) seems to ignore the "restart" command offered as a parameter to the service resource as well, we tried that. :)

~~I would bet based on "POSIX compliance" type reasoning that it also works for Linux, but I have not tested.~~ (see my reply, this does not work in Linux)

On a side note, I talked about this in #puppet on Puppet Community Slack, and someone else (Richard Lawson) had to come up with a similar workaround.

I did not get a chance to test on Solaris, but from what I recall that is also the correct way to "notify" automount to refresh its auto_master file.

This is ugly, but the best thing I could come up with that actually works without restarting the daemon.

CRAP: I just tested on Linux and because there is not an "automountd" it thinks you are trying to start the daemon again. Obviously this won't work as is... let me think on it and get back to you.

FWIW, Linux suffers the same problem. It uses "restart" (which is a stop and start) vs a "reload" (which is a HUP)

CentOS 6

Info: Concat[/etc/auto.master]: Scheduling refresh of Service[autofs] Debug: Executing: '/sbin/service autofs status' Debug: Executing: '/sbin/chkconfig autofs' Debug: Executing: '/sbin/service autofs status' Debug: Executing: '/sbin/service autofs restart' Notice: /Stage[main]/Autofs::Service/Service[autofs]: Triggered 'refresh' from 1 event

CentOS 7

Info: Concat[/etc/auto.master]: Scheduling refresh of Service[autofs] Debug: Executing: '/bin/systemctl is-active autofs' Debug: Executing: '/bin/systemctl is-enabled autofs' Debug: Executing: '/bin/systemctl is-active autofs' Debug: Executing: '/bin/systemctl restart autofs' Notice: /Stage[main]/Autofs::Service/Service[autofs]: Triggered 'refresh' from 1 event

Looks like I am going to have to have a @param reload_command, but making that work will probably require %{facts.os.release.major} unless I play real fast and loose with something like pkill -HUP automount :)

bastelfreak · 2018-02-24T00:41:40Z

spec/defines/mount_spec.rb

@@ -29,7 +43,7 @@
          is_expected.to contain_concat('/etc/auto.home').with(
            'ensure' => 'present',
            'owner'  => 'root',
-            'group'  => 'root',
+            'group'  => group.to_s,


the .to_s isn't needed?

I am not sure, I wouldn't think so, given that I assigned it as 'groupname', but I "borrowed" that idea from the puppetlabs-ntp spec tests.

can you remove it and test if it works? IMO that isn't needed.

it works without the .to_s

TJM · 2018-02-24T01:44:51Z

RE: AIX Facts.... Their readme also says it "supports" AIX, perhaps it used to? I will have to copy the puppet facts output over and "sanitize" it a bit, but I will send them a PR next week.

dhollinger · 2018-02-26T15:47:07Z

manifests/init.pp

+  String $auto_master_map,
+  String $map_file_owner,
+  String $map_file_group,
+  String $reload_command,


@TJM Is the plan here to supersede the Service resource in the resource abstraction layer?

Yes, see below

dhollinger · 2018-02-26T15:48:26Z

manifests/map.pp

@@ -38,14 +38,15 @@

  unless $::autofs::package_ensure == 'absent' {
    Concat {
-      notify => Service['autofs'],
+      before => Service[$autofs::service_name],
+      notify => Exec['automount-reload'],


Is this an AIX/Solaris requirement only or is it global to autofs?

If so, would it be better to build this into a native type/provider? (I'm not a fan of Exec, I've had too many issues with them not working or executing when they shouldn't)

It is global to autofs. If you use the puppet "service" resource, the "notify" will restart (stop then start) automount(d). This causes automount to attempt to unmount all automounted filesystems for a simple configuration change, which is not desirable. The EL6 init script and EL7 systemd svc have a reload option which send a HUP to the automount PID to tell automount to re-read its auto.master map without completely restarting.

I tried to use the restart command parameter, but the AIX service provider (src) completely ignores it, and just runs a stop/start operation anyhow (see my previous comments about lssrc). I think the best thing would be https://tickets.puppetlabs.com/browse/PUP-1054 .. but until then, a refreshonly exec was the best workaround I could think of. The type/provider mechanism would be a more proper/permanent fix, but I don't really know enough ruby to go down that path. It does occur to me that you might be able to "sub-class" (?) the service provider and just provide the reload capability, but if you go to that much trouble, it should probably be its own module? (or a PR to PUP-1054)

Unfortunately I already combined my commits, or you could cherrypick around it ... I guess I could split the "better reload" capability off to a different branch as it is technically NOT related to AIX.

Admittedly, using pkill was just a shot from the hip, as per my previous comments. While it worked in my test systems, it did not pass tests, likely due to the lack of pkill in the most minimal builds.

~tommy

bastelfreak · 2018-02-27T20:29:57Z

I'm fine with this, thanks for the work @TJM. @dhollinger can you review as well?

alexjfisher · 2018-02-27T21:39:47Z

data/common.yaml

+autofs::maps: {}
+autofs::package_ensure: installed
+autofs::package_name: autofs
+autofs::package_source: null


Is null correct? I thought ~ was used for setting undef via hiera.

null works correctly

Thanks for confirming. I struggled to find a definitive answer on any difference between using null or ~

I didn't see this while reviewing, thanks for catching this @alexjfisher. Should we switch to ~ here? As far as I know, ~ is the best practice.

They're equivalent: http://www.yaml.org/refcard.html

(Or you can nerd out and read the spec itself!)

bastelfreak · 2018-02-28T16:04:34Z

@TJM can you please rebase?

TJM · 2018-02-28T19:11:38Z

meh, now there are failing tests, I will --amend when I figure them out

TJM · 2018-03-01T17:27:12Z

Do you need anything else from me on this?

dhollinger

Looks good to me. Though I want one more set of eyes on it as well.

@alexjfisher or @bastelfreak can you take a second look, make sure I didn't miss anything?

TJM force-pushed the issue-96 branch from 0bd6199 to 60cf1b7 Compare February 23, 2018 18:38

bastelfreak reviewed Feb 24, 2018

View reviewed changes

TJM changed the title ~~Fix Issue-96 Add AIX Suport~~ WIP: Fix Issue-96 Add AIX Suport Feb 24, 2018

dhollinger reviewed Feb 26, 2018

View reviewed changes

bastelfreak approved these changes Feb 27, 2018

View reviewed changes

alexjfisher added the needs-squash label Feb 27, 2018

alexjfisher changed the title ~~WIP: Fix Issue-96 Add AIX Suport~~ WIP: Fix Issue-96 Add AIX Support Feb 27, 2018

alexjfisher reviewed Feb 27, 2018

View reviewed changes

dhollinger added the needs-rebase label Feb 28, 2018

TJM changed the title ~~WIP: Fix Issue-96 Add AIX Support~~ Fix Issue-96 Add AIX Support Feb 28, 2018

TJM force-pushed the issue-96 branch from 146c15f to 7f389ea Compare February 28, 2018 19:01

Fix Issue-96 Add AIX Compatibility

f0c8120

TJM force-pushed the issue-96 branch from 7f389ea to f0c8120 Compare February 28, 2018 19:25

dhollinger approved these changes Mar 2, 2018

View reviewed changes

dhollinger merged commit cd03a7e into voxpupuli:master Mar 2, 2018

dhollinger mentioned this pull request Mar 5, 2018

Add AIX support #96

Closed

Fix Issue-96 Add AIX Support #97

Fix Issue-96 Add AIX Support #97

Conversation

TJM commented Feb 22, 2018

bastelfreak commented Feb 22, 2018

TJM commented Feb 23, 2018

TJM commented Feb 23, 2018

TJM commented Feb 23, 2018

bastelfreak commented Feb 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhollinger Feb 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TJM Feb 24, 2018 • edited Loading

Choose a reason for hiding this comment

TJM Feb 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TJM commented Feb 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TJM Feb 26, 2018 • edited Loading

Choose a reason for hiding this comment

bastelfreak commented Feb 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bastelfreak commented Feb 28, 2018

TJM commented Feb 28, 2018

TJM commented Mar 1, 2018

dhollinger left a comment • edited Loading

Choose a reason for hiding this comment

dhollinger Feb 26, 2018 •

edited

Loading

TJM Feb 24, 2018 •

edited

Loading

TJM Feb 24, 2018 •

edited

Loading

TJM Feb 26, 2018 •

edited

Loading

dhollinger left a comment •

edited

Loading