The Puppet Labs Issue Tracker has Moved: https://tickets.puppetlabs.com

This issue tracker is now in read-only archive mode and automatic ticket export has been disabled. Redmine users will need to create a new JIRA account to file tickets using 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

Added by Stefan Schulte over 5 years ago. Updated almost 3 years ago.

Status:Needs More InformationStart date:12/19/2010
Priority:NormalDue 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 of resource.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

Blocks Puppet - Feature #5660: Puppet should handle port entries in /etc/services with a... Tests Insufficient 12/23/2010

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 to iskeyattribute.
  • 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

Also available in: Atom PDF