(MODULES-7178) Report types dynamically #69

Iristyle · 2018-07-16T23:24:57Z

~~Determine if new tests can / should be added~~ basic specs added
Should resource_name be canonicalized? i.e. Does FiLe show up differently than file... do we care? (Report logic already calls capitalize and the report looks fine, but should the internal tracking maintain state differently?) Canonicalized by calling downcase on the resource_name property so that case differences don't matter internally.
Make sure nothing else weird happens in the system when the type reports as :dsc_foo instead of :dsc. ~~Are there any other dependencies on the symbol :dsc?~~ There are no other usages of :dsc
Make sure that behavior on an older PE install is reasonable (without the PUP-8746 / puppetlabs/puppet@28240e5 fix) - it may end up behaving partially correctly if metrics summary is out of sync with actual resource types Screenshots / explanation below. Behavior is OK and as expected.
~~Test in conjunction with the DSC resource that may refer to the same types (should we use DscLite_ instead of Dsc_ for the prefix?)~~ Vetted this with @michaeltlombardi - didn't include screenshots, but it was necessary to prefix with dsc_lite_ to avoid collisions with DSC module
Prior to this commit, all types would report the dsc type as their
identity. This is problematic for the sake of reporting as every type
of resource is grouped together under dsc, rather than the specific
resource_name modeled in a manifest.

This new approach allows for types to report under something more
useful that indicates the original DSC resource type specified in the
resource_name attribute.

For instance, given a manifest with the following resources:

   dsc { 'sample_file':
     resource_name     => 'File',
     module            => 'PSDesiredStateConfiguration',
     properties        => {
       ensure          => 'present',
       contents        => 'hello',
       destinationpath => 'c:\\test.txt',
     }
  }

  dsc { 'lmhosts':
     resource_name     => 'Service',
     module            => 'PSDesiredStateConfiguration',
     properties        => {
       name            => 'lmhosts',
       ensure          => 'present',
       state           => 'running',
     }
  }

The report now shows:

Dsc_lite_file[sample_file] Unchanged
Dsc_lite_service[lmhosts] Unchanged

Instead of what it previously reported:

Dsc[sample_file] Unchanged
Dsc[lmhosts] Unchanged

Selection of the PowerShell code was previously determined by type,
but given it has changed to being specified as :Dsc_lite_XXXX, its
not as easy to do that anymore. Instead, add a simple boolean
property to the generic dsc wrapper that may be checked instead.
Note that parameter validation for module, resource_name and
properties does not currently work, but will be fixed in another
ticket by adding a global validator. The type method performs some
additional guarding around usage of the resource_name parameter
for 2 reasons:
- It's unclear if the parser or other logic in Puppet uses the type
  name without the resource being instantiated fully with values from
  a manifest
- Due to the incorrect validation, the PE console errors with the
  following message when resource_name is left unspecified:
undefined method value for nil:NilClass

Iristyle · 2018-07-16T23:37:30Z

Given a site.pp on a 2018.1.2 master (which contains the patch for PUP-8746 at puppetlabs/puppet#6838, which is part of Puppet 5.5.2 and 5.5.3)

node default {
      dsc {'8c8ffeaf-ddee-46a7-8bc4-0749ad256be4':
      resource_name => 'file',
      module => 'PSDesiredStateConfiguration',
      properties => {
        ensure          => 'absent',
        contents  => '36b5ccba-84fe-470d-a064-8d12c671918b',
        destinationpath => 'C:\8c8ffeaf-ddee-46a7-8bc4-0749ad256be4'
      }
    }

}

dsc {'sample_foo':
  resource_name     => 'service',
  module            => 'PSDesiredStateConfiguration',
  properties        => {
    ensure          => 'present',
    state           => 'running',
    name            => 'lmhosts',
  }
}

dsc {'sample_file2':
  resource_name     => 'File',
  module            => 'PSDesiredStateConfiguration',
  properties        => {
    ensure          => 'present',
    contents        => 'hello',
    destinationpath => 'c:\\test2.txt',
  }
}

The behavior is as desired:

To repro, I provisioned using the following procedure:

GEM_SOURCE=http://rubygems.delivery.puppetlabs.net BEAKER_keyfile=~/.ssh/id_rsa-acceptance BEAKER_debug=true BEAKER_destroy=no BEAKER_PE_DIR=http://pe-releases.puppetlabs.lan/2018.1.2/ BEAKER_PE_VER=2018.1.2 BEAKER_setfile=2012r2.yaml TEST_FRAMEWORK=beaker-rspec BEAKER_TESTMODE=agent PUPPET_INSTALL_TYPE=pe bundle exec rake beaker

Once provisioned, I modified the site.pp on the master using vi /etc/puppetlabs/code/environments/production/manifests/site.pp, adding the above code. The suite installs the module to /etc/puppetlabs/code/modules/dsc_lite/, where it may be further edited. To get pry-byebug / pry-stack_explorer on the Windows node for debugging purposes, I did the following:

Installed Ruby 2.4 installer from https://github.com/oneclick/rubyinstaller2/releases/download/rubyinstaller-2.4.4-2/rubyinstaller-devkit-2.4.4-2-x64.exe
Made sure that devkit / msys were also installed
Performed an install of the relevant gems using gem install pry-byebug pry-stack_explorer --no-ri --no-rdoc
Copied the built copies of the gems from C:\Ruby24-x64\lib\ruby\gems\2.4.0 to the Puppet vendored Ruby at C:\Program Files\Puppet Labs\Puppet\sys\ruby\lib\ruby\gems\2.4.0. I copied the following installed / built bits from the directories cache, gems and specifications:
- binding_of_caller
- byebug
- coderay
- debug_inspector
- method_source
- pry
- pry-byebug
- pry-stack_explorer
I then opened the Start Command Prompt with Puppet
From inside that Ruby environment, I installed one additional gem to get pry to work properly - gem install rb-readline --no-ri --no-rdoc

glennsarti · 2018-07-17T06:45:04Z

lib/puppet/type/dsc.rb

@@ -4,6 +4,15 @@
  require Pathname.new(__FILE__).dirname + '../../' + 'puppet/type/base_dsc_lite'
  require Pathname.new(__FILE__).dirname + '../../puppet_x/puppetlabs/dsc_lite/dsc_type_helpers'

+  def type
+    # for the sake of report status, masquerade as a different type
+    ("Dsc_" + parameters[:resource_name].value).to_sym


Huh... TIL ... I thought you needed to use .intern

Changing to DscLite_ as the prefix to prevent collisions with existing DSC module.

To canonicalize the type name, I think we should do a parameters[:resource_name].value.capitalize

glennsarti · 2018-07-17T06:48:39Z

Those tests you mentioned seem to cover most of where I think the "unintended consequences" would come about. I would also consider adding in a resource name collision on purpose to see what happens. e.g.

dsc_file { 'sample_file1':
....
}

dsc {'sample_file2':
  resource_name     => 'File',
...
}

And possibly two resources with the same title

dsc_file { 'collide':
....
}

dsc {'collide':
  resource_name     => 'File',
...
}

Both in an agent only and a master/agent setup.

To see what the fall out would be.

Iristyle · 2018-07-17T20:27:46Z

Verified behavior in 2017.3.8 is acceptable / has a few tradeoffs.

GEM_SOURCE=http://rubygems.delivery.puppetlabs.net BEAKER_keyfile=~/.ssh/id_rsa-acceptance BEAKER_debug=true BEAKER_destroy=no BEAKER_PE_DIR=http://pe-releases.puppetlabs.lan/2017.3.8/ BEAKER_PE_VER=2017.3.8 BEAKER_setfile=2012r2.yaml TEST_FRAMEWORK=beaker-rspec BEAKER_TESTMODE=agent PUPPET_INSTALL_TYPE=pe bundle exec rake beaker

As expected, since the agent is 5.3.8 (puppet 5.3.7), it doesn't include the PUP-8746 fix for how metrics are grouped.

The list of resources itself renders correctly.

However, the grouping (by dsc instead of by dsc_file or dsc_service) is incorrect. The timing is correct for all resources of type dsc, but selecting dsc does not show the resources. This trade-off is fine. The metrics view has changed in PE 2018 anyhow, so that this grouping construct no longer exists.

Iristyle · 2018-07-18T00:16:15Z

lib/puppet/type/dsc.rb

+  def type
+    # in the event this value is consumed early in the lifecycle / before
+    # parameters have been populated, use 'unspecified'
+    name = ( parameters[:resource_name].nil? || parameters[:resource_name].value.nil? || parameters[:resource_name].value.match?(/\W/) ) ?


This extra check can be removed once https://tickets.puppetlabs.com/browse/MODULES-7485 is fixed.

- Prior to this commit, all types would report the `dsc` type as their identity. This is problematic for the sake of reporting as every type of resource is grouped together under `dsc`, rather than the specific `resource_name` modeled in a manifest. This new approach allows for types to report under something more useful that indicates the original DSC resource type specified in the resource_name attribute. For instance, given a manifest with the following resources: dsc { 'sample_file': resource_name => 'File', module => 'PSDesiredStateConfiguration', properties => { ensure => 'present', contents => 'hello', destinationpath => 'c:\\test.txt', } } dsc { 'lmhosts': resource_name => 'Service', module => 'PSDesiredStateConfiguration', properties => { name => 'lmhosts', ensure => 'present', state => 'running', } } The report now shows: * Dsc_lite_file[sample_file] Unchanged * Dsc_lite_service[lmhosts] Unchanged Instead of what it previously reported: * Dsc[sample_file] Unchanged * Dsc[lmhosts] Unchanged - Selection of the PowerShell code was previously determined by type, but given it has changed to being specified as :Dsc_lite_XXXX, its not as easy to do that anymore. Instead, add a simple boolean property to the generic dsc wrapper that may be checked instead. - Note that parameter validation for `module`, `resource_name` and properties does not currently work, but will be fixed in another ticket by adding a global validator. The `type` method performs some additional guarding around usage of the `resource_name` parameter for 2 reasons: * It's unclear if the parser or other logic in Puppet uses the type name without the resource being instantiated fully with values from a manifest * Due to the incorrect validation, the PE console errors with the following message when `resource_name` is left unspecified: undefined method `value` for nil:NilClass

Iristyle requested review from ThoughtCrhyme, glennsarti and michaeltlombardi July 17, 2018 05:18

glennsarti reviewed Jul 17, 2018

View reviewed changes

Iristyle force-pushed the MODULES-7178-report-generic-dsc-resources-with-specific-types branch 3 times, most recently from f89af75 to bc487d2 Compare July 18, 2018 00:15

Iristyle commented Jul 18, 2018

View reviewed changes

Iristyle force-pushed the MODULES-7178-report-generic-dsc-resources-with-specific-types branch from bc487d2 to 87e841e Compare July 18, 2018 15:35

michaeltlombardi approved these changes Jul 18, 2018

View reviewed changes

michaeltlombardi merged commit 8344792 into puppetlabs:master Jul 18, 2018

Iristyle deleted the MODULES-7178-report-generic-dsc-resources-with-specific-types branch July 18, 2018 20:12

Iristyle mentioned this pull request Jul 19, 2018

(MODULES-7485) Fix validation for required parameters #71

Merged

jordanbreen28 added the feature label Jun 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(MODULES-7178) Report types dynamically #69

(MODULES-7178) Report types dynamically #69

Iristyle commented Jul 16, 2018 •

edited

Loading

Iristyle commented Jul 16, 2018

glennsarti Jul 17, 2018

Iristyle Jul 17, 2018

Iristyle Jul 17, 2018

glennsarti commented Jul 17, 2018

Iristyle commented Jul 17, 2018

Iristyle Jul 18, 2018

(MODULES-7178) Report types dynamically #69

(MODULES-7178) Report types dynamically #69

Conversation

Iristyle commented Jul 16, 2018 • edited Loading

Iristyle commented Jul 16, 2018

glennsarti Jul 17, 2018

Choose a reason for hiding this comment

Iristyle Jul 17, 2018

Choose a reason for hiding this comment

Iristyle Jul 17, 2018

Choose a reason for hiding this comment

glennsarti commented Jul 17, 2018

Iristyle commented Jul 17, 2018

Iristyle Jul 18, 2018

Choose a reason for hiding this comment

Iristyle commented Jul 16, 2018 •

edited

Loading