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

Dejan Muhamedagic dejanmm at fastmail.fm
Tue Mar 8 11:07:55 UTC 2011


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