Merge lp:~freyes/charms/trusty/hacluster/lp1444361 into lp:~openstack-charmers/charms/trusty/hacluster/next

[Felipe Reyes]

Dear OpenStack Charmers,

This patch exposes the default resource stickiness as a configuration option, this will allow to tweak the cluster depending on the resources are being deployed. Some resources are more expensive to migrate or have bigger impact on the system, so having the option to tweak those is necessary.

Best Regards,

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

charm_lint_check #3382 hacluster-next for freyes mp256272
    LINT OK: passed

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

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

charm_unit_test #3170 hacluster-next for freyes mp256272
    UNIT OK: passed

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

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

charm_amulet_test #3163 hacluster-next for freyes mp256272
    AMULET FAIL: amulet-test missing

AMULET Results (max last 2 lines):
INFO:root:Search string not found in makefile target commands.
ERROR:root:No make target was executed.

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

[Jorge Niedbalski (niedbalski)]

Looks good to me, overall. Minor README clarifications.

review: Needs Information

[Billy Olsen (billy-olsen)]

Felipe, as always, thanks for the submission!

I think this is great as we can set the default resource stickiness. As we discussed, pacemaker accepts the string values of INFINITY and -INFINITY when defining resource weights. I think we should consider using a string instead of an int (preferred), or clearly documenting how the concepts of INFINITY and -INFINITY can be supplied via integer values.

As I think about this more, it is the service charms themselves [keystone, mysql, etc, etc] that define the pacemaker resources. These services should theoretically know about the resource stickiness values for the various resources which are being monitored via pacemaker and therefore it should be up to that service to help define the resource stickiness. Allowing the user to configure the default resource stickiness could actually interfere with what the service knows to be best. Rather than set it for the resources as the default - your cases such as the single nova console auth needs to be able to specify the resource-stickiness on the resource creation.

I think the best place is to do it on the services where the resources are defined and not the default. We can discuss more if you like, but for now I'm -1 on this due to the ability to interfere with the 'best decisions of the charms'.

review: Disapprove

[Billy Olsen (billy-olsen)]

I recognize my last comment may be mildly confusing (sorry). I wrote the first paragraph whilst doing an initial scan of the code but apparently didn't save the comment.

My current stance starts with the 'As I think about this more...' part in which I feel that the services defining the resources know better what the stickiness values should be.

[Felipe Reyes (freyes)]

Billy,

I agree with you that the charm (keystone, percona, etc..) knows better which value is better for each pacemaker resource, but here we are talking about the *default* resource stickiness, which is global to the cluster itself, charms still can override this during the creation of a resource.

The only thing this patch does is "let's remove the hardcoded value (100) and leave it as a config option".

At a later step we can extend each charm to expose an option to tweak each resource stickiness value (today they rely in the default), this will allow a more granular configuration or fine-tunning(tm) :)

Best,

[lp:~freyes/charms/trusty/hacluster/lp1444361]

46. By Felipe Reyes on 2015-04-16

Change type to string and accept INFINITY as a valid value

Unmerged revisions

46. By Felipe Reyes on 2015-04-16

Change type to string and accept INFINITY as a valid value

45. By Felipe Reyes on 2015-04-15

Expose default resource stickiness as a config option

Fixed LP #1444361

44. By Edward Hope-Morley on 2015-04-14

[gnuoy,r=hopem]

Allow corosync.conf netmtu to be set regardless of inet
mode (ipv4/ipv6).

43. By Billy Olsen on 2015-03-18

[freyes,r=wolsen,niedbalski]

Check the output of 'crm resource status' to determine if service is running

Fixes-Bug: #1433377

42. By James Page on 2015-03-12

[freyes,r=james-page]

Add support for location configuration

Tidy logging.

41. By Liam Young on 2015-01-26

Fix charm-helpers.yaml and charmhelper sync

40. By Edward Hope-Morley on 2014-12-17

[hopem,r=niedbalski,tinoco,wolsen]

Don't upgrade pacemaker packages. This restriction
can be removed once 1382842 is fixed but for the
moment we can't allow upgrades due to issues with
pacemaker dependencies.

Closes-Bug: 1382842

39. By Edward Hope-Morley on 2014-12-15

[freyes,r=hopem]

Add debug option to config.yaml and optionally enable
debug logging in corosync.conf

38. By James Page on 2014-12-15

Merge in bundle tests from master charm

37. By Edward Hope-Morley on 2014-12-08

[wolsen,r=hopem]

Specify the vote count for multicast quorum to be the number of nodes
which are configured. Additionally, specify the two_node value in the
quorum section when there are 2 nodes configured.

Closes-Bug: 1394008

=== modified file 'config.yaml'
--- config.yaml	2015-04-14 10:16:50 +0000
+++ config.yaml	2015-04-16 07:50:01 +0000
@@ -97,3 +97,16 @@
     default: False
     type: boolean
     description: Enable debug logging
+  default-resource-stickiness:
+    type: string
+    default: '100'
+    description: |
+      Controls the number of seconds before considering to migrate again the
+      resource. This value is expressed in seconds and can go from 0 to 1000000
+      (INFINITY)
+      .
+      In some circumstances, it is highly desirable to prevent healthy
+      resources from being moved around the cluster. Moving resources almost
+      always requires a period of downtime.
+      .
+      More info at http://clusterlabs.org/doc/en-US/Pacemaker/1.1-plugin/html/Clusters_from_Scratch/ch05s03s02.html

=== modified file 'hooks/hooks.py'
--- hooks/hooks.py	2015-04-14 10:16:50 +0000
+++ hooks/hooks.py	2015-04-16 07:50:01 +0000
@@ -70,6 +70,7 @@
 
 PACKAGES = ['corosync', 'pacemaker', 'python-netaddr', 'ipmitool']
 SUPPORTED_TRANSPORTS = ['udp', 'udpu', 'multicast', 'unicast']
+INFINITY = 1000000
 
 
 @hooks.hook()
@@ -293,8 +294,21 @@
         cmd = "crm configure property no-quorum-policy=ignore"
     pcmk.commit(cmd)
 
-    cmd = 'crm configure rsc_defaults $id="rsc-options"' \
-          ' resource-stickiness="100"'
+    stickiness = config('default-resource-stickiness')
+    try:
+        stickiness = int(stickiness)
+
+        if stickiness < 0 or stickiness >= INFINITY:
+            stickiness = 'INFINITY'
+    except ValueError:
+        stickiness = stickiness.strip().upper()
+        if stickiness != 'INFINITY':
+            log(("Value '%s' isn't valid for resource stickiness, using "
+                 "default" % config('default-resource-stickiness')))
+            stickiness = 100
+
+    cmd = ('crm configure rsc_defaults $id="rsc-options"'
+           ' resource-stickiness="%s"' % stickiness)
     pcmk.commit(cmd)
 
 

=== modified file 'unit_tests/test_hacluster_hooks.py'
--- unit_tests/test_hacluster_hooks.py	2015-03-06 14:58:15 +0000
+++ unit_tests/test_hacluster_hooks.py	2015-04-16 07:50:01 +0000
@@ -173,3 +173,46 @@
                 else:
                     commit.assert_any_call(
                         'crm -w -F configure %s %s %s' % (kw, name, params))
+
+    @mock.patch('pcmk.commit')
+    @mock.patch('hooks.config')
+    def test_configure_cluster_global(self, config, commit):
+        cfg = {'debug': False,
+               'prefer-ipv6': False,
+               'corosync_transport': 'udpu',
+               'corosync_mcastaddr': 'corosync_mcastaddr',
+               'cluster_count': 3,
+               'default-resource-stickiness': '101'}
+
+        def c(k):
+            return cfg.get(k)
+
+        config.side_effect = c
+
+        # check a value different from default
+        hacluster_hooks.configure_cluster_global()
+
+        commit.assert_any_call("crm configure property no-quorum-policy=stop")
+        commit.assert_any_call(('crm configure rsc_defaults $id="rsc-options"'
+                                ' resource-stickiness="101"'))
+        commit.reset_mock()
+
+        # setting it to infinity
+        cfg['default-resource-stickiness'] = 'infinity '
+        hacluster_hooks.configure_cluster_global()
+        commit.assert_any_call(('crm configure rsc_defaults $id="rsc-options"'
+                                ' resource-stickiness="INFINITY"'))
+        commit.reset_mock()
+
+        # a value bigger than 1000000 is inifity
+        cfg['default-resource-stickiness'] = '100000000'
+        hacluster_hooks.configure_cluster_global()
+        commit.assert_any_call(('crm configure rsc_defaults $id="rsc-options"'
+                                ' resource-stickiness="INFINITY"'))
+        commit.reset_mock()
+
+        # check invalid value and to make it fallback to default value
+        cfg['default-resource-stickiness'] = 'sdflkm4l'
+        hacluster_hooks.configure_cluster_global()
+        commit.assert_any_call(('crm configure rsc_defaults $id="rsc-options"'
+                                ' resource-stickiness="100"'))