The Puppet Labs Issue Tracker has Moved: https://tickets.puppetlabs.com

Bug #5510

facter custom fact ruby files should be loaded deterministically

Added by Rick Bradley almost 4 years ago. Updated over 1 year ago.

Status:ClosedStart date:12/12/2010
Priority:NormalDue date:
Assignee:-% Done:

0%

Category:-
Target version:1.5.9
Keywords: Affected Facter version:
Branch:https://github.com/rick/facter/commit/c811154a8bb313ff62863d2ebe131cf7524bdd95

We've Moved!

Ticket tracking is now hosted in JIRA: https://tickets.puppetlabs.com

This issue is currently not available for export. If you are experiencing the issue described below, please file a new ticket in JIRA. Once a new ticket has been created, please add a link to it that points back to this Redmine ticket.


Description

When defining a set of custom facts via ruby libraries, the libraries are not loaded deterministically on different operating systems and/or filesystems. When a ruby-defined fact definition relies on the definition of other ruby-defined facts, the facts may not be evaluated depending upon the operating system / filesystem in use (e.g., Mac OSX by default appears to sort, while some variants of linux on ext3 appears to use another ordering).

The problem is that the return result from ruby’s Dir.entries varies from platform to platform.

The obvious fix is to sort the return value from Dir.entries before use.

A pull request with specs for the problem and a resolution is forthcoming.

History

#1 Updated by Rick Bradley almost 4 years ago

Pull request for the problem is here:

https://github.com/puppetlabs/facter/pull/3

#2 Updated by Anonymous almost 4 years ago

Hey. Can you identify an actual problem caused by this, other than the order of facts changing when they are loaded?

My expectation would be that this should not matter – ordering dependencies between facts should be explicit, if they exist, rather than depending on an implementation detail like load order.

I would also expect that committing to loading facts in a specific order runs the risk that we are tied to providing that API forever, which means that any changes to the implementation later (like lazy evaluation, say) might suddenly run into the additional complexity of having to emulate this commitment to implementations…

Having a real world problem caused by that failure would make it much easier to work out if this is a commitment we should make. :)

#3 Updated by Anonymous almost 4 years ago

…and I went and reviewed the code and tests, which look good to me from a “checked the diff” point of view. If this is a problem to be solved I would be happy to see this implementation used. :)

#4 Updated by Michael Stahnke almost 4 years ago

Basically, we have N files with facts in them (not just one fact per file). Some facts in file 03-whatever.rb depend on files in 01-whatever.rb. Initially, we thought facter was loading the facts in filename order, after testing on some different systems, it was clear that it was not, and was actually dependent on inode ordering.

As for creating an API that you’re afraid might break something in the future, I think that’s how this needs to be. If I can’t rely on facter to use previous facts when making new ones, I would have to put all facts in one file and order them top down. This is pain when we already have a few dozen facts and certainly will have more by the time we are complete with our implementation.

For example, if I have facts doing some external lookup, I need to know the location of the URI to look something up, before I I try to grab a value form the URI.

The other option would be to have some formal dependency ordering with facts. Filename ordering certainly isn’t totally ideal, but also does not violate the principle of least surprise when working with custom facts.

#5 Updated by Nigel Kersten almost 4 years ago

  • Status changed from Unreviewed to Investigating
  • Assignee set to Nigel Kersten

First, thanks for the patch.

I’m worried we’re not addressing the actual problem here though.

If I can’t rely on facter to use previous facts when making new ones

Is that the real problem here? How are you referencing the dependent facts? Facter.value ?

#6 Updated by Michael Stahnke almost 4 years ago

Yes, I am using Facter.value(:fact_name) to load previous facts.

It seems like it might be possible to use Facter.load, but this could cause performance hits if we have to rescan facts several times in order to build out to the current dependency required, specifically if facts are doing external lookups. Yes, I could do some form of caching on external lookups, but if they only have to run once, it’s a much simpler problem to solve.

#7 Updated by Nigel Kersten almost 4 years ago

Are you creating dynamic fact names? Do the filenames match the fact names?

#8 Updated by Andrew Forgue almost 4 years ago

What if the fact depends on facts in different directories? How do you ensure those get loaded in the right order? IMHO, other facter issues aside I don’t think Facts should depend on load order, they should be located in a file with the name of the fact as the filename. Fact.value already searches the search path for factname.rb to load it.

#9 Updated by Paul Nasrat almost 4 years ago

The thinking for 2.0 is to completely rework the load system anyway, as noted by the reporter it is fragile and obtuse to fact developers.

However just sorting here doesn’t make sense – what we want is a well defined set of core facts with coherent meaning (see architecture fact discussion amongst others) and a reliable way of stating dependencies/loading.

http://projects.puppetlabs.com/projects/facter/wiki/ArchitectureForTwoDotOh

However this patch does get us incrementally there – by adding a sort, we could potentially then add a custom sort to order the core first. I’d want to look at this in a bit more depth – Rick can you mail this to list using the instructions below, I find larger design discussions over email easier to deal with than a textbox in a redmine.

http://projects.puppetlabs.com/projects/puppet/wiki/Development_Development_Lifecycle

#10 Updated by Nigel Kersten almost 4 years ago

  • Status changed from Investigating to Needs Decision
  • Assignee changed from Nigel Kersten to Paul Nasrat

Paul, are you happy to make the design decision here? I’m always willing to be involved.

#11 Updated by Nick Lewis almost 4 years ago

  • Branch set to https://github.com/rick/facter/commit/c811154a8bb313ff62863d2ebe131cf7524bdd95

#12 Updated by Paul Nasrat almost 4 years ago

  • Status changed from Needs Decision to In Topic Branch Pending Review

Yeah I’m happy with this – if you could just do some smoke testing across the test lab to ensure no regressions on our primary supported platforms and report back.

#13 Updated by Anonymous over 3 years ago

  • Status changed from In Topic Branch Pending Review to Merged - Pending Release
  • Assignee changed from Paul Nasrat to Anonymous
  • Target version set to 1.6.0

Now available in next as commit:ecd7cda

#14 Updated by James Turnbull over 3 years ago

  • Target version changed from 1.6.0 to 1.5.9

#15 Updated by James Turnbull over 3 years ago

  • Status changed from Merged - Pending Release to Closed

Pushed in commit:b88a088c6e53ef96914280e6937b9b9214b6c64b in branch master

#16 Updated by Anonymous over 1 year ago

  • Assignee deleted (Anonymous)

Also available in: Atom PDF