Merge ~txiao/charm-storage-connector:iscsi-discovery-and-login into charm-storage-connector:defer-service-restarts-integration
- Git
- lp:~txiao/charm-storage-connector
- iscsi-discovery-and-login
- Merge into defer-service-restarts-int...
Status: | Merged |
---|---|
Approved by: | Tianqi Xiao |
Approved revision: | 37b83ef6eab6b315e36e14bc53961e4af918d089 |
Merged at revision: | 37b83ef6eab6b315e36e14bc53961e4af918d089 |
Proposed branch: | ~txiao/charm-storage-connector:iscsi-discovery-and-login |
Merge into: | charm-storage-connector:defer-service-restarts-integration |
Diff against target: |
252 lines (+85/-57) 4 files modified
config.yaml (+5/-4) requirements.txt (+4/-2) src/charm.py (+7/-15) tests/unit/test_charm.py (+69/-36) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Paul Goins | Approve | ||
🤖 prod-jenkaas-bootstack (community) | continuous-integration | Approve | |
Andrea Ieri | Approve | ||
Review via email:
|
Commit message
Add logic to allow auto discovery and login after iscsi service restart
Description of the change
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote : | # |
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Andrea Ieri (aieri) wrote (last edit ): | # |
looks generally good, just a few small comments
and of course the CI failure needs to be investigated
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Tianqi Xiao (txiao) wrote : | # |
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:/
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:/
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Tianqi Xiao (txiao) : | # |
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote : | # |
PASSED: Continuous integration, rev:cc80e72a86d
https:/
Executed test runs:
SUCCESS: https:/
None: https:/
Click here to trigger a rebuild:
https:/
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Paul Goins (vultaire) wrote : | # |
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?
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Tianqi Xiao (txiao) wrote (last edit ): | # |
> 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:/
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Paul Goins (vultaire) wrote : | # |
OK - that makes sense.
Added one more comment based upon that chat thread.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Tianqi Xiao (txiao) wrote : | # |
I have pushed the changes accordingly. Thanks :)
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote : | # |
FAILED: Continuous integration, rev:37b83ef6eab
https:/
Executed test runs:
FAILURE: https:/
None: https:/
Click here to trigger a rebuild:
https:/
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote : | # |
FAILED: Continuous integration, rev:37b83ef6eab
https:/
Executed test runs:
FAILURE: https:/
None: https:/
Click here to trigger a rebuild:
https:/
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote : | # |
PASSED: Continuous integration, rev:37b83ef6eab
https:/
Executed test runs:
SUCCESS: https:/
None: https:/
Click here to trigger a rebuild:
https:/
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Paul Goins (vultaire) : | # |
Preview Diff
1 | diff --git a/config.yaml b/config.yaml |
2 | index c283b6f..c962a7f 100644 |
3 | --- a/config.yaml |
4 | +++ b/config.yaml |
5 | @@ -21,10 +21,11 @@ options: |
6 | type: boolean |
7 | default: True |
8 | description: | |
9 | - If set to True (default) and storage-type is iscsi, the charm will configure |
10 | - the iscsid and multipath configuration files, and then run an iscsiadm |
11 | - discovery and login against the target. If set to False, the charm will simply |
12 | - configure the unit but not run the discovery and login against the target. |
13 | + If set to True (default) and storage-type is iscsi, the charm will run an |
14 | + iscsiadm discovery and login against the target after charm-managed iscsi |
15 | + services restart (triggered by either config changes or by the restart-services |
16 | + action). If set to False, the charm will not automatically run the discovery |
17 | + and login against the target. |
18 | initiator-dictionary: |
19 | type: string |
20 | default: '{}' |
21 | diff --git a/requirements.txt b/requirements.txt |
22 | index 814be4a..49af55c 100644 |
23 | --- a/requirements.txt |
24 | +++ b/requirements.txt |
25 | @@ -1,3 +1,5 @@ |
26 | jinja2 |
27 | -ops |
28 | -charmhelpers |
29 | \ No newline at end of file |
30 | +charmhelpers |
31 | + |
32 | +# Pin ops version to support bionic deployments |
33 | +ops < 2.0 |
34 | \ No newline at end of file |
35 | diff --git a/src/charm.py b/src/charm.py |
36 | index 8f680a4..21ebdb2 100755 |
37 | --- a/src/charm.py |
38 | +++ b/src/charm.py |
39 | @@ -351,17 +351,21 @@ class StorageConnectorCharm(CharmBase): |
40 | except subprocess.CalledProcessError: |
41 | logging.exception('An error occured while restarting %s.', service) |
42 | |
43 | + # Clear deferred restart events |
44 | deferred_events.clear_deferred_restarts(services) |
45 | |
46 | + # If any iscsi services restarted and iscsi-discovery-and-login config is |
47 | + # set to true, run iscsiadm discovery and login. |
48 | + if (any(svc in self.ISCSI_SERVICES for svc in services) and |
49 | + self.model.config.get('iscsi-discovery-and-login')): |
50 | + self._iscsi_discovery_and_login() |
51 | + |
52 | def _reload_multipathd_service(self): |
53 | """Reload multipathd service.""" |
54 | logging.info('Reloading multipathd service') |
55 | try: |
56 | subprocess.check_call(['systemctl', 'reload', self.MULTIPATHD_SERVICE]) |
57 | except subprocess.CalledProcessError: |
58 | - self.unit.status = BlockedStatus( |
59 | - "An error occured while reloading the multipathd service." |
60 | - ) |
61 | logging.exception( |
62 | '%s', |
63 | "An error occured while reloading the multipathd service." |
64 | @@ -374,8 +378,6 @@ class StorageConnectorCharm(CharmBase): |
65 | charm_config = self.model.config |
66 | if charm_config.get('enable-auto-restarts') or not self._stored.started: |
67 | self._restart_services(services=self.ISCSI_SERVICES) |
68 | - if charm_config.get('iscsi-discovery-and-login'): |
69 | - self._iscsi_discovery_and_login() |
70 | else: |
71 | self._defer_service_restart( |
72 | services=self.ISCSI_SERVICES, |
73 | @@ -533,9 +535,6 @@ class StorageConnectorCharm(CharmBase): |
74 | '-p', target + ':' + port]) |
75 | except subprocess.CalledProcessError: |
76 | logging.exception('Iscsi discovery failed.') |
77 | - self.unit.status = BlockedStatus( |
78 | - 'Iscsi discovery failed against target' |
79 | - ) |
80 | return |
81 | |
82 | try: |
83 | @@ -545,13 +544,6 @@ class StorageConnectorCharm(CharmBase): |
84 | except subprocess.CalledProcessError as err: |
85 | logging.exception('Iscsi login failed. \n%s', err.output.decode("utf-8")) |
86 | |
87 | - # Check if exception occurred because session already present. |
88 | - # No error if it is, otherwise set block status. |
89 | - if "already present" not in err.output.decode("utf-8"): |
90 | - self.unit.status = BlockedStatus( |
91 | - 'Iscsi login failed against target' |
92 | - ) |
93 | - |
94 | def _fc_scan_host(self): |
95 | hba_adapters = subprocess.getoutput('ls /sys/class/scsi_host') |
96 | logging.debug('hba_adapters: {}'.format(hba_adapters)) |
97 | diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py |
98 | index 37f78a5..18daa18 100644 |
99 | --- a/tests/unit/test_charm.py |
100 | +++ b/tests/unit/test_charm.py |
101 | @@ -13,7 +13,7 @@ import charm |
102 | import charmhelpers.contrib.openstack.deferred_events as deferred_events |
103 | |
104 | from ops.framework import EventBase |
105 | -from ops.model import ActiveStatus, BlockedStatus |
106 | +from ops.model import ActiveStatus |
107 | from ops.testing import Harness |
108 | |
109 | |
110 | @@ -228,8 +228,10 @@ class TestCharm(unittest.TestCase): |
111 | action_event.results['failed'], 'No valid services are specified.') |
112 | |
113 | @patch("charm.subprocess.check_call") |
114 | - def test_on_restart_services_actionservices_success(self, |
115 | - m_check_call): |
116 | + @patch("charm.StorageConnectorCharm._iscsi_discovery_and_login") |
117 | + def test_on_restart_services_action_services_success(self, |
118 | + m_iscsi_discovery_and_login, |
119 | + m_check_call): |
120 | """Test on restart servcices action successfully run with services param.""" |
121 | action_event = FakeActionEvent(params={ |
122 | "deferred-only": False, "services": "iscsid open-iscsi multipathd"}) |
123 | @@ -243,6 +245,7 @@ class TestCharm(unittest.TestCase): |
124 | any_order=True |
125 | ) |
126 | self.assertEqual(action_event.results['success'], 'True') |
127 | + m_iscsi_discovery_and_login.assert_called_once() |
128 | |
129 | @patch("charm.deferred_events.get_deferred_restarts") |
130 | def test_on_show_deferred_restarts_action(self, m_get_deferred_restarts): |
131 | @@ -370,9 +373,64 @@ class TestCharm(unittest.TestCase): |
132 | any_order=False |
133 | ) |
134 | |
135 | + @patch("charm.subprocess.check_call") |
136 | + @patch("charm.StorageConnectorCharm._iscsi_discovery_and_login") |
137 | + def test_on_restart_non_iscsi_services(self, |
138 | + m_iscsi_discovery_and_login, |
139 | + m_check_call): |
140 | + """Test on restarting non-iscsi services.""" |
141 | + self.harness.charm._restart_services(services=["multipathd"]) |
142 | + m_check_call.assert_has_calls( |
143 | + [ |
144 | + call(["systemctl", "restart", "multipathd"]) |
145 | + ], |
146 | + any_order=True |
147 | + ) |
148 | + m_iscsi_discovery_and_login.assert_not_called() |
149 | + |
150 | + @patch("charm.subprocess.check_call") |
151 | + @patch("charm.StorageConnectorCharm._iscsi_discovery_and_login") |
152 | + def test_on_restart_iscsi_services_with_discovery_login( |
153 | + self, |
154 | + m_iscsi_discovery_and_login, |
155 | + m_check_call |
156 | + ): |
157 | + """Test on restarting a iscsi service with discovery and login.""" |
158 | + self.harness.update_config({'iscsi-discovery-and-login': True}) |
159 | + self.harness.charm._restart_services(services=["iscsid"]) |
160 | + m_check_call.assert_has_calls( |
161 | + [ |
162 | + call(["systemctl", "restart", "iscsid"]) |
163 | + ], |
164 | + any_order=True |
165 | + ) |
166 | + m_iscsi_discovery_and_login.assert_called_once() |
167 | + |
168 | + @patch("charm.subprocess.check_call") |
169 | + @patch("charm.StorageConnectorCharm._iscsi_discovery_and_login") |
170 | + def test_on_restart_iscsi_services_without_discovery_login( |
171 | + self, |
172 | + m_iscsi_discovery_and_login, |
173 | + m_check_call |
174 | + ): |
175 | + """Test on restarting a iscsi service without running discovery and login.""" |
176 | + self.harness.update_config({'iscsi-discovery-and-login': False}) |
177 | + self.harness.charm._restart_services(services=["iscsid"]) |
178 | + m_check_call.assert_has_calls( |
179 | + [ |
180 | + call(["systemctl", "restart", "iscsid"]) |
181 | + ], |
182 | + any_order=True |
183 | + ) |
184 | + m_iscsi_discovery_and_login.assert_not_called() |
185 | + |
186 | @patch("charm.subprocess.check_output") |
187 | @patch("charm.subprocess.check_call") |
188 | - def test_iscsiadm_discovery_failed(self, m_check_call, m_check_output): |
189 | + @patch("charm.logging.exception") |
190 | + def test_iscsiadm_discovery_failed(self, |
191 | + m_log_exception, |
192 | + m_check_call, |
193 | + m_check_output): |
194 | """Test response to iscsiadm discovery failure.""" |
195 | self.harness.update_config({ |
196 | "storage-type": "iscsi", |
197 | @@ -386,14 +444,15 @@ class TestCharm(unittest.TestCase): |
198 | '-p', "abc" + ':' + "443"], |
199 | ) |
200 | self.harness.charm._iscsi_discovery_and_login() |
201 | - self.assertEqual( |
202 | - self.harness.charm.unit.status, |
203 | - BlockedStatus('Iscsi discovery failed against target') |
204 | - ) |
205 | + m_log_exception.assert_called_once() |
206 | |
207 | @patch("charm.subprocess.check_output") |
208 | @patch("charm.subprocess.check_call") |
209 | - def test_iscsiadm_login_failed(self, m_check_call, m_check_output): |
210 | + @patch("charm.logging.exception") |
211 | + def test_iscsiadm_login_failed(self, |
212 | + m_log_exception, |
213 | + m_check_call, |
214 | + m_check_output): |
215 | """Test response to iscsiadm login failure.""" |
216 | self.harness.update_config({ |
217 | "storage-type": "iscsi", |
218 | @@ -407,33 +466,7 @@ class TestCharm(unittest.TestCase): |
219 | output=b'iscsiadm: Could not log into all portals' |
220 | ) |
221 | self.harness.charm._iscsi_discovery_and_login() |
222 | - self.assertEqual( |
223 | - self.harness.charm.unit.status, |
224 | - BlockedStatus('Iscsi login failed against target') |
225 | - ) |
226 | - |
227 | - @patch("charm.subprocess.check_output") |
228 | - @patch("charm.subprocess.check_call") |
229 | - def test_iscsiadm_login_exist_session(self, m_check_call, m_check_output): |
230 | - """Test response to iscsiadm login to exist session.""" |
231 | - self.harness.update_config({ |
232 | - "storage-type": "iscsi", |
233 | - "iscsi-target": "abc", |
234 | - "iscsi-port": "443" |
235 | - }) |
236 | - self.harness.charm.unit.status = ActiveStatus("Unit is ready") |
237 | - m_check_output.side_effect = charm.subprocess.CalledProcessError( |
238 | - returncode=15, |
239 | - cmd=['iscsiadm', '-m', 'node', '--login'], |
240 | - output=b'iscsiadm: default: 1 session requested, ' + |
241 | - b'but 1 already present.\n' + |
242 | - b'iscsiadm: Could not log into all portals' |
243 | - ) |
244 | - self.harness.charm._iscsi_discovery_and_login() |
245 | - self.assertEqual( |
246 | - self.harness.charm.unit.status, |
247 | - ActiveStatus("Unit is ready") |
248 | - ) |
249 | + m_log_exception.assert_called_once() |
250 | |
251 | @patch("storage_connector.metrics_utils.uninstall_exporter") |
252 | @patch("storage_connector.metrics_utils.install_exporter") |
FAILED: Continuous integration, rev:7940b41964a e57f9eeddb2dd59 c2544a5ddb93a7 /jenkins. canonical. com/bootstack/ job/lp- charm-storage- connector- ci/12/ /jenkins. canonical. com/bootstack/ job/lp- charm-test- functest/ 224/ /jenkins. canonical. com/bootstack/ job/lp- update- mp/86/
https:/
Executed test runs:
FAILURE: https:/
None: https:/
Click here to trigger a rebuild: /jenkins. canonical. com/bootstack/ job/lp- charm-storage- connector- ci/12// rebuild
https:/