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

Dejan Muhamedagic dejanmm at fastmail.fm
Fri Mar 11 11:11:03 UTC 2011


Hi,

On Thu, Mar 10, 2011 at 07:08:20PM +0100, Holger Teutsch wrote:
> Hi Dejan,
> On Thu, 2011-03-10 at 10:14 +0100, Dejan Muhamedagic wrote:
> > 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,
> > > 
> 
> > > 
> > > 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)
> 
> See patch set_obj_all.diff
> 
> > 
> > > > 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.
> 
> OK
> 
> > 
> > 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 :)
> 
> The good explanation is: Not only params are in <instance_atributes ...>
> but OCF_CHECK_LEVEL as well within <operations ...>

Yes, it's instance_attributes within operations.

> The latest version no longer blows the test -> semantic_check.diff

Applied. Many thanks for the patch.

Cheers,

Dejan

> Regards
> Holger

> # HG changeset patch
> # User Holger Teutsch <holger.teutsch at web.de>
> # Date 1299775617 -3600
> # Branch hot
> # Node ID 30730ccc0aa09c3a476a18c6d95c680b35999995
> # Parent  9fa61ee6e35ef190f4126e163e9bfe6911e35541
> Low: Shell: Rename variable set_obj_verify to set_obj_all as it always contains all objects
> Simplify usage of this var in [_]verify, pass to CibObjectSet.semantic_check
> 
> diff -r 9fa61ee6e35e -r 30730ccc0aa0 shell/modules/cibconfig.py
> --- a/shell/modules/cibconfig.py	Wed Mar 09 13:41:27 2011 +0100
> +++ b/shell/modules/cibconfig.py	Thu Mar 10 17:46:57 2011 +0100
> @@ -230,7 +230,7 @@
>          See below for specific implementations.
>          '''
>          pass
> -    def semantic_check(self):
> +    def semantic_check(self, set_obj_all):
>          '''
>          Test objects for sanity. This is about semantics.
>          '''
> diff -r 9fa61ee6e35e -r 30730ccc0aa0 shell/modules/ui.py.in
> --- a/shell/modules/ui.py.in	Wed Mar 09 13:41:27 2011 +0100
> +++ b/shell/modules/ui.py.in	Thu Mar 10 17:46:57 2011 +0100
> @@ -1425,12 +1425,10 @@
>          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):
> +        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
> @@ -1438,7 +1436,8 @@
>          "usage: verify"
>          if not cib_factory.is_cib_sane():
>              return False
> -        return self._verify(mkset_obj("xml"))
> +        set_obj_all = mkset_obj("xml")
> +        return self._verify(set_obj_all, set_obj_all)
>      def save(self,cmd,*args):
>          "usage: save [xml] <filename>"
>          if not cib_factory.is_cib_sane():

> # HG changeset patch
> # User Holger Teutsch <holger.teutsch at web.de>
> # Date 1299779740 -3600
> # Branch hot
> # Node ID 73021c988d92c5dad4c503af9f8826f5d1c34373
> # Parent  30730ccc0aa09c3a476a18c6d95c680b35999995
> Med: Shell: Check for violations of uniqueness for instance parameters during commit
> Implemented in CibObjectSet.semantic_check()
> 
> diff -r 30730ccc0aa0 -r 73021c988d92 shell/modules/cibconfig.py
> --- a/shell/modules/cibconfig.py	Thu Mar 10 17:46:57 2011 +0100
> +++ b/shell/modules/cibconfig.py	Thu Mar 10 18:55:40 2011 +0100
> @@ -230,11 +230,70 @@
>          See below for specific implementations.
>          '''
>          pass
> +
> +    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()
> +
> +            for a in prim.getElementsByTagName("instance_attributes"):
> +                # params are in instance_attributes just below the parent
> +                # operations may have some as well, e.g. OCF_CHECK_LEVEL
> +                if a.parentNode != prim:
> +                    continue
> +
> +                for p in a.getElementsByTagName("nvpair"):
> +                    name = p.getAttribute("name")
> +                    if ra_params[ name ].get("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

> _______________________________________________
> 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