Hi Holger,
On Tue, Mar 08, 2011 at 09:15:01AM +0100, Holger Teutsch wrote:
> On Fri, 2011-03-04 at 13:06 +0100, Holger Teutsch wrote:
> > On Thu, 2011-03-03 at 10:55 +0100, Florian Haas wrote:
> > > On 2011-03-03 10:43, Holger Teutsch wrote:
> > > > Hi,
> > > > I submit a patch for
> > > > "bugzilla 2541: Shell should warn if parameter uniqueness is violated"
> > > > for discussion.
> > >
> > > I'll leave it do Dejan to review the code, but I love the functionality.
> > > Thanks a lot for tackling this. My only suggestion for an improvement is
> > > to make the warning message a bit more terse, as in:
> > >
> > > WARNING: Resources ip1a, ip1b violate uniqueness for parameter "ip":
> > > "1.2.3.4"
> > >
> >
> > Florian,
> > I see your point. Although my formatting allows for an unlimited number
> > of collisions ( 8-) ) in real life we will only have 2 or 3. Will change
> > this together with Dejan's hints.
> >
> > > Cheers,
> > > Florian
> > >
> Florian + Dejan,
> here the version with terse output. The code got terser as well.
It looks good, just a few notes. The check function should move
to the CibObjectSetRaw class and be invoked from
semantic_check(). There's
rc1 = set_obj_verify.verify()
if user_prefs.check_frequency != "never":
rc2 = set_obj_semantic.semantic_check()
The last should be changed to:
rc2 = set_obj_semantic.semantic_check(set_obj_verify)
set_obj_verify always contains all CIB elements (well, that means
that its name should probably be changed too :). Now, the code
should check _only_ new and changed primitives which are
contained in set_obj_semantic. That's because we don't want to
repeatedly print warnings for all objects on commit, but only for
those which were added/changed in the meantime. On the other
hand, verify is an explicit check and in that case the whole CIB
is always verified.
> - holger
>
> crm(live)configure# primitive ip1a ocf:heartbeat:IPaddr2 params ip="1.2.3.4"
> meta target-role="stopped"
> crm(live)configure# primitive ip1b ocf:heartbeat:IPaddr2 params ip="1.2.3.4"
> meta target-role="stopped"
> crm(live)configure# primitive ip2a ocf:heartbeat:IPaddr2 params ip="1.2.3.5"
> meta target-role="stopped"
> crm(live)configure# primitive ip2b ocf:heartbeat:IPaddr2 params ip="1.2.3.5"
> meta target-role="stopped"
> crm(live)configure# primitive ip3 ocf:heartbeat:IPaddr2 params ip="1.2.3.6"
> meta target-role="stopped"
> crm(live)configure# primitive dummy_1 ocf:heartbeat:Dummy params fake="abc"
> meta target-role="stopped"
> crm(live)configure# primitive dummy_2 ocf:heartbeat:Dummy params fake="abc"
> meta target-role="stopped"
> crm(live)configure# primitive dummy_3 ocf:heartbeat:Dummy meta
> target-role="stopped"
> crm(live)configure# commit
> WARNING: Resources ip1a,ip1b violate uniqueness for parameter "ip": "1.2.3.4"
> WARNING: Resources ip2a,ip2b violate uniqueness for parameter "ip": "1.2.3.5"
> Do you still want to commit?
>
>
> diff -r cf4e9febed8e shell/modules/ui.py.in
> --- a/shell/modules/ui.py.in Wed Feb 23 14:52:34 2011 +0100
> +++ b/shell/modules/ui.py.in Tue Mar 08 09:11:38 2011 +0100
> @@ -1509,6 +1509,55 @@
> return False
> set_obj = mkset_obj("xml")
> return ptestlike(set_obj.ptest,'vv',cmd,*args)
> +
> + def __check_unique_clash(self):
> + '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)
There's a convenience function get_ra(node) for this.
> + 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
> +
> + clash_dict = {}
> + for p in cib_factory.mkobj_list("xml","type:primitive"):
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).
Many thanks for the effort. Good work!
Cheers,
Dejan
> +
> + no_clash = 1
> + for param, resources in clash_dict.items():
> + if len(resources) > 1:
> + no_clash = 0
> + msg = 'Resources %s violate uniqueness for parameter "%s":
> "%s"' %\
> + (",".join(sorted(resources)), param[3], param[4])
> + common_warning(msg)
> +
> + return no_clash
> +
> def commit(self,cmd,force = None):
> "usage: commit [force]"
> if force and force != "force":
> @@ -1523,7 +1572,8 @@
> rc1 = cib_factory.is_current_cib_equal()
> rc2 = cib_factory.is_cib_empty() or \
> self._verify(mkset_obj("xml","changed"),mkset_obj("xml"))
> - if rc1 and rc2:
> + rc3 = self.__check_unique_clash()
> + if rc1 and rc2 and rc3:
> return cib_factory.commit()
> if force or user_prefs.get_force():
> common_info("commit forced")
> _______________________________________________
> 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
