Skip to content

Bring rule schemas, documentation, and code in alignment#3409

Merged
kgaillot merged 19 commits into
ClusterLabs:mainfrom
kgaillot:best-practices
Apr 3, 2024
Merged

Bring rule schemas, documentation, and code in alignment#3409
kgaillot merged 19 commits into
ClusterLabs:mainfrom
kgaillot:best-practices

Conversation

@kgaillot
Copy link
Copy Markdown

@kgaillot kgaillot commented Apr 1, 2024

This includes a number of fixes in rule processing, deprecates some more rule syntax, prepares the schemas for dropping deprecated syntax, and overhauls the rules documentation.

Ken Gaillot added 4 commits April 1, 2024 13:02
…ions

The schema defines the "attribute" XML attribute as text, which allows empty
strings as values, so the code should follow that (as it did before 9236a29).
… meta-attributes

The scheduler does not evaluate node attribute expressions when getting
meta-attributes, so the fencer shouldn't either.
…xpressions

The cluster ignores node expressions in rules within resource meta-attribute
blocks, so crm_resource should too when getting a meta-attribute value or
creating meta-attributes for direct agent execution.
@kgaillot
Copy link
Copy Markdown
Author

kgaillot commented Apr 1, 2024

@nrwahl2 , here's the most significant and useful rules PR

@nrwahl2
Copy link
Copy Markdown
Contributor

nrwahl2 commented Apr 1, 2024

I do like significance and usefulness

Copy link
Copy Markdown
Contributor

@nrwahl2 nrwahl2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed through "Test: cts-cli: update expected outputs for new schema version". Still need to review the schema changes and, most tediously and usefully, the doc changes.

Comment thread lib/pacemaker/pcmk_sched_location.c
Comment thread lib/pacemaker/pcmk_sched_location.c
Comment thread tools/crm_resource.c
Comment thread tools/crm_resource.c
Comment thread tools/crm_resource_runtime.c Outdated
Comment thread lib/pengine/status.c Outdated
Comment thread lib/pengine/complex.c
Comment thread tools/crm_resource.c
Comment thread lib/pacemaker/pcmk_sched_location.c
Comment thread cts/cli/regression.tools.exp Outdated
Ken Gaillot added 9 commits April 2, 2024 11:26
This drops the pe_get_configured_timeout() function, because it duplicated what
other functions already do, but poorly. It did not handle rsc_expression and
op_expression in rules, did not default to the minimum-interval monitor timeout
for probes, etc.

Also, use guint for the timeout, since that's how it's eventually used.
The existing behavior varies greatly depending on numerous criteria. The
documented default is 20s, so we should stick with that.
The schema already prevents them, but the code currently processes them
if validation is disabled
Contrary to the log message as it was, rules cannot simply be moved from the
lifetime element to its parent constraint element. Colocation, ordering, and
ticket constraints do not support rules at all, and location constraints
support them with different semantics.

The feature is undocumented and has been deprecated since 1.0.8, so it should
be fine to not recommend any alternative.
… changes

The rule 3.9 schema is copied to 3.10.

The constraints, nvset, and options schemas are copied and updated to include
the new rule schema. The alerts, nodes, and resources schemas are copied and
updated to include the new nvset schema.
@kgaillot
Copy link
Copy Markdown
Author

kgaillot commented Apr 2, 2024

Latest push addresses most of review so far

Copy link
Copy Markdown
Contributor

@nrwahl2 nrwahl2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed everything except the final doc commit. I'll do that next. Going ahead and making these comments visible.

Comment thread xml/resources-3.10.rng
@COMPAT: Support for node attribute expressions is deprecated
here. We can just delete this define when we drop support.
-->
<define name="element-nvset.rule">
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit message: "Node expressions are allowed only in ... and in meta_attributes in op and op_defaults elements" We're deprecating that.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why have the define in the first place? The default element-rule already allows node attribute expressions, under a COMPAT header.

Note that we didn't redefine element-nvset.rule in the other meta_attributes blocks in this schema.

Are we wanting to maintain support for node attribute expressions under <op> <meta_attributes> for longer than we maintain it in the other <meta_attributes> contexts?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit message: "Node expressions are allowed only in ... and in meta_attributes in op and op_defaults elements" We're deprecating that.

Right, the schema as of this commit does allow them. Deprecation is noted in comment.

Why have the define in the first place? The default element-rule already allows node attribute expressions, under a COMPAT header.

Just for clarity of intent. element-rule (as of this commit) is intended for places that do not support node attribute expressions (as of this commit). The other meta-attribute blocks already don't support node attribute expressions, so element-rule is appropriate there.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, the schema as of this commit does allow them. Deprecation is noted in comment.

The schema allows them in every single rule regardless of rule context, both before and after this commit. It's no more and no less "allowed" in other contexts than in op and op_defaults

Copy link
Copy Markdown
Contributor

@nrwahl2 nrwahl2 Apr 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, the schema as of this commit does allow them. Deprecation is noted in comment.

The schema allows them in every single rule regardless of rule context, both before and after this commit. It's no more and no less "allowed" in other contexts than in op and op_defaults

More to my point, compared the other places where it's been deprecated, it's now the same amount of deprecated in op and op_defaults... so I don't think we should make a distinction in the deprecation status by saying it's "allowed" in op/op_defaults

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, the schema as of this commit does allow them. Deprecation is noted in comment.

The schema allows them in every single rule regardless of rule context, both before and after this commit. It's no more and no less "allowed" in other contexts than in op and op_defaults

What I actually meant was "the schema allows them and the code implements them". The other deprecated contexts are allowed but ignored. I'll update the commit message.

Comment thread xml/resources-3.10.rng Outdated
Comment thread xml/resources-3.10.rng Outdated
Comment thread xml/resources-3.10.rng Outdated
Comment thread xml/rule-3.10.rng
Comment thread xml/rule-3.10.rng Outdated
Comment thread xml/rule-3.10.rng
Comment thread xml/rule-3.10.rng
Comment thread doc/sphinx/Pacemaker_Explained/fencing.rst
Comment thread doc/sphinx/Pacemaker_Explained/rules.rst
Comment thread doc/sphinx/Pacemaker_Explained/reusing-configuration.rst
@kgaillot
Copy link
Copy Markdown
Author

kgaillot commented Apr 2, 2024

Added commit for sed change

Comment thread cts/cli/regression.tools.exp
Copy link
Copy Markdown
Contributor

@nrwahl2 nrwahl2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pshew

Comment thread doc/sphinx/Pacemaker_Explained/rules.rst Outdated
Comment thread doc/sphinx/Pacemaker_Explained/rules.rst Outdated
Comment thread doc/sphinx/Pacemaker_Explained/rules.rst Outdated
Comment thread doc/sphinx/Pacemaker_Explained/rules.rst
Comment thread doc/sphinx/Pacemaker_Explained/rules.rst Outdated
Comment thread doc/sphinx/Pacemaker_Explained/rules.rst Outdated
Comment thread doc/sphinx/Pacemaker_Explained/rules.rst Outdated
Comment thread doc/sphinx/Pacemaker_Explained/rules.rst Outdated
Comment thread tools/crm_resource.c
Comment thread doc/sphinx/Pacemaker_Explained/rules.rst
Ken Gaillot added 5 commits April 2, 2024 20:03
This extends previous work to more exactly specify which rule syntax is allowed
in which contexts, specifically:

* The lifetime element (within constraints) is deprecated

* Node expressions are both allowed and implemented only in instance_attributes
  within bundle, clone, group, op, primitive, and template elements, and in
  meta_attributes in op and op_defaults elements

* value-source is allowed in expression elements, but only with literal as its
  value

* value is prohibited for defined and not_defined operations, and required for
  all other operations

* The role attribute of rule elements is used only within location constraints

* Support for multiple rules within a location constraint is deprecated.

To preserve backward compatibility, the invalid syntax is merely marked with
comments for now, for easier removal later.
This corrects various errors and omissions in the rules documentation, improves
the wording, and converts all tables to list tables. Deprecated syntax is left
undocumented.
@kgaillot
Copy link
Copy Markdown
Author

kgaillot commented Apr 3, 2024

Updated for remaining review items

Comment thread doc/sphinx/Pacemaker_Explained/constraints.rst
Comment thread doc/sphinx/Pacemaker_Explained/rules.rst Outdated
Copy link
Copy Markdown
Contributor

@nrwahl2 nrwahl2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good

Comment thread doc/sphinx/Pacemaker_Explained/utilization.rst Outdated
Comment thread doc/sphinx/Pacemaker_Explained/utilization.rst Outdated
Comment thread doc/sphinx/Pacemaker_Explained/utilization.rst
@kgaillot
Copy link
Copy Markdown
Author

kgaillot commented Apr 3, 2024

updated per most recent review

@nrwahl2
Copy link
Copy Markdown
Contributor

nrwahl2 commented Apr 3, 2024

missed the bandwidth typo in latest push

Copy link
Copy Markdown
Contributor

@nrwahl2 nrwahl2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hooray!

@kgaillot kgaillot merged commit 7b429c1 into ClusterLabs:main Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants