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@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
_______________________________________________
Pacemaker mailing list: Pacemaker@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
