The Puppet Labs Issue Tracker has Moved: https://tickets.puppetlabs.com
https://tickets.puppetlabs.com
. See the following page for information on filing tickets with JIRA:
Bug #5605
Prefetch doesnt work when resourcetype uses composite keys
Status: | Needs More Information | Start date: | 12/19/2010 | |
---|---|---|---|---|
Priority: | Normal | Due date: | ||
Assignee: | - | % Done: | 0% | |
Category: | transactions | |||
Target version: | - | |||
Affected Puppet version: | 3.2.3 | Branch: | https://github.com/stschulte/puppet/tree/ticket/2.6.x/5605 | |
Keywords: | transaction, prefetch, provider | |||
We've Moved!Ticket tracking is now hosted in JIRA: https://tickets.puppetlabs.com |
Description
One can create types with multiple keyattributes by using isnamevar multiple times. (Example: Port in /etc/services with the keyattributes name and protocol).
Unfortunately prefetching of these types is broken in transaction.rb
def prefetch
prefetchers = {}
@catalog.vertices.each do |resource|
if provider = resource.provider and provider.class.respond_to?(:prefetch)
prefetchers[provider.class] ||= {}
prefetchers[provider.class][resource.name] = resource
end
end
[...]
end
Because resource.name
is not uniq anymore we will eventually overwrite existing resources in the prefetch-hash and thus not all resources are passed to the prefetch method of the underlying provider. So if the user defined the following manifest:
port { 'telnet:tcp': name => 'telnet', protocol => 'tcp', number => '23'}
port { 'telnet:udp': name => 'telnet', protocol => 'udp', number => '23'}
The providers defined def self.prefetch(resources)
will only get resources['telnet'] = <second resource with protocol udp>
I have the following suggestions:
- use
resource.uniqueness_key
instead ofresource.name
.uniqueness_key
is an array of all the key_attributes. This can only work if you can reliably use arrays as hashkeys. I don’t know if thats true for all ruby versions. This solution will probably break every existing provider who uses prefetch - same as firstone except use uniqueness_key[0].intern if we only have one keyattribute. This solution will hopefully not break existing providers
- use
prefetchers[provider.class][resource.name] << resource
so we will not overwrite but build an array if name is not uniq. - build a nested hash, so in my port example we whould get
prefetchers[provider.class][resource[:name]][resource[:protocol]]
Related issues
History
#1
Updated by James Turnbull over 5 years ago
- Status changed from Unreviewed to Accepted
- Assignee set to Stefan Schulte
- Target version set to 2.7.x
- Affected Puppet version set to 2.6.0
Stefan – I’d just submit a patch with the preferred approach.
#2
Updated by Stefan Schulte over 5 years ago
- Branch set to https://github.com/stschulte/puppet/tree/ticket/2.6.x/5605
Ok I have created a patch that solved the issues I’ve been having while building #5660 and it doesn’t break existing specs.
Comments are in the commitmessage
#3
Updated by James Turnbull over 5 years ago
- Status changed from Accepted to In Topic Branch Pending Review
#4
Updated by Jesse Wolfe about 5 years ago
- Status changed from In Topic Branch Pending Review to Code Insufficient
I’d strongly prefer using uniqueness_key instead of trying to use .name at all. That’s what it’s there for.
#5
Updated by Stefan Schulte about 5 years ago
I’d strongly prefer using uniqueness_key instead of trying to use .name at all. That’s what it’s there for.
I am using uniqueness key to build the prefetch hash. But to not break current providers and prefetch methods, uniqueness_key is the namevar if we only have one keyattribute. Otherwise it is an array of all keyattributes.
At least it fixed the problem with #5660 but you’re right, it is kind of ugly. In my opinion
:name
shouldnt be automatically translated to any other property. (Dropping the whole concept of isnamevar)isnamevar
should be renamed toiskeyattribute
.- uniqueness_key should always return an array of all keyattributes. Even if we have just one.
- to identify a resource puppet should always use uniqueness_key. Never resource[:name]
I actually started with a refactor but it has been quite a while since I last looked at it. But does this approach sound saner to you?
#6
Updated by Jesse Wolfe about 5 years ago
- Status changed from Code Insufficient to Investigating
Stefan, you’re 100% right – that would be the ideal situation.
I’m going to spend some time today trying to see if I can get prefetch to use uniqueness_key – I started looking into it yesterday, and it wasn’t as straightforward as I imagined.
If I can’t get it working, I’ll merge in your original patch – better to have something that works as a stopgap than to leave the system broken.
#7
Updated by Stefan Schulte about 5 years ago
I just want to point out: If you’re leaving the implementation of uniqueness_key
as it is right now (before my change) there is a problem when uniqueness_key triggers the retrieve
method before we can retrieve anything (I described the problem in #5610)
#8
Updated by Jesse Wolfe about 5 years ago
Yikes. Thanks for the warning.
#9
Updated by Jesse Wolfe about 5 years ago
- Status changed from Investigating to Code Insufficient
- Assignee changed from Stefan Schulte to Paul Berry
OK: I’m not satisfied that I can get this working before I leave on vacation.
I really wanted to merge your submitted version as a fallback (in fact I actually did, and reverted it), but collapsing uniqueness_key into a string value instead of an array causes a regression in lib/puppet/resource/catalog.rb, where names and titles collide in alias-space.
I’m handing this off to Paul Berry.
#10
Updated by James Turnbull about 5 years ago
- Status changed from Code Insufficient to Needs Decision
- Assignee changed from Paul Berry to Nigel Kersten
#11
Updated by Nigel Kersten about 5 years ago
- Status changed from Needs Decision to Needs More Information
- Assignee changed from Nigel Kersten to Jesse Wolfe
Jesse, can you provide a summary of where we are with this?
#12
Updated by Jesse Wolfe about 5 years ago
I’ve got a sketch of a solution that changes the keys of the hash passed to “prefetch”, so it might not be compatible with third-party providers. I have a vague idea of how to fix it in a back-compatible way, if that’s necessary. Either way, I’d like to spend a little time thinking about how to test it, as it’s a fairly cross-cutting change.
#13 Updated by Anonymous over 4 years ago
- Assignee deleted (
Jesse Wolfe)
This issue was assigned to a former Puppet Labs employee. Adding back to the pool of unreviewed issues.
#14 Updated by Anonymous over 3 years ago
- Target version deleted (
2.7.x)
#15 Updated by Anonymous over 3 years ago
As the 2.7.x line is winding down, I am removing the target at 2.7.x from tickets in the system. The 2.7 line should only receive fixes for major problems (crashes, for instance) or security problems.
#16
Updated by Eric Badger almost 3 years ago
- Affected Puppet version changed from 2.6.0 to 3.2.3