[Pacemaker] Patch for bugzilla 2541: Shell should warn if parameter uniqueness is violated

Dejan Muhamedagic dejanmm at fastmail.fm
Thu Mar 10 04:14:02 EST 2011


Hi Holger,

On Wed, Mar 09, 2011 at 07:58:02PM +0100, Holger Teutsch wrote:
> Hi Dejan,
> 
> On Wed, 2011-03-09 at 14:00 +0100, Dejan Muhamedagic wrote:
> > Hi Holger,
> 
> > > > > 
> > > > > This would become:
> > > > > 
> > > > >            for p in all_obj_list: # passed from _verify()
> > > > > 			   if is_primitive(p.node):
> > > > > 
> > > > > > +            process_primitive(p.node, clash_dict)
> > > > > 
> > > > > Or perhaps to loop through self.obj_list and build clash_dict
> > > > > against all elements? Otherwise, you'll need to skip elements
> > > > > which don't pass the check but are not new/changed (in
> > > > > self.obj_list).
> > > > > 
> > > > 
> > > 
> > > I did not pass "set_obj_verify" in semantic check as this variable "only
> > > by chance" contains the right values.
> > 
> > But it's not by chance. As I wrote earlier, it always contains
> > the whole CIB. It has to, otherwise crm_verify wouldn't work. It
> > should actually be renamed to set_obj_all or similar. Since we
> > already have that list created, it's better to reuse it than to
> > create another one from scratch. Further, we may need to add more
> > semantic checks which would require looking at the whole CIB.
> > 
> 
> OK, I implemented it this way.
> 
> In order to show the intention of the arguments clearer:
> 
> Instead of
> 
>     def _verify(self, set_obj_semantic, set_obj_all = None):
>         if not set_obj_all:
>             set_obj_all = set_obj_semantic
>         rc1 = set_obj_all.verify()
>         if user_prefs.check_frequency != "never":
>             rc2 = set_obj_semantic.semantic_check(set_obj_all)
>         else:
>             rc2 = 0
>         return rc1 and rc2 <= 1
>     def verify(self,cmd):
>         "usage: verify"
>         if not cib_factory.is_cib_sane():
>             return False
>         return self._verify(mkset_obj("xml"))
> 
> This way (always passing both args):
> 
>     def _verify(self, set_obj_semantic, set_obj_all):
>         rc1 = set_obj_all.verify()
>         if user_prefs.check_frequency != "never":
>             rc2 = set_obj_semantic.semantic_check(set_obj_all)
>         else:
>             rc2 = 0
>         return rc1 and rc2 <= 1
>     def verify(self,cmd):
>         "usage: verify"
>         if not cib_factory.is_cib_sane():
>             return False
> 	set_obj_all = mkset_obj("xml")
>         return self._verify(set_obj_all, set_obj_all)

OK.

> > My only remaining concern is performance. Though the meta-data is
> > cached, perhaps it will pay off to save the RAInfo instance with
> > the element. But we can worry about that later.
> > 
> 
> I can work on this as next step.

I'll do some testing on really big configurations and try to
gauge the impact.

The patch makes some regression tests blow:

+  File "/usr/lib64/python2.6/site-packages/crm/ui.py", line 1441, in verify
+    return self._verify(mkset_obj("xml"))
+  File "/usr/lib64/python2.6/site-packages/crm/ui.py", line 1433, in _verify
+    rc2 = set_obj_semantic.semantic_check(set_obj_all)
+  File "/usr/lib64/python2.6/site-packages/crm/cibconfig.py", line 294, in semantic_check
+    rc = self.__check_unique_clash(set_obj_all)
+  File "/usr/lib64/python2.6/site-packages/crm/cibconfig.py", line 274, in __check_unique_clash
+    process_primitive(node, clash_dict)
+  File "/usr/lib64/python2.6/site-packages/crm/cibconfig.py", line 259, in process_primitive
+    if ra_params[ name ]['unique'] == '1':
+KeyError: 'OCF_CHECK_LEVEL'

Can't recall why OCF_CHECK_LEVEL appears here. There must be some
good explanation :)

Cheers,

Dejan

> > Cheers,
> > 
> > Dejan
> > 
> 
> - holger
> 

> diff -r a35d8d6d0ab1 shell/modules/cibconfig.py
> --- a/shell/modules/cibconfig.py	Wed Mar 09 11:21:03 2011 +0100
> +++ b/shell/modules/cibconfig.py	Wed Mar 09 19:53:50 2011 +0100
> @@ -230,11 +230,68 @@
>          See below for specific implementations.
>          '''
>          pass
> -    def semantic_check(self):
> +
> +    def __check_unique_clash(self, set_obj_all):
> +        'Check whether resource parameters with attribute "unique" clash'
> +
> +        def process_primitive(prim, clash_dict):
> +            '''
> +            Update dict clash_dict with
> +            (ra_class, ra_provider, ra_type, name, value) -> [ resourcename ]
> +            if parameter "name" should be unique
> +            '''
> +            ra_class = prim.getAttribute("class")
> +            ra_provider = prim.getAttribute("provider")
> +            ra_type = prim.getAttribute("type")
> +            ra_id = prim.getAttribute("id")
> +
> +            ra = RAInfo(ra_class, ra_type, ra_provider)
> +            if ra == None:
> +                return
> +            ra_params = ra.params()
> +
> +            attributes = prim.getElementsByTagName("instance_attributes")
> +            if len(attributes) == 0:
> +                return
> +
> +            for p in attributes[0].getElementsByTagName("nvpair"):
> +                name = p.getAttribute("name")
> +                if ra_params[ name ]['unique'] == '1':
> +                    value = p.getAttribute("value")
> +                    k = (ra_class, ra_provider, ra_type, name, value)
> +                    try:
> +                        clash_dict[k].append(ra_id)
> +                    except:
> +                        clash_dict[k] = [ra_id]
> +            return
> +
> +        # we check the whole CIB for clashes as a clash may originate between
> +        # an object already committed and a new one
> +        clash_dict = {}
> +        for obj in set_obj_all.obj_list:
> +            node = obj.node
> +            if is_primitive(node):
> +                process_primitive(node, clash_dict)
> +
> +        # but we only warn if a 'new' object is involved 
> +        check_set = set([o.node.getAttribute("id") for o in self.obj_list if is_primitive(o.node)])
> +
> +        rc = 0
> +        for param, resources in clash_dict.items():
> +            # at least one new object must be involved
> +            if len(resources) > 1 and len(set(resources) & check_set) > 0:
> +                    rc = 2
> +                    msg = 'Resources %s violate uniqueness for parameter "%s": "%s"' %\
> +                            (",".join(sorted(resources)), param[3], param[4])
> +                    common_warning(msg)
> +
> +        return rc
> +
> +    def semantic_check(self, set_obj_all):
>          '''
>          Test objects for sanity. This is about semantics.
>          '''
> -        rc = 0
> +        rc = self.__check_unique_clash(set_obj_all)
>          for obj in self.obj_list:
>              rc |= obj.check_sanity()
>          return rc
> diff -r a35d8d6d0ab1 shell/modules/ui.py.in
> --- a/shell/modules/ui.py.in	Wed Mar 09 11:21:03 2011 +0100
> +++ b/shell/modules/ui.py.in	Wed Mar 09 19:53:50 2011 +0100
> @@ -1425,12 +1425,12 @@
>          set_obj = mkset_obj(*args)
>          err_buf.release() # show them, but get an ack from the user
>          return set_obj.edit()
> -    def _verify(self, set_obj_semantic, set_obj_verify = None):
> -        if not set_obj_verify:
> -            set_obj_verify = set_obj_semantic
> -        rc1 = set_obj_verify.verify()
> +    def _verify(self, set_obj_semantic, set_obj_all = None):
> +        if not set_obj_all:
> +            set_obj_all = set_obj_semantic
> +        rc1 = set_obj_all.verify()
>          if user_prefs.check_frequency != "never":
> -            rc2 = set_obj_semantic.semantic_check()
> +            rc2 = set_obj_semantic.semantic_check(set_obj_all)
>          else:
>              rc2 = 0
>          return rc1 and rc2 <= 1

> _______________________________________________
> Pacemaker mailing list: Pacemaker at oss.clusterlabs.org
> http://oss.clusterlabs.org/mailman/listinfo/pacemaker
> 
> Project Home: http://www.clusterlabs.org
> Getting started: http://www.clusterlabs.org/doc/Cluster_from_Scratch.pdf
> Bugs: http://developerbugs.linux-foundation.org/enter_bug.cgi?product=Pacemaker





More information about the Pacemaker mailing list