Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable reports by default #9461

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ekohl
Copy link
Contributor

@ekohl ekohl commented Aug 28, 2024

The default value for this goes all the way back to when it was introduced in 566bf78. I'd argue it a bad default, because it just writes files without any clean up process in place. That only causes hard disks to fill up.

Having reports should be an opt in feature, where they make a careful choice. PuppetDB is an obvious recommended option but not one that can be relied on by default.

This came up because of a discussion in Debian. I never thought about this because I always set up Foreman as reports and don't rely on it, but this to me is the sanest default.

@ekohl ekohl requested a review from a team as a code owner August 28, 2024 20:53
@puppetlabs-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

@binford2k
Copy link
Contributor

if reports were also automatically purged after 30 days, then I'd argue for it to be left on. But as things are, none is the safest choice.

@kenyon
Copy link
Contributor

kenyon commented Aug 28, 2024

Documentation would also need an update: https://www.puppet.com/docs/puppet/latest/report.html

@jcharaoui
Copy link
Contributor

Documentation would also need an update: https://www.puppet.com/docs/puppet/latest/report.html

I couldn't find the source for this documentation, but it should remove the mention that store is the default, and also document the none option.

@joshcooper
Copy link
Contributor

filebuckets suffered from the same unbounded growth. Changing to none makes sense. Seems like a puppet 9 thing.

I couldn't find the source for this documentation, but it should remove the mention that store is the default, and also document the none option.

The source for reference docs is in puppet now, so this will be easy to fix. See

desc "Generate report reference"
task :report do
body = puppet_doc('report')
generate_reference('report', REPORT_ERB, body, REPORT_MD)
end

@jcharaoui
Copy link
Contributor

My suggestion to fix the documentation page:

diff --git a/references/report.md b/references/report.md
index dba027d1f7..80ef9218a3 100644
--- a/references/report.md
+++ b/references/report.md
@@ -39,12 +39,15 @@ log
 Send all received logs to the local log destinations.  Usually
 the log destination is syslog.
 
+none
+----
+Discard all reports received. This is the default handler when the `reports`
+setting is unset.
+
 store
 -----
 Store the yaml report on disk.  Each host sends its report as a YAML dump
 and this just stores the file on disk, in the `reportdir` directory.
 
 These files collect quickly -- one every half hour -- so it is a good idea
-to perform some maintenance on them if you use this report (it's the only
-default report).
-
+to perform some maintenance on them if you use this report.

The default value for this goes all the way back to when it was
introduced in 566bf78. I'd argue it a
bad default, because it just writes files without any clean up process
in place. That only causes hard disks to fill up.

Having reports should be an opt in feature, where they make a careful
choice. PuppetDB is an obvious recommended option but not one that can
be relied on by default.
@ekohl
Copy link
Contributor Author

ekohl commented Aug 30, 2024

Something like this?

@jcharaoui
Copy link
Contributor

In terms of whether to ship this in Puppet 8 or 9, my preference is to ship this as an update in Puppet 8. The consequence of the current default is likely to lead to filesystems filling up and breaking the system, as we've seen from bug reports in Debian (eg. #1078911)

If it's delayed to Puppet 9 we're sure to backport the change to 8 in Debian. We're also seriously considering backporting this to 7, in Debian stable.

@joshcooper
Copy link
Contributor

joshcooper commented Sep 3, 2024

In terms of whether to ship this in Puppet 8 or 9, my preference is to ship this as an update in Puppet 8.

As much as I agree that none should have been the default from the beginning, it doesn't take away the fact that someone is currently relying on the default store and likely has been for many years, so this will need to wait until Puppet 9.

Related, we had the same problem with filebuckets and we changed the default behavior in 8e9657f first released in puppet 7.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants