Merge lp:~seyeongkim/charms/trusty/cinder/lp1383801 into lp:~openstack-charmers-archive/charms/trusty/cinder/next

[Seyeong Kim]

add restarting cinder-volume service on config-changed hook

[uosci-testing-bot (uosci-testing-bot)]

charm_lint_check #7127 cinder-next for xtrusia mp266176
    LINT OK: passed

Build: http://10.245.162.77:8080/job/charm_lint_check/7127/

[uosci-testing-bot (uosci-testing-bot)]

charm_unit_test #6622 cinder-next for xtrusia mp266176
    UNIT OK: passed

Build: http://10.245.162.77:8080/job/charm_unit_test/6622/

[Billy Olsen (billy-olsen)]

Adding a service_restart will guarantee that the service gets restarted whenever juju feels the urge to run the config_changed hook, which at the minimum per documentation (https://jujucharms.com/docs/devel/authors-charm-hooks#config-changed) includes:

- after install
- after upgrade charm
- after an agent is restarted
- whenever a configuration value is changed

And not in the documentation but also:
- whenever an agent loses connection with the state server(s)
- whenever a juju backup is created
- whenever else juju feels like it should

The config-changed hook should be expected to be invoked frequently. As such, restarting the cinder service every time will be disruptive.

We may need to see if we can further isolate this scenario a bit -or- use the underlying framework to detect if the value has changed (there is _some_ support in charmhelpers code, but I've little and less experience using it - see charmhelpers/core/unitdata.py iirc).

review: Needs Fixing

[uosci-testing-bot (uosci-testing-bot)]

charm_lint_check #7256 cinder-next for xtrusia mp266344
    LINT OK: passed

Build: http://10.245.162.77:8080/job/charm_lint_check/7256/

[uosci-testing-bot (uosci-testing-bot)]

charm_unit_test #6716 cinder-next for xtrusia mp266344
    UNIT FAIL: unit-test failed

UNIT Results (max last 2 lines):
make: *** [test] Error 1
ERROR:root:Make target returned non-zero.

Full unit test output: http://paste.ubuntu.com/11963789/
Build: http://10.245.162.77:8080/job/charm_unit_test/6716/

[uosci-testing-bot (uosci-testing-bot)]

charm_lint_check #7257 cinder-next for xtrusia mp266344
    LINT FAIL: lint-test failed

LINT Results (max last 2 lines):
make: *** [lint] Error 1
ERROR:root:Make target returned non-zero.

Full lint test output: http://paste.ubuntu.com/11964374/
Build: http://10.245.162.77:8080/job/charm_lint_check/7257/

[uosci-testing-bot (uosci-testing-bot)]

charm_unit_test #6717 cinder-next for xtrusia mp266344
    UNIT OK: passed

Build: http://10.245.162.77:8080/job/charm_unit_test/6717/

[uosci-testing-bot (uosci-testing-bot)]

charm_lint_check #7262 cinder-next for xtrusia mp266344
    LINT OK: passed

Build: http://10.245.162.77:8080/job/charm_lint_check/7262/

[uosci-testing-bot (uosci-testing-bot)]

charm_unit_test #6722 cinder-next for xtrusia mp266344
    UNIT OK: passed

Build: http://10.245.162.77:8080/job/charm_unit_test/6722/

[uosci-testing-bot (uosci-testing-bot)]

charm_amulet_test #5497 cinder-next for xtrusia mp266344
    AMULET FAIL: amulet-test failed

AMULET Results (max last 2 lines):
make: *** [functional_test] Error 1
ERROR:root:Make target returned non-zero.

Full amulet test output: http://paste.ubuntu.com/11965539/
Build: http://10.245.162.77:8080/job/charm_amulet_test/5497/

[Billy Olsen (billy-olsen)]

Thanks for the making the changes requested Seyeong. This looks much better and I'm happy to approve it once OSCI gets another vote on it.

Thanks!

[uosci-testing-bot (uosci-testing-bot)]

charm_lint_check #7327 cinder-next for xtrusia mp266344
    LINT OK: passed

Build: http://10.245.162.77:8080/job/charm_lint_check/7327/

[uosci-testing-bot (uosci-testing-bot)]

charm_unit_test #6778 cinder-next for xtrusia mp266344
    UNIT OK: passed

Build: http://10.245.162.77:8080/job/charm_unit_test/6778/

[uosci-testing-bot (uosci-testing-bot)]

charm_amulet_test #5536 cinder-next for xtrusia mp266344
    AMULET OK: passed

Build: http://10.245.162.77:8080/job/charm_amulet_test/5536/

[Billy Olsen (billy-olsen)]

LGTM, thanks Seyeong!

review: Approve

=== modified file 'hooks/cinder_hooks.py'
--- hooks/cinder_hooks.py	2015-07-21 19:28:23 +0000
+++ hooks/cinder_hooks.py	2015-07-30 07:50:42 +0000
@@ -143,6 +143,10 @@
                 relation_set(relation_id=rid,
                              upgrade_nonce=uuid.uuid4())
 
+    # overwrite config is not in conf file. so We can't use restart_on_change
+    if config_value_changed('overwrite'):
+        service_restart('cinder-volume')
+
     CONFIGS.write_all()
     configure_https()
     update_nrpe_config()

=== modified file 'unit_tests/test_cinder_hooks.py'
--- unit_tests/test_cinder_hooks.py	2015-07-22 19:16:31 +0000
+++ unit_tests/test_cinder_hooks.py	2015-07-30 07:50:42 +0000
@@ -147,7 +147,9 @@
 
     @patch.object(hooks, 'configure_https')
     @patch.object(hooks, 'git_install_requested')
-    def test_config_changed(self, git_requested, conf_https):
+    @patch.object(hooks, 'config_value_changed')
+    def test_config_changed(self, config_val_changed,
+                            git_requested, conf_https):
         'It writes out all config'
         git_requested.return_value = False
         self.openstack_upgrade_available.return_value = False
@@ -160,7 +162,9 @@
 
     @patch.object(hooks, 'configure_https')
     @patch.object(hooks, 'git_install_requested')
-    def test_config_changed_block_devices(self, git_requested, conf_https):
+    @patch.object(hooks, 'config_value_changed')
+    def test_config_changed_block_devices(self, config_val_changed,
+                                          git_requested, conf_https):
         'It writes out all config'
         git_requested.return_value = False
         self.openstack_upgrade_available.return_value = False
@@ -178,7 +182,10 @@
 
     @patch.object(hooks, 'configure_https')
     @patch.object(hooks, 'git_install_requested')
-    def test_config_changed_uses_remove_missing_force(self, git_requested,
+    @patch.object(hooks, 'config_value_changed')
+    def test_config_changed_uses_remove_missing_force(self,
+                                                      config_val_changed,
+                                                      git_requested,
                                                       conf_https):
         'It uses the remove-missing-force config option'
         git_requested.return_value = False
@@ -193,7 +200,9 @@
 
     @patch.object(hooks, 'configure_https')
     @patch.object(hooks, 'git_install_requested')
-    def test_config_changed_upgrade_available(self, git_requested, conf_https):
+    @patch.object(hooks, 'config_value_changed')
+    def test_config_changed_upgrade_available(self, config_val_changed,
+                                              git_requested, conf_https):
         'It writes out all config with an available OS upgrade'
         git_requested.return_value = False
         self.openstack_upgrade_available.return_value = True
@@ -226,6 +235,25 @@
         self.assertFalse(self.do_openstack_upgrade.called)
         self.assertTrue(conf_https.called)
 
+    @patch('charmhelpers.core.host.service')
+    @patch.object(hooks, 'configure_https')
+    @patch.object(hooks, 'git_install_requested')
+    @patch.object(hooks, 'config_value_changed')
+    def test_config_changed_overwrite_changed(self, config_val_changed,
+                                              git_requested, conf_https,
+                                              _services):
+        'It uses the overwrite config option'
+        git_requested.return_value = False
+        self.openstack_upgrade_available.return_value = False
+        config_val_changed.return_value = True
+        hooks.hooks.execute(['hooks/config-changed'])
+        self.assertTrue(self.CONFIGS.write_all.called)
+        self.assertTrue(conf_https.called)
+        self.configure_lvm_storage.assert_called_with(['sdb'],
+                                                      'cinder-volumes',
+                                                      False, False, False)
+        self.service_restart.assert_called_with('cinder-volume')
+
     def test_db_changed(self):
         'It writes out cinder.conf on db changed'
         self.relation_get.return_value = 'cinder/0 cinder/1'