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

WIP/RFC: Add run_agent task #709

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

alexjfisher
Copy link
Contributor

No description provided.

tasks/run_agent.rb Outdated Show resolved Hide resolved
@@ -0,0 +1,160 @@
#!/opt/puppetlabs/puppet/bin/ruby
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems to be the most sensible default. Non AIO users can override this in their bolt configuration with interpreters.

require 'etc'
require 'json'
require 'yaml'
require_relative '../../ruby_task_helper/files/task_helper.rb'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I add puppetlabs/ruby_task_helper to the metadata.json?
As far as the module is concerned, it's a 'soft' dependency. But I also don't envisage a module namespace clash, so adding it to the metadata might be more helpful to users.

require 'yaml'
require_relative '../../ruby_task_helper/files/task_helper.rb'

class RunAgentTask < TaskHelper
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No idea what to use for the class name. The docs aren't especially helpful.

if agent_locked?
raise TaskHelper::Error.new(
'Lockfile still exists after waiting',
'theforeman-puppet/lockfile_timeout_expired'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

)
end

waited
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think I'm actually using this returned value at the moment. I suppose I could bung it into the response??

cmd << if @noop
'--noop'
else
'--no-noop'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to override the value in the config file. I should probably make this clearer in whatever docs I write or in the task metadata.

case status.exitstatus
when 0
{
result: 'The run succeeded with no changes or failures; the system was already in the desired state (or --noop was used).',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of this text was taken from the puppet docs. I added the bit about noop myself. Maybe I should open a PR to fix this upstream ;)

@runejuhl
Copy link

Quoting myself from https://gist.github.com/alexjfisher/753c61e09e891c9c9336ba0522b581a4#gistcomment-3057265

If you plan on using this not just for development you should probably avoid using -t (--test):

--test

Enable the most common options used for testing. These are 'onetime', 'verbose', 'no-daemonize', 'no-usecacheonfailure', 'detailed-exitcodes', 'no-splay', and 'show_diff'.

https://puppet.com/docs/puppet/latest/man/agent.html

Specifically you don't want the no-usecacheonfailure:

usecacheonfailure

Whether to use the cached configuration when the remote configuration will not compile. This option is useful for testing new configurations, where you want to fix the broken configuration rather than reverting to a known-good one.

Default: true

https://puppet.com/docs/puppet/latest/configuration.html#usecacheonfailure

That is, if you use -t/--test you implicitly set no-usecacheonfailure, which usually isn't what you want for a production run.

end

def run_puppet
cmd = [@puppet_bin, 'agent', '-t', '--detailed-exitcodes']

Choose a reason for hiding this comment

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

Sorry, with #709 (comment) I was of course referring to this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For a manual run (as opposed to a normal scheduled run), I think I'd personally never want to accidentally be using the cache. I do need a way of making this and other options configurable though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

--detailed-exitcodes is redundant here though (already included in -t).

Choose a reason for hiding this comment

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

Yes, agreed, having a simple toggle for --test (which doesn't use cache) and --no-daemonize --onetime --no-splay --detailed-exitcodes (which would use cache) would be nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you override options set with --test by setting conflicting arguments?
eg does

puppet agent -t --usecacheonfailure

work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

answering my own question... Apparently not.

Choose a reason for hiding this comment

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

Seems like it:

$ puppet apply --noop -e 'file { "/tmp/omgtest": ensure => file, }'
Notice: Compiled catalog for managua.lan in environment production in 0.01 seconds
Notice: /Stage[main]/Main/File[/tmp/omgtest]/ensure: current_value 'absent', should be 'file' (noop)
Notice: Class[Main]: Would have triggered 'refresh' from 1 event
Notice: Stage[main]: Would have triggered 'refresh' from 1 event
Notice: Applied catalog in 0.02 seconds

$ puppet apply --noop -e 'file { "/tmp/omgtest": ensure => file, }' --no-noop
Notice: Compiled catalog for managua.lan in environment production in 0.01 seconds
Notice: /Stage[main]/Main/File[/tmp/omgtest]/ensure: created
Notice: Applied catalog in 0.01 seconds

And yes, it'd be much better to use the default --test.

Choose a reason for hiding this comment

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

Seems to work just fine with puppet apply. Last argument takes precedence:

$ puppet apply --no-noop -e 'file { "/tmp/omgtest": ensure => file, }' --noop
Notice: Compiled catalog for managua.lan in environment production in 0.01 seconds
Notice: /Stage[main]/Main/File[/tmp/omgtest]/ensure: current_value 'absent', should be 'file' (noop)
Notice: Class[Main]: Would have triggered 'refresh' from 1 event
Notice: Stage[main]: Would have triggered 'refresh' from 1 event
Notice: Applied catalog in 0.01 seconds

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went for puppet agent -t --splay and the run didn't wait a random amount of time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@runejuhl I've added a puppet_settings variable. It's an optional hash where you can override the default settings.

@alexjfisher alexjfisher force-pushed the run_agent_task branch 8 times, most recently from e6687f8 to 9182626 Compare October 23, 2019 13:27
extra:
- gem: bolt
version: '~> 1.15'
# TODO: Add something to foreman-installer-modulesync to support `unless ENV['PUPPET_VERSION'] =~ /^5/`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ekohl a raw_append option??

@ekohl
Copy link
Member

ekohl commented Apr 9, 2020

@alexjfisher what do we want to do with this? I don't have bolt or the capability to maintain this. Would you be interested in becoming a co-maintainer for the module?

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

Successfully merging this pull request may close these issues.

5 participants