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

fixes #14447 - add cache around parser using serialised YAML file #135

Merged
merged 1 commit into from Apr 11, 2016

Conversation

domcleal
Copy link
Contributor

@domcleal domcleal commented Apr 4, 2016

A cache of parsed modules can be configured with :parser_cache_path to
skip calls out to kafo_parsers when loading Puppet modules. This helps
when no parser is available for the current Puppet installation (e.g.
under Puppet 4 with AIO at the time of writing), and may provide a small
peformance benefit.

The cache can be generated with kafo-export-params -f parsercache --no-parser-cache.

The cache is skipped if the mtime of the manifest is greater than the
mtime when the cache was generated, causing Kafo to use kafo_parsers as
normal.


This is my suggested implementation of the third JSON-based parser mentioned at https://groups.google.com/d/msg/foreman-dev/Ijvodu1hJkE/Ssl83t35IAAJ and in theforeman/kafo_parsers#13.

I opted to put it in kafo rather than as a new parser in kafo_parsers for a few reasons:

  • Kafo already had kafo-export-params, which I thought would be the perfect tool to extend to generate the YAML files. It already handles loading the configuration, output formats etc, so adding a new output format for the cache was trivial.
  • If extending kafo-export-params to write the cache, it made sense to keep the cache reader and writer in the same project so it's easy to keep in sync.
  • The cache location can be set in the Kafo configuration, which is hard to access from kafo_parsers right now.

This isn't everything for AIO support. The validations still require loading Puppet, but I will look at replacing Puppet functions with built in validations, and paths to puppet are not automatic.

This does delay the requirement for a puppet-strings parser, as we can generate the parser cache on the current Puppet 3 installation at build time and use it with a Puppet 4 AIO installation. It also means users won't have to install puppet-strings. We will still want a puppet-strings parser so we're not limited to building on Puppet 3.

@ares
Copy link
Member

ares commented Apr 4, 2016

From quickscan looks good, I'll get to proper review and testing later. I agree that it nicely fits into Kafo core, we should be able to generate the same format using puppet-strings parser in kafo-export-params later.

Would you mind opening a redmine issue and put all information you've mentioned there? I find it better place for keeping information, it's easier to find it later and associate with other issues.

@domcleal domcleal changed the title Add cache around parser using serialised YAML file fixes #14447 - add cache around parser using serialised YAML file Apr 4, 2016
@domcleal
Copy link
Contributor Author

domcleal commented Apr 4, 2016

we should be able to generate the same format using puppet-strings parser in kafo-export-params later

Yes, any parser in kafo_parsers that generates the standard format should be fine and work in exactly the same way. I'll probably look at the item on the puppet-strings PR to select available/ready parsers automatically.

Would you mind opening a redmine issue and put all information you've mentioned there?

Sure, done: http://projects.theforeman.org/issues/14447

@domcleal domcleal force-pushed the parser-cache branch 2 times, most recently from e7d96c3 to c7bbb62 Compare April 4, 2016 14:56
@domcleal
Copy link
Contributor Author

domcleal commented Apr 4, 2016

I made a small update to change the serialised mtime to an integer (seconds since epoch) rather than the serialised Time object. When deserialised and compared before, it was using floats and so the comparisons were unreliable causing the cache to be ignored.

@domcleal domcleal force-pushed the parser-cache branch 2 times, most recently from 3b55e0b to 78337c2 Compare April 5, 2016 09:17
@domcleal
Copy link
Contributor Author

domcleal commented Apr 5, 2016

Rebased on master.

return nil
end

new(parsed)
Copy link
Member

Choose a reason for hiding this comment

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

would you mind adding logger.debug "Using #{cache_path} cache with parsed modules" in here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, added.

@ares
Copy link
Member

ares commented Apr 11, 2016

I needed unrelated fix #141 in order to test it. Except one small comment it seems to work nicely.

@ares
Copy link
Member

ares commented Apr 11, 2016

@domcleal sorry, it seems it needs another rebase

A cache of parsed modules can be configured with `:parser_cache_path` to
skip calls out to kafo_parsers when loading Puppet modules. This helps
when no parser is available for the current Puppet installation (e.g.
under Puppet 4 with AIO at the time of writing), and may provide a small
peformance benefit.

The cache can be generated with `kafo-export-params -f parsercache
--no-parser-cache`.

The cache is skipped if the mtime of the manifest is greater than the
mtime when the cache was generated, causing Kafo to use kafo_parsers as
normal.
@domcleal
Copy link
Contributor Author

Thanks, done.

@ares
Copy link
Member

ares commented Apr 11, 2016

Great addition, thanks @domcleal! Merging.

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