How do we handle time units in gatherers and checks?

[scmschmidt]

When testing Check 373DB8 on different clusters on AWS, I saw:

# crm_attribute -t crm_config -G -n stonith-timeout
scope=crm_config  name=stonith-timeout value=600
  
# trento-agent facts gather --gatherer cibadmin --argument cib.configuration.crm_config.cluster_property_set
  ...
        #{
          "id": "cib-bootstrap-options-stonith-timeout",
          "name": "stonith-timeout",
          "value": "600"
        },
  ...

The check passes.

# crm_attribute -t crm_config -G -n stonith-timeout
scope=crm_config  name=stonith-timeout value=600s
 
# trento-agent facts gather --gatherer cibadmin --argument cib.configuration.crm_config.cluster_property_set
 ...
       #{
         "id": "cib-bootstrap-options-stonith-timeout",
         "name": "stonith-timeout",
         "value": "600s"
       },
 ...

The check fails.
The difference is the superficial but valid(?) unit s for seconds.

The big questions is, how to handle units? Either the check writer needs to take care of it or the gatherer normalizes values either directly or with a second attribute.
I tend to vote for the handling this inside the gatherer, because it makes check writing much safer.

This is certainly not limited to stonith-timeout, but to all parameters which allow units in values.

[stefanotorresi]

Pacemaker attributes are completely opaque so we have no way to infer what the default unit should be.
I think we should instead encourage the checks to be as explicit as possible, so we check against values with the unit, and if the user doesn't set the unit, the check fails.
We might create a dedicated check for this so that it's more informative for the user that they are expected to put the unit in some Pacemaker attributes, or add an expectation to those checks involving durations.

[scmschmidt]

The opaque part surprises me. The unit makes it from the cluster configuration to the the check, so it has passed through the gatherer. Knowing which parameters support which kind of units would allow to act. If we want this, is of cause a different question.

Forcing a unit might not be a good idea. I assume that most clusters are configured without one, which becomes somehow the default. Since I lack experiences here, I'll raise this in our weekly Trento Meeting for the checks team. There we have HA people, who are better judges on this.

[stefanotorresi]

We just asked Yan and it turns out we can actually safely assume seconds as the default unit, so we can do the type conversion in the gatherer.

[arbulu89]

Hey @scmschmidt ,
The timing settings configured in cib have "second" as the default unit. The solution we are thinking of is to try to parse the values as duration (checking for suffixes like s, m, etc), and if the parsing works converting the value to seconds.

This requires a simple implementation in the gatherer side. This way, the returned value wouldn't have any unit afterwards anymore, and the check should compare it as it was in seconds.

So the check implementations remains.
Would this be Ok @scmschmidt ?

We will create a ticket for this on our backlog and give priority.

[scmschmidt]

Hi@arbulu89,
Yep, that was the idea. Check writers can rely on getting the value as settings without any unit.
I can discuss with the others in tomorrows Check Team Meeting, if there is any reason, we would need the original value passed through to the check as well. I cannot imagine one.
Thanks.

[scmschmidt]

Just to have the complete story here as well, since we discussed this in the last Check Team Meeting.
We found out, that even milliseconds can happen. The implementation details will be handled in a Jira issue which is to come. Maybe we stick with seconds as default , but return a float.

[arbulu89]

@scmschmidt milliseconds are handled like 500ms, right?
I mean, we still continue considering that if you have 500 as value, this is on seconds.
If we find 500ms, we would return 0.5 and if it is 5ms 0.005.
And yes, we could return a float in this case.

@fabriziosestito This is still easy doable, right?

[fabriziosestito]

yes

[scmschmidt]

@arbulu89 Sorry, I don't know such details about cluster. Let me just add @ksanjeet who will help to collect that kind of information.

[arbulu89]

@scmschmidt Fixed in the agent: trento-project/agent#204
Now on, values coming in the cib with the next units are converted to seconds as to float: "ns", "us" (or "µs"), "ms", "s", "m", "h".
We will update the gatherer docs soonish