+++ This bug was initially created as a clone of Bug #2123570 +++

Description of problem:

There are no error checks or warnings when setting invalid resource meta attributes. This can lead to unexpected behaviors for users who believed they set a valid option.

Version-Release number of selected component (if applicable):

$ rpm -q pcs
pcs-0.10.8-1.el8.x86_64

How reproducible:

Set an invalid meta attribute to either a specific resource or for resource defaults:

[root@clusterb-rhel8 ~]# pcs resource defaults update resource-sti=1000
Warning: Defaults do not apply to resources which override them with their own defined values

[root@clusterb-rhel8 ~]# pcs resource update dummy1 meta resource-sti=100

Actual results:

This results in an update to the configuration, but the values won't actually do anything since the attribute are invalid:

[root@clusterb-rhel8 ~]# pcs config
---------------->8--------------------
Resources:
Resource: dummy1 (class=ocf provider=heartbeat type=Dummy)
Attributes: fake=1
Meta Attrs: resource-sti=100
---------------->8--------------------
Resources Defaults:
Meta Attrs: rsc_defaults-meta_attributes
resource-sti=1000
resource-stickiness=1000

Expected results:

A warning or error that prevents an update to configuration, and requests additional "--force" option to set invalid values.

— Additional comment from Tomas Jelinek on 2022-09-05 16:03:33 CEST —

I agree this is unfortunate. The problem is, that users are allowed to set custom meta attributes for their purposes [1]. So there are no 'invalid' meta attributes, there are just meta attributes ignored by pacemaker. Implementing the requested validation would break the use case of setting custom meta attributes. Ken, what's your opinion?

[1]: https://clusterlabs.org/pacemaker/doc/2.1/Pacemaker_Explained/singlehtml/index.html#resource-meta-attributes

— Additional comment from Reid Wahl on 2022-09-06 00:03:14 CEST —

Maybe just a warning (without requiring --force) that the specified meta attribute doesn't match any cluster-defined ones, and the user should make sure they're intentionally setting a custom attribute? That way we don't add friction to a valid use case and possibly break scripts.

— Additional comment from Ken Gaillot on 2022-09-06 17:27:19 CEST —

A warning makes sense. I don't see a problem with requiring --force, but you might want to save that for the next major version.

Pacemaker's behavior is less than ideal here, but changing it would be a massive compatibility break. If we did want to change it, I'd probably keep meta_attributes for user-defined attributes and move the Pacemaker-aware ones to new element(s). But that would be at least a major version away and would keep the old support deprecated for at least another major version.

— Additional comment from Tomas Jelinek on 2022-09-06 17:40:11 CEST —

Thanks for your input. We can go with printing a warning and possibly require --force in the next major version.

Ken, does pacemaker provide a definition of supported meta attributes? Something similar to '/usr/libexec/pacemaker/pacemaker-schedulerd metadata'?

— Additional comment from Ken Gaillot on 2022-09-06 18:05:01 CEST —

(In reply to Tomas Jelinek from comment #5)
> Thanks for your input. We can go with printing a warning and possibly require --force in the next major version.
>
> Ken, does pacemaker provide a definition of supported meta attributes? Something similar to '/usr/libexec/pacemaker/pacemaker-schedulerd metadata'?

No. That information is currently not collected or collectable anywhere. Also, meta-attributes are effective only in certain contexts (for example clone-max only matters inside a clone element). So this will not be practical from the pcs side.

I think the only way we could make this simple would be to use a new attributes block for Pacemaker-aware properties, and have the schema enumerate which are allowed where. Then we could rely on schema validation, and pcs wouldn't need any changes. Something like:

<primitive id="rsc1" ...>
<control id="..."> <!-- for Pacemaker-aware properties -->
<nvpair .../>
</control>
<meta_attributes id="..."> <!-- for user use, and backward compatibility -->
<nvpair .../>
</meta_attributes>
<instance_attributes id="..."/> <!-- for agent parameters -->
<nvpair .../>
</instance_attributes>
</primitive>

pcs could add a new [control <control options> ...] syntax to commands, but that would require users to change their behavior. Alternatively, pcs could continue using meta for control options, and add a new syntax for user-defined meta_attributes, which would be safer but somewhat confusing.

Thoughts?

— Additional comment from Tomas Jelinek on 2023-01-23 14:13:31 CET —

(In reply to Ken Gaillot from comment #6)
> (In reply to Tomas Jelinek from comment #5)
> > Thanks for your input. We can go with printing a warning and possibly require --force in the next major version.
> >
> > Ken, does pacemaker provide a definition of supported meta attributes? Something similar to '/usr/libexec/pacemaker/pacemaker-schedulerd metadata'?
>
>
> I think the only way we could make this simple would be to use a new attributes block for Pacemaker-aware properties, and have the schema enumerate which are allowed where. Then we could rely on schema validation, and pcs wouldn't need any changes. Something like:
>
> <primitive id="rsc1" ...>
> <control id="..."> <!-- for Pacemaker-aware properties -->
> <nvpair .../>
> </control>
> <meta_attributes id="..."> <!-- for user use, and backward compatibility -->
> <nvpair .../>
> </meta_attributes>
> <instance_attributes id="..."/> <!-- for agent parameters -->
> <nvpair .../>
> </instance_attributes>
> </primitive>

This doesn't really help. Even if meta-attributes were validated by schema, pcs would have to do its own validation to prevent 'CIB does not conform to schema' errors. From my point of view, this solution adds complexity and backward compatibility problems, while not getting us closer to solution. In the worst case, pcs would have to keep it's own list of allowed meta-attributes' names, possibly lagging behind pacemaker.

Having a definition of meta-attributes provided by pacemaker seems like a better approach to me. If meta-attributes are context dependent, let's build that info into the definition. Either as a CLI option (e.g. pcmk-metaattrs-tool metadata --context primitive|group|bundle|clone|promotable|operation|rsc-defaults|op-defaults) or into XML output as an attribute or element. Thinking about it, a CLI option sounds better, as it would allow using existing OCF schema for the XML.

If meta-attributes were defined this way, pcs could use it to validate both their names and values as well as to print meta-attributes names and their description in a similar manner it does with instance-attributes.

Opinions?

— Additional comment from Ken Gaillot on 2023-01-23 17:57:51 CET —

(In reply to Tomas Jelinek from comment #7)
> Even if meta-attributes were validated by schema, pcs would have to do its own validation to prevent 'CIB does not conform to schema' errors. From my point of view, this solution adds complexity and backward compatibility problems, while not getting us closer to solution. In the worst case, pcs would have to keep it's own list of allowed meta-attributes' names, possibly lagging behind pacemaker.

Definitely want to avoid that

> Having a definition of meta-attributes provided by pacemaker seems like a better approach to me. If meta-attributes are context dependent, let's build that info into the definition. Either as a CLI option (e.g. pcmk-metaattrs-tool metadata --context primitive|group|bundle|clone|promotable|operation|rsc-defaults|op-defaults) or into XML output as an attribute or element. Thinking about it, a CLI option sounds better, as it would allow using existing OCF schema for the XML.
>
> If meta-attributes were defined this way, pcs could use it to validate both their names and values as well as to print meta-attributes names and their description in a similar manner it does with instance-attributes.
>
> Opinions?

That does make more sense. We can clone this bz for the Pacemaker side.