Merge ~txiao/charm-storage-connector:iscsi-discovery-and-login into charm-storage-connector:defer-service-restarts-integration

[Tianqi Xiao]

Add logic to allow auto discovery and login after iscsi service restart

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

FAILED: Continuous integration, rev:7940b41964ae57f9eeddb2dd59c2544a5ddb93a7
https://jenkins.canonical.com/bootstack/job/lp-charm-storage-connector-ci/12/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/224/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/86/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-storage-connector-ci/12//rebuild

review: Needs Fixing (continuous-integration)

[Andrea Ieri (aieri)]

looks generally good, just a few small comments

and of course the CI failure needs to be investigated

review: Needs Fixing

[Tianqi Xiao (txiao)]

The CI failure on bionic is unrelated to this MP. I've ran func tests against the upstream repo and observed the same error: https://pastebin.canonical.com/p/6SXgjjkb8V/.

After some digging, I found out ops library is trying to import dataclasses, which only exist in python 3.7+ (bionic is on python 3.6 by default). And according to their release note, ops2 supports only python3.8+. This change was introduced 2 weeks ago in this commit [0], which means all charms built with latest ops library would not be able to run on bionic machine.

[0]: https://github.com/canonical/operator/commit/34e5dfb08251295c96cbfd3976c78b41c991b4ec

[Andrea Ieri (aieri)]

lgtm

review: Approve

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

PASSED: Continuous integration, rev:cc80e72a86dd9caa34e13ec410d5665de5d18a30
https://jenkins.canonical.com/bootstack/job/lp-charm-storage-connector-ci/13/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/228/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/90/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-storage-connector-ci/13//rebuild

review: Approve (continuous-integration)

[Paul Goins (vultaire)]

Basically it seems OK - although I'm concerned regarding the removal of the BlockedStatuses does jump out. I presume there's been some discussion about this?

My feeling is that simply logging errors without any status visible via the model is likely to result in silent problems that engineers are less likely to notice - that's why I'm concerned. But I also see these statuses were very deliberately removed.

Can you explain the "why" here?

review: Needs Information

[Tianqi Xiao (txiao)]

> Basically it seems OK - although I'm concerned regarding the removal of the
> BlockedStatuses does jump out. I presume there's been some discussion about
> this?
>
> My feeling is that simply logging errors without any status visible via the
> model is likely to result in silent problems that engineers are less likely to
> notice - that's why I'm concerned. But I also see these statuses were very
> deliberately removed.
>
> Can you explain the "why" here?

Thanks for your review.

Yes, they are deliberately removed after some discussions with Andrea and the Charm team (partial convo here[1]). The reason is that charm, by definition, should just be a wrapper of the actual payload, therefore its status should only reflect the state of the automation process, not of its underlying services. Monitoring services should be responsible for catching abnormalities for the latter.

Additionally, in the discovery and login case specifically, there are many reasons why it would fail and not all of them can be resolved in the charm (e.g. iscsi target is dead or mis-configured). So setting a BlockedStatus in such a case without having a way to unblock it from the charm side is a bit counter-intuitive.

Therefore, we decided to remove them and only keeping the ones that can be resolved from the charm side.

[1]: https://chat.charmhub.io/charmhub/pl/cj3f7pb1ktg8jg8hqj7oxk5dph

[Paul Goins (vultaire)]

OK - that makes sense.

Added one more comment based upon that chat thread.

[Tianqi Xiao (txiao)]

I have pushed the changes accordingly. Thanks :)

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

FAILED: Continuous integration, rev:37b83ef6eab6b315e36e14bc53961e4af918d089
https://jenkins.canonical.com/bootstack/job/lp-charm-storage-connector-ci/14/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/265/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/93/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-storage-connector-ci/14//rebuild

review: Needs Fixing (continuous-integration)

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

FAILED: Continuous integration, rev:37b83ef6eab6b315e36e14bc53961e4af918d089
https://jenkins.canonical.com/bootstack/job/lp-charm-storage-connector-ci/15/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/266/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/94/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-storage-connector-ci/15//rebuild

review: Needs Fixing (continuous-integration)

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

PASSED: Continuous integration, rev:37b83ef6eab6b315e36e14bc53961e4af918d089
https://jenkins.canonical.com/bootstack/job/lp-charm-storage-connector-ci/16/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/267/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/95/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-storage-connector-ci/16//rebuild

review: Approve (continuous-integration)

[Paul Goins (vultaire)]

review: Approve

diff --git a/config.yaml b/config.yaml
index c283b6f..c962a7f 100644
--- a/config.yaml
+++ b/config.yaml
@@ -21,10 +21,11 @@ options:
         type: boolean
         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 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.
+            If set to True (default) and storage-type is iscsi, the charm will run an
+            iscsiadm discovery and login against the target after charm-managed iscsi
+            services restart (triggered by either config changes or by the restart-services
+            action). If set to False, the charm will not automatically run the discovery
+            and login against the target.
     initiator-dictionary:
         type: string
         default: '{}'
diff --git a/requirements.txt b/requirements.txt
index 814be4a..49af55c 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -1,3 +1,5 @@
 jinja2
-ops
-charmhelpers
 \ No newline at end of file
+charmhelpers
+
+# Pin ops version to support bionic deployments
+ops < 2.0
 \ No newline at end of file
diff --git a/src/charm.py b/src/charm.py
index 8f680a4..21ebdb2 100755
--- a/src/charm.py
+++ b/src/charm.py
@@ -351,17 +351,21 @@ class StorageConnectorCharm(CharmBase):
                 except subprocess.CalledProcessError:
                     logging.exception('An error occured while restarting %s.', service)
 
+            # Clear deferred restart events
             deferred_events.clear_deferred_restarts(services)
 
+            # If any iscsi services restarted and iscsi-discovery-and-login config is
+            # set to true, run iscsiadm discovery and login.
+            if (any(svc in self.ISCSI_SERVICES for svc in services) and
+                    self.model.config.get('iscsi-discovery-and-login')):
+                self._iscsi_discovery_and_login()
+
     def _reload_multipathd_service(self):
         """Reload multipathd service."""
         logging.info('Reloading multipathd service')
         try:
             subprocess.check_call(['systemctl', 'reload', self.MULTIPATHD_SERVICE])
         except subprocess.CalledProcessError:
-            self.unit.status = BlockedStatus(
-                "An error occured while reloading the multipathd service."
-            )
             logging.exception(
                 '%s',
                 "An error occured while reloading the multipathd service."
@@ -374,8 +378,6 @@ class StorageConnectorCharm(CharmBase):
         charm_config = self.model.config
         if charm_config.get('enable-auto-restarts') or not self._stored.started:
             self._restart_services(services=self.ISCSI_SERVICES)
-            if charm_config.get('iscsi-discovery-and-login'):
-                self._iscsi_discovery_and_login()
         else:
             self._defer_service_restart(
                 services=self.ISCSI_SERVICES,
@@ -533,9 +535,6 @@ class StorageConnectorCharm(CharmBase):
                                   '-p', target + ':' + port])
         except subprocess.CalledProcessError:
             logging.exception('Iscsi discovery failed.')
-            self.unit.status = BlockedStatus(
-                'Iscsi discovery failed against target'
-            )
             return
 
         try:
@@ -545,13 +544,6 @@ class StorageConnectorCharm(CharmBase):
         except subprocess.CalledProcessError as err:
             logging.exception('Iscsi login failed. \n%s', err.output.decode("utf-8"))
 
-            # Check if exception occurred because session already present.
-            # No error if it is, otherwise set block status.
-            if "already present" not in err.output.decode("utf-8"):
-                self.unit.status = BlockedStatus(
-                    'Iscsi login failed against target'
-                )
-
     def _fc_scan_host(self):
         hba_adapters = subprocess.getoutput('ls /sys/class/scsi_host')
         logging.debug('hba_adapters: {}'.format(hba_adapters))
diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py
index 37f78a5..18daa18 100644
--- a/tests/unit/test_charm.py
+++ b/tests/unit/test_charm.py
@@ -13,7 +13,7 @@ import charm
 import charmhelpers.contrib.openstack.deferred_events as deferred_events
 
 from ops.framework import EventBase
-from ops.model import ActiveStatus, BlockedStatus
+from ops.model import ActiveStatus
 from ops.testing import Harness
 
 
@@ -228,8 +228,10 @@ class TestCharm(unittest.TestCase):
             action_event.results['failed'], 'No valid services are specified.')
 
     @patch("charm.subprocess.check_call")
-    def test_on_restart_services_actionservices_success(self,
-                                                        m_check_call):
+    @patch("charm.StorageConnectorCharm._iscsi_discovery_and_login")
+    def test_on_restart_services_action_services_success(self,
+                                                         m_iscsi_discovery_and_login,
+                                                         m_check_call):
         """Test on restart servcices action successfully run with services param."""
         action_event = FakeActionEvent(params={
             "deferred-only": False, "services": "iscsid open-iscsi multipathd"})
@@ -243,6 +245,7 @@ class TestCharm(unittest.TestCase):
             any_order=True
         )
         self.assertEqual(action_event.results['success'], 'True')
+        m_iscsi_discovery_and_login.assert_called_once()
 
     @patch("charm.deferred_events.get_deferred_restarts")
     def test_on_show_deferred_restarts_action(self, m_get_deferred_restarts):
@@ -370,9 +373,64 @@ class TestCharm(unittest.TestCase):
             any_order=False
         )
 
+    @patch("charm.subprocess.check_call")
+    @patch("charm.StorageConnectorCharm._iscsi_discovery_and_login")
+    def test_on_restart_non_iscsi_services(self,
+                                           m_iscsi_discovery_and_login,
+                                           m_check_call):
+        """Test on restarting non-iscsi services."""
+        self.harness.charm._restart_services(services=["multipathd"])
+        m_check_call.assert_has_calls(
+            [
+                call(["systemctl", "restart", "multipathd"])
+            ],
+            any_order=True
+        )
+        m_iscsi_discovery_and_login.assert_not_called()
+
+    @patch("charm.subprocess.check_call")
+    @patch("charm.StorageConnectorCharm._iscsi_discovery_and_login")
+    def test_on_restart_iscsi_services_with_discovery_login(
+        self,
+        m_iscsi_discovery_and_login,
+        m_check_call
+    ):
+        """Test on restarting a iscsi service with discovery and login."""
+        self.harness.update_config({'iscsi-discovery-and-login': True})
+        self.harness.charm._restart_services(services=["iscsid"])
+        m_check_call.assert_has_calls(
+            [
+                call(["systemctl", "restart", "iscsid"])
+            ],
+            any_order=True
+        )
+        m_iscsi_discovery_and_login.assert_called_once()
+
+    @patch("charm.subprocess.check_call")
+    @patch("charm.StorageConnectorCharm._iscsi_discovery_and_login")
+    def test_on_restart_iscsi_services_without_discovery_login(
+        self,
+        m_iscsi_discovery_and_login,
+        m_check_call
+    ):
+        """Test on restarting a iscsi service without running discovery and login."""
+        self.harness.update_config({'iscsi-discovery-and-login': False})
+        self.harness.charm._restart_services(services=["iscsid"])
+        m_check_call.assert_has_calls(
+            [
+                call(["systemctl", "restart", "iscsid"])
+            ],
+            any_order=True
+        )
+        m_iscsi_discovery_and_login.assert_not_called()
+
     @patch("charm.subprocess.check_output")
     @patch("charm.subprocess.check_call")
-    def test_iscsiadm_discovery_failed(self, m_check_call, m_check_output):
+    @patch("charm.logging.exception")
+    def test_iscsiadm_discovery_failed(self,
+                                       m_log_exception,
+                                       m_check_call,
+                                       m_check_output):
         """Test response to iscsiadm discovery failure."""
         self.harness.update_config({
             "storage-type": "iscsi",
@@ -386,14 +444,15 @@ class TestCharm(unittest.TestCase):
                  '-p', "abc" + ':' + "443"],
         )
         self.harness.charm._iscsi_discovery_and_login()
-        self.assertEqual(
-            self.harness.charm.unit.status,
-            BlockedStatus('Iscsi discovery failed against target')
-        )
+        m_log_exception.assert_called_once()
 
     @patch("charm.subprocess.check_output")
     @patch("charm.subprocess.check_call")
-    def test_iscsiadm_login_failed(self, m_check_call, m_check_output):
+    @patch("charm.logging.exception")
+    def test_iscsiadm_login_failed(self,
+                                   m_log_exception,
+                                   m_check_call,
+                                   m_check_output):
         """Test response to iscsiadm login failure."""
         self.harness.update_config({
             "storage-type": "iscsi",
@@ -407,33 +466,7 @@ class TestCharm(unittest.TestCase):
             output=b'iscsiadm: Could not log into all portals'
         )
         self.harness.charm._iscsi_discovery_and_login()
-        self.assertEqual(
-            self.harness.charm.unit.status,
-            BlockedStatus('Iscsi login failed against target')
-        )
-
-    @patch("charm.subprocess.check_output")
-    @patch("charm.subprocess.check_call")
-    def test_iscsiadm_login_exist_session(self, m_check_call, m_check_output):
-        """Test response to iscsiadm login to exist session."""
-        self.harness.update_config({
-            "storage-type": "iscsi",
-            "iscsi-target": "abc",
-            "iscsi-port": "443"
-        })
-        self.harness.charm.unit.status = ActiveStatus("Unit is ready")
-        m_check_output.side_effect = charm.subprocess.CalledProcessError(
-            returncode=15,
-            cmd=['iscsiadm', '-m', 'node', '--login'],
-            output=b'iscsiadm: default: 1 session requested, ' +
-                   b'but 1 already present.\n' +
-                   b'iscsiadm: Could not log into all portals'
-        )
-        self.harness.charm._iscsi_discovery_and_login()
-        self.assertEqual(
-            self.harness.charm.unit.status,
-            ActiveStatus("Unit is ready")
-        )
+        m_log_exception.assert_called_once()
 
     @patch("storage_connector.metrics_utils.uninstall_exporter")
     @patch("storage_connector.metrics_utils.install_exporter")