Merge branch rebase-charm-ops-openstack

- implement basic charm functionality and configuration
- unit-tests
- pep8

.vscode/tasks.json

@@ -0,0 +1,12 @@
+{
+    // See https://go.microsoft.com/fwlink/?LinkId=733558
+    // for the documentation about the tasks.json format
+    "version": "2.0.0",
+    "tasks": [
+        {
+            "label": "Charm build",
+            "type": "shell",
+            "command": "charmcraft pack"
+        }
+    ]
+}

config.yaml

@@ -1,4 +1,23 @@
 options:
+  install_sources:
+    type: string
+    default: "deb https://repo.infinidat.com/packages/main-stable/apt/linux-ubuntu {distrib_codename} main"
+    description: |
+      Optional configuration to support use of additional sources such as:
+        - ppa:myteam/ppa
+        - cloud:trusty-proposed/kilo
+        - http://my.archive.com/ubuntu main
+      The last option should be used in conjunction with the key configuration
+      option. See https://repo.infinidat.com/home/main-stable for details.
+      The charm also supports templating of the distribution codename via 
+      automatic expansion of {distrib_codename} depending on the host system
+      version.
+  install_keys:
+    type: string
+    default:
+    description: |
+      Key ID to import to the apt keyring to support use with arbitary source
+      configuration from outside of Launchpad archives or PPA's. 
   use-multipath:
     type: boolean
     default: True
@@ -8,9 +27,9 @@ options:
       "use-multipath" option in the nova-compute charm.
   protocol:
     type: string
-    default:
+    default: iscsi
     description: |
-      SAN protocol to use. Choose between iscsi or fc.
+      SAN protocol to use. Choose between "iscsi" or "fc".
   volume-backend-name:
     type: string
     description: |
@@ -20,3 +39,64 @@ options:
         A common backend name can be set to multiple backends with the
         same characters so that those can be treated as a single virtual
         backend associated with a single volume type.
+  infinibox-ip:
+    type: string
+    description: |
+        Management VIP address of the Infinibox.
+    default: !!null
+  infinibox-login:
+    type: string
+    description: |
+        The username for management api on the Infinibox.
+    default: !!null
+  infinibox-password:
+    type: string
+    description: |
+        The password for management api on the Infinibox.
+    default: !!null
+  iscsi-netspaces:
+    type: string
+    description: Comma seperated list of iSCSI netspaces to use.
+    default: !!null ""
+  use-chap:
+    type: boolean
+    description: |
+        Choose whether to use CHAP authentication for iSCSI.
+    default: !!bool "false"
+  chap-username:
+    type: string
+    description: |
+        Username for CHAP authentication.
+    default: !!null ""
+  chap-password:
+    type: string
+    description: |
+        Password for CHAP authentication.
+    default: !!null ""
+  pool-name:
+    type: string
+    description: |
+        Storage pool on the Infinibox from which Cinder allocates volumes.
+        The pool must exist, otherwise Cinder would fail.
+    default: !!null ""
+  use-compression:
+    type: boolean
+    description: |
+        Choose whether to use compression on volumes.
+        Volume compression is available on InfiniBox 3.0 onward.
+    default: !!bool "true"
+  thin-provision:
+    type: boolean
+    description: |
+        Choose whether to allocate thin provisioned volumes.
+    default: !!bool "true"
+  infinibox-use-ssl:
+    type: boolean
+    description: |
+        Specifies whether to use SSL for Cinder to Infinibox network communication.
+    default: !!bool "true"
+  infinibox-ssl-ca:
+    type: string
+    description: |
+        Optional CA certificate used to verify Infinibox API certificate.
+    default: !!null

osci.yaml

@@ -4,5 +4,5 @@
       - charm-unit-jobs-py39
     vars:
       needs_charm_build: true
-      charm_build_name: infinidat
+      charm_build_name: cinder-infinidat
       build_type: charmcraft

src/charm.py

@@ -14,16 +14,50 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-
 from ops_openstack.plugins.classes import CinderStoragePluginCharm
-from ops_openstack.core import charm_class, get_charm_class_for_release
 from ops.main import main
 
+from charmhelpers.core.host import (
+    service_start,
+    service_running,
+    service_resume,
+    install_ca_cert,
+    lsb_release,
+)
 
-class CinderCharmBase(CinderStoragePluginCharm):
+from charmhelpers.fetch import (
+    apt_install,
+    apt_update,
+    add_source,
+)
+
+from ops.model import (
+    ActiveStatus,
+    BlockedStatus,
+)
+
+import logging
+
+
+class CinderInfinidatCharm(CinderStoragePluginCharm):
+
+    PACKAGES = ['python3-infinisdk', 'infinishell']
+
+    MANDATORY_CONFIG = ['infinibox-ip', 'infinibox-login',
+                        'infinibox-password', 'pool-name', 'protocol']
+
+    REQUIRED_RELATIONS = ['storage-backend']
+
+    RESTART_MAP = {'/etc/iscsi/iscsid.conf': ['iscsid']}
+
+    PROTOCOL_VALID_VALUES = ['fc', 'iscsi']
+    VOLUME_DRIVER = 'cinder.volume.drivers.infinidat.InfiniboxVolumeDriver'
+
+    DISTRIB_CODENAME_PATTERN = '$codename'
+
+    DEFAULT_REPO_BASEURL = \
+        'https://repo.infinidat.com/packages/main-stable/apt/linux-ubuntu'
 
-    PACKAGES = ['cinder-common']
-    MANDATORY_CONFIG = ['protocol']
     # Overriden from the parent. May be set depending on the charm's properties
     stateless = True
     active_active = True
@@ -31,29 +65,154 @@ class CinderCharmBase(CinderStoragePluginCharm):
     def __init__(self, *args, **kwargs):
         super().__init__(*args, **kwargs)
 
+        self.register_status_check(self.check_mandatory_params)
+        self.register_status_check(self.check_protocol_valid)
+        self.register_status_check(self.check_iscsi_netspaces)
+
+    def check_protocol_valid(self):
+        if self.config.get('protocol').lower() not in \
+                self.PROTOCOL_VALID_VALUES:
+            return BlockedStatus(
+                "valid values for 'protocol' are " +
+                ','.join(self.PROTOCOL_VALID_VALUES)
+            )
+        return ActiveStatus()
+
+    def check_iscsi_netspaces(self):
+        if self.config.get('protocol').lower() == 'iscsi' \
+                and not self.config.get('iscsi-netspaces'):
+
+            return BlockedStatus("'iscsi-netspaces' must be set "
+                                 "when using 'iscsi' protocol")
+        return ActiveStatus()
+
+    def _validate_config(self):
+        for func in self.custom_status_checks:
+            _result = func()
+
+            if not isinstance(_result, ActiveStatus):
+                self.unit.status = _result
+                logging.error(_result.message)
+                return False
+        return True
+
+    def check_mandatory_params(self):
+        """
+        Implements more detailed validation of the config params.
+        Also prevents cinder config from updating if some of the crucial charm
+        options are not specified. Add more checks that should prevent
+        cinder.conf from updating.
+        """
+
+        missing = []
+        for param in self.MANDATORY_CONFIG:
+            if param not in self.config:
+                missing.append(param)
+        if missing:
+            return BlockedStatus(
+                'missing option(s): ' + ','.join(missing)
+            )
+        return ActiveStatus()
+
+    def _install_ca_cert(self):
+        config = dict(self.framework.model.config)
+        install_ca_cert(config.get('infinibox-ssl-ca'))
+
+    def _on_config(self, event):
+
+        self._install_ca_cert()
+
+        if not self._validate_config():
+            return
+
+        # This is called to trigger set_data()
+        # for the cinder relation on a config change.
+        self.on_config(event)
+        try:
+            self.install_pkgs()
+        except Exception as e:
+            self.unit.status = BlockedStatus(str(e))
+
+        # See self._stored.is_started in OSBaseCharm
+        # Without this line here the charm will be stuck in 'waiting' until
+        # all the config parameters are provided correctly by the user
+        # completely bypassing 'blocked' status, and we need 'blocked' status
+        # because human intervention is needed.
+
+        self._stored.is_started = True
+        self.update_status()
+
+    def on_storage_backend(self, event):
+        # Prevent broken config from being propagated to Cinder
+        if self._validate_config():
+            super().on_storage_backend(event)
+        else:
+            event.defer()
+
+    def install_pkgs(self):
+        logging.info("Installing packages")
+
+        # we implement $codename expansion here
+        # see the default value for 'source' in config.yaml
+        if self.model.config.get('install_sources'):
+            distrib_codename = lsb_release()['DISTRIB_CODENAME'].lower()
+            add_source(
+                self.model.config['install_sources']
+                    .format(distrib_codename=distrib_codename),
+                self.model.config.get('install_keys'))
+        apt_update(fatal=True)
+        apt_install(self.PACKAGES, fatal=True)
+        self.update_status()
+
+    def on_install(self, event):
+        self.install_pkgs()
+
+        # start iscsid if it is not running
+        # neded for cinder's boot-from-volume
+        if not service_running('iscsid'):
+            logging.info('Starting iscsid service')
+            service_resume('iscsid')
+            service_start('iscsid')
+
+        self.update_status()
+
     def cinder_configuration(self, config):
         # Return the configuration to be set by the principal.
         backend_name = config.get('volume-backend-name',
                                   self.framework.model.app.name)
-        volume_driver = ''
+        # As per https://docs.openstack.org/cinder/latest/configuration/block-storage/drivers/infinidat-volume-driver.html # noqa: E501
+
         options = [
-            ('volume_driver', volume_driver),
+            ('volume_driver', self.VOLUME_DRIVER),
+            ('use_multipath_for_image_xfer', config.get('use-multipath')),
+            ('infinidat_storage_protocol', config.get('protocol')),
             ('volume_backend_name', backend_name),
+            ('san_ip', config.get('infinibox-ip')),
+            ('san_login', config.get('infinibox-login')),
+            ('san_password', config.get('infinibox-password')),
+            ('infinidat_iscsi_netspaces', config.get('iscsi-netspaces')),
         ]
 
-        if config.get('use-multipath'):
+        use_chap_auth = config.get('use-chap', False)
+        options.extend([
+            ('use_chap_auth', use_chap_auth),
+        ])
+
+        if use_chap_auth:
             options.extend([
-                ('use_multipath_for_image_xfer', True),
+                ('chap_username', config.get('chap-username')),
-                ('enforce_multipath_for_image_xfer', True)
+                ('chap_password', config.get('chap-password')),
             ])
 
+        options.extend([
+            ('infinidat_pool_name', config.get('pool-name')),
+            ('infinidat_use_compression', config.get('use-compression')),
+            ('san_thin_provision', config.get('thin-provision')),
+            ('infinidat_use_ssl', config.get('infinibox-use-ssl')),
+        ])
+
         return options
 
 
-@charm_class
-class CinderInfinidatCharm(CinderCharmBase):
-    release = 'ussuri'
-
-
 if __name__ == '__main__':
-    main(get_charm_class_for_release())
+    main(CinderInfinidatCharm)

test-requirements.txt

@@ -1,7 +1,6 @@
 # This file is managed centrally.  If you find the need to modify this as a
 # one-off, please don't.  Intead, consult #openstack-charms and ask about
 # requirements management in charms via bot-control.  Thank you.
-charm-tools>=2.4.4
 coverage>=3.6
 mock>=1.2
 flake8>=4.0.1; python_version >= '3.6'

tox.ini

@@ -64,6 +64,11 @@ basepython = python3.9
 deps = -r{toxinidir}/requirements.txt
        -r{toxinidir}/test-requirements.txt
 
+[testenv:py310]
+basepython = python3.10
+deps = -r{toxinidir}/requirements.txt
+       -r{toxinidir}/test-requirements.txt
+
 [testenv:py3]
 basepython = python3
 deps = -r{toxinidir}/requirements.txt

unit_tests/test_cinder_infinidat_charm.py

@@ -13,39 +13,242 @@
 # limitations under the License.
 
 import unittest
-from src.charm import CinderCharmBase
+from src.charm import CinderInfinidatCharm
-from ops.model import ActiveStatus
+
+from ops.model import ActiveStatus, BlockedStatus
 from ops.testing import Harness
 
+from unittest import mock
+
+from charmhelpers.core.host_factory.ubuntu import UBUNTU_RELEASES
+
+SOURCE = "deb https://repo.infinidat.com/packages/main-stable/apt/linux-ubuntu focal main"  # noqa: E501
+KEY = """\
+-----BEGIN PGP PUBLIC KEY BLOCK-----
+Version: GnuPG v1.4.11 (GNU/Linux)
+
+mQENBFESDRIBCADMR7MQMbH4GdCQqfrOMt35MhBwwH4wv9kb1WRSTxa0CmuzYaBB
+1nJ0nLaMAwHsEr9CytPWDpMngm/3nt+4F2hJcsOEkQkqeJ31gScJewM+AOUV3DEl
+qOeXXYLcP+jUY6pPjlZpOw0p7moUQPXHn+7amVrk7cXGQ8O3B+5a5wjN86LT2hlX
+DlBlV5bX/DYluiPUbvQLOknmwO53KpaeDeZc4a8iIOCYWu2ntuAMddBkTps0El5n
+JJZMTf6os2ZzngWMZRMDiVJgqVRi2b+8SgFQlQy0cAmne/mpgPrRq0ZMX3DokGG5
+hnIg1mF82laTxd+9qtiOxupzJqf8mncQHdaTABEBAAG0IWFwcF9yZXBvIChDb21t
+ZW50KSA8bm9AZW1haWwuY29tPokBOAQTAQIAIgUCURINEgIbLwYLCQgHAwIGFQgC
+CQoLBBYCAwECHgECF4AACgkQem2D/j05RYSrcggAsCc4KppV/SZX5XI/CWFXIAXw
++HaNsh2EwYKf9DhtoGbTOuwePvrPGcgFYM3Tu+m+rziPnnFl0bs0xwQyNEVQ9yDw
+t465pSgmXwEHbBkoISV1e4WYtZAsnTNne9ieJ49Ob/WY4w3AkdPRK/41UP5Ct6lR
+HHRXrSWJYHVq5Rh6BakRuMJyJLz/KvcJAaPkA4U6VrPD7PFtSecMTaONPjGCcomq
+b7q84G5ZfeJWb742PWBTS8fJdC+Jd4y5fFdJS9fQwIo52Ff9In2QBpJt5Wdc02SI
+fvQnuh37D2P8OcIfMxMfoFXpAMWjrMYc5veyQY1GXD/EOkfjjLne6qWPLfNojA==
+=w5Os
+-----END PGP PUBLIC KEY BLOCK-----
+"""
+
+PROTOCOL_INVALID_VALUE = 'not_fc_or_iscsi'
+
 
 class TestCinderInfinidatCharm(unittest.TestCase):
 
     def setUp(self):
-        self.harness = Harness(CinderCharmBase)
+        self.harness = Harness(CinderInfinidatCharm)
         self.addCleanup(self.harness.cleanup)
         self.harness.begin()
         self.harness.set_leader(True)
         backend = self.harness.add_relation('storage-backend', 'cinder')
-        self.harness.update_config({'volume-backend-name': 'test'})
         self.harness.add_relation_unit(backend, 'cinder/0')
 
+    def _get_partial_config_sample(self):
+        """
+        A config with all mandatory params set
+        """
+        return {
+            'infinibox-ip': '123.123.123.123',
+            'infinibox-login': 'login',
+            'infinibox-password': 'password',
+            'pool-name': 'test',
+        }
+
+    def _get_valid_config_sample(self):
+        """
+        A minimal config that would transition the charm to ActiveState
+        """
+        partial_config = self._get_partial_config_sample()
+        partial_config.update({
+            'protocol': 'iscsi',
+            'iscsi-netspaces': 'A,B',
+        })
+        return partial_config
+
+    def _get_source(self, codename, pocket, baseurl=None):
+        if baseurl is None:
+            baseurl = self.harness.charm.DEFAULT_REPO_BASEURL
+        return ' '.join((
+            'deb',
+            baseurl,
+            codename,
+            pocket))
+
     def test_cinder_base(self):
         self.assertEqual(
             self.harness.framework.model.app.name,
             'cinder-infinidat')
-        # Test that charm is active upon installation.
+
         self.harness.update_config({})
+
         self.assertTrue(isinstance(
-            self.harness.model.unit.status, ActiveStatus))
+            self.harness.model.unit.status, BlockedStatus))
 
-    def test_multipath_config(self):
+    @mock.patch('src.charm.CinderInfinidatCharm.install_pkgs')
-        self.harness.update_config({'use-multipath': True})
+    @mock.patch('src.charm.service_running')
-        conf = dict(self.harness.charm.cinder_configuration(
+    @mock.patch('src.charm.add_source')
-            dict(self.harness.model.config)))
+    @mock.patch('src.charm.apt_update')
-        self.assertEqual(conf['volume_backend_name'], 'test')
+    @mock.patch('src.charm.apt_install')
-        self.assertTrue(conf.get('use_multipath_for_image_xfer'))
+    @mock.patch('ops_openstack.core.os_utils.ows_check_services_running')
-        self.assertTrue(conf.get('enforce_multipath_for_image_xfer'))
+    def test_protocol_config_validation(self, check, apt_install, apt_update,
+                                        add_source, service_running,
+                                        install_pkgs):
+        install_pkgs.return_value = None
+        service_running.return_value = True
+        check.return_value = None, None
 
-    def test_cinder_configuration(self):
+        add_source.return_value = None
-        # Add check here that configuration is as expected.
+        apt_install.return_value = None
+        apt_update.return_value = None
+        # Make sure we won't let incorrect value
+        # for choice list on Cinder's side
+
+        cfg = self._get_partial_config_sample()
+        cfg.update({'protocol': PROTOCOL_INVALID_VALUE})
+
+        self.harness.update_config(cfg)
+
+        self.assertTrue(isinstance(
+            self.harness.model.unit.status, BlockedStatus
+        ))
+
+        self.harness.update_config({'protocol': 'iscsi'})
+        self.assertTrue(isinstance(
+            self.harness.model.unit.status, BlockedStatus,
+        ))
+
+        self.harness.update_config({'iscsi-netspaces': 'A,B'})
+        self.assertTrue(isinstance(
+            self.harness.model.unit.status, ActiveStatus
+        ))
+
+        self.harness.update_config({'protocol': 'fc'}, unset='iscsi-netspaces')
+        self.assertTrue(isinstance(
+            self.harness.model.unit.status, ActiveStatus
+        ))
+
+    def test_cinder_conf_static_params(self):
+        self.harness.update_config({})
+
+        config = self.harness.framework.model.config
+
+        self.assertIn(
+            ('volume_driver', self.harness.charm.VOLUME_DRIVER),
+            self.harness.charm.cinder_configuration(config)
+        )
+
+    def test_cinder_conf_backend_name_param(self):
+        self.harness.update_config({})
+
+        config = self.harness.framework.model.config
+        self.assertIn(
+            ('volume_backend_name', self.harness.model.app.name),
+            self.harness.charm.cinder_configuration(config)
+        )
+
+        self.harness.update_config({'volume-backend-name': 'overridden'})
+        self.assertIn(
+            ('volume_backend_name', 'overridden'),
+            self.harness.charm.cinder_configuration(config)
+        )
+
+    @mock.patch('src.charm.CinderInfinidatCharm.install_pkgs')
+    @mock.patch('ops_openstack.core.os_utils.ows_check_services_running')
+    def test_mandatory_config_params(self, check_services_running,
+                                     install_pkgs):
+        cfg = self._get_valid_config_sample()
+        check_services_running.return_value = None, None
+        install_pkgs.return_value = None
+
+        self.harness.update_config(cfg)
+
+        self.assertTrue(isinstance(
+            self.harness.model.unit.status, ActiveStatus
+        ))
+
+        for p in self.harness.charm.MANDATORY_CONFIG:
+            self.harness.update_config(unset=[p])
+            # If the MANDATORY_CONFIG param has a default value,
+            # it won't transition to the BlockedStatus because of
+            # MANDATORY_CONFIG validation, so we unset it:
+            if not self.harness.model.config.get(p):
+                self.assertTrue(isinstance(
+                    self.harness.model.unit.status, BlockedStatus
+                ))
+
+            self.harness.update_config(cfg)
+
+            self.assertTrue(isinstance(
+                self.harness.model.unit.status, ActiveStatus
+            ))
+
+    @mock.patch('src.charm.add_source')
+    @mock.patch('src.charm.apt_update')
+    @mock.patch('src.charm.apt_install')
+    @mock.patch('src.charm.service_running')
+    @mock.patch('src.charm.service_start')
+    @mock.patch('src.charm.service_resume')
+    @mock.patch('src.charm.lsb_release')
+    def test_repo_management(self, lsb_release, svc_resume, svc_start,
+                             svc_running, apt_install, apt_update, add_source):
+
+        add_source.return_value = None
+        apt_install.return_value = None
+        apt_update.return_value = None
+        svc_resume.return_value = None
+        svc_start.return_value = None
+        svc_running.return_value = None
+
+        # we'll need the config the charm considers valid
+        # in order to test repo management:
+        cfg = self._get_valid_config_sample()
+
+        dynamic_source = self._get_source('{distrib_codename}', 'main')
+
+        # generate test data for both 'source' values that need substituion
+        # and for the static ones
+
+        test_data = []
+
+        for release in UBUNTU_RELEASES:
+            static_source = self._get_source(release, 'main')
+            test_data.append(
+                (dynamic_source, release,
+                    self._get_source(release, 'main')),
+            )
+            test_data.append(
+                (static_source, release, static_source),
+            )
+
+        for i in test_data:
+            # distro codename the charm runs on
+            lsb_release.return_value = {'DISTRIB_CODENAME': i[1]}
+
+            # configure to use specific repo version
+            cfg['install_sources'] = i[0]
+            cfg['install_keys'] = KEY
+
+            # on_config calls package installation
+            self.harness.update_config(cfg)
+
+            # make sure the repo management calls were correct
+            add_source.assert_called_with(i[2], KEY)
+            apt_install.assert_called_with(self.harness.charm.PACKAGES,
+                                           fatal=True)
+
+    def test_enable_ssl(self):
         pass