Project

General

Profile

Refactor #27906

Use modern Facter 3 facts

Added by Ewoud Kohl van Wijngaarden over 2 years ago. Updated 3 months ago.

Status:
Closed
Priority:
Normal
Category:
Facts
Target version:
-

Description

Currently we still rely on various Facter 2 facts that are deprecated in Facter 3 and will be removed in Facter 4.

https://puppet.com/docs/facter/3.11/core_facts.html#legacy-facts is a list of legacy facts.


Related issues

Related to Foreman - Refactor #32130: Drop support of Facter 2 factsDuplicate
Related to Foreman - Feature #32337: Drop support for Facter versions older than 2.2Ready For Testing
Related to Foreman - Refactor #32345: Provide non-DB model methods and deprecate DB model methods in FactParserNew

Associated revisions

Revision 50a336d5 (diff)
Added by Ewoud Kohl van Wijngaarden over 2 years ago

Refs #27906 - Clean up PuppetFactParser (#7059)

As a preparation to moving to modern facts, this refactor cleans up the
operatingsystem method to make a future refactor easier.

Revision cc4a95ff (diff)
Added by Ewoud Kohl van Wijngaarden 6 months ago

Fixes #27906 - Always prefer modern facts in Facter

Facter started to introduce structured facts in Facter 2. Since Facter
3, the old facts are deprecated. This introduces helper methods to
provide the abstraction and always prefers modern facts.

It does make a change that it no longer considers the lsbdistrelease
fact. 5d61c18fa7bdee314ac146ba6b35353c16c63b37 already preferred the
os.release.full fact which is present with Facter 2.2 or newer. It
should also fall back to the operatingsystemrelease fact which is
present since Facter 1.5 and a more reliable fact.

To make things easier, it annotates which for each fact which version of
Facter introduced it.

Revision e98fc011 (diff)
Added by Ewoud Kohl van Wijngaarden 6 months ago

Refs #27906 - Add FacterDB test

It should be noted that these tests are fairly slow. On my system they
add a minute of runtime to the tests.

History

#1 Updated by The Foreman Bot over 2 years ago

  • Assignee set to Ewoud Kohl van Wijngaarden
  • Status changed from New to Ready For Testing
  • Pull request https://github.com/theforeman/foreman/pull/7059 added

#2 Updated by Ewoud Kohl van Wijngaarden about 1 year ago

#3 Updated by Ewoud Kohl van Wijngaarden about 1 year ago

  • Assignee deleted (Ewoud Kohl van Wijngaarden)
  • Status changed from Ready For Testing to New

#4 Updated by Ewoud Kohl van Wijngaarden about 1 year ago

It should also be noted that Facter 4 ended up keeping the legacy facts. However, we should still update to the modern facts so at some point legacy facts can be turned off.

#5 Updated by Marek Hulán about 1 year ago

from the other issue:

Today we still support so called legacy facts in places we parse puppet facts. The drop of the Facter 2 was announced a while ago at https://community.theforeman.org/t/the-road-to-making-puppet-optional/17983 and Foreman 2.6/3.0 would be a good time to clean up this code

While some work has been done, there are still facter 2 facts supported, they should be removed from the codebase.

#6 Updated by Lukas Zapletal about 1 year ago

Guys, are we sure about dropping facter 2.x from the codebase? There was a 2.4 user complaining about 2.x facter broken after upgrade, I think we might still have some users. And this should probably be communicated in advance, we haven't announced or deprecated anything yet IIRC.

https://community.theforeman.org/t/puppet-fact-parser-is-failing-with-string-does-not-have-dig-method/23138

From discovery standpoint we are now good, it took me a while to upgrade to f4 but we are good. But honestly I think we should actually do the very opposite - fix f2 support, then do some announcement and deprecation warnings and schedule this to 3.1 or something like that.

#7 Updated by Ewoud Kohl van Wijngaarden about 1 year ago

IMHO we should make sure the codebase 100% works when no legacy facts are present. That will be a first step to deprecation.

The particular case mentioned doesn't appear to be related. The structured os fact should be present in 2.4 already.

#8 Updated by Lukas Zapletal about 1 year ago

While I agree, I think we should keep the status-quo for some time before we are ready for the switch. I suggest that we explore what exactly is wrong, since this was broken quite recently (the user reports 2.4) chances are we can fix that easily and buy some more time.

#9 Updated by Ewoud Kohl van Wijngaarden about 1 year ago

In the linked post I have said that it should work if the user doesn't use stringify_facts = true. In Puppet 4 that setting has been removed and is always false. We have also dropped support for Puppet < 5 so it would be weird to document "this is how you make unsupported software work". Also note that the user reported they upgraded to 1.24 which is also unsupported.

What we can do is more graceful error handling. IMHO this isn't very helpful to a user. I'm thinking about adding abstractions for all facts with rescue statements everywhere that says "failed to read fact x" so it's at least easier to understand.

#10 Updated by Marek Hulán about 1 year ago

Not a bad idea about improving the error reporting, Ewoud could you turn that into a new RM issue and describe the expected behavior on several examples please? Like if uploaded facts does not contain fact xyz, he should see an error saying "fact xyz is not present". Ideally also specify where (notification drawer, log, API response, host status or something else)

I agree we should go ahead and drop support for legacy facts, I asked few users in our community and there was no concern with that. People seem to be upgrading their Puppet infrastructure (good). If they have older facters deployed, they probably also have older Foreman installed.

#11 Updated by Ewoud Kohl van Wijngaarden about 1 year ago

https://github.com/puppetlabs/facter/commit/58901c0101e8c8d7f1ec80bcb9356e61d7cadee3 did add a schema of facts. Its history can be used as a source of when a fact was introduced. That's the best way right now that I know to determine it.

#12 Updated by The Foreman Bot about 1 year ago

  • Assignee set to Ewoud Kohl van Wijngaarden
  • Status changed from New to Ready For Testing
  • Pull request https://github.com/theforeman/foreman/pull/8449 added

#13 Updated by Ewoud Kohl van Wijngaarden about 1 year ago

  • Related to Feature #32337: Drop support for Facter versions older than 2.2 added

#14 Updated by Ewoud Kohl van Wijngaarden about 1 year ago

  • Assignee deleted (Ewoud Kohl van Wijngaarden)
  • Category deleted (Facts)

In the attached PR I updated all facts to their modern version where possible. I also annotated all facts and when they were introduced. It looks like if we raise the version to at least 2.2, we can simplify the parser so I opened #32337 for that.

Let's keep the scope of this PR to only make sure we can operate without any legacy facts present.

#15 Updated by The Foreman Bot about 1 year ago

  • Assignee set to Ewoud Kohl van Wijngaarden

#16 Updated by Ewoud Kohl van Wijngaarden about 1 year ago

  • Related to Refactor #32345: Provide non-DB model methods and deprecate DB model methods in FactParser added

#17 Updated by The Foreman Bot 6 months ago

  • Pull request https://github.com/theforeman/foreman/pull/8450 added

#18 Updated by The Foreman Bot 6 months ago

  • Fixed in Releases 3.2.0 added

#19 Updated by Ewoud Kohl van Wijngaarden 6 months ago

  • Status changed from Ready For Testing to Closed

#20 Updated by Amit Upadhye 3 months ago

  • Category set to Facts

Also available in: Atom PDF