changes from review

README.md

@@ -7,7 +7,7 @@ It acts as a subordinate charm, which can be deployed on any bare metal or virtu
 alongside a main charm. It is not supported in containers due to lack of ability for lxd 
 containers to access iSCSI and Fibre Channel hardware of the underlying kernel. 
 
-If you configure this charm for iSCSI, it will:
+If you configure this charm for iSCSI, this charm will:
 - Generate an iSCSI initiator name and put it in /etc/iscsi/initiatorname.iscsi
 - Install the package multipath-tools
 - Configure multipath under /etc/multipath/conf.d directory
@@ -16,19 +16,21 @@ If you configure this charm for iSCSI, it will:
 - Login to the target
 - Reload and restart the service multipathd
 
-If you configure it for Fibre Channel, it will:
+If you configure it for Fibre Channel, this charm will:
 - Install the package multipath-tools
 - Scan the host for HBA adapters
 - Retrieve the WWID for the Fibre Channel connection
 - Configure multipath under /etc/multipath/conf.d directory
 - Reload and restart multipathd.service
 
+Please note that the Fiber Channel option currently supports only one device. 
+
 If iSCSI, the user can input a initiator name dictionary in config.yaml if they wish to use a
 specific IQN for a specific unit. Also, the target IP and port are needed to perform
 the discovery and login with iscsiadm. 
 
-For Fibre Channel, the user can choose the device alias to be used when mapping the disk by 
-configuring the "fc-lun-alias" option in the bundle.
+For Fibre Channel, the user can choose the device alias to be used when mapping the disk.
+See the option "fc-lun-alias" for further details.
 
 
 ## Quickstart
@@ -40,7 +42,8 @@ make build
 ```
 This will create the `storage-connector.charm` file and place it in the `.build` directory.
 
-To deploy this subordinate charm on an ubuntu unit, deploy `cs:ubuntu` first.
+To deploy this subordinate charm on an ubuntu unit, deploy `cs:ubuntu` first , or other principal
+charm of your choosing (i.e nova-compute, mysql, etc.).
 ```
 juju add-model my-test-model
 juju deploy cs:ubuntu --series focal
@@ -51,9 +54,10 @@ juju deploy cs:storage-connector
 
 Edit the config of the target or the port:
 ```
-juju config storage-connector storage-type='iscsi'
-juju config storage-connector iscsi-target=<TARGET_IP> 
-juju config storage-connector iscsi-port=<PORT>
+juju config storage-connector \
+    storage-type='iscsi' \
+    iscsi-target=<TARGET_IP> \
+    iscsi-port=<PORT>
 ```
 
 To restart services manually, two actions exist:
@@ -64,18 +68,17 @@ juju run-action --unit ubuntu/0 reload-multipathd-service
 
 ### To configure this charm for Fibre Channel, do the following.
 
-Set the storage type to FC:
-```
-juju config storage-connector storage-type='fc'
-```
-Set the various configuration parameters:
+Set the storage type to FC and the various configuration parameters:
 ```
-juju config storage-connector fc-lun-alias='data1' multipath-defaults='{"user_friendly_names":"yes", "find_multipaths":"yes", "polling_interval":"10"}' multipath-devices='{"vendor":"PURE", "product":"FlashArray", "fast_io_fail_tmo":"10", "path_selector":"queue-length 0", "path_grouping_policy":"group_by_prio", "rr_min_io":"1", "path_checker":"tur", "fast_io_fail_tmo":"1", "dev_loss_tmo":"infinity", "no_path_retry":"5", "failback":"immediate", "prio":"alua", "hardware_handler":"1 alua", "max_sectors_kb":"4096"}'
+juju config storage-connector storage-type='fc' \
+    fc-lun-alias='data1' \
+    multipath-defaults='{"user_friendly_names":"yes", "find_multipaths":"yes", "polling_interval":"10"}' \
+    multipath-devices='{"vendor":"PURE", "product":"FlashArray", "fast_io_fail_tmo":"10", "path_selector":"queue-length 0", "path_grouping_policy":"group_by_prio", "rr_min_io":"1", "path_checker":"tur", "fast_io_fail_tmo":"1", "dev_loss_tmo":"infinity", "no_path_retry":"5", "failback":"immediate", "prio":"alua", "hardware_handler":"1 alua", "max_sectors_kb":"4096"}'
 ```
 
-### After the configuration set, relate the charm to ubuntu
+### After the configuration isset, relate the charm to ubuntu
 
-This will apply the configuration to the desired host.
+This will apply the configuration to hosts running the "ubuntu" application.
 ```
 juju relate ubuntu storage-connector
 ```

config.yaml

@@ -22,7 +22,7 @@ options:
         default: True
         description: |
             If set to True (default) and storage-type is iscsi, the charm will configure 
-            the iscsid and multipath configuration files, and then run a iscsiadm 
+            the iscsid and multipath configuration files, and then run an iscsiadm 
             discovery and login against the target. If set to False, the charm will simply 
             configure the unit but not run the discovery and login against the target. 
     initiator-dictionary:
@@ -70,9 +70,9 @@ options:
         default: 
         description: |
             In multipath config, add a blacklist device section to exlude local disks from being handled by multipath-tools. It is possible
-            to blacklist by vendor/product, by devnode or WWID. String should be of JSON dictionary format.
+            to blacklist by vendor/product, by devnode or WWID. String should be of JSON dictionary format. Double quotes are essential to the correct format of this JSON string.
             Example:
-                value: {"vendor": "QEMU", "product": "*"}
+                value: '{"vendor": "QEMU", "product": "*"}'
                 Which will produce this configuration:
                 blacklist {
                     device {

metadata.yaml

@@ -4,7 +4,7 @@ maintainers:
     - Camille Rodriguez <[email protected]>
 description: |
     The Storage Connector charm is an open-source charm that can be installed as a
-    subordinate to any VM or baremetal machine. It has two modes: iSCSI or Fibre Channel.
+    subordinate to any VM or bare metal machine. It has two modes: iSCSI or Fibre Channel.
     In the iSCSI mode, it will connect the host to an iSCSI target and configure multipath 
     accordingly. For Fibre Channel, this charm detects the WWID of the FC LUNs and 
     configures multipathd so that the device is ready to be used.

src/charm.py

@@ -40,17 +40,31 @@ class StorageConnectorCharm(CharmBase):
     ISCSI_SERVICES = ['iscsid', 'open-iscsi']
     MULTIPATHD_SERVICE = 'multipathd'
 
-    ISCSI_MANDATORY_CONFIG = [
-        'storage-type',
-        'iscsi-target',
-        'iscsi-port',
-        'multipath-devices'
-    ]
-    FC_MANDATORY_CONFIG = [
-        'storage-type',
-        'fc-lun-alias',
-        'multipath-devices'
-    ]
+    # ISCSI_MANDATORY_CONFIG = [
+    #     'storage-type',
+    #     'iscsi-target',
+    #     'iscsi-port',
+    #     'multipath-devices'
+    # ]
+    # FC_MANDATORY_CONFIG = [
+    #     'storage-type',
+    #     'fc-lun-alias',
+    #     'multipath-devices'
+    # ]
+    VALID_STORAGE_TYPES = ["fc", "iscsi"]
+    MANDATORY_CONFIG = {
+        "iscsi": [
+            'storage-type',
+            'iscsi-target',
+            'iscsi-port',
+            'multipath-devices'
+        ],
+        "fc": [
+            'storage-type',
+            'fc-lun-alias',
+            'multipath-devices'
+        ],
+    }
 
     def __init__(self, *args):
         """Initialize charm and configure states and events to observe."""
@@ -78,7 +92,7 @@ def __init__(self, *args):
     def on_install(self, event):
         """Handle install state."""
         self.unit.status = MaintenanceStatus("Installing charm software")
-        if self.check_if_container():
+        if self._check_if_container():
             return
 
         if not self._check_mandatory_config():
@@ -101,41 +115,40 @@ def on_install(self, event):
                     logging.info('Enabling %s service', service)
                     subprocess.check_call(['systemctl', 'enable', service])
                 except subprocess.CalledProcessError:
-                    logging.exception('Unable to enable %s.', service)
+                    logging.exception('Failed to enable %s.', service)
 
         self.unit.status = MaintenanceStatus("Install complete")
         logging.info("Install of software complete")
         self._stored.installed = True
 
     def render_config(self, event):
         """Render configuration templates upon config change."""
-        if self.check_if_container():
+        if self._check_if_container():
             return
 
         if not self._check_mandatory_config():
             return
 
         if self._stored.storage_type == 'fc' and not self._stored.fc_scan_ran_once:
-            if not self._fc_scan_host():
+            if not self._fc_scan_host_successfully():
                 return
 
         self.unit.status = MaintenanceStatus("Rendering charm configuration")
         self._create_directories()
 
-        charm_config = self.model.config
         tenv = Environment(loader=FileSystemLoader('templates'))
 
         if self._stored.storage_type == 'iscsi':
-            self._iscsi_initiator(tenv, charm_config)
-            self._iscsid_configuration(tenv, charm_config)
+            self._iscsi_initiator(tenv)
+            self._iscsid_configuration(tenv)
             self._restart_iscsi_services()
 
-            if charm_config.get('iscsi-discovery-and-login'):
+            if self.model.config.get('iscsi-discovery-and-login'):
                 logging.info('Launching iscsiadm discovery and login')
-                self._iscsiadm_discovery(charm_config)
+                self._iscsiadm_discovery()
                 self._iscsiadm_login()
 
-        if not self._multipath_configuration(tenv, charm_config):
+        if not self._multipath_configuration(tenv):
             return
         if not self._validate_multipath_config():
             return
@@ -200,9 +213,9 @@ def _reload_multipathd_service(self):
     def _check_mandatory_config(self):
         charm_config = self.model.config
 
-        if charm_config['storage-type'] in ['iscsi', 'fc']:
+        if charm_config['storage-type'] in self.VALID_STORAGE_TYPES:
             if (self._stored.storage_type == 'None' or
-                    self._stored.storage_type not in ['iscsi', 'fc']):
+                    self._stored.storage_type not in self.VALID_STORAGE_TYPES):
                 # allow user to change storage type only if initial entry was incorrect
                 self._stored.storage_type = charm_config['storage-type'].lower()
                 logging.debug(
@@ -219,10 +232,7 @@ def _check_mandatory_config(self):
             )
             return False
 
-        if self._stored.storage_type == "fc":
-            mandatory_config = self.FC_MANDATORY_CONFIG
-        elif self._stored.storage_type == "iscsi":
-            mandatory_config = self.ISCSI_MANDATORY_CONFIG
+        mandatory_config = self.MANDATORY_CONFIG[self._stored.storage_type]
         missing_config = []
         for config in mandatory_config:
             if charm_config.get(config) is None:
@@ -261,7 +271,8 @@ def _create_directories(self):
                 exist_ok=True,
                 mode=0o750)
 
-    def _iscsi_initiator(self, tenv, charm_config):
+    def _iscsi_initiator(self, tenv):
+        charm_config = self.model.config
         initiator_name = None
         hostname = socket.getfqdn()
         initiators = charm_config.get('initiator-dictionary')
@@ -283,7 +294,8 @@ def _iscsi_initiator(self, tenv, charm_config):
         rendered_content = template.render(ctxt)
         self.ISCSI_INITIATOR_NAME.write_text(rendered_content)
 
-    def _iscsid_configuration(self, tenv, charm_config):
+    def _iscsid_configuration(self, tenv):
+        charm_config = self.model.config
         ctxt = {
             'node_startup': charm_config.get('iscsi-node-startup'),
             'node_fastabort': charm_config.get('iscsi-node-session-iscsi-fastabort'),
@@ -300,23 +312,28 @@ def _iscsid_configuration(self, tenv, charm_config):
         self.ISCSI_CONF.write_text(rendered_content)
         self.ISCSI_CONF.chmod(0o600)
 
-    def _multipath_configuration(self, tenv, charm_config):
+    def _multipath_configuration(self, tenv):
+        charm_config = self.model.config
         ctxt = {}
         multipath_sections = ['defaults', 'devices', 'blacklist']
         for section in multipath_sections:
             config = charm_config.get('multipath-' + section)
             if config:
-                logging.info("Gather information for the multipaths section")
+                logging.info("Gather information for the multipaths section " + section)
+                logging.debug("multipath-" + section + " data: " + config)
                 try:
                     ctxt[section] = json.loads(config)
                 except Exception as e:
                     logging.info("An exception has occured. Please verify the format \
-                                 of the multipath config options. Traceback: %s", e)
+                                 of the multipath config option %s. \
+                                Traceback: %s", section, e)
                     self.unit.status = BlockedStatus(
                         "Exception occured during the multipath \
                         configuration. Please check logs."
                     )
                     return False
+            else:
+                logging.debug("multipath-" + section + " is empty.")
 
         if self._stored.storage_type == 'fc':
             wwid = self._retrieve_multipath_wwid()
@@ -335,7 +352,8 @@ def _multipath_configuration(self, tenv, charm_config):
         self.mp_path.chmod(0o600)
         return True
 
-    def _iscsiadm_discovery(self, charm_config):
+    def _iscsiadm_discovery(self):
+        charm_config = self.model.config
         target = charm_config.get('iscsi-target')
         port = charm_config.get('iscsi-port')
         logging.info('Launching iscsiadm discovery against target')
@@ -358,7 +376,7 @@ def _iscsiadm_login(self):
                 'Iscsi login failed against target'
             )
 
-    def _fc_scan_host(self):
+    def _fc_scan_host_successfully(self):
         hba_adapters = subprocess.getoutput('ls /sys/class/scsi_host')
         logging.debug('hba_adapters: {}'.format(hba_adapters))
         if not hba_adapters:
@@ -404,7 +422,7 @@ def _validate_multipath_config(self):
             return False
         return True
 
-    def check_if_container(self):
+    def _check_if_container(self):
         """Check if the charm is being deployed on a container host."""
         if utils.is_container():
             self.unit.status = BlockedStatus(