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

Feature #2157

External fact support

Added by Paul Nasrat over 5 years ago. Updated over 1 year ago.

Status:ClosedStart date:05/18/2012
Priority:NormalDue date:05/18/2012
Assignee:-% Done:

0%

Category:interface
Target version:1.7.0
Keywords: Affected Facter version:
Branch:https://github.com/puppetlabs/facter/pull/244

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

Facter should support non-ruby facts, preferably in /etc/facter.d. It should support these facts being either executable, in which case the result is the value of the named fact, or in a data format such as yaml, in which case the data file is read in and interpreted as the fact value.

It probably makes sense to initially stick to yaml for data formats, since json doesn’t ship with ruby, and to also allow executable facts to return either a plain string or yaml.

Note that we can do this without supporting any kind of overriding, but it’d be much better if we supported multiple (configurable?) fact directories, with a search path. Thus, if Facter shipped with /etc/facter.d/myfactname and you wanted to override it, you could do so by creating a new file and putting it in a higher-priority location rather than editing a file distributed with the core.

Given we’re adding structured data support, namespaced facts would be supported with directory structures; e.g., /etc/facter.d/my/fact/name would resolve to my::fact::name.

Ideally, the long-term direction here would be not to require any pure-ruby facts, such that the Facter library could be rewritten in any other language and it would function the same, because all of its actual data is outside of ruby.


Related issues

Related to Facter - Feature #3361: install.rb script needs to be updated when facter starts ... Closed 03/10/2010
Related to Facter - Feature #15596: External fact location should be configurable via an envi... Code Insufficient 07/18/2012
Related to Facter - Bug #15585: External facts does not parse text facts with an '=' corr... Closed 07/18/2012
Related to Facter - Feature #7559: Fact for identifying Amazon VPC instances. Merged - Pending Release 05/17/2011
Related to Puppet Documentation - Bug #21738: Document external executable facts don't work on Windows Closed
Blocked by Facter - Feature #10964: Add the ability to directly set values Closed 11/19/2011
Precedes Puppet - Feature #9546: Plugin sync support for external facts Closed 05/19/2012 05/19/2012
Follows Facter - Feature #14570: Separate fact definition from fact resolution Accepted 05/17/2012

History

#1 Updated by James Turnbull over 5 years ago

  • Status changed from Unreviewed to Accepted

#2 Updated by Udo Waechter over 5 years ago

This is a wonderful idea. I think that it would be cleaner to have:

@/etc/facter/facter.d@

At least debian-like OSes introduce more and more packages that have @/etc/packagename/@ top directories for theis config files. This is a pretty clean approach and avoids isorder in @/etc@. Maybe something like facter.conf is introduced some time in the future, then this would be @/etc/facter/facter.conf@

And second: Custom facts coud output multiple key-value pairs. This would lead to less files in @facter.d@ The problem that would then arise is, that it would make dynamic facts more difficult to handle: @facter @ might be difficult to implement when one would have another language than ruby for facts.

#3 Updated by Anonymous over 4 years ago

  • Tracker changed from Bug to Feature

#4 Updated by Luke Kanies about 4 years ago

  • Subject changed from Scripted fact support (eg /etc/fact.d to External fact support in /etc/facter.d

Original description from Paul:

As a fact writer I want a mechanism to write facts in my favourite language and for facter to run them automatically so that I can easily write custom facts.

Given I have a configured directory for scripting facts When a executable script that returns a key value pair is executed Then the fact should be available in facter

#5 Updated by Luke Kanies about 3 years ago

  • Status changed from Accepted to In Topic Branch Pending Review
  • Assignee changed from Paul Nasrat to Luke Kanies

I’ve got a version of this in my tickets/master/2157-external_fact_support branch, and I’ve sent it to the list. We need to make a couple of decisions:

  • Where should the facts go? E.g., /etc/facts.d, /etc/facter.d, /var/, ?
  • Should we support scripts, or just plain data like yaml?
  • Where should we cache data?
  • Should any of this be configurable, and if so, how?

Otherwise, this is ready to go.

#6 Updated by Yury Zaytsev about 3 years ago

Luke Kanies wrote:

  • Where should the facts go? E.g., /etc/facts.d, /etc/facter.d, /var/, ?

Is it possible to make this configurable at the package build time?

I would say /etc/facter.d makes sense on RHEL if these facts are mostly expected to be one file, written by hand and placed in there by the system administrator (well, ok, it doesn’t totally exclude the package manager, i.e. php-* packages own stuff in /etc/php.d, but these are really more like config files, then there are init scripts, /etc/profile.d, network configuration and so on), but other distributions might have their own preferences and adopt their own standard path for facts that they intend to package.

I.e. generally plugin-like things would go to /usr/lib/facter, but on the other hand, of they are noarch, the standard place would rather be /usr/share/facter and so on. It’s really hard to please all distributions with one path.

#7 Updated by James Turnbull about 3 years ago

Questions/Answers:

  1. Where should the facts go? E.g., /etc/facts.d, /etc/facter.d, /var/, ?

/etc/facter.d/

  1. Should we support scripts, or just plain data like yaml?

Both – be like Nagios plugins – anything that outputs data that facter can consume. Invalidate any other output.

  1. Where should we cache data?

Where do we cache it now?

  1. Should any of this be configurable, and if so, how?

Perhaps the fact location for FHS reasons on distros (and Windows?). Perhaps the distinction for inputs between files with .pl/.py/.rb/.json/.yaml versus a plain executable?

#8 Updated by James Turnbull about 3 years ago

I think cache should be /var/tmp perhaps – FHS for /var/tmp – http://www.pathname.com/fhs/2.2/fhs-5.15.html

#9 Updated by Nigel Kersten about 3 years ago

Luke Kanies wrote:

I’ve got a version of this in my tickets/master/2157-external_fact_support branch, and I’ve sent it to the list. We need to make a couple of decisions:

  • Where should the facts go? E.g., /etc/facts.d, /etc/facter.d, /var/, ?

I feel like we need /etc/facter with sub-directories. We may end up getting to a point where we need a config file, and it seems more sane to follow puppet and have a parent /etc/facter dir.

Thinking about supporting multiple distros with different standards for dynamic plugins, I’m thinking we’ll need to make these locations configurable anyway.

  • Should we support scripts, or just plain data like yaml?

Both. I thought we had a plan hashed out on the list a while ago to have something like:

/etc/facter/facts.d (static content) /etc/facter/plugins.d ? ???? (dynamic scripts)

  • Where should we cache data?

Again, I think this will need to be configurable. Platforms like OS X/BSD have different standard locations here.

  • Should any of this be configurable, and if so, how?

Otherwise, this is ready to go.

#10 Updated by Luke Kanies about 3 years ago

One other thing I forgot to mention – there’s no real relationship in the current code between file names and fact names.

That is, you could have a file at /etc/facts.d/foo.yaml that set facts named bar, baz, and boo, but nothing named foo.

This isn’t necessarily bad, but it’s also not necessarily intuitive.

I’d prefer to have everything be namespaced in some way so there was a relationship, but obviously, we don’t yet support structured data.

#11 Updated by R.I. Pienaar about 3 years ago

Luke Kanies wrote:

One other thing I forgot to mention – there’s no real relationship in the current code between file names and fact names.

That is, you could have a file at /etc/facts.d/foo.yaml that set facts named bar, baz, and boo, but nothing named foo.

I thought about this and figured it would be really tedious to make many files one each per fact, so supported both.

On the other hand I can how that makes it much easier to mess things up by having foo fact defined twice – but this is unavoidable since foo can be in yaml, json and shell script.

How does facter behave if you try to define the same fact twice? Is this something we should call out and raise an exception or just last-one-wins or something?

#12 Updated by Luke Kanies about 3 years ago

And another:

The directory loader should actually be able to load facts from multiple directories. E.g., it should default to /etc/facter.d and ~/.facter.d for non-root users.

There’s no reason to restrict it to just one directory.

#13 Updated by Luke Kanies about 3 years ago

  • Assignee changed from Luke Kanies to Michael Stahnke

Assigning to Mike because he’s going to marshall people to figure out where these various files should go.

#14 Updated by Michael Stahnke about 3 years ago

  • Assignee changed from Michael Stahnke to Luke Kanies

The more I’ve looked into this the more I’ve learned that the FHS isn’t clear. I think in order to not violate the principle of least surprise, we should place it in etc. Ideally something like /etc/facter/facts.d Note that this should not require the files in facts.d to be executable. We also may want to do something like ignore extensions like .bak or .orig as well.

#15 Updated by Luke Kanies about 3 years ago

Michael Stahnke wrote:

The more I’ve looked into this the more I’ve learned that the FHS isn’t clear. I think in order to not violate the principle of least surprise, we should place it in etc. Ideally something like /etc/facter/facts.d Note that this should not require the files in facts.d to be executable. We also may want to do something like ignore extensions like .bak or .orig as well.

What about the cache? It’s currently in /tmp.

And your statement is a bit confusing – are you saying we should not execute facts in /etc, or that we shouldn’t require that they be executable?

#16 Updated by Luke Kanies about 3 years ago

  • Assignee changed from Luke Kanies to Ken Barber
  • Branch set to luke/tickets/master/2157-external_fact_support

Ok, I’ve updated my branch to use /etc/facter/facts.d instead of /etc/facts.d.

It still caches files in /tmp/facts_cache.yaml.

None of this is configurable.

Even worse, I just realized none of this is Windows-compatible. So, assigning to Ken, as apparently he and James are going to work on getting this merged. It’s basically ready, minus some actual testing.

#17 Updated by R.I. Pienaar about 3 years ago

I had a bit of nasty logic in the code I initially submitted, we need a retry count or something here: https://github.com/lak/facter/blob/a53d7d9e5ba9985b27b3b80e83c998bdae5928d6/lib/facter/util/parser.rb#L94-99 to avoid a infinite loop if JSON gem isnt installed.

#18 Updated by Ken Barber about 3 years ago

  • Branch changed from luke/tickets/master/2157-external_fact_support to kbarber/tickets/master/2157-external_fact_support

Fixed infinite loop case as per RI’s last comment.

#19 Updated by Ken Barber about 3 years ago

Found small typo/bug in parser.rb (ttl does not exist in parser – should use cache.ttl(filename). Now fixed in my branch.

I’ve add a new example that tries to return results with an unprimed cache to ensure we catch this kind of thing again.

#20 Updated by Ken Barber about 3 years ago

I’ve committed a patch with tests that removes extraneous whitespace from LHS/RHS entries. This caught me when trying to use these formats and I’d anticipate most users would be less surprised in this case.

#21 Updated by Ken Barber about 3 years ago

Added rdocs to all the new classes and methods – boot polish I know.

#22 Updated by Ken Barber about 3 years ago

I’ve added two commits to deal with:

  • Handle more failure cases with warnings rather then exceptions.
  • Fix temp file handling in Windows for rspec tests. The handling for tmp file/dir is now in a library like Puppet.

I’ve now managed to get the amount of errors in windows down to just 4 errors – related to the script plugin. The script plugin detects executable files … which in Windows picks up every single file. We need to adapt batch (.bat), powershell (.ps1) and other executable extension handling for Windows.

#23 Updated by Ken Barber about 3 years ago

Added batch file handling for ScriptParser in windows. All tests succeed in Windows now.

Will need to add handling for ps1 (powershell), com and exe files which means I’ll probably modify match_extension to take an array.

#24 Updated by Ken Barber about 3 years ago

Added poweshell, com & exe handling for windows. Powershell handling is done through a new parser since I need to special case the calling of such scripts (prefix of ‘powershell ’ to the exec call) and I anticipate other special casing most probably.

Parsers can now take arrays for match_extension so to handle the ‘bat’,‘com’ & ‘exe’ cases all in one.

Tests have been added/updated for all of this.

Now I need to deal with default paths for windows as /tmp & /etc/facter/facts.d are not sufficient for that platform. If anyone has any suggestions for the paths on windows chime in now :–). I’ll look at what we are using on Puppet for windows to find out what they are using as I believe we went through this investigation already.

#25 Updated by Yury Zaytsev about 3 years ago

Ken Barber wrote:

Now I need to deal with default paths for windows as /tmp

On Windows, %TEMP% environment variable should contain the location of the folder for temporary data.

& /etc/facter/facts.d are not sufficient for that platform.

Probably common ‘Application Data’ folder is most appropriate. See:

http://stackoverflow.com/questions/2517940/windows-application-data-directory

for details.

#26 Updated by Ken Barber about 3 years ago

Hey thanks for the help Yury.

In regards to temp space – something I haven’t yet discussed in the ticket is that /tmp is an insecure place to have a named file so I’m probably going to need to change that as well on Unix to a proper data area.

Its a well known problem – if the file doesn’t exist yet its trivial for any user to create the file with bogus content and thereby prime the cache with bad content.

Windows doesn’t suffer this problem as the temp area generally belongs to a user … but I would want to align both using a more valid data storage area.

Generally for data the paths that make sense are:

%APPDATA%\facter (which is %HOMEPATH%\AppData\Remote\facter)
%LOCALAPPDATA%\facter  (which is %HOMEPATH%\AppData\Local\facter)
%ProgramData%\facter (which is C:\ProgramData\facter)

In Puppet … we use:

require 'rubygems'
require 'win32/dir'

Dir::COMMON_APPDATA

Which actually resolves to:

C:\ProgramData

In Windows 2008R2. I’m leaning towards aligning with Puppet here most probably:

File.join(Dir::COMMON_APPDATA, "Puppetlabs", "facter")

Which is:

C:\ProgramData\Puppetlabs\facter

And sub-directories thereof …

#27 Updated by Yury Zaytsev about 3 years ago

Hi!

Ken Barber wrote:

In regards to temp space – something I haven’t yet discussed in the ticket is that /tmp is an insecure place to have a named file so I’m probably going to need to change that as well on Unix to a proper data area.

Not sure I understand you correctly. On Unix-like platforms I am generally doing mktemp and setting permissions on the resulting directory to make sure that only the invoking user can write to it. A portable way to do it from a shell script would be something like this (increase the amount of X’s to get more random characters):

TEST_TMPDIR="$(mktemp -d "${TMPDIR:-/tmp}/facter.XXXXX")"

Not sure how this can be mapped to Ruby…

In Windows 2008R2. I’m leaning towards aligning with Puppet here most probably:

File.join(Dir::COMMON_APPDATA, "Puppetlabs", "facter")

Makes total sense to me.

#28 Updated by Ken Barber about 3 years ago

Not sure I understand you correctly. On Unix-like platforms I am generally doing mktemp and setting permissions on the resulting directory to make sure that only the invoking user can write to it. A portable way to do it from a shell script would be something like this (increase the amount of X’s to get more random characters):

The cache is not really a temporary file in that sense. The cache persists between runs so it has to be a named file so we can find it again.

#29 Updated by Ken Barber about 3 years ago

Added support for —timing in external scripts. It also reports if the cache was used or not as well.

This change means I’ve re-factored the way we use ‘results’ by introducing a wrapper method that we call internally to get results. This wrapper method now takes care of timing and caching so the author of a new Parser no longer has to do this himself.

The side benefit was in json parsing. I managed to get a single json parse from 50 ms down to 0.05 ms with caching, and I imagine with say json_pure this would be an even greater benefit.

#30 Updated by Ken Barber about 3 years ago

Updates to my branch since last comment:

  • Changed logic so a TTL of zero means never cache, and -1 means cache forever. This got rid of a couple of oddities in how we were treating absent TTL with a zero. This is less surprisingly as well IMHO.
  • Ignore mixed case for extension handling. This is mainly so windows users can mix case and the external scripts will still work as expected.
  • I’ve re-factored the cache class so now its a static class. This is so I can tap the cache from different places instead of handing a cache object around. This paves the way for Ruby based caching.
  • I now cache data even if the results were nil. A nil result is not considered a failure necessarily and better to cache these operations as valid returned data.
  • Changes cache serialization to use Marshal not YAML. This file is internal anyway and under heavy writes I saw much better performance. I tried just ‘writing at the end’ but Facter has multiple entry/exit points its hard to do a final ‘write’ of a cache at the correct place and be assured this would work each time. Increasing the performance of the cache writes by using Marshal was a simpler solution – at least for now.
  • Added cache support for ruby facts. You can now pass an option of ‘ttl’ to a Ruby fact upon creation and it will be cached much like we have been doing for external facts. Using this mechanism and turning on cache for most of the facts I was seeing performance improvements from 2100ms (unprimed cache) to 230 ms when cached which is awesome. However a point of note is that the ec2 fact does most of its work outside of setcode, whereas I pretty much only cache the setcode part. This could potentially be fixed with hierarchal data support (which would move all the logic inside the setcode). Just need to make this point now to be clear.
  • I have not committed any change of TTL in the existing facts (although I have obviously tested working with TTL on our existing facts). I would imagine this to occur later when people are ready and all the desired feature sets are shaken up.

I now will return to the path issues for windows (and for Linux) as I see this as the last major roadblock to this work. Also – we will need to handle path creation in install.rb most probably. I need to handle failure when paths don’t exist/aren’t writeable etc.

There has already been a discussion about these path issues which is covered here:

https://groups.google.com/group/puppet-dev/browse_frm/thread/74682b39a943b809#

#31 Updated by Nigel Kersten about 3 years ago

Ken Barber wrote:

  • Changes cache serialization to use Marshal not YAML. This file is internal anyway and under heavy writes I saw much better performance. I tried just ‘writing at the end’ but Facter has multiple entry/exit points its hard to do a final ‘write’ of a cache at the correct place and be assured this would work each time. Increasing the performance of the cache writes by using Marshal was a simpler solution – at least for now.

Isn’t there a problem with different versions of Marshal support being incompatible with one another?

#32 Updated by Ken Barber about 3 years ago

Nigel Kersten wrote:

Ken Barber wrote:

  • Changes cache serialization to use Marshal not YAML. This file is internal anyway and under heavy writes I saw much better performance. I tried just ‘writing at the end’ but Facter has multiple entry/exit points its hard to do a final ‘write’ of a cache at the correct place and be assured this would work each time. Increasing the performance of the cache writes by using Marshal was a simpler solution – at least for now.

Isn’t there a problem with different versions of Marshal support being incompatible with one another?

Interesting point. It is a concern, although I’m more worried about errors, then trying to preserve data across Ruby upgrades.

So let me drill down into more detail … if I use YAML using the current Cache mechanism, the cache will get written each time there is a cache change. When you get up near the ~100 fact mark something that took 2 seconds will now take 3 seconds for that first run. Obviously subsequent cached runs are much much faster, but the last thing I need is a big delay for that first run.

Now I could find a way of writing once, which does work when I say put this write in facter/application.rb or in load_facts or some other catch-all place – but this will not work if someone just uses Facter.value in their application. If I put it somewhere in Facter.value … well … I have limited means of telling if this is a once off call or a full fact run thus leaving us with a write per cache change again.

So either I find a place to limit the amount of writes … or I use something faster like Marshal to get around the perf hit. I still want to keep the cache mechanism very simple … so whatever mechanism we use should be fast and available with standard Ruby.

I could just look for Marshal version exceptions (TypeError) and alert on them or perhaps overwrite the file using the new format. I don’t see this is as a massively complex work-around, although I would need a clean way of picking up false positives (since even just junk data throws a TypeError it would seem).

Interesting: switching between 1.8.7-p334 and 1.9.2-p290 doesn’t throw this exception.

#33 Updated by Nigel Kersten about 3 years ago

To be clear, I’m not sure how big a problem this actually is, but i know this is one reason we’ve moved away from marshal in Puppet.

Given it’s a cache, it may be fine to use it for performance reasons and simply wipe it out if you can’t read it.

#34 Updated by Ken Barber almost 3 years ago

Okay – so I’ve made paths configurable on the command line now, and I’m dealing with automatically working them out depending on the OS.

The remaining tasks are largely:

  • Dealing with creation of external script dir during installation.
  • Handling of TypeError as per Nigel’s comments above.
  • Updating documentation on docs.puppetlabs.com.

Not counting more general testing/bugfixing.

#35 Updated by Adrien Thebo almost 3 years ago

Hi Ken,

I’m currently doing merges into facter en masse, and I was wondering if this was actually ready for merge or not. Is the status of this ticket incorrect?

On Fri, Sep 9, 2011 at 2:02 PM, tickets@puppetlabs.com wrote:

Issue #2157 has been updated by Ken Barber.

Okay – so I’ve made paths configurable on the command line now, and I’m dealing with automatically working them out depending on the OS.

The remaining tasks are largely:

  • Dealing with creation of external script dir during installation.
  • Handling of TypeError as per Nigel’s comments above.
  • Updating documentation on docs.puppetlabs.com.

Not counting more general testing/bugfixing.

Feature #2157: External fact support in /etc/facter.dhttps://projects.puppetlabs.com/issues/2157

  • Author: Paul Nasrat
  • Status: In Topic Branch Pending Merge
  • Priority: Normal
  • Assignee: Ken Barber
  • Category:
  • Target version: 2.0.0
  • Keywords:
  • Branch: kbarber/tickets/master/2157-external_fact_support
  • Affected Facter version:

Facter should support non-ruby facts, preferably in /etc/facter.d. It should support these facts being either executable, in which case the result is the value of the named fact, or in a data format such as yaml, in which case the data file is read in and interpreted as the fact value.

It probably makes sense to initially stick to yaml for data formats, since json doesn’t ship with ruby, and to also allow executable facts to return either a plain string or yaml.

Note that we can do this without supporting any kind of overriding, but it’d be much better if we supported multiple (configurable?) fact directories, with a search path. Thus, if Facter shipped with /etc/facter.d/myfactname and you wanted to override it, you could do so by creating a new file and putting it in a higher-priority location rather than editing a file distributed with the core.

Given we’re adding structured data support, namespaced facts would be supported with directory structures; e.g., /etc/facter.d/my/fact/name would resolve to my::fact::name.

Ideally, the long-term direction here would be not to require any pure-ruby facts, such that the Facter library could be rewritten in any other language and it would function the same, because all of its actual data is outside of

ruby.

You have received this notification because you have either subscribed to it, or are involved in it. To change your notification preferences, please click here: http://projects.puppetlabs.com/my/account

#36 Updated by Ken Barber almost 3 years ago

Adrien Thebo wrote:

Hi Ken,

I’m currently doing merges into facter en masse, and I was wondering if this was actually ready for merge or not. Is the status of this ticket incorrect?

Nope – the status is incorrect. Last time I looked I don’t have access to change it back to a stage that is suitable. If you can do this I would appreciate it. Also the target version is incorrect – it should be 1.7.0 I believe.

#37 Updated by James Turnbull almost 3 years ago

  • Status changed from In Topic Branch Pending Review to Code Insufficient
  • Target version changed from 14 to 2.0.0

#38 Updated by Ken Barber almost 3 years ago

TypeError handling now done.

#39 Updated by Ken Barber almost 3 years ago

I’ve committed some doc changes to a topic branch for puppet-doc:

https://github.com/kbarber/puppet-docs/commits/2157-external_fact_support

And asked Nick Fagerlund to take a look.

#40 Updated by Nick Fagerlund almost 3 years ago

I’ve gone ahead and pushed the docs on this, since we have to put a version caveat in this info anyway and I don’t think having slightly-pre-release info in the custom facts guide is a huge deal.

#41 Updated by Michael Stahnke almost 3 years ago

I’m a jerk and thought about this more. I hate executable scripts in etc. It’s wrong. While FHS isn’t clear, maybe we could do something better than put it in etc. Something in /var/lib/facter/facts.d or /usr/share/facter/facts.d makes more sense. Some people mounst /usr as read-only, so that might be a problem. /var/lib is probably best. We can easily symlink from etc to /var/lib.

Sorry for a reversal on my decision, but after reviewing some other code with scripts in etc, I don’t think we want to be part of that problem.

As for Windows, I have no clue what the right way is. punt

As far cache, the pathing discussed the far is ok. We could use /var/cache/facter as well.

In the end will this be configurable for path locations?

If I’ve spoken up to late, just tell me and I’ll shut up and deal with it :)

#42 Updated by Yury Zaytsev almost 3 years ago

Michael Stahnke wrote:

I’m a jerk and thought about this more. I hate executable scripts in etc. It’s wrong. While FHS isn’t clear, maybe we could do something better than put it in etc.

Yes, it doesn’t feel right, but consider

1) rc.local and all the like

2) this is about the only place that it consistently considered to contain files that are directly modified by the user / administrator

3) the scripts don’t have to have executable bits set, one can fork an interpreter according to the shebang line and if it is not there, exec it

The latter probably doesn’t make a difference / solve anything, but anyway.

/usr/share/facter/facts.d makes more sense. Some people mounst /usr as read-only, so that might be a problem.

/usr/share is only supposed to have

1) noarch stuff, and generally not executable directly

2) stuff not directly modifiable by the user (which is why many people mount it as read-only)

Something in /var/lib/facter/facts.d /var/lib is probably best. We can easily symlink from etc to /var/lib.

Sounds better than /usr/share but still does feel SO good. Many paranoid people mount /var with noexec, for instance (which will never happen to /etc for reasons above), just because /var is for software-generated transient data which is normally not to be executed.

AFAIK, according to FHS this is what /usr/libexec/packagename is officially for, although mostly for arch-dependent binaries. Still it doesn’t solve your read-only concern.

All this is getting real ugly :–( Maybe just go for /etc and get over it?

As far cache, the pathing discussed the far is ok. We could use /var/cache/facter as well.

Sounds like it’s a perfect place for cache.

In the end will this be configurable for path locations?

Err, apparently yes.

0.02 €

#43 Updated by Ken Barber almost 3 years ago

Michael Stahnke wrote:

I’m a jerk and thought about this more. I hate executable scripts in etc. It’s wrong. While FHS isn’t clear, maybe we could do something better than put it in etc. Something in /var/lib/facter/facts.d or /usr/share/facter/facts.d makes more sense. Some people mounst /usr as read-only, so that might be a problem. /var/lib is probably best. We can easily symlink from etc to /var/lib.

I hear you – I’ve been pondering it myself actually.

The closest thing I can find is: “/usr/lib includes object files, libraries, and internal binaries that are not intended to be executed directly by users or shell scripts.”

/usr/share is architecture independent (which may or may not be true). /var/lib is for variable ‘data’. So I would rule these out.

Maybe /usr/lib/facter/facts.d ? (another note – Does the ‘something.d’ sound silly to you or is it just me? This generally applies to breaking up configuration which isn’t what we are doing here. Maybe just /usr/lib/facter/ext or something.)

The closest example of this in another application is actually:

/usr/lib/nagios/plugins/check_foo

(at least thats the path on debian)

Sorry for a reversal on my decision, but after reviewing some other code with scripts in etc, I don’t think we want to be part of that problem.

Fair enough I think. FHS does explicitly state no execs.

In the end will this be configurable for path locations?

It will be – either through API or command line arguments. But the sensible defaults should be the ‘best’ option.

If I’ve spoken up to late, just tell me and I’ll shut up and deal with it :)

No its cool. We can rework it. Its stuff like this we need to get right up front.

#44 Updated by R.I. Pienaar almost 3 years ago

The closest example of this in another application is actually:

/usr/lib/nagios/plugins/check_foo

+1, also where mcollective agents/plugins etc is on debian. I wanted to make RH the same though the community didnt seem to like the change – not the location but having to deal with change. /usr/lib/foo is right though i think

#45 Updated by Yury Zaytsev almost 3 years ago

/usr/lib/nagios/plugins/check_foo

AFAIK those are supposed to be sourced / run via the embedded Perl interpreter or something like that. Putting executables in /usr/lib is bad style, that’s what /usr/libexec is for.

#46 Updated by Yury Zaytsev almost 3 years ago

/usr/share is architecture independent (which may or may not be true). /var/lib is for variable ‘data’. So I would rule these out.

re: yes, /usr/share is supposed to be noarch. I don’t know if exceptions are allowed to this rule.

#47 Updated by Ken Barber almost 3 years ago

Yury Zaytsev wrote:

/usr/lib/nagios/plugins/check_foo

AFAIK those are supposed to be sourced / run via the embedded Perl interpreter or something like that. Putting executables in /usr/lib is bad style, that’s what /usr/libexec is for.

/usr/libexec is not mentioned in the FHS. Having said that – this is probably the correct place on Mac OS X according to ‘man hier’.

#48 Updated by Ken Barber almost 3 years ago

Yury Zaytsev wrote:

/usr/lib/nagios/plugins/check_foo

AFAIK those are supposed to be sourced / run via the embedded Perl interpreter or something like that. Putting executables in /usr/lib is bad style, that’s what /usr/libexec is for.

Oh yeah – and Nagios plugins are just exec’d. They don’t need to be perl.

#49 Updated by James Turnbull almost 3 years ago

I’m comfortable with that as long as we make it configurable somehow I think.

#50 Updated by Yury Zaytsev almost 3 years ago

Ken Barber wrote:

/usr/libexec is not mentioned in the FHS. Having said that – this is probably the correct place on Mac OS X according to ‘man hier’.

It used to be in FHS, but I have checked out the latest version and it was removed indeed, because they felt it doesn’t have a meaning clear enough. Apparently that’s why Debian uses /usr/lib instead, whereas RH/Fedora keep on using /usr/libexec.

The discussion on re-introducing it again is ongoing, but I can’t check the status, because Linux Foundation infrastructure is still down:

http://bugs.freestandards.org/show_bug.cgi?id=3D101

#51 Updated by R.I. Pienaar almost 3 years ago

Are you suggesting we make this linux FHS compliant and put it elsewhere on other OS to make it comply to their standards? That would be a mistake imho, as would forcing linux standards on other unixen.

#52 Updated by Ken Barber almost 3 years ago

R.I. Pienaar wrote:

Are you suggesting we make this linux FHS compliant and put it elsewhere on other OS to make it comply to their standards? That would be a mistake imho, as would forcing linux standards on other unixen.

There are cases where this kind of thing needs to occur and is hard to avoid. Take the cache – its /var/cache/something on linux but /var/db/something on Mac because mac lacks a /var/cache (although it has a /Library/Cache which is world writeable and not really safe imho).

I think the /usr/lib and /usr/libexec thing between just Redhat and Debian is going to be argued. Debian doesn’t even have a /usr/libexec so it can’t be there on Debian … but I’m sure Redhat/Fedora repackagers may feel they want to modify it to /usr/libexec to suit their ‘standards’. sigh.

#53 Updated by Yury Zaytsev almost 3 years ago

Ken Barber wrote:

but I’m sure Redhat/Fedora repackagers may feel they want to modify it to /usr/libexec to suit their ‘standards’. sigh.

You bet!

— RH Packager

#54 Updated by Ken Barber almost 3 years ago

Yury Zaytsev wrote:

Ken Barber wrote:

but I’m sure Redhat/Fedora repackagers may feel they want to modify it to /usr/libexec to suit their ‘standards’. sigh.

You bet!

— RH Packager

Thats funny :–). So Redhats official position is:

http://fedoraproject.org/wiki/Packaging:Guidelines#Libexecdir

Which is a ‘may’ not a ‘must’.

On BSD – it looks like ‘libexec’ is the correct place on the other-hand … as libexec seems to have spawned from BSD. A lot of GNU tools also used libexec (like emacs) so I think this is how it found its way onto linux as well.

I’m thinking we should take the high road and use the FHS for Linux:

/usr/lib/facter

To keep documentation consistent. There is an argument for using:

/usr/libexec/facter

For BSD based OS though … what do people think? I think BSD people (and OS X) would expect libexec to be used. But keeping it consistent for Linux would be nice – at least it would make documentation a little clearer. As it stands I’ll need a table to point this stuff out :–). In a way its a shame libexec is not in the FHS – but alas that is a different discussion.

I’m still trying to find out what Solaris uses … all these years and I’m still unable to find a published standard for how they lay things out. If anyone has anything let me know. I presume its going to be /usr/libexec as well.

Also … beyond the basic “libexec” path … I wanted to use the following as the full path for external facts:

$FACTER_LIBEXEC/ext

… but I’m totally open to suggestions. ‘ext’ is brief but probably not that obvious. I plan on dropping a README file in their at least to explain what the directory is for in case people stumble upon it.

#55 Updated by Ken Barber almost 3 years ago

Another thought – right now the configurability of this location is on the command line – or via API. Maybe we need a configuration file where this path is defined. This would at least provide a place for people to find out what the path is easily without having to rely on magic path detection and documentation.

We haven’t used configuration files up until now … but perhaps its time.

#56 Updated by Ken Barber almost 3 years ago

Looks like Solaris lacks /usr/libexec.

#57 Updated by Ken Barber almost 3 years ago

SLES 11 does not have /usr/libexec Gentoo still uses /usr/libexec it would seem … but I think its being deprecated. OpenBSD has /usr/libexec … which aligns with my BSD assumption.

#58 Updated by Ken Barber almost 3 years ago

I’ve now updated install.rb to create the necessary directory for external facts – at the moment I’m just assuming for now:

/usr/lib/facter/ext

(And COMMON_APPDATA/Puppetlabs/facter/ext for windows)

I’ve also updated the rpm spec file to handle this.

I’m happy to change this depending on the outcome of this conversation regarding paths … otherwise I’m more or less done with this feature.

#59 Updated by Ken Barber almost 3 years ago

  • Subject changed from External fact support in /etc/facter.d to External fact support & caching
  • Status changed from Code Insufficient to In Topic Branch Pending Review

Pull request submitted. This is a big patch set – so I’m happy to review with someone when I get into Portland next week.

#60 Updated by Ken Barber almost 3 years ago

  • Category set to interface
  • Status changed from In Topic Branch Pending Review to Code Insufficient
  • Branch changed from kbarber/tickets/master/2157-external_fact_support to https://github.com/kbarber/facter/tree/ticket/2157-external_fact_support

Rebased the original branch based on current master.

Based on code review I need to do the following cleanups:

  • break out watchr work into its own request (as it can benefit 1.6.x as well anyway)
  • break out windows warning fixup into its own request
  • break out ruby ttl change into its own commit if possible
  • a warning for ruby 1.9.x is being thrown that needs fixing
  • unnecessary use of an attribute

#61 Updated by Ken Barber almost 3 years ago

  • Status changed from Code Insufficient to In Topic Branch Pending Review

Submitted pull requested … and asked Adrien to review again:

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

#62 Updated by Ken Barber almost 3 years ago

  • Status changed from In Topic Branch Pending Review to Code Insufficient

So Daniel kindly made some detailed comments on my code – and I’m going back through them now to clean up. So yeah – this still isn’t ready for ‘prime time’ but I have a few days now to work on this again.

#63 Updated by Ken Barber almost 3 years ago

  • Assignee changed from Ken Barber to Adrien Thebo

Moving cache back into another ticket – since Adrien wants to pick up the external facts work. Cache is moving back to #4519.

#64 Updated by Ken Barber almost 3 years ago

  • Subject changed from External fact support & caching to External fact support

#65 Updated by Adrien Thebo over 2 years ago

  • Branch changed from https://github.com/kbarber/facter/tree/ticket/2157-external_fact_support to https://github.com/adrienthebo/facter/tree/ticket/master/2157-external_fact_support

I’ve done a lot of work to split up Ken’s work into a lot of small commits, but I don’t have the bandwidth to finish this off. The code is at https://github.com/adrienthebo/facter/tree/ticket/master/2157-external_fact_support and while it could use some rebasing and squashing, the code looks solid (to me) and just needs a little gardening.

#66 Updated by Adrien Thebo over 2 years ago

  • Assignee deleted (Adrien Thebo)

#67 Updated by Jeff Weiss over 2 years ago

  • Assignee set to Jeff Weiss

Needs – rebase (off current tree?) – review – move addl functionality to atomic tickets & pull reqs (eg powershell) – squashed ?

We have docs already of what these external facts should look like with custom facts

#68 Updated by Jeff Weiss over 2 years ago

  • Target version changed from 2.0.0 to 2.1.0

#69 Updated by Jeff Weiss over 2 years ago

  • Branch changed from https://github.com/adrienthebo/facter/tree/ticket/master/2157-external_fact_support to https://github.com/jeffweiss/facter/commits/ticket/master/2157-external_fact_support

#70 Updated by Jeff Weiss over 2 years ago

Current branch from ken->adrien->me (jeffweiss) currently relies on json gem. Just reiterating json as an additional dep. As this gets closer to prime time, we’ll want to evaluate if we want to actually introduce that dependency or handle it in a more Facter.feature.json? kind of way.

#71 Updated by Anonymous about 2 years ago

  • Branch changed from https://github.com/jeffweiss/facter/commits/ticket/master/2157-external_fact_support to https://github.com/puppetlabs/facter/pull/244

#72 Updated by Adrien Thebo about 2 years ago

I just want to comment that you guys rock for handling this.

#73 Updated by Jeff McCune about 2 years ago

  • Status changed from Code Insufficient to Merged - Pending Release
  • Assignee deleted (Jeff Weiss)
  • Target version changed from 2.1.0 to 2.0.0

#74 Updated by Jeff McCune about 2 years ago

Adrien, you might want to check out some of the cleanup work we did.

In particular, the spec tests were doing a lot of filesystem IO which should (“must”) be avoided when writing unit tests:

I think the patch in 84bc0f8 does a reasonable job of preserving your otherwise pretty good tests while avoiding and filesystem I/O.

Thanks again for all of this. Nice to have it merged into 2.x

-Jeff

#75 Updated by Jeff McCune over 1 year ago

  • Target version changed from 2.0.0 to 1.7.0

External facts are included in the 1.7.x branch in commit 9f3d8a94. I’m re-targeting this ticket at 1.7.0 as a result.

See commit 9f3d8a94

-Jeff

#76 Updated by Matthaus Owens over 1 year ago

  • Status changed from Merged - Pending Release to Closed

Released in Facter 1.7.0

Also available in: Atom PDF