sensu::check unable to remove a check property

[fessyfoo]

We (accidentally) set the property subdue (to an empty string) on a sensu::check resource and then later tried to remove it by setting the property to undef, however setting a property to undef seems to make the property go unmanaged. Puppet runs resulted in "no change" to that resource. I think this is because puppet is treating an unspecified property as "insync" I could be wrong there, hard to follow the puppet code.

is there a way to remove a property from a previously defined check?

note: setting subdue to something other than a hash prevents sensu-client from restarting. so as a workaround we have to set subdue to an empty hash.

[fessyfoo]

validating that subdue is a hash seems a sensible protection against getting into this situation, i submitted PR #536

[fessyfoo]

given the somewhat vague terms from the documentation. I think it may be expected behavior for an unspecified property to go "unmanaged"

The undef value is usually useful for testing whether a variable has been set. It can also be used as the value of a resource attribute, which can let you un-set any value inherited from a resource default and cause the attribute to be unmanaged

[1] https://docs.puppet.com/puppet/latest/reference/lang_data_undef.html
[2] https://docs.puppet.com/puppet/3.8/reference/lang_datatypes.html#undef

however one might interpret that "unmanaged" in practice does allow for defaults as some custom types use defaultto from the property dsl. Since a default behavior is acceptable i suggest that sensu_check for properties that are undefined should delete the key from the corresponding sensu json config.

[fessyfoo]

in the latest sensu changes subdue is now checked more deeply and setting to an empty hash is no longer a viable workaround.

we really need to be able to remove the subdue parameter via puppet.

[ttarczynski]

I've used an empty hash as a workaround (to cleanup subdue settings) with sensu version 0.25:

subdue => {},

With sensu version 0.26 sensu doesn't accept empty hash for subdue:

..."level":"fatal","message":"check subdue days must be a hash","object":{... "subdue":{}...
..."level":"fatal","message":"check subdue days must include at least one day of the week or 'all'","object":{... "subdue":{"days":{}},...
..."level":"fatal","message":"check subdue days all must include at least one time window","object":{... "subdue":{"days":{"all":[]}},...

With sensu version 0.26 I have an ugly workaround :

subdue => {
  'days' => {
    'all' => [
      {
        'begin' => '00:00',
        'end'   => '00:00',
      },
    ],
  },
},

This ensures that subdue is cleaned up but it produces a confusing check configuration.
We need a possibility to remove a check property instead.

[jaxxstorm]

I spent a few hours today taking a bash at this, and didn't really get very far. I'm not familiar enough with the provider code to figure out how to remove values with the insync settings.

I wonder if @jamtur01 fancies coming in to help out?

[fessyfoo]

@jaxxstorm i too spent some time learning types and providers to try to help this.

this is where I got: master...fessyfoo:clear-undeclared-properties

basically I clear any properties from the config that weren't explicitly set on the resource or via custom just before flush. however I got stalled out on both the design being wrong and testing.

design wrong:
biggest problem with this is puppet won't show an undefined property as being cleared. it will still consider it "insync" ie the type is not executing the state change from something to empty. so I think this is a bit of a hack. [ aside: take note of the change to getting the property list from the type rather than hard coding the list in a method that keeps forgetting to get updated. I should submit that as a seprate PR]

along the lines of the design i did test (through some pry hackery) that the defaultto for the property can be set to a symbol such as :absent and these things can't be set to a symbol from the resource declaration in the puppet manifets. so seems ok? We could test against that in the provider and delete any properties set this way. that should solve the puppet noticing the change problem. and was where I was headed next. though it would result in a lot more boiler plate which really made me think about further refactoring to reduce that.

before getting into all that I was workign to determine how to put in place testing for the actual types and providers classes using the code in puppet source as examples.

but so far I've stalled for 7 days and haven't gotten back to it. so posting this to let you know what I figured out.

hope it helps.

[ttarczynski]

@fessyfoo @jaxxstorm
I've tried to work on this and followed puppet type yumrepo implementation. The pattern used there is to remove property by setting it explicitly to 'absent'.

Here it's applied for subdue only: #565. I've also tried using defaultto :absent but I get the same behavior as with undef.

What do you think of this approach?

[jaxxstorm]

@ttarczynski I like it. I'll likely merge this early next week. Thanks for the contribution!

[jaxxstorm]

It's worth nothing I think we should expand this for all optional params

[ttarczynski]

I'm willing to work further on this.
I will look into removing all optional params with 'absent' with some generic method similar to this pattern in 'yumrepo':

• https://github.com/puppetlabs/puppet/blob/4.7.0/lib/puppet/provider/yumrepo/inifile.rb#L283-L295

I just wanted to start with a small change (#565) and then iterate on this.

[ttarczynski]

I've submitted two more changes trying to follow how yumrepo is implemented:

• use constant SENSU_CHECK_PROPERTIES instead of hardcoded check_args in sensu_check provider #572 eliminates the hard-coded property args (as suggested by @fessyfoo above)
• transorm input with munge in type rather than in sensu_check/json.rb provider #573 moves the input munging from provider to type definition

With #573 it's more tricky because rspec tests don't cover the provider implementation and I don't know how to properly tests such changes against all edge cases.