puppetlabs / puppetlabs-node_manager

Create and manage PE node groups as resources.
Apache License 2.0
10 stars 21 forks source link

Add utilites to enable Bolt Plan usage #49

Closed reidmv closed 3 years ago

reidmv commented 4 years ago

Because Bolt operates Puppet out of a temporary config dir, it is necessary to dynamically build a node_manager.yaml config file in order to successfully use node_group types in a Bolt Plan apply block. This commit provides:

For using node_manager with Bolt Plans.

reidmv commented 4 years ago

@WhatsARanjit here's the gist of what I found to be necessary to successfully use node_group types with Bolt Plan apply blocks:

apply($master_target) {
  file { 'node_manager.yaml':
    ensure   => file,
    mode     => '0644',
    path     => Deferred('node_manager::config_path'),
    content  => epp('node_manager/node_manager.yaml.epp', {
      server => $master_certname,
    }),
  }

  node_group { 'Example group':
    ensure  => present,
    parent  => 'All Nodes',
    require => File['node_manager.yaml'],
  }
}

Opening this PR to provide the necessary utility function and a simple template to make it cookie-cutter to do using the node_manager module only, without having to implement the function and template yourself.

reidmv commented 4 years ago

@WhatsARanjit ^^

WhatsARanjit commented 4 years ago

@reidmv The function: is there a use case to generate content for a node_manager.yaml on the fly outside of the Bolt use case? The template: I'd rather have a template that is very parameterized if this is to span all use cases. Does it work?: Please supply some testing to support your new code.

At first glance, it seems like my module adheres to the Classifier API and your PR is more an implementation-oriented thing. I'd love to see an addition to WhatsARanjit/puppeteer for an implementation task.

reidmv commented 4 years ago

The function: is there a use case to generate content for a node_manager.yaml on the fly outside of the Bolt use case?

To my knowledge, no, there is not. Generation on-the-fly is required purely because A) puppet is effectively invoked with a non-standard --confdir option by Bolt, and B) the value passed to --confdir is impossible to predict.

The template: I'd rather have a template that is very parameterized if this is to span all use cases.

The template is intentionally an 80/20 rule template. Rather than providing for all likely use cases, it provides for the use case that is likely to apply 80% of the time (or better). When it doesn't apply, it is trivial for a user to make their own.

It could certainly be improved but the point of 80/20 is that it's probably not worth the investment, and in any case it would be speculative to guess at what is useful without real-world input. The template as-is matches every use case I am personally aware of. Incremental improvement based on user feedback would be the appropriate path to betterment.

Does it work?: Please supply some testing to support your new code.

It works. The initial implementation was performed as a non-reusable code embedded in a separate module, here. This PR is aimed at shipping the required primitive(s) in the node_manager module itself so that each consuming module does not have to re-implement the critical function, node_manager::config_path().

If the design gets a 👍 I will consider allocating some time for writing tests. I am not going to invest effort in that prior to a decision.

At first glance, it seems like my module adheres to the Classifier API and your PR is more an implementation-oriented thing. I'd love to see an addition to WhatsARanjit/puppeteer for an implementation task.

Implementation could be placed in WhatsARanjit/puppeteer. If that were the case, the critical function would be called something like puppeteer::node_manager_yaml_location(). The template then becomes puppeteer/node_manager.yaml.epp.

IMO, this doesn't feel any cleaner than providing the needed primitives directly in the only module they would be useful for, and it adds an otherwise unnecessary dependency. It is possible to do it that way though.

To be clear: this is not task content. This is desired state Puppet DSL content. It is used inside apply() blocks in plans.

Thoughts?

reidmv commented 4 years ago

For reference: see related conversation thread in #43. That terminated with:

@WhatsARanjit the node_manager.yaml file is probably a decent workaround. In a Bolt plan a user could first lay down such a file, before invoking any Node_group resources.

I'll close this PR. Thanks!

...it turned out however that laying down a node_manager.yaml file that works with Bolt isn't possible without something like Deferred('node_manager::config_path').

WhatsARanjit commented 4 years ago

I get what the problem statement is. I'm just trying to avoid Bolt-specific implementation things in the module. If you're trying to resolve to $settings::confdir in a function, that exists with stdlib's getvar("settings::confdir"). I don't yet see the reason for a node_manager-specific function. Thanks.

reidmv commented 4 years ago

I hadn't thought of using Deferred('getvar', ...). That's a great idea!

After some testing it looks like while something like things like facts.os.family can be returned by getvar() in a Deferred(), values from settings::* cannot. This means that a fact returning the Puppet confdir is required.

The puppetlabs-puppet_agent module provides a puppet_confdir fact. That is Yet Another Dependency™ but I guess not a particularly onerous one. The call would then become

path => Deferred('getvar', ['facts.puppet_confdir'])

The last missing piece then would be a template node_manager.yaml to put there.

Is it your stance that it should be user responsibility, or 3rd module responsibility, to provide a template node_manager.yaml?

Instructions for using node_manager with Bolt then become:

  1. Add dependency module puppetlabs-puppet_agent to your project

  2. IF the template ???/node_manager.yaml.epp isn't part of WhatsARanjit-node_manager, either create a template to use, or include another dependency module to supply it

  3. Paste this code at the top of the apply() block containing node_group resources:

    file { 'node_manager.yaml':
      ensure   => file,
      mode     => '0644',
      path     => Deferred('getvar', ['facts.puppet_confdir']),
      content  => epp('???/node_manager.yaml.epp', {
        server => $master_certname,
      }),
    }
  4. Add require => File['node_manager.yaml' to each node_group resource in the apply() block.

What do you think: is that more aligned with your vision for how this should work?

reidmv commented 3 years ago

Closing due to inactivity. @WhatsARanjit still curious your thoughts on the last comment.