Feature #1241

Improve performance of group resource lookups.

Added by Redmine Admin over 3 years ago. Updated almost 2 years ago.

Status:Closed Start date:
Priority:Normal Due date:
Assignee:Luke Kanies % Done:

0%
Category:unknown
Target version:0.24.8
Affected Puppet version:0.22.1 Branch:
Keywords:
Votes: 0

Description

Due to the libc bug Luke made me aware of, Puppet changed somewhere after 0.22.4 to poll all groups on the system rather than querying specific ones.

This patch does some rearranging of the user/group types and posix utils so that the “fast path” of querying a specific user or group is done first, then a reverse lookup is done to check the validity of the result. Only if this reverse lookup produces inconsistent results does it then go through the polling method.

Also some changes to the directoryservice provider were needed to make Mac OS X pass the POSIX unit tests when bound to a remote directory service node. The provider now uses the /Search node (all local and remote nodes) for all querying operations, but defaults to the local node when performing destructive operations.

Two successive commits on github to the 0.24.x branch, the latter after chatting to JJM and realizing the implications of the node change:

http://github.com/nigelkersten/puppet/commit/695f174eb1d228d382aee2be6afcb655d4c31318

http://github.com/nigelkersten/puppet/commit/c1a6adb84ecdcc1c2a2657b4d603c96ae3282125

History

#1
Updated by James Turnbull over 3 years ago

Nigel

Both these commits are based on older code. Can you pull and rebase and re-submit the patches. Thanks.

#2
Updated by Redmine Admin over 3 years ago

  • Status changed from 1 to Needs More Information
  • 6 changed from Needs more info to Needs more information

#3
Updated by Luke Kanies over 3 years ago

  • Status changed from Needs More Information to Accepted
  • Assignee changed from Redmine Admin to Luke Kanies
  • Target version set to 0.24.5

I’d like to see these fixes make it into 0.24.5.

We can always rebase them ourselves if Nigel’s not available. I’ll take point on it, I guess.

#4
Updated by Luke Kanies over 3 years ago

I applied these patches and ran the tests, and got multiple failures that appear to be directly related. E.g.,

  5) Failure:
test_allusermodelproperties(TestUser)
    [./ral/type/user.rb:202:in `attrtest_gid'
     ./ral/type/user.rb:445:in `send'
     ./ral/type/user.rb:445:in `test_allusermodelproperties'
     ./ral/type/user.rb:443:in `each'
     ./ral/type/user.rb:443:in `test_allusermodelproperties'
     /Users/luke/git/puppet/lib/../vendor/gems/mocha-0.5.6/lib/mocha/test_case_adapter.rb:19:in `__send__'
     /Users/luke/git/puppet/lib/../vendor/gems/mocha-0.5.6/lib/mocha/test_case_adapter.rb:19:in `run']:
Failed to specify group by id for 4294967295.
Exception raised:
Class: 
Message: <"Munging failed for value [4294967295] in class gid: Did not get id">
---Backtrace---
/Users/luke/git/puppet/lib/puppet/util/posix.rb:9:in `get_posix_field'
/Users/luke/git/puppet/lib/puppet/util/posix.rb:137:in `gid'
/Users/luke/git/puppet/lib/puppet/type/user.rb:120:in `unsafe_munge'
/Users/luke/git/puppet/lib/puppet/parameter.rb:107:in `munge'
/Users/luke/git/puppet/lib/puppet/property.rb:401:in `should='
/Users/luke/git/puppet/lib/puppet/property.rb:400:in `collect'
/Users/luke/git/puppet/lib/puppet/property.rb:400:in `should='
/Users/luke/git/puppet/lib/puppet/property.rb:447:in `value='
/Users/luke/git/puppet/lib/puppet/metatype/attributes.rb:506:in `[]='
./ral/type/user.rb:203:in `attrtest_gid'
./ral/type/user.rb:202:in `attrtest_gid'
./ral/type/user.rb:445:in `send'
./ral/type/user.rb:445:in `test_allusermodelproperties'
./ral/type/user.rb:443:in `each'
./ral/type/user.rb:443:in `test_allusermodelproperties'
/Users/luke/git/puppet/lib/../vendor/gems/mocha-0.5.6/lib/mocha/test_case_adapter.rb:19:in `__send__'
/Users/luke/git/puppet/lib/../vendor/gems/mocha-0.5.6/lib/mocha/test_case_adapter.rb:19:in `run'
---------------

#5
Updated by Luke Kanies over 3 years ago

  • Target version changed from 0.24.5 to 4

If the patches can be fixed, I’ll still try to get it into 0.24.5; otherwise, I’m putting it off.

#6
Updated by Nigel Kersten over 3 years ago

Sorry, got distracted for a while.

The posixtest actually fails out of the box for me with no patches applied, I’ll work on that then re-do this patch. Against the 0.2.4.x/head branch I take it?

#7
Updated by Luke Kanies over 3 years ago

Yes, 0.24.x, please.

#8
Updated by Nigel Kersten over 3 years ago

ok. This should be it.

http://github.com/nigelkersten/puppetmaster/commit/399e2a954b31250b254177cc8325f41f071883e1

Note that I modified the posixtest.rb test suite to no longer check that the string version of a numeric uid/gid is handled correctly as that’s all now done in the gid() and uid() methods. I did check, and no other code directly calls the get_posix_field methods.

I also switched from the regexp match /\d+$/ to attempting to make an integer inside a begin/rescue block as I don’t think that was doing what it was intended to do with unquoted numeric identifiers in puppet syntax unless I’ve missed something.

$ irb

puts “match” if “1234” =~ /\d+$/ match => nil puts “match” if 1234 =~ /\d+$/ => nil

All the unit tests pass on the linux boxes I’ve checked, and I see the same behavior on OS X as I do before this patch, which is that the posixtest.rb suite only passes if I’m running as a local user, and not as a local user who is a member of LDAP groups. I’ll poke into that in a separate changelist.

#9
Updated by Luke Kanies over 3 years ago

  • Status changed from Accepted to Ready For Checkin

#10
Updated by James Turnbull over 3 years ago

  • Status changed from Ready For Checkin to Closed
  • Patch changed from Insufficient to Code

Pushed in commit:97987a705da7b8126569b1f5b7c3676ad0220f66 in branch 0.24.x

#11
Updated by James Turnbull over 2 years ago

  • Target version changed from 4 to 0.24.8
  • Affected Puppet version set to 0.22.1

Also available in: Atom PDF