Merge ~d-info-e/cloud-init:salt-freebsd-patch into cloud-init:master

[do3meli]

Make salt minion module work on FreeBSD.

Previously the module was not working under FreeBSD due to a different
package name and some different paths. The module now has OS specific
default values which can even be customized via corresponding cloud config
variables.

LP: #1721503

the salt minion module was not really usable on FreeBSD so far. I have adjusted/extended the module with some more config variables that can be set based on the distro for better flexibility and compatibility between different distributions. In particular on FreeBSD system paths, package names and service names are different than on other operating systems. This fixes LP: #1721503

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/796/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    FAILED: Ubuntu LTS: Integration

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/796/rebuild

review: Needs Fixing (continuous-integration)

[do3meli (d-info-e)]

can anyone of the jenkins team please have a look at the failed integration test. the error is:

2018-02-28 15:17:37,740 - tests.cloud_tests - ERROR - stage part: setup func for --deb, install deb encountered error: install deb version "17.1-46-g7acc9e68-0ubuntu1~16.04.1" does not match expected "17.1-28-gbee5b162-1~bddeb"

that looks to me like a jenkins setup error instead of a code issue. thanks

[Scott Moser (smoser)]

This is an interesting set of changes.
In order to configure salt, you have to know details of 2 things
a.) the distribution
b.) salt software

Generally knowledge of the distribution has been in the distribution class and the software in the 'config module' (cc_salt).

You've mostly maintained that here.

It seems to me that information about how to install software X on distribution Y is actually more closely coupled to the software X than the distribution.

So it feels to me that its better to just keep distro specific knowledge of a given package in the config module rather than putting it into the distro.

Does that make sense?

Thoughts?

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/801/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: MAAS Compatability Testing
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/801/rebuild

review: Approve (continuous-integration)

[do3meli (d-info-e)]

hi scott,

from a theoretical point of view i totally agree with you. the reason why i added those 3 cfg variables to the distro module was that it is currently not maintained like this everywhere. for example the variable cfg['ssh_svcname'] is also configured in the distro classes instead of the corresponding cc_ssh*.py classes. one may say ssh is such a basic service which is used in many different places in the code so it makes sense to have it in the distro...?

never the less i think in the case of cc_salt_minion it really makes sense to have these params not defined in the distro. i have created an implementation idea that follows the mentioned standard but to be honest the new version doesn't look as neat to me as the previous version. what are your thoughts? check out the patch here: https://git.launchpad.net/~d-info-e/cloud-init/commit/?id=c7c9f90714dcd127faf861e63e465a65d79277fa

i haven't tested it yet but if you think that is nice and good i will merge it to this branch here.

cheers
dom

[Scott Moser (smoser)]

Dom,
Yeah, we have knowledge of ssh in the distro module. It more qualifies as a "basic service". Similarly, recent work is being done to do ntp configuration. ntp also seems "basic".

So yeah, I like your suggestion in the link you provided.

Some thing like the PuppetConstants might be helpful.

class SaltConstants(object):
   def init(self):
       if util.is_FreeBSD():
           self.package_name = 'salt_minion'
           self.config_dir = '/usr/local/etc/salt'
...

[do3meli (d-info-e)]

hi scott,

i have just updated this branch with what i think looks much better than before. as an additional feature users can now configure those 3 variables if they have their own service names / packages / config paths.

i haven't tested this yet. will do so on Monday and give feedback again here.

cheers
dom

[Scott Moser (smoser)]

Please let me know if you don't like hte suggestions.

review: Needs Fixing

[do3meli (d-info-e)]

hi scott, your suggestion makes sense. i have implemented it accordingly. please review it again. thanks. dom

[Scott Moser (smoser)]

http://paste.ubuntu.com/p/q8X4WRWH4s/

thats my suggestion, then thats all I have.

[Scott Moser (smoser)]

I started a jenkins c-i run.
https://jenkins.ubuntu.com/server/job/cloud-init-ci/814/

assuming that goes well, i'm ok with this.

review: Approve

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/814/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: MAAS Compatability Testing
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/814/rebuild

review: Approve (continuous-integration)

[do3meli (d-info-e)]

folks,

i have added another commit that includes an additional integration test and enables the salt minion integration tests again. unfortunately this additional commit https://git.launchpad.net/~d-info-e/cloud-init/commit/?id=48f21de68a38085ce342278a17c98bb16ab6246d is not visible in the MR here. maybe because the status of the MR was "merged" when i pushed and i set it back to WIP after the push. what do we have to do to have it visible here in the MR?

once it is visible you guys can trigger another ci run, approve it and merge it finally.

cheers,
dom

[do3meli (d-info-e)]

above problem solved by - pushing again. please review/approve/merge

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:42aacc0fcc3c91f3931204445058b9bfc330db27
https://jenkins.ubuntu.com/server/job/cloud-init-ci/827/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: MAAS Compatability Testing
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/827/rebuild

review: Approve (continuous-integration)

[Chad Smith (chad.smith)]

Thank you for this integration test update Dominic. We like the approach you've taken here. +1

review: Approve

diff --git a/cloudinit/config/cc_salt_minion.py b/cloudinit/config/cc_salt_minion.py
index 5112a34..d6a21d7 100644
--- a/cloudinit/config/cc_salt_minion.py
+++ b/cloudinit/config/cc_salt_minion.py
@@ -12,7 +12,9 @@ key is present in the config parts, then salt minion will be installed and
 started. Configuration for salt minion can be specified in the ``conf`` key
 under ``salt_minion``. Any conf values present there will be assigned in
 ``/etc/salt/minion``. The public and private keys to use for salt minion can be
-specified with ``public_key`` and ``private_key`` respectively.
+specified with ``public_key`` and ``private_key`` respectively. Optionally if
+you have a custom package name, service name or config directory you can
+specify them with ``pkg_name``, ``service_name`` and ``config_dir``.
 
 **Internal name:** ``cc_salt_minion``
 
@@ -23,6 +25,9 @@ specified with ``public_key`` and ``private_key`` respectively.
 **Config keys**::
 
     salt_minion:
+        pkg_name: 'salt-minion'
+        service_name: 'salt-minion'
+        config_dir: '/etc/salt'
         conf:
             master: salt.example.com
         grains:
@@ -42,7 +47,34 @@ import os
 
 from cloudinit import util
 
-# Note: see http://saltstack.org/topics/installation/
+# Note: see https://docs.saltstack.com/en/latest/topics/installation/
+# Note: see https://docs.saltstack.com/en/latest/ref/configuration/
+
+
+class SaltConstants(object):
+    """
+    defines default distribution specific salt variables
+    """
+    def __init__(self, cfg):
+
+        # constants tailored for FreeBSD
+        if util.is_FreeBSD():
+            self.pkg_name = 'py27-salt'
+            self.srv_name = 'salt_minion'
+            self.conf_dir = '/usr/local/etc/salt'
+        # constants for any other OS
+        else:
+            self.pkg_name = 'salt-minion'
+            self.srv_name = 'salt-minion'
+            self.conf_dir = '/etc/salt'
+
+        # if there are constants given in cloud config use those
+        self.pkg_name = util.get_cfg_option_str(cfg, 'pkg_name',
+                                                self.pkg_name)
+        self.conf_dir = util.get_cfg_option_str(cfg, 'config_dir',
+                                                self.conf_dir)
+        self.srv_name = util.get_cfg_option_str(cfg, 'service_name',
+                                                self.srv_name)
 
 
 def handle(name, cfg, cloud, log, _args):
@@ -52,45 +84,49 @@ def handle(name, cfg, cloud, log, _args):
                    " no 'salt_minion' key in configuration"), name)
         return
 
-    salt_cfg = cfg['salt_minion']
+    s_cfg = cfg['salt_minion']
+    const = SaltConstants(cfg=s_cfg)
 
     # Start by installing the salt package ...
-    cloud.distro.install_packages(('salt-minion',))
+    cloud.distro.install_packages(const.pkg_name)
 
     # Ensure we can configure files at the right dir
-    config_dir = salt_cfg.get("config_dir", '/etc/salt')
-    util.ensure_dir(config_dir)
+    util.ensure_dir(const.conf_dir)
 
     # ... and then update the salt configuration
-    if 'conf' in salt_cfg:
-        # Add all sections from the conf object to /etc/salt/minion
-        minion_config = os.path.join(config_dir, 'minion')
-        minion_data = util.yaml_dumps(salt_cfg.get('conf'))
+    if 'conf' in s_cfg:
+        # Add all sections from the conf object to minion config file
+        minion_config = os.path.join(const.conf_dir, 'minion')
+        minion_data = util.yaml_dumps(s_cfg.get('conf'))
         util.write_file(minion_config, minion_data)
 
-    if 'grains' in salt_cfg:
+    if 'grains' in s_cfg:
         # add grains to /etc/salt/grains
-        grains_config = os.path.join(config_dir, 'grains')
-        grains_data = util.yaml_dumps(salt_cfg.get('grains'))
+        grains_config = os.path.join(const.conf_dir, 'grains')
+        grains_data = util.yaml_dumps(s_cfg.get('grains'))
         util.write_file(grains_config, grains_data)
 
     # ... copy the key pair if specified
-    if 'public_key' in salt_cfg and 'private_key' in salt_cfg:
-        if os.path.isdir("/etc/salt/pki/minion"):
-            pki_dir_default = "/etc/salt/pki/minion"
-        else:
-            pki_dir_default = "/etc/salt/pki"
+    if 'public_key' in s_cfg and 'private_key' in s_cfg:
+        pki_dir_default = os.path.join(const.conf_dir, "pki/minion")
+        if not os.path.isdir(pki_dir_default):
+            pki_dir_default = os.path.join(const.conf_dir, "pki")
 
-        pki_dir = salt_cfg.get('pki_dir', pki_dir_default)
+        pki_dir = s_cfg.get('pki_dir', pki_dir_default)
         with util.umask(0o77):
             util.ensure_dir(pki_dir)
             pub_name = os.path.join(pki_dir, 'minion.pub')
             pem_name = os.path.join(pki_dir, 'minion.pem')
-            util.write_file(pub_name, salt_cfg['public_key'])
-            util.write_file(pem_name, salt_cfg['private_key'])
+            util.write_file(pub_name, s_cfg['public_key'])
+            util.write_file(pem_name, s_cfg['private_key'])
+
+    # we need to have the salt minion service enabled in rc in order to be
+    # able to start the service. this does only apply on FreeBSD servers.
+    if cloud.distro.osfamily == 'freebsd':
+        cloud.distro.updatercconf('salt_minion_enable', 'YES')
 
-    # restart salt-minion.  'service' will start even if not started.  if it
+    # restart salt-minion. 'service' will start even if not started. if it
     # was started, it needs to be restarted for config change.
-    util.subp(['service', 'salt-minion', 'restart'], capture=False)
+    util.subp(['service', const.srv_name, 'restart'], capture=False)
 
 # vi: ts=4 expandtab
diff --git a/config/cloud.cfg.tmpl b/config/cloud.cfg.tmpl
index fad1184..cf2e240 100644
--- a/config/cloud.cfg.tmpl
+++ b/config/cloud.cfg.tmpl
@@ -113,9 +113,9 @@ cloud_final_modules:
 {% if variant not in ["freebsd"] %}
  - puppet
  - chef
- - salt-minion
  - mcollective
 {% endif %}
+ - salt-minion
  - rightscale_userdata
  - scripts-vendor
  - scripts-per-once
diff --git a/tests/cloud_tests/testcases/modules/salt_minion.py b/tests/cloud_tests/testcases/modules/salt_minion.py
index f13b48a..70917a4 100644
--- a/tests/cloud_tests/testcases/modules/salt_minion.py
+++ b/tests/cloud_tests/testcases/modules/salt_minion.py
@@ -31,4 +31,9 @@ class Test(base.CloudTestCase):
         out = self.get_data_file('grains')
         self.assertIn('role: web', out)
 
+    def test_minion_installed(self):
+        """Test if the salt-minion package is installed"""
+        out = self.get_data_file('minion_installed')
+        self.assertEqual(1, int(out))
+
 # vi: ts=4 expandtab
diff --git a/tests/cloud_tests/testcases/modules/salt_minion.yaml b/tests/cloud_tests/testcases/modules/salt_minion.yaml
index ab0e05b..f20b976 100644
--- a/tests/cloud_tests/testcases/modules/salt_minion.yaml
+++ b/tests/cloud_tests/testcases/modules/salt_minion.yaml
@@ -3,7 +3,7 @@
 #
 # 2016-11-17: Currently takes >60 seconds results in test failure
 #
-enabled: False
+enabled: True
 cloud_config: |
   #cloud-config
   salt_minion:
@@ -35,5 +35,8 @@ collect_scripts:
   grains: |
     #!/bin/bash
     cat /etc/salt/grains
+  minion_installed: |
+    #!/bin/bash
+    dpkg -l | grep salt-minion | grep ii | wc -l
 
 # vi: ts=4 expandtab