Skip to content

Add params and ini_settings for node/report/purge ttls #35

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.

Sign up for GitHub

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

Merged
kbarber merged 3 commits into from
Apr 4, 2013
Merged

Add params and ini_settings for node/report/purge ttls #35

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9efe80b
Add params and ini_settings for node/report/purge ttls
nicklewis Feb 5, 2013
76fa901
Document ttl settings in README
nicklewis Feb 7, 2013
735f18f
Document which versions of PuppetDB the ttl settings work in
nicklewis Feb 28, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,19 @@ The password for the database user (defaults to `puppetdb`; ignored for `embedde

The name of the database instance to connect to (defaults to `puppetdb`; ignored for `embedded` db).

####`database_package`
####`node_ttl`

The length of time a node can go without receiving any new data before it's automatically deactivated. (defaults to '0', which disables auto-deactivation). This option is supported in PuppetDB >= 1.1.0.

####`node_purge_ttl`

The length of time a node can be deactivated before it's deleted from the database. (defaults to '0', which disables purging). This option is supported in PuppetDB >= 1.2.0.

####`report_ttl`

The length of time reports should be stored before being deleted. (defaults to '7d', which is a 7-day period). This option is supported in PuppetDB >= 1.1.0.

####`puppetdb_package`

The puppetdb package name in the package manager.

Expand Down
15 changes: 15 additions & 0 deletions manifests/init.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,18 @@
# (defaults to `puppetdb`; ignored for `embedded` db)
# ['database_name'] - The name of the database instance to connect to.
# (defaults to `puppetdb`; ignored for `embedded` db)
# ['node_ttl'] - The length of time a node can go without receiving
# any new data before it's automatically deactivated.
# (defaults to '0', which disables auto-deactivation)
# This option is supported in PuppetDB >= 1.1.0.
# ['node_purge_ttl'] - The length of time a node can be deactivated before
# it's deleted from the database.
# (defaults to '0', which disables purging)
# This option is supported in PuppetDB >= 1.2.0.
# ['report_ttl'] - The length of time reports should be stored before
# being deleted.
# (defaults to '7d', which is a 7-day period)
# This option is supported in PuppetDB >= 1.1.0.
# ['open_postgres_port'] - If true, open the postgres port on the firewall.
# (defaults to true).
# ['puppetdb_package'] - The puppetdb package name in the package manager
Expand Down Expand Up @@ -83,6 +95,9 @@
$database_username = $puppetdb::params::database_username,
$database_password = $puppetdb::params::database_password,
$database_name = $puppetdb::params::database_name,
$node_ttl = $puppetdb::params::node_ttl,
$node_purge_ttl = $puppetdb::params::node_purge_ttl,
$report_ttl = $puppetdb::params::report_ttl,
$puppetdb_package = $puppetdb::params::puppetdb_package,
$puppetdb_version = $puppetdb::params::puppetdb_version,
$puppetdb_service = $puppetdb::params::puppetdb_service,
Expand Down
5 changes: 5 additions & 0 deletions manifests/params.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,11 @@
$database_username = 'puppetdb'
$database_password = 'puppetdb'

# These settings manage the various auto-deactivation and auto-purge settings
$node_ttl = '0'
$node_purge_ttl = '0'
$report_ttl = '7d'

@cprice404 cprice404 Feb 6, 2013

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been trying to put some thought into what our strategy is going to be for maintaining backward-compatibility with older versions of PuppetDB. Two of these settings are harmless on that front--though perhaps misleading. However, for the node-ttl setting we are not providing a way to define the older node-ttl-days setting here, which means we're exposing a setting that was available in 1.0 but not the means to actually set it.

I worry that the module is going to get really hairy if we try to maintain backward compatibility too far into the past, so I think my current inclination is to simply start documenting (very prominently--at the top of the main README or similar) that certain versions of the module are only compatible with certain versions of puppetdb. If you want to use an older version of puppetdb then you use an older version of the module.

Thoughts? @kbarber ?

@kbarber kbarber Feb 6, 2013

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, for the node-ttl setting we are not providing a way to define the older node-ttl-days setting here, which means we're exposing a setting that was available in 1.0 but not the means to actually set it.

If we can tell what version of the module they are using, its easy enough to make this conditional. If the semantics are the same the newer node-ttl option could set node-ttl-days or node-ttl conditionally. Question is, if we expose it and don't check - and yet we claim we support 1.0 in the module and someone uses it, what happens?

I worry that the module is going to get really hairy if we try to maintain backward compatibility too far into the past, so I think my current inclination is to simply start documenting (very prominently--at the top of the main README or similar) that certain versions of the module are only compatible with certain versions of puppetdb

@cprice-puppet I agree, we have to be clear on what version of puppetdb we support with the module. Its about being PuppetDB version focused ... not config option/feature focused.

Can we treat it like any feature and use semantic versioning and deprecation rules around this? Ie. 0.9 is now deprecated in say 2.0.5, next major release (3.0.0) its not supported any more. Keep it in line with how we support puppetdb itself would be useful, so if we are officially not releasing patches for 0.9 then the module gets deprecation messages and later unsupported?

@cprice404 cprice404 Feb 6, 2013

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If someone uses the setting in the module but is running PuppetDB 1.0.x, I'm about 99% sure that it would just silently ignore the setting. I'm pretty darn sure that our config parsing does not currently check for extra/unsupported key/val pairs in the config files (though I would prefer if it did).

We could certainly use semantic versioning and deprecation rules, and that might be the best way to go... but it's going to cause a bunch of spaghetti code in here that makes me tempted to just document the compatibility constraints and not hassle with it. The former would probably be a better end user experience but I'm not 100% sure how much payoff there would be.

And yes, we'd have to add a fact or something that would detect what version of PuppetDB was installed (if any). Actually, I don't know if it could be a fact because then it wouldn't work on the first run (before they'd installed the package initially)... unless we had some if/pick statements that were smart enough to distinguish between the installed version (or lack thereof), the latest version available (if they're using 'present'), or the user-specified version (if they pass the param).

@kbarber kbarber Feb 7, 2013

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another angle on this ... @cprice @nicklewis ... So we can almost know what the version is today without a fact, by looking at the $puppetdb_version. However, if it is set to 'present' in the past, we are kind of stuck as it could be anything today. If its 'latest' we know its - well, the latest so whatever the 'latest' supported at the time of module release should work. This all seems a little dodgy though, as I'm guessing the majority will just not define a specific version and let if fall back to 'present'. So yeah, today - no good way to detect version.

Maybe we should just add the old parameter as an option. Ie. Add the old parameter node-ttl-days right ... saying in docs its only supported on 1.0.0 and is now deprecated if you are using 1.1.0. The new parameter is also provided, and we mention in docs it was introduced in 1.1.0. Effectively we are mapping the config to the class parameters 1 to 1 ... which is probably a reasonable approach, and it means that users familiar with our config docs can just use our class without having to do a mental mapping somewhere.

We don't have a super-great way of obviously doing proper validation on this without being certain of the puppetdb version, seems to me if we return a deprecation message in PuppetDB itself on node-ttl-days then at least we are warning somewhere. Do we do this today for example? If not we probably should have, or at least allow node-ttl-days to work as intended until a 2.0 release, based on semver.

@cprice404 cprice404 Feb 7, 2013

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're totally correct that puppetdb should be logging a deprecation warning on node-ttl-days, and it's not. D'oh.

Here's another random idea to throw out there... we could tie the semver of the module to the semver of puppetdb itself... So, we could do what you said for now (add support for node-ttl-days to 1.1 of the module) but not put the purge setting in there, and then we could release version 1.2 of the module and add the purge setting with it. Then from here on out we could simply document that you need to use the same semver series of the module as the version of puppetdb you're running. It'd be a slight bending of the semver rules for the module for this release but I think it would probably end up matching up fairly well going forward, and would prevent us from having to maintain any spaghetti code in the module for trying to guess which version you're running.

Seems like this would work pretty intuitively for the 90% use-case (where the user is installing the module and adding 'ensure=>present' to their manifests at the same time). It would mean that you'd want/need to upgrade the module when you upgrade puppetdb, but that seems fairly reasonable I think...

Thoughts?

@cprice404 cprice404 Feb 7, 2013

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NLEW : any opinion?

@nicklewis nicklewis Feb 7, 2013

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern about that approach is it prevents us from using semver for the module itself. If we want to add new module features (rather than support for new PuppetDB features), we have no way of conveying that in the version number in a semver-compatible way. We could just do a point release, but since the forge is pretty strict on semver, that seems bad.

There's another issue, which I discussed with @cprice-puppet in person just now, which is that we have to add a variable and ini_setting for every new param now, even though it's just key-value. Some settings, like the database-related ones, have their own logic so this makes sense. But for ordinary settings, maybe it would serve users better to add some simple define for puppetdb_setting or somesuch, which just wraps ini_setting and sets the filename. Then users don't need to wait for us to get around to releasing a new version of the module, and then upgrade, to be able to take advantage of new settings. It also reduces the duplication we face now, of having to add the parameter in three places and document it in there more. The main disadvantage of this is that users have to refer to the central PuppetDB documentation for the list of available parameters, rather than the PuppetDB module docs. Personally, I think that's not so bad.

@kbarber what are your thoughts on this?

@cprice404 cprice404 Feb 7, 2013

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach seems reasonable to me too. Pro: module is more future-proof and we can use meaningful semver on it. Con: module documentation is not sufficient on its own to provide user with the info that they need to configure things... but as long as the module docs contain a link to the puppetdb docs I don't think that's too big of a deal.

@kbarber kbarber Feb 7, 2013

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to raise that concern earlier, I'm not a massive fan of the direct mapping from class/define parameter to config setting. Normally I have a defined resource for each setting - and have a class take a hash and use create_resources. This means you get class-time settings and later on users can add settings outside of the class if they desire.

Whatever the implementation is, like you say @nicklewis this means you can add features to puppetdb without having to have the module always keep up. 👍

@cprice404 cprice404 Feb 7, 2013

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we should close this pull, then, and open a new one that introduces a new defined resource type called puppetdb_setting, yes?


$puppetdb_version = 'present'

# TODO: figure out a way to make this not platform-specific
Expand Down
17 changes: 16 additions & 1 deletion manifests/server.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,19 @@
# (defaults to `puppetdb`; ignored for `embedded` db)
# ['database_name'] - The name of the database instance to connect to.
# (defaults to `puppetdb`; ignored for `embedded` db)
# ['database_package'] - The puppetdb package name in the package manager
# ['node_ttl'] - The length of time a node can go without receiving
# any new data before it's automatically deactivated.
# (defaults to '0', which disables auto-deactivation)
# This option is supported in PuppetDB >= 1.1.0.
# ['node_purge_ttl'] - The length of time a node can be deactivated before
# it's deleted from the database.
# (defaults to '0', which disables purging)
# This option is supported in PuppetDB >= 1.2.0.
# ['report_ttl'] - The length of time reports should be stored before
# being deleted.
# (defaults to '7d', which is a 7-day period)
# This option is supported in PuppetDB >= 1.1.0.
# ['puppetdb_package'] - The puppetdb package name in the package manager
# ['puppetdb_version'] - The version of the `puppetdb` package that should
# be installed. You may specify an explicit version
# number, 'present', or 'latest'. Defaults to
Expand Down Expand Up @@ -93,6 +105,9 @@
$database_username = $puppetdb::params::database_username,
$database_password = $puppetdb::params::database_password,
$database_name = $puppetdb::params::database_name,
$node_ttl = $puppetdb::params::node_ttl,
$node_purge_ttl = $puppetdb::params::node_purge_ttl,
$report_ttl = $puppetdb::params::report_ttl,
$puppetdb_package = $puppetdb::params::puppetdb_package,
$puppetdb_version = $puppetdb::params::puppetdb_version,
$puppetdb_service = $puppetdb::params::puppetdb_service,
Expand Down
15 changes: 15 additions & 0 deletions manifests/server/database_ini.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,4 +113,19 @@
setting => 'gc-interval',
value => $puppetdb::params::gc_interval,
}

ini_setting {'puppetdb_node_ttl':
setting => 'node-ttl',
value => $puppetdb::params::node_ttl,
}

ini_setting {'puppetdb_node_purge_ttl':
setting => 'node-purge-ttl',
value => $puppetdb::params::node_purge_ttl,
}

ini_setting {'puppetdb_report_ttl':
setting => 'report-ttl',
value => $puppetdb::params::report_ttl,
}
}