Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 19 commits into from
Apr 3, 2024

Conversation

kgaillot
Copy link
Contributor

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

kgaillot 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
Contributor Author

kgaillot commented Apr 1, 2024

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

@nrwahl2
Copy link
Contributor

nrwahl2 commented Apr 1, 2024

I do like significance and usefulness

Copy link
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.

kgaillot 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
Contributor Author

kgaillot commented Apr 2, 2024

Latest push addresses most of review so far

Copy link
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.

@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
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
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
Contributor 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
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
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
Contributor 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.

@kgaillot
Copy link
Contributor Author

kgaillot commented Apr 2, 2024

Added commit for sed change

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

kgaillot 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
Contributor Author

kgaillot commented Apr 3, 2024

Updated for remaining review items

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

@kgaillot
Copy link
Contributor Author

kgaillot commented Apr 3, 2024

updated per most recent review

@nrwahl2
Copy link
Contributor

nrwahl2 commented Apr 3, 2024

missed the bandwidth typo in latest push

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