Fixes #27906 - Always prefer modern facts in Facter #8449

ekohl · 2021-04-16T11:34:36Z

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. 5d61c18 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.

theforeman-bot · 2021-04-16T11:34:49Z

Issues: #27906

ares

Leaving some comments for just reading, happy to test once it's ready.

app/services/fact_parser.rb

ares · 2021-04-16T14:49:25Z

app/services/puppet_fact_parser.rb

-  def model
-    name = facts[:productname] || facts[:model] || facts[:boardproductname]
+  def model_name
+    # TODO: not sure where model comes from, not Facter


could it be something in FDI? perhaps that was a custom fact, that should have been prefixed with foreman_, we may need to search in git history

It was added in 8ac3ea7 way back in 2010 as part of https://projects.theforeman.org/issues/289. I don't think FDI existed back then. My guess is that it was either a custom fact or a bug because it meant the fact hardwaremodel (which did exist back in the released Facter version at the time, which was 1.5).

This does not come from FDI. No idea, let's just remove these.

I am fine to keep it this way, I really don't know. Perhaps a custom fact, yeah. Maybe it is in use.

ares · 2021-04-16T14:51:45Z

app/services/puppet_fact_parser.rb

    end
  end
+
+  def os_release_full


should we have os_ and distro_ methods in abstract parser so all plugins parsers implement in a more standard way? I think you mentioned you wanted to do that, definitely not a must for this PR.

That is indeed where I think we should go to. In #8450 I did some of that work by using the minor releases from facter.

ofedoren · 2021-04-16T12:50:41Z

app/services/puppet_fact_parser.rb

+        if distro_codename
+          os.release_name = distro_codename
        elsif os.release_name.blank?
          os.release_name = 'unknown'
        end


Isn't this equal to

os.release_name = distro_codename || 'unknown'

Not really because it should only be set to unknown if it was blank. The distro code name is an optional fact that typically depends on lsb-release to be installed (maybe no longer in Facter 4, not sure). You don't want a host where that's not installed to reset a correct value back to unknown.

In the future I think the unknown part should be dropped, but that's out of scope for this PR.

In the future I think the unknown part should be dropped, but that's out of scope for this PR.

For what it's worth, I dropped it in #8450, specifically f667ef7. I'd be happy to submit that as a standalone PR with its own issue.

app/services/fact_parser.rb

ekohl · 2021-04-19T10:53:55Z

I opened #8451 to split off the discussion of model methods or not. I would prefer to merge that PR first but I'm fine either way. Please take a look.

ekohl · 2021-04-19T11:44:12Z

This now removes all changes that are #8451. It still lacks tests, but at least it's a bit smaller to review.

ekohl · 2021-04-19T14:25:18Z

I opened #8451 to split off the discussion of model methods or not. I would prefer to merge that PR first but I'm fine either way. Please take a look.

I changed my mind: let's get this in mergable order first.

ekohl · 2021-04-21T08:45:47Z

Test failures related, will look into them.

ekohl · 2021-04-21T09:14:12Z

Ok, updated the failing tests. I have updated the PR description with the latest commit message and I think it's ready for proper review now. I think dropping lsbdistrelease is safe and shouldn't affect anyone.

lzap

All those new methods require tests. This is a huge chance to start testing properly.

I suggest you add a JSON into our static_fixtures that is literally generated on the oldest supported facter you aim for (is that 2.2?) and please state in the filename clearly what is it. What we carry in our fixtures is a terrible mixture of various facts from unknown sources and I know that facter version is in facts themselves, but it should be in the filename (thus test helper) so it is obvious what we are testing.

We already have the following:

test/static_fixtures/facts/example_3.14.16.json
test/static_fixtures/facts/example_4.0.52.json

Ideally we would run a set of tests in a each loop for each major version we aim to support. This would be a huge stepup for the future proofing our puppet parsing - it has always been a mess.

app/services/puppet_fact_parser.rb

lzap · 2021-04-22T18:25:30Z

Well I am not gonna block this but I feel like you are defining here what is the minimum supported facter version, here in this very patch. And I think it is worth making a fixture that clearly communicates that.

ekohl · 2021-04-23T17:17:07Z

I did add tests, but they're fairly slow. I still would like to get this into 2.5 so please take a look.

bundler.d/test.rb

lzap

It's okay, apologies for a late reply.

My only concern is that we are adding another minute to our 40-50 minutes test suite. We want to do exactly the opposite. I think I found a way how to shave a lot of the time tho - see my patch.

Now, I think we can merge this patch and if facterdb upstream likes my patch, it will speed up automatically after we bump the version. If not, I would like you to do similar thing in our codebase because I do not want another minute to be added. Are you okay with that?

lzap · 2021-06-18T08:14:28Z

test/unit/puppet_fact_parser_test.rb

+  # They are structured primairly based on OS rather than Facter version
+  # because FacterDB doesn't contain facts for every combination
+  describe 'Using FacterDB' do
+    subject { get_parser(get_facterdb_facts(facterversion, os_name, os_major)) }


The major problem of why these tests take 1 minute is not necessary in the fact that facterdb is slow, but because this subject is called 12 times /each test/ for each operating system /major, minor/, but it yields to the very same result. Let's cache it and I expect the time to drop significantly.

Yes, I saw that too. I also wondered if we should use the private method read_json_file.

test/unit/puppet_fact_parser_test.rb

lzap · 2021-06-18T08:53:07Z

Oh wait, can you add that after(:suite) block to reclaim memory? And rebase please to retest it's been a while.

ezr-ondrej · 2021-06-29T22:24:59Z

Ping @ekohl ? This is missing just the last comment and rebase to run the tests, if I'm reading correctly.

lzap · 2021-11-04T11:13:58Z

If you can get back to this @ekohl it would be nice to have this prior doing any refactoring changes that are planned by @domitea

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. 5d61c18 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.

ekohl · 2021-11-10T10:43:26Z

Rebased & set FacterDB 1.7 as the minimum version so it can safely call FacterDB.cleanup.

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

lzap

I am good, great work and improvement of test coverage.

I assume that performance is now better and it should not add a minute or something to our test suite, I am not testing this. Last Jenkins duration seems okay, 53 mins. I will ensure this does not break Discovery and I am merging. Thanks!

lzap · 2021-11-10T15:18:47Z

Discovery works fine. I think we are still in the pre-branching period tho, not merging today.

ekohl · 2021-11-16T10:12:35Z

We're past branching. Shall we merge this?

ezr-ondrej

Thanks @ekohl ! 👍

theforeman-bot added Needs testing Not yet reviewed labels Apr 16, 2021

ekohl mentioned this pull request Apr 16, 2021

Fixes #32337 - Drop Facter < 2.2 support #8450

Closed

ares reviewed Apr 16, 2021

View reviewed changes

theforeman-bot added the Waiting on contributor label Apr 16, 2021

ofedoren reviewed Apr 19, 2021

View reviewed changes

ekohl mentioned this pull request Apr 19, 2021

Fixes #32345 - Provide non-DB model methods for FactParser #8451

Closed

ekohl force-pushed the 27906-modern-facts branch from 08279c0 to 23e6b4a Compare April 19, 2021 11:42

theforeman-bot removed the Waiting on contributor label Apr 19, 2021

ezr-ondrej assigned lzap and ezr-ondrej Apr 21, 2021

ekohl marked this pull request as ready for review April 21, 2021 09:11

ekohl force-pushed the 27906-modern-facts branch 2 times, most recently from 35545c9 to 1ffea7a Compare April 21, 2021 09:12

lzap requested changes Apr 22, 2021

View reviewed changes

app/services/puppet_fact_parser.rb Show resolved Hide resolved

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Apr 22, 2021

lzap reviewed Apr 22, 2021

View reviewed changes

app/services/puppet_fact_parser.rb Show resolved Hide resolved

ekohl force-pushed the 27906-modern-facts branch from 1ffea7a to 32a809b Compare April 23, 2021 17:12

ekohl requested a review from a team as a code owner April 23, 2021 17:12

theforeman-bot added Needs re-review and removed Waiting on contributor labels Apr 23, 2021

evgeni reviewed Apr 26, 2021

View reviewed changes

bundler.d/test.rb Outdated Show resolved Hide resolved

lzap previously approved these changes Jun 18, 2021

View reviewed changes

theforeman-bot removed the Needs re-review label Jun 18, 2021

ekohl dismissed lzap’s stale review via 925fdd1 November 10, 2021 10:42

ekohl force-pushed the 27906-modern-facts branch from 32a809b to 925fdd1 Compare November 10, 2021 10:42

Refs #27906 - Add FacterDB test

1dd674f

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

ekohl force-pushed the 27906-modern-facts branch from 925fdd1 to 1dd674f Compare November 10, 2021 10:55

lzap approved these changes Nov 10, 2021

View reviewed changes

lzap added Ready to merge and removed Needs testing labels Nov 10, 2021

ezr-ondrej approved these changes Nov 16, 2021

View reviewed changes

ezr-ondrej merged commit e98fc01 into theforeman:develop Nov 16, 2021

ekohl deleted the 27906-modern-facts branch November 16, 2021 13:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #27906 - Always prefer modern facts in Facter #8449

Fixes #27906 - Always prefer modern facts in Facter #8449

ekohl commented Apr 16, 2021 •

edited

Loading

theforeman-bot commented Apr 16, 2021

ares left a comment

ares Apr 16, 2021

ekohl Apr 19, 2021

lzap Apr 22, 2021

lzap Nov 10, 2021

ares Apr 16, 2021

ekohl Apr 18, 2021

ofedoren Apr 16, 2021

ekohl Apr 19, 2021

ekohl Apr 21, 2021

ekohl commented Apr 19, 2021

ekohl commented Apr 19, 2021

ekohl commented Apr 19, 2021

ekohl commented Apr 21, 2021

ekohl commented Apr 21, 2021

lzap left a comment

lzap commented Apr 22, 2021

ekohl commented Apr 23, 2021

lzap left a comment

lzap Jun 18, 2021

ekohl Jun 18, 2021

lzap commented Jun 18, 2021

ezr-ondrej commented Jun 29, 2021

lzap commented Nov 4, 2021

ekohl commented Nov 10, 2021

lzap left a comment •

edited

Loading

lzap commented Nov 10, 2021

ekohl commented Nov 16, 2021

ezr-ondrej left a comment

Fixes #27906 - Always prefer modern facts in Facter #8449

Fixes #27906 - Always prefer modern facts in Facter #8449

Conversation

ekohl commented Apr 16, 2021 • edited Loading

theforeman-bot commented Apr 16, 2021

ares left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekohl commented Apr 19, 2021

ekohl commented Apr 19, 2021

ekohl commented Apr 19, 2021

ekohl commented Apr 21, 2021

ekohl commented Apr 21, 2021

lzap left a comment

Choose a reason for hiding this comment

lzap commented Apr 22, 2021

ekohl commented Apr 23, 2021

lzap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzap commented Jun 18, 2021

ezr-ondrej commented Jun 29, 2021

lzap commented Nov 4, 2021

ekohl commented Nov 10, 2021

lzap left a comment • edited Loading

Choose a reason for hiding this comment

lzap commented Nov 10, 2021

ekohl commented Nov 16, 2021

ezr-ondrej left a comment

Choose a reason for hiding this comment

ekohl commented Apr 16, 2021 •

edited

Loading

lzap left a comment •

edited

Loading