Fixes #36731 - Use facts, not operatingsystem name, to identify RHEL hosts #10738
Conversation
|
Issues: #36731 |
|
@ekohl mind taking a look? I recommend part of this approach based on my knowledge, but I think it's worth you checking the approach. |
There was a problem hiding this comment.
Some context: there is a setting ignore_facts_for_operatingsystem which can be used to either update the OS based on facts or disable it. The default is false, so (double negative) the default is to update the OS based on facts.
Some users don't like that syncing very much, so they disable it. There has been discussion about always storing it in a reported data facet. Then you can, even if you disable syncing, always easily use the parsed data. The idea being that application code should never use the raw facts but always handled processed data.
My biggest worry is cost. I think loading facts is pretty expensive, especially if you have a lot of them.
Perhaps this is a good excuse to move forward storing the OS as a reported data facet and then consume it here.
I've been noticing this as well, as I'm also working on theforeman/foreman#9819 Once that's merged I can update this to take advantage of it. Also, this particular use case of facts shouldn't be too bad, since the status is only updated on host check-in, and will occur in a staggered fashion and not for all hosts at once. |
|
Updated to take advantage of theforeman/foreman#9819 which is now merged. |
| def probably_rhel? | ||
| # Get the os name from sub-man facts rather than operatingsystem. This is | ||
| # less likely to have been changed by the user. | ||
| os_name, = facts('distribution::name').values # only query for that one fact, then get its value |
There was a problem hiding this comment.
I'm using facts('distribution::name') here instead of facts['distribution::name'] because this guarantees that the SQL query will only retrieve that one fact, not all facts for the host.
With the comma, this line is equivalent to
os_name = facts('distribution::name').values.first
but avoids the need for safe navigation (&.).
|
Would you sqash your commits? |
|
@lfu squashed and merged, thanks! 👍 |
What are the changes introduced in this pull request?
When determining the host status for RHEL lifecycle, do what's described in the PR title.
Considerations taken when implementing this change?
We now rely on sub-man facts instead of operatingsystem name, which is a bit more reliable. These facts won't be present for "unmanaged" hosts, but that's okay because we only care about managed hosts.
What are the testing steps for this pull request?
In rails console, run
Then check all host statuses and make sure they are still as expected.