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

(PUP-4530) FreeBSD specific service provider fix #3913

Closed
wants to merge 70 commits into from
Closed

(PUP-4530) FreeBSD specific service provider fix #3913

wants to merge 70 commits into from

Conversation

infracaninophile
Copy link
Contributor

Ticket ref: PUP-4530

nelhage and others added 30 commits January 30, 2015 11:16
I profiled `puppet agent --test` on one of my servers using stackprof
[1] on Ruby 2.1.

Something like 30% or more of the time was spent in
`Puppet::Graph::SimpleGraph#upstream_from_vertex`, called from
`Puppet::Transaction#failed_dependencies?`

It turns out that, while evaluating the resource graph, when considering
a resource, we look at *the complete set of transitive dependencies* to
evaluate whether any of them have failed. This is hugely expensive, and
moreover, is wasted work in the success case where no resources fail.

Instead of all that work, this patch pushes the work to the *failed*
nodes; When a node fails, we transitively walk the dependents of that
node, and mark them as having failed dependencies, and then check that
flag directly when considering whether to skip a node later.

On my test system, this patch drops `puppet agent --test` runtime from
about 40s to about 23s.

[1] https://github.com/tmm1/stackprof
This fixes behavior with dynamically-generated resources, which would
previously not exist during the mark_failed walk, and thus not get
flagged as having failed dependencies.

Add a test case exhibiting the desired behavior, which failed on the
previous commit.
This is to allow it to be shared between the agent and other
applications.
Restore reporting of *which* dependencies have failed when we skip a
resource due to failed dependencies.
This updates the systemd service type to add support for masking. If a
service is masked, it is deemed to also be disabled. If a service is
masked and changed to enabled, it will first be unmasked since the
standard 'systemctl enable' command does not properly unmask the command
first.

The check against the delcared feature is in place so that users do not
have update their code due to differences between systemd and
non-systemd systems.
(PUP-1253) Add systemd service masking support
This change allows puppet to respect the no_proxy ENV var when set on the system.  Previously, Puppet would respect the http(s)_proxy ENV var when connecting HTTP services (e.g. puppetforge) but ignore the no_proxy setting.

Documentation is a bit inconsistent with regards to specifying domain level exception.  WGET docs for example, state that the no_proxy list should be of suffixes and thus an entry with a preceeding '.' (e.g. .example.com) creates an exception for any host under that domain.  Curl man pages and other intenet resources, suggest the use of a '*' as a wildcard.

This change supports both notations.
This change adds an acceptance test to ensure that the 3x forbidden
environment names now work, and continue to work in 4x.
There are 4 previously forbidden names, which means this test has to run
puppet 5 times, unfortunately.
…ceptance_test_that_3_x_forbidden_env_names_work_in_4_x

(PUP-4413) Add acceptance test that 3x forbidden env names work in 4x
The apply application works by overriding the :current_environment with the
file that was specified on the command line. This had no effect when the node
object (from the ENC) specified an environment of its own.

Remedy this by looking up the node before the environment, favoring its
environment over the configured value and overriding the :manifest of the
chosen one.
It was decided that (for the time being) puppet apply should behave
like agent/master when an environment is configured (or specified via
command line argument) and prefer the environment that was returned
by the ENC. A warning to that effect should be displayed to the user.

(The warning is not added through this branch - we had this already.)
(PUP-4517) Add type name to Puppet.newtype deprecation warning
This commit adds a launchd plist for use with OSX AIO packaging.
(PUP-4532) Add launchd plist for use with OSX AIO
Previously, the test was joining the log messages with newlines, and
then testing if the error message matched.

Rspec 3 matchers are composable, so we can assert more directly that
the error message matches one of the log entries.
Rather than rely on executing ruby to manipulate a file to "emulate" a
service start/status/stop command, stub command execution itself to
ensure the base service provider is executing the commands as expected.
@puppetcla
Copy link

Waiting for CLA signature by @infracaninophile

@infracaninophile - We require a Contributor License Agreement (CLA) for people who contribute to Puppet, but we have an easy click-through license with instructions, which is available at https://cla.puppetlabs.com/

Note: if your contribution is trivial and you think it may be exempt from the CLA, please post a short reply to this comment with details. http://docs.puppetlabs.com/community/trivial_patch_exemption.html

@puppetcla
Copy link

CLA signed by all contributors.

@peterhuene
Copy link
Contributor

Hi @infracaninophile. Thanks for the contribution! Before we can consider merging this, could you amend your commit message to be the expected format of "(PUP-4530)" per our contributing guidelines?

Also, it appears that the AppVeyor failure is a transient issue that may be fixed by rebasing; speaking of, it's generally a good idea to do work in a branch other than master to make rebasing easier.

Eric Thompson and others added 4 commits May 11, 2015 17:04
This change adds a test for all parser functions.
This test calls all the parser functions that ship with puppet.  It
ensures sane output in most cases.  The remainder of the coverage for
functions lies in the spec tests.
Take just the leading string of non-whitespace characters
from the first line of the rcvar output as the service name
@infracaninophile
Copy link
Contributor Author

Commit message amended as requested. Sorry about not working in a branch.

@peterhuene peterhuene changed the title FreeBSD specific service provider fix (PUP-4530) FreeBSD specific service provider fix May 12, 2015
@melissa melissa added the Unix label May 12, 2015
peterhuene and others added 2 commits May 12, 2015 11:36
(PUP-4363) Support splay in apply as well
…alling_all_parser_functions

(PUP-4342) test calling all parser functions; failing spec tests are unrelated to this commit.
@peterhuene
Copy link
Contributor

Hi @infracaninophile. Thanks for pushing up the changes! It looks like the original commit wasn't amended properly, though, as there are now two commits for the same thing in the PR. Additionally, there shouldn't be any merge commits in the history.

I would do a git pull --rebase upstream master to rebase. This should likely get rid of that merge commit. From there, a git rebase -i can be used to discard one of the two identical commits.

There's one last thing I should have mentioned earlier: could you reword the commit title (either with git rebase -i or git commit --amend) like (PUP-4530) Fix FreeBSD service provider to parse service names correctly or something along those lines? It may seem a bit pedantic, but we prefer seeing commit messages in a sort of "imperative" form.

With the above fixed up, we'll get this merged in. Cheers!

Eric Thompson and others added 9 commits May 12, 2015 16:13
this change removes an acceptance test which only exercises an apply
manifest using the File resource which is even better tested elsewhere
Commit e1c8724 resolved a conflict
in defaults.rb that re-introduced the stringify_facts,
trusted_node_data, and immutable_node_data settings that were removed in
4.0.

This commit removes them again.
…urious_test

(maint) remove spurious test
This will prevent gem upgrade on environments with ruby 1.8.7
(maint) Fix bad merge from 3.x branch.
(PUP-4601) Add ruby version requirement to .gemspec.
* upstream/stable:
  (PUP-4436) Generate error message in eventlog differently
Take just the leading string of non-whitespace characters from the
first line of the rcvar output as the service name
@infracaninophile
Copy link
Contributor Author

Did that do what you wanted? This is getting beyond what git-usage I understand.

@peterhuene
Copy link
Contributor

@infracaninophile Unfortunately the rebase wasn't performed correctly as now there's a ton of commits in your branch that the PR is attempting to merge (there should be only 1). There shouldn't be any merge commits from your master branch in your commit log (commit 3b9b43b should be deleted). git pull --rebase upstream master should have pulled puppetlabs' master into yours and then replayed any commits that are local to your branch (preferably only commit d390578).

If you like, I can put your commit up in a different PR (your commit goes in as-is), but unfortunately GitHub doesn't allow me to fix this PR in that way. Thanks!

@infracaninophile
Copy link
Contributor Author

If you like, I can put your commit up in a different PR (your commit goes in as-is), but unfortunately
GitHub doesn't allow me to fix this PR in that way. Thanks!

That would probably be the best.

@peterhuene
Copy link
Contributor

@infracaninophile Thanks again for the contribution! I've opened PR #3928 with just your commit. I'm going to close this PR in favor of that one.

@peterhuene peterhuene closed this May 13, 2015
@puppetcla
Copy link

Waiting for CLA signatures by @er0ck, @trevor-vaughan

@er0ck, @trevor-vaughan - We require a Contributor License Agreement (CLA) for people who contribute to Puppet, but we have an easy click-through license with instructions, which is available at https://cla.puppetlabs.com/

Note: if your contribution is trivial and you think it may be exempt from the CLA, please post a short reply to this comment with details. http://docs.puppetlabs.com/community/trivial_patch_exemption.html

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.

None yet