[Pacemaker] Patch for bugzilla 2541: Shell should warn if parameter uniqueness is violated
Dejan Muhamedagic
dejanmm at fastmail.fm
Wed Mar 9 08:00:56 EST 2011
Hi Holger,
On Wed, Mar 09, 2011 at 01:27:16PM +0100, Holger Teutsch wrote:
> Hi Dejan,
>
> On Tue, 2011-03-08 at 19:26 +0100, Holger Teutsch wrote:
> > Hi Dejan,
> >
> > On Tue, 2011-03-08 at 12:07 +0100, Dejan Muhamedagic wrote:
> > > 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.
> > > > > >
> > ...
> > > It looks good, just a few notes. The check function should move
> > > to the CibObjectSetRaw class and be invoked from
> >
> > Will move it there.
> >
> > > 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.
> > >
> > > >
> > > > + 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.
> > >
> >
> > I did not use this as I need all ra_XXX value anyhow later in the code
> > for building k.
OK.
> > > > + 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).
> > >
> >
>
> 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.
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.
Cheers,
Dejan
> - holger
>
> Output:
> 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# commit
> WARNING: Resources ip1a,ip1b violate uniqueness for parameter "ip": "1.2.3.4"
> Do you still want to commit? y
> crm(live)configure# primitive ip2a ocf:heartbeat:IPaddr2 params ip="1.2.3.5" meta target-role="stopped"
> crm(live)configure# commit
> 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# commit
> WARNING: Resources ip2a,ip2b violate uniqueness for parameter "ip": "1.2.3.5"
> Do you still want to commit? y
> 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
> crm(live)configure# verify
> 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"
> crm(live)configure#
>
> 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 13:20:14 2011 +0100
> @@ -230,11 +230,66 @@
> See below for specific implementations.
> '''
> pass
> +
> + 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)
> + 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 p in cib_factory.mkobj_list("xml","type:primitive"):
> + process_primitive(p.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):
> '''
> Test objects for sanity. This is about semantics.
> '''
> - rc = 0
> + rc = self.__check_unique_clash()
> 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