(FM-2303, FM-2790, FM-2445) WMI / Registry impl of Discovery Report #116
Conversation
|
Supercedes #83 |
Iristyle
changed the title
|
Need to verify 2nd commit - but ready for a look @cyberious. I believe this should effectively reproduce the behavior you were looking for. |
Iristyle
force-pushed
the
ticket/master/FM-2303-feature-hash
branch
3 times, most recently
from
17cc9b6 to
95a6d5d
Compare
Iristyle
changed the title
Iristyle
changed the title
|
@cyberious / @ferventcoder whenever you get a chance - I think this guy should be pretty good to go. |
Iristyle
force-pushed
the
ticket/master/FM-2303-feature-hash
branch
from
95a6d5d to
5a5946f
Compare
Iristyle
changed the title
|
|
puppet resource sqlserver_instance on existing nodes |
|
I'm not seeing that issue @cyberious
|
|
I've been manually copying the module code over VMs that have been setup for acceptance testing. To verify the Facter facts, I've been adding the sqlserver modules And then I can run Running And Since it looks like there's a failure in generating the Facter fact in your tests, we should start there and see if
|
|
This is all with |
|
The only facts being reported to the master are |
|
added stack trace in ticket as it is easier to track |
| require 'win32/registry' | ||
|
|
||
| installed = reg_key_feat_hash.select do |subkey, feat_name| | ||
| Win32::Registry::HKEY_LOCAL_MACHINE.open("#{key_name}\\#{subkey}") do |feat_key| |
There was a problem hiding this comment.
Requires a begin rescue in case subkey does not exist. I tried and this resolved the issue with not returning anything, facter swallowed the error and says there is nothing to return which is the nil class exception
There was a problem hiding this comment.
begin
Win32::Registry::HKEY_LOCAL_MACHINE.open("#{key_name}\\#{subkey}") do |feat_key|
get_reg_key_val(feat_key, instance_name, Win32::Registry::REG_SZ)
end
rescue Exception => e
end
There was a problem hiding this comment.
Yup, looks like we both came to the same conclusion on this one. I forgot that I was going to do this - and failed to test the scenario where both instances had OLAP off.
I was able to repro locally the crash, and the fix - and I repushed with code that rescues the Win32::Registry::Error. In the other spot where there's a rescue I changed the error to be more specific as well.
Iristyle
force-pushed
the
ticket/master/FM-2303-feature-hash
branch
from
5a5946f to
896aa16
Compare
|
We should be in better shape now @cyberious - thanks for catching that bug. |
|
Comment no longer valid. |
| :features => | ||
| PuppetX::Sqlserver::ServerHelper.translate_features( | ||
| jsonResult['Generic Features']).sort! | ||
| :features => result['features'].sort |
There was a problem hiding this comment.
Should set :ensure => result['features'].empty? ? :absent : :present,
There was a problem hiding this comment.
@cyberious Did the previous PowerShell based Discovery Report code omit 'Generic Features' completely from the hash when empty?
There was a problem hiding this comment.
Yes, which is why we had the `has_key?['features']``
There was a problem hiding this comment.
So if we do what you're suggesting, we end up following a different code path. To mimic the existing behavior we'd want
if !result['features'].empty?instead of
if result.has_key?('features')Correct?
Iristyle
force-pushed
the
ticket/master/FM-2303-feature-hash
branch
4 times, most recently
from
4fd3734 to
30e7597
Compare
Iristyle
changed the title
| features = ['Tools', 'BC', 'Conn', 'SSMS', 'ADV_SSMS', 'SDK', 'IS', 'MDS'] | ||
|
|
||
| before(:all) do | ||
| names = eval(fact_on(host, 'sqlserver_instance_names')).values.flatten |
There was a problem hiding this comment.
With the test change in b2e989d to introduce fact_on, a new failure has cropped up. I'm guessing there's an issue with Beaker being able to retrieve facts from modules - not sure if this can be fixed with facter -p, but I'm reluctant to do that given there is no facter -p in cfacter.
The actual Beaker code that implements the facter call is at https://github.com/puppetlabs/beaker/blob/0ac1460dc3af4d4e10062333c8d931351157d71d/lib/beaker/dsl/wrappers.rb#L15-L20
Failures:
1) sqlserver_features with no installed instances can install all possible features
Failure/Error: names = eval(fact_on(host, 'sqlserver_instance_names')).values.flatten
NoMethodError:
undefined method `values' for nil:NilClass
# ./spec/acceptance/sqlserver_features_spec.rb:244:in `block (4 levels) in <top (required)>'
Finished in 52 minutes 8 seconds (files took 17 minutes 47 seconds to load)
13 examples, 1 failure, 1 pending
@kylog / @peterhuene / @MikaelSmith / @anodelman any feedback here?
There was a problem hiding this comment.
I've temporarily removed the usage of fact_on, so I'm OK with this being merged as-is - with a solution for fact_on in a subsequent commit @cyberious
There was a problem hiding this comment.
fact_on by default uses no arguments, and it's true that facter -p is being deprecated. One way to test would be to point --custom-dir at the module's lib/facter directory, but that will only work with Facter 3. So the only way that works with modules is to use puppet facts, and parse the json output.
There was a problem hiding this comment.
So if you look at the Beaker code above @MikaelSmith - you can actually pass in a hash with :environment - so I can use the FACTERLIB (which is something I'm testing now) -- but it seems a little hokey to jam in #{distmoduledir}/sqlserver/lib/facter. Undecided on that approach as of yet.
There was a problem hiding this comment.
FACT-1015 is related to this, and may lead to a new solution for Beaker.
There was a problem hiding this comment.
FYI - my test worked through Beaker - see commit at Iristyle@c184ed9
Not sure yet if this the way forward or not....
Iristyle
force-pushed
the
ticket/master/FM-2303-feature-hash
branch
from
30e7597 to
21f7bca
Compare
| instances = instance_names | ||
| .map { |name| [ name, get_instance_info(version, name) ] } | ||
|
|
||
| instances.push(['features', get_shared_features(version, SQL_REG_ROOT)]) |
There was a problem hiding this comment.
Should be wrapped in a try as retrieving could throw an error.
Iristyle
force-pushed
the
ticket/master/FM-2303-feature-hash
branch
from
21f7bca to
0923382
Compare
- Previously, tests relied on the assumption that SQL Server instances were installed already. Add a new test that verifies all shared features can be installed through a manifest, even with no SQL instances installed. - This test is necessary to validate the reimplementation of the underlying feature detection code from using sqlcmd.exe to using a WMI / Registry approach.
Iristyle
force-pushed
the
ticket/master/FM-2303-feature-hash
branch
4 times, most recently
from
69dd9a0 to
62410a0
Compare
- Provide a nearly drop-in replacement for the existing SQL discovery
report based on sqlcmd.exe - but by using WMI queries and Registry
spleunking. This code:
- does not shell to PowerShell
- does not call sqlcmd.exe
- does not parse XML
- runs all code in the Ruby process
- As a result:
- it's a bit quicker to execute
- it doesn't require any heuristics to find files
- it doesn't need to write / secure temporary files
- it can use native Ruby symbols
- Of note, the public API on PuppetX::SqlServer::Features is rather
small and comprises only 3 public methods:
- get_instances - returns a hash of all instance specific details
for both SQL_2012 and SQL_2014 instances munged together given
in a side by side SQL install, name collisions are not allowed
- get_features - returns a hash of SQL_2012 / SQL_2014 shared
features
- get_instance_info(version, instance_name) - returns a hash for a
given instance with a given version. Called by get_instances
- The shape of the data returned is slightly different than what
run_discovery_report previously used, but it was close to a drop-in
replacement.
- Note that since the module doesn't specify a SQL version for install
and the current code is not set to handle multiple versions. The
Features.get_instances returns info for both SQL_2014 and
SQL_2012 should they both be installed. For the sake of consumers
of the discovery_script code path, only feature shared feature data
for one version of SQL will be installed, with SQL_2014 prioritized.
- Note that when run under Puppet 32-bit, the flag is passed for
Registry access to ensure that the right hive is used. Furthermore,
the WMI scripting access passes the 64-bit flag, so that WMI data
can be queried correctly for 64-bit SQL server from a 32-bit Ruby
process.
- Fact 'sqlserver_instances' contains all detailed instance information and is not segregated by SQL version given instance names are unique even in a side by side installation - Fact 'sqlserver_features' contains all shared features segregated by SQL version - run_discovery_script method has been completely removed in favor of leveraging Facts - note that to make the output compatible with Facter, the Ruby symbols used in the hashes returned from PuppetX::Sqlserver::Features have all been changed to bare strings
- Now that Ruby code is used to build the description of a current installation, it's no longer necessary to perform string translation from SQL feature names such as 'SQL Server Replication' to their command line switch counterparts. This should remove any concerns around localization since command line switches are universal. - Remove code that's no longer needed as a result. - Fact values for sqlserver_instances and sqlserver_features now contain these command line switches instead of human-friendly feature names.
- According to the MSDN documentation for the 'SQL' feature https://msdn.microsoft.com/en-us/library/ms144259.aspx#Feature The following feature parameters should be included: BC Conn SSMS ADV_SSMS SDK
- Previously the test that removes instance was hard-coded to use 'MSSQLSERVER', but this makes the test a bit more fragile. Instead, use Beaker to load the agents facts, passing the module sqlserver/lib/facter directory in via FACTERLIB so that the names of all instances can be retrieved.
Iristyle
force-pushed
the
ticket/master/FM-2303-feature-hash
branch
from
62410a0 to
24d7d8a
Compare
| require 'win32ole' | ||
| ver = SQL_WMI_PATH[version] | ||
| context = WIN32OLE.new('WbemScripting.SWbemNamedValueSet') | ||
| context.Add("__ProviderArchitecture", 64) |
There was a problem hiding this comment.
@cyberious this is the solution for the 32-bit Puppet agent not seeing the 64-bit SQL servers. WMI has redirection as well (since it maps over the registry), so the solution was to always force it look at 64-bit data, since we only support SQL 64-bit.
There was a problem hiding this comment.
Nice find 👍 fixed issue on 32, testing further to see if anything else shakes out but I think we are good to go now
There was a problem hiding this comment.
Thanks for testing with the 32-bit installer - otherwise this would have never been caught.
|
FYI @cyberious - this code has passed via a local beaker acceptance run, and via Jenkins CI (aside from the unrelated future parser issues). So as long as it passes your manual verification, it should be good to go. Finally! |
(FM-2303, FM-2790, FM-2445) WMI / Registry impl of Discovery Report
report based on sqlcmd.exe - but by using WMI queries and Registry
spleunking. This code:
small and comprises only 3 public methods:
SQL_2014 that contains instances and their features
installed shared features
given instance with a given version. Called by get_installations
run_discovery_report previously used, but it was close to a drop-in
replacement.
and the current code is not set to handle multiple versions. The
Features.get_installations returns info for both SQL_2014 and
SQL_2012 should they both be installed. For the sake of consumers
of the discovery_script code path, only feature data for one
version of SQL will be installed, and SQL_2014 is prioritized.