Uploaded image for project: 'Puppet'
  1. Puppet
  2. PUP-8009

Performance regression with lots of modules and gettext

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: PUP 5.3.1
    • Fix Version/s: PUP 5.3.2
    • Component/s: None
    • Labels:
      None
    • Template:
    • Team:
      Direct Change
    • Method Found:
      Needs Assessment
    • Release Notes:
      Bug Fix
    • Release Notes Summary:
      Hide
      Introduction of i18n for modules can cause significant performance regressions if environment caching is not used (i.e. environment_timeout==0), even if no translations from those modules are used. Introduce a disable_i18n flag to allow turning off translation functionality if not needed.
      Show
      Introduction of i18n for modules can cause significant performance regressions if environment caching is not used (i.e. environment_timeout==0), even if no translations from those modules are used. Introduce a disable_i18n flag to allow turning off translation functionality if not needed.
    • QA Risk Assessment:
      Needs Assessment

      Description

      We've seen a ~30% reduction in compilation capacity with Puppet 5.3 (over Puppet 4.10). It appears to be related to gettext setup at https://github.com/puppetlabs/puppet/blob/master/lib/puppet/gettext/config.rb#L37-L69.

      We haven't fully isolated why, but the best guess is that code related to accessing modules during compilation is being called too frequently, and resulting in lots of disk-related activity.

        Attachments

          Issue Links

            Activity

            Hide
            casey.williams Casey Williams added a comment -

            Had some puppet i18n test failures in the 5.3.2_release branch tonight after disabling gettext here, all like this:

            19:26:10 Errored Tests Cases:
            19:26:10   Test Case tests/i18n/check_puppet_run_message.rb reported: #<NoMethodError: undefined method `pending' for #<Beaker::TestCase:0x00000003528f70>>
            19:26:10     Test line: tests/i18n/check_puppet_run_message.rb:8:in `block (2 levels) in run_test'
            

            Show
            casey.williams Casey Williams added a comment - Had some puppet i18n test failures in the 5.3.2_release branch tonight after disabling gettext here , all like this: 19:26:10 Errored Tests Cases: 19:26:10 Test Case tests/i18n/check_puppet_run_message.rb reported: #<NoMethodError: undefined method `pending' for #<Beaker::TestCase:0x00000003528f70>> 19:26:10 Test line: tests/i18n/check_puppet_run_message.rb:8:in `block (2 levels) in run_test'
            Hide
            maggie Maggie Dreyer added a comment -

            Looks like we may not have skipped the beaker test the right way. I'll look into the correct way to do that.

            Show
            maggie Maggie Dreyer added a comment - Looks like we may not have skipped the beaker test the right way. I'll look into the correct way to do that.
            Hide
            maggie Maggie Dreyer added a comment -

            Update, fixed by Michael here: https://github.com/puppetlabs/puppet/pull/6253

            Show
            maggie Maggie Dreyer added a comment - Update, fixed by Michael here: https://github.com/puppetlabs/puppet/pull/6253
            Hide
            kenn Kenn Hussey added a comment -

            Michael Smith please provide release notes for this issue, thanks!

            Show
            kenn Kenn Hussey added a comment - Michael Smith please provide release notes for this issue, thanks!
            Hide
            rajasree Rajasree Talla added a comment -

            Michelle Fredette This is the issue we will need release notes for. This is a new fix to help performance degradation

            Show
            rajasree Rajasree Talla added a comment - Michelle Fredette This is the issue we will need release notes for. This is a new fix to help performance degradation
            Hide
            matthaus Past Haus added a comment -

            Michael Smith It seems like some investigation work to try to get a reproduction with just puppet and its dependencies might be worthwhile. That might allow us to create either a benchmark in puppet itself or some other modestly lightweight test that would prevent regressing on the issue (and also allow us to gather numbers from glisan, before the regression was introduced). There aren't any tickets around that yet are there?

            Show
            matthaus Past Haus added a comment - Michael Smith It seems like some investigation work to try to get a reproduction with just puppet and its dependencies might be worthwhile. That might allow us to create either a benchmark in puppet itself or some other modestly lightweight test that would prevent regressing on the issue (and also allow us to gather numbers from glisan, before the regression was introduced). There aren't any tickets around that yet are there?
            Hide
            michael.smith Michael Smith added a comment -

            Moses Mendoza added https://github.com/puppetlabs/puppet/pull/6266, which shows some degradation. Not sure if it's trying to compile with all those modules or just includes them in the modulepath, but this seems like the order of regression we've seen (just that it applies to all requests that use modulepath, like file and metadata requests).

            Show
            michael.smith Michael Smith added a comment - Moses Mendoza added https://github.com/puppetlabs/puppet/pull/6266 , which shows some degradation. Not sure if it's trying to compile with all those modules or just includes them in the modulepath, but this seems like the order of regression we've seen (just that it applies to all requests that use modulepath, like file and metadata requests).
            Hide
            matthaus Past Haus added a comment -

            Ah awesome, I wasn't aware of that PR. Seems like it would be worth capturing some data from 4.10 puppet so we have a target to aim for with i18n enabled them?

            Show
            matthaus Past Haus added a comment - Ah awesome, I wasn't aware of that PR. Seems like it would be worth capturing some data from 4.10 puppet so we have a target to aim for with i18n enabled them?
            Hide
            michelle.fredette Michelle Fredette added a comment -

            Melissa Amos is going to create.

            Show
            michelle.fredette Michelle Fredette added a comment - Melissa Amos is going to create.
            Hide
            melissa.amos Melissa Amos added a comment -

            I'm logging PE-related docs updates in PE-22416. Unassigning myself here.

            Show
            melissa.amos Melissa Amos added a comment - I'm logging PE-related docs updates in PE-22416. Unassigning myself here.

              People

              • Assignee:
                Unassigned
                Reporter:
                michael.smith Michael Smith
              • Votes:
                0 Vote for this issue
                Watchers:
                13 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Zendesk Support