[FACT-1111] Restore 'facter --puppet' Created: 2015/05/21  Updated: 2015/12/08  Resolved: 2015/07/15

Status: Closed
Project: Facter
Component/s: None
Affects Version/s: None
Fix Version/s: FACT 3.0.2

Type: Bug Priority: Major
Reporter: Jo Rhett Assignee: Unassigned
Resolution: Fixed Votes: 1
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Blocks
is blocked by FACT-96 Deprecate 'facter --puppet' Closed
Relates
relates to FACT-1283 facter --puppet doesn't load plugin s... Closed
Template:
Story Points: 3
Sprint: Client 2015-07-22

 Description   

This ticket went through some evolution, so I'll attempt to summarize.

The '--puppet' option to facter introduces a circular dependency between facter and puppet, so we had plans to break that cycle. Specifically, in 2.4.4 we introduced a deprecation notice for 'facter --puppet', and in facter 3.0 we removed it (by not reimplementing it). Our recommendation was to use 'puppet facts' instead.

However, as raised in this ticket and in some conversations in IRC and meatspace, there are a couple concerns with that deprecation and removal:

  • 'puppet facts' doesn't implement the full set of functionality, especially the ability to specify a single fact to retrieve
  • there's a fair bit of scripting around the current behavior, and we really need to have the full replacement behavior in place for an overlapping period of time to support an easy transition away from the deprecated behavior
  • deprecation or not, we haven't addressed additional functionality such as specifying elements of a structured fact from the CLI

So for the time being, we're restoring 'facter --puppet'. Ultimately, we still want to break the circular dependency between facter and puppet, but we won't do so until we have a clear path forward for how the two interact.

------------------

Original description below.

------------------
So while I understand the goal behind FACT-96. there's a significant loss of functionality there. I see no method of retrieving a single value, which is very highly used within external scripts which make use of puppet plugin-synced facts.

$ facter system_uptime
{"seconds"=>532, "hours"=>0, "days"=>0, "uptime"=>"0:08 hours"}

Note in this context, just from my personal source archives I found more than 1000 uses of this exact functionality, and I can attest to tens of thousands more uses at companies where I am not allowed to take their source offsite on my laptop.

Resolution: we're restoring `facter -p` in Facter 3.0.2, with plans to address FACT-96 more completely in the future.



 Comments   
Comment by Josh Cooper [ 2015/05/22 ]

I could have sworn we already had a ticket on this, but I can't find it. Yep, the functionality is important, and something we plan on addressing before removing the --puppet option.

Comment by Kylo Ginsberg [ 2015/05/22 ]

Ah I saw the comment on FACT-96 but missed this ticket initially. So I'll ask here

Jo Rhett I didn't connect your comment to your example. Specifically, system_uptime is a built-in fact, not a plugin-synced fact. Are you asking about the ability to get an individual element of a structured fact (like system_uptime) from the command-line?

If so, with facter 3, you can use a dotted syntax on the command line, e.g.

$ ./bin/facter system_uptime
{
  days => 27,
  hours => 669,
  seconds => 2410743,
  uptime => "27 days"
}
$ ./bin/facter system_uptime.days
27

Comment by Jo Rhett [ 2015/05/23 ]

Kylo: I'm asking about the ability to get ANY fact provided through puppet pluginsync on the command line after --puppet is removed. String, structured, etc.

In my experience it is very common practice to make new facts available, and then use them in shell scripts run on the host – say for cron jobs, scripts invoked by jenkins, etc.

The facter 3 functionality shown above would be nice, but is nearly irrelevant without the ability to see Puppet's facts. There's only a few structured facts available by default.

Comment by Kylo Ginsberg [ 2015/05/25 ]

Jo Rhett gotcha, and that makes perfect sense. Given that I think I'll move this to the PUP project and adjust the summary to something like "'puppet facts' should allow retrieval of a specified fact".

Once that is done, there could be a follow-on effort to further extend "puppet facts" to provide something like facter 3's dotted syntax for pulling out an individual element of a structured fact. That really needs to be sanity-checked for UX consistency across the various projects though, so out of scope for this ticket.

Comment by Michael Smith [ 2015/06/29 ]

Other functionality we should support is dumping yaml - which should be provided by puppet facts --render_as yaml - and providing just the facts rather than the whole facts node indirector response (which puts facts in the values key).

Comment by Jo Rhett [ 2015/06/29 ]

No, I'm sorry but whitewashing this into an Improvement is not acceptable.

You are removing functionality which works today, the removal of which will break hundreds of customers, and at least three dozen modules on the Puppet Forge. This is a bug. Restoring the functionality is not an improvement, it's fixing your bug.

Comment by Jo Rhett [ 2015/06/29 ]

I'm restoring the original title since the title change was an attempt to whitewash this bug into a feature request, which it most absolutely is not.

Comment by Kylo Ginsberg [ 2015/07/06 ]

After more dialog, an alternate approach might be:

  • since 'facter' as the owner of facts is reasonably intuitive (compared to 'puppet facts')
  • short-term: modify facter to look in puppet's default plugin sync locations for custom and external facts
  • longer-term: look into a config file for facter (again, treating facter as the owner of facts) and have facter tell puppet where to put them

Short-term, that would break the circular dependency concern of fact-96, but wouldn't address usage of 'facter -p' at sites that override the plugin sync directories.

Comment by Kylo Ginsberg [ 2015/07/08 ]

Okay, I got nods on the above in meatspace. So moving this back to FACT

Comment by Jo Rhett [ 2015/07/08 ]

look into a config file for facter (again, treating facter as the owner of facts) and have facter tell puppet where to put them

Short-term, that would break the circular dependency concern of fact-96, but wouldn't address usage of 'facter -p' at sites that override the plugin sync directories.

These two sentences seem pretty obviously related. Have facter check the puppet configuration for the plugin sync directories?

Or, document that if you override the plugin sync directories you must also set facter's load path?

Comment by Michael Smith [ 2015/07/08 ]

We should also take into account that some module custom facts assume Puppet is initialized, see https://github.com/puppetlabs/puppetlabs-firewall/blob/master/lib/facter/iptables_persistent_version.rb#L8.

Comment by Michael Smith [ 2015/07/09 ]

To summarize a short in-person conversation with Branan Riley:

In response to my own comment yesterday, we need to gracefully handle failures to load custom facts (pretty sure we already do, but worth verifying) and move people to modules to requiring Puppet explicitly if they need it and want to be usable. Tempted to make this an INFO level message, rather than a warning.

Also, we may want to reset plugindest and pluginfactdest (and maybe other things under libdir) in the puppetlabs-puppet_agent module.

Comment by Kylo Ginsberg [ 2015/07/09 ]

Agreed on gracefully handling failures to load custom facts.

As for log level, I'd incline the other way, i.e. to making it a warning or error if custom facts fail to load - that is something that the admin should fix. Otoh, it may be that it's super common and we don't want to spam people who've been blithely living with custom fact load failures for years. Perhaps we should look at what facter 2 does as input to the log level question.

As for resetting plugin*dest, IIRC Josh Cooper suggested that those two settings couldn't be productively or correctly set in puppet anyway, meaning maybe no one is doing so. Not sure if I got that right, or how to validate that though. So it couldn't hurt to reset those settings in puppet_agent.

Comment by Branan Riley [ 2015/07/09 ]

I'm fine with saying that those facts need to explicitly 'require puppet'. We probably do need to make sure that puppet is in our $LOAD_PATH, though.

Comment by Branan Riley [ 2015/07/09 ]

Kylo Ginsberg Currently we reset libdir. If somebody had (for some reason) set both libdir and plugindest to the same value (even though plugindest defaults to libdir), we'd be causing breakages in the upgrade process. Given it's a one-liner to fix plugindest where we fixup libdir, it seems a worthwhile "just in case"... but def is unrelated to this ticket.

Comment by Michael Smith [ 2015/07/09 ]

Facts that rely on things like Puppet::Util would work with facter -p, because Facter actually does require 'puppet'. So this still breaks some facts that would've worked with facter -p, but in a fairly trivial way.

For quick reference, facter -p did https://github.com/puppetlabs/facter/blob/2.x/lib/facter/application.rb#L190-L203.

Comment by Peter Huene [ 2015/07/09 ]

re: custom facts failing to load: currently we log an error in Facter along with any exception message; --trace will cause the output to include the back trace, like we would have from Ruby Facter. I think that's sufficient and desirable.

re: facts that implicitly use Puppet: I think it's not entirely unreasonable to ask users to require puppet from their fact instead of relying on factor to magically require it only when -p was passed. It's not ideal as it is a breaking change, but I'd like to avoid requiring puppet all the time as it is a perf hit. Alternatively, we could still implement a -p switch that does this magic, but ugh.

Comment by Josh Cooper [ 2015/07/09 ]

re: plugindest, if plugindest is changed from the default, then puppet agent won't be able to load pluginsynced code: see https://projects.puppetlabs.com/issues/18459. The reason is because libdir is not also updated to reflect the new location, and the hook for that setting is what changes puppet's $LOAD_PATH. So tl; dr, puppet agent will not work if you change the default plugindest.

The other thing to consider is pluginfactsdest for pluginsync'ed external facts. If that value was changed, I think puppet will still work (could tell facter to load those external facts). But I'm not sure if facter -p would have worked previously.

Comment by Peter Huene [ 2015/07/09 ]

From Michael's link, it doesn't look like Facter 2.x added pluginfactsdest to the external search paths, so I don't think pluginsync'd external facts worked even with -p in 2.x. Still, that's something we should get working in 3.0.2, even if it's for the default location only.

Comment by Josh Cooper [ 2015/07/09 ]

re:pluginfactsdest, was ticketed as FACT-696

Comment by Jo Rhett [ 2015/07/09 ]

Peter: that is completely untrue. Not only have I written dozens of modules which sync down facts that are used in bash scripts on the host, but I've found that dozens of community modules I depend on do similar things, pushing down facts to be used for cron jobs, etc.

That is why I opened this issue. This did and does today work with Facter 2.

Comment by Josh Cooper [ 2015/07/09 ]

Jo Rhett you are confusing pluginsynced custom vs pluginsynced external facts. The former did work previously in 2.x, the latter did not (see FACT-696). Unfortunately we don't have great names for the three variations of facts: pluginsynced custom vs pluginsynced external vs non-pluginsynced external...

Comment by Michael Smith [ 2015/07/09 ]

Current puppetlabs modules that expect Puppet to be set in their custom facts

puppetlabs-concat
puppetlabs-firewall
puppetlabs-stdlib

Looks like just a few we'd have to patch up. puppetlabs-policy_engine already uses require 'puppet'.

Update: I have a longer list of the entire Forge I need to prune through, to verify just latest versions.

Comment by Michael Smith [ 2015/07/13 ]

This was incorrectly put into master instead of stable, wait on verifying until I cherry-pick up to stable.

Comment by Michael Smith [ 2015/07/13 ]

Also failed CI on Windows Server 2003.

Comment by Sean Griffin [ 2015/07/15 ]

In facter 3.0.1 (this package built 09-Jul-2051 23:23), the command 'facter --puppet' results in an error (unrecognized option).

[root@ghz00vl9mluu6j6 ~]# facter --version
3.0.1 (commit c5daeac41a7a9004bc6c3d824c87b79c27c78d2b)
[root@ghz00vl9mluu6j6 ~]# facter --puppet system_uptime
error: unrecognised option '--puppet'
 
...

In (what will be) facter 3.0.2, the --puppet option has been restored and is described in the help.

This version was built on 15-Jul-2015 07:31.

[root@xa3dmo6k7x1541s bin]# facter --version
3.0.1 (commit 6da8c340fb385aff0cd90ea666fe0b9a0e8e6ad6)
[root@xa3dmo6k7x1541s bin]# facter --puppet system_uptime
{
  days => 0,
  hours => 3,
  seconds => 10844,
  uptime => "3:00 hours"
}
[root@xa3dmo6k7x1541s bin]#

Comment by Michael Smith [ 2015/07/15 ]

It would be worth verifying that facter --puppet behaves as expected, finding facts in pluginsynced external and custom fact locations. See pluginfactdest and libdir in https://github.com/puppetlabs/puppet-specifications/blob/master/file_paths.md.

Comment by Peter Huene [ 2015/07/15 ]

Also a test case where you use facter --puppet without puppet installed would be good to test.

Comment by Sean Griffin [ 2015/07/20 ]

Additional test cases:
1. pluginsynced external fact
2. facter cmds in a system absent puppet

1. pluginsynced external fact:
tested on centos-7-x86_64

On the Master root directory I (1) created a module: hellofact, containing
an external fact, hellofact.rb, which return string "Hello facter!",
(2) built it with 'puppet module build hellofact', (3) installed it with
install hellofact/pkg/sean-hellofact-0.1.0.tgz, (4) called 'puppet agent -t'.
On the agent node I also called 'puppet agent -t'.

On both agent and master, facter behaves the same way when fetching
the external fact hellofact. When called with --puppet, the fact is displayed.
When called without --puppet, the fact is not displayed.

[root@t08kvwquaxnqa35 ~]# facter -p hellofact
Hello facter!
[root@t08kvwquaxnqa35 ~]# facter hellofact
 
[root@t08kvwquaxnqa35 ~]#

2. facter cmds in a system absent puppet
Running facter with no puppet.

Tested on fedora-20-x86_64
stable branch 7/22/15 2200
3.0.1 (commit fe9df8a0719361c88fb3726dc535d04b80759871)

In this test, I compiled facter on a Fedora vm and ran it without puppet installed. facter -p generated a the following warning to stderr but did not cause failure (very nice):

2015-07-20 22:30:31.402088 WARN puppetlabs.facter - Could not load puppet; some facts may be unavailable: cannot load such file – puppet

Except for that, the output from invocations with and without the -p option are the same. Tested with --custom-dir, --external-dir

[root@yiyjcvx1os3k4sx bin]# pwd
/root/facter/release/bin
[root@yiyjcvx1os3k4sx bin]# ls
facter  facts.d  f.out  fp.out  libfacter_test
[root@yiyjcvx1os3k4sx bin]# PATH=.:$PATH
[root@yiyjcvx1os3k4sx bin]# facter --version
3.0.1 (commit fe9df8a0719361c88fb3726dc535d04b80759871)
[root@yiyjcvx1os3k4sx bin]# ### stable branch of facter, leatherman
[root@yiyjcvx1os3k4sx bin]#
[root@yiyjcvx1os3k4sx bin]# facter os.family
[root@yiyjcvx1os3k4sx bin]# facter -p os.family
2015-07-20 22:30:31.402088 WARN  puppetlabs.facter - Could not load puppet; some facts may be unavailable: cannot load such file -- puppet
RedHat
[root@yiyjcvx1os3k4sx bin]# cat facts.d/hellofact.rb
Facter.add("hellofact") do
  setcode do
    Facter::Core::Execution.exec("echo hello factor!\n")
  end
end
 
[root@yiyjcvx1os3k4sx bin]# facter --custom-dir facts.d/ hellofact
hello factor!
[root@yiyjcvx1os3k4sx bin]# facter -p --custom-dir facts.d/ hellofact
2015-07-20 22:31:40.677669 WARN  puppetlabs.facter - Could not load puppet; some facts may be unavailable: cannot load such file -- puppet
hello factor!
[root@yiyjcvx1os3k4sx bin]
[root@yiyjcvx1os3k4sx bin]# cat facts.d/hf
#! /bin/sh
echo "key=value"
[root@yiyjcvx1os3k4sx bin]# ls -l facts.d/
total 8
-rw-r--r--. 1 root root 108 Jul 20 22:21 hellofact.rb
-rwxr-xr-x. 1 root root  28 Jul 20 22:57 hf
[root@yiyjcvx1os3k4sx bin]# facter --external-dir facts.d/ key
value
[root@yiyjcvx1os3k4sx bin]# facter -p --external-dir facts.d/ key
2015-07-20 23:04:47.886185 WARN  puppetlabs.facter - Could not load puppet; some facts may be unavailable: cannot load such file -- puppet
value
[root@yiyjcvx1os3k4sx bin]#

Generated at Tue Jan 28 07:46:42 PST 2020 using JIRA 7.7.1#77002-sha1:e75ca93d5574d9409c0630b81c894d9065296414.