[Pacemaker] Patch for bugzilla 2541: Shell should warn if parameter uniqueness is violated
Dejan Muhamedagic
dejanmm at fastmail.fm
Fri Mar 11 06:11:03 EST 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