(MODULES-5566) Rewrite Instance Discovery #241

michaeltlombardi · 2017-10-16T14:22:36Z

This commit refactors the logic for discovering SQL instances to
use registry instead of WMI. In order to do so, we use the Puppet
Util Windows Registry module and ensure that SQL instances with
valid names outside the current code page can be discovered.

This commit also removes code supporting calls to WMI to discover
SQL instances.

Iristyle · 2017-10-16T21:17:46Z

lib/puppet_x/sqlserver/features.rb

        return nil if version.nil?
-        sql_instance = get_wmi_instance_info(version, instance_name)


Does this mean get_wmi_instance_info is no longer needed?

Correct. It's removed further up in the diff.

Correct. We replaced the various functionality for pulling from WMI by pulling from registry to identify and enumerate instances.

glennsarti · 2017-10-16T21:22:37Z

lib/puppet_x/sqlserver/features.rb

@@ -266,7 +279,7 @@ def self.get_instances
            major_version = SQL_CONFIGURATION[version][:major_version]

            instances = []
-            get_reg_instance_info(version).each do |instance|
+            Array(get_reg_instance_info(version)).each do |instance|


Curious about why the need to force an array type. get_instance_names_by_reg should always be returning an array.

I was getting errors regarding the each method not existing for nil.

Ahh ok...so that method returns nil or an array.

Iristyle · 2017-10-16T21:26:07Z

lib/puppet_x/sqlserver/features.rb

-              names.push(name)
-              roots.push("Software\\Microsoft\\Microsoft SQL Server\\#{registry[name]}")
-            }
+        if key_exists?(instance_root)


Instead of guarding the whole method / further indenting... should be easier to just return early like:

return [] unless key_exists?(instance_root)

Good point, I didn't even think to do this. Will update.

Iristyle · 2017-10-20T17:16:48Z

lib/puppet_x/sqlserver/features.rb

+            }
+          end
+        end
+        names.each do |name|


You can make this a little simpler

instances = names.select do |name| my_roots = roots.select { |path| /#{name}/.match(path) } version = hklm.open("#{my_roots[0]}\\MSSQLServer\\CurrentVersion", KEY_READ | KEY_WOW64_64KEY) do |registry| registry['CurrentVersion'] end {'name' => name, 'version_friendly' => friendly_version, 'version' => version, 'reg_root' => my_roots}) end

I changed the .each to a .select.

You might also think about changing the hash keys from literal strings like 'name' to symbols like :name,
but that will require modifying the callers of this method as well. Symbols for hash keys are more idiomatic in Ruby.

I assume the .uniq is need because there are dupes somehow?

Correct, though thinking about it now, the problem is that names provides a duplicate list of names. Further, the required output is that unique list of instances, mapping a name to one or more registry roots.

A simpler solution would be to add the uniq to names at the start of this call, like this, maybe?

names.uniq.select do |name| my_roots = roots.select{|path| /#{name}/.match(path)} version = hklm.open("#{my_roots[0]}\\MSSQLServer\\CurrentVersion", KEY_READ | KEY_WOW64_64KEY) do |registry| registry['CurrentVersion'] end {'name' => name, 'version_friendly' => friendly_version, 'version' => version, 'reg_root' => my_roots} end

Iristyle · 2017-10-20T17:19:18Z

lib/puppet_x/sqlserver/features.rb

+        subkeys.each do |subkey|
+          hklm.open("#{instance_root}\\#{subkey}", KEY_READ | KEY_WOW64_64KEY) do |registry|
+            registry.each_value{|name, _ |
+              names.push(name)


I think names and roots are intended to be a matching set? If so, we can group them together during this iteration?

But I'm not totally certain of that based on the later line:

my_roots = roots.select{|path| /#{name}/.match(path)}

Correct, each instance name will have one or more roots.

The problem I ran into and didn't know how to elegantly solve is this:

There are three containers inside of Instance Names - one each for analytic services, reporting services, and the database engine. Each of those may have an instance whose name is in another container (and thus has another registry root). I needed to select out all of the roots associated with a name.

Rereading my code with some distance, there's got to be a better way to solve for this problem than what I've done in this function.

Iristyle · 2017-10-20T17:21:17Z

lib/puppet_x/sqlserver/features.rb

@@ -55,6 +45,15 @@ def self.get_reg_key_val(win32_reg_key, val_name, reg_type)
          nil
      end

+      def self.key_exists?(path)


Ruby trick.. this can be shortened to:

def self.key_exists?(path) Win32::Registry::HKEY_LOCAL_MACHINE.open(path, ::Win32::Registry::KEY_READ) true rescue false end

Iristyle · 2017-10-20T18:15:22Z

lib/puppet_x/sqlserver/features.rb

+        instances = []
+        # retrieve the list of subkeys - OLAP, RS, and SQL, but only if an instance exists.
+        hklm.open("#{instance_root}", KEY_READ | KEY_WOW64_64KEY) do |registry|
+          registry.each_key{|key, _ | subkeys.push(key)}


You should also be able to continue enumerating subkeys inside of this initial open

Iristyle · 2017-10-20T18:19:58Z

The only other thing I'll mention... at a higher level... is that we have a Puppet::Util::Windows::Registry set of helpers in https://github.com/puppetlabs/puppet/blob/3a5dfed0fef8fc5cb1b6ec168766115de3e8f23b/lib/puppet/util/windows/registry.rb

I should have mentioned this earlier.

We typically prefer to use those over Win32::Registry as it fixes a number of bugs present in Ruby's base implementation (specifically around Unicode handling). You can see demonstrations of some of the fixes in the tests at https://github.com/puppetlabs/puppet/blob/ee07637150b2b714d10ab1d8ba4d8d268736b4fc/spec/integration/util/windows/registry_spec.rb

Does any of the user data specified in the registry have the potential of being Unicode? (I'm particularly wondering about instance names for Japanese users). If so, it might make sense to use Puppets registry helpers. If the reg values tend to be well-known ASCII where Unicode is unlikely, we can probably leave things as-is.

Iristyle · 2017-10-25T21:06:30Z

lib/puppet_x/sqlserver/features.rb

@@ -1,3 +1,6 @@
+require 'puppet'


You shouldn't need to require 'puppet' I don't think

You're right, this is a holdover from execution outside of puppet during pry testing.

Does this need to be removed then?

Possibly. I will test! Can't believe I missed this....

Iristyle · 2017-10-25T21:08:54Z

lib/puppet_x/sqlserver/features.rb

@@ -10,39 +13,25 @@ module Sqlserver
    class Features
      private

+      extend Puppet::Util::Windows::Registry


For future reference, and since this probably isn't obvious... the only reason that this line doesn't blow up tests on non-Windows platforms is because of the line at https://github.com/puppetlabs/puppet/blob/master/lib/puppet/util/windows.rb#L8

That line creates a Puppet::Util::Windows::Registry module when the file is included on a non-Windows platform. Otherwise, we would have to guard this line to execute only on Windows.

Iristyle · 2017-10-25T21:11:00Z

lib/puppet_x/sqlserver/features.rb

        begin
-          hklm = Win32::Registry::HKEY_LOCAL_MACHINE
-          vals = hklm.open(key_name, KEY_READ | KEY_WOW64_64KEY) do |key|
+          vals = open(HKLM, key_name) do |key|


Still might want to supply values explicitly as was done before for a few reasons:

In case Puppet itself changes the defaults (unlikely)

For inline documentation

You should be able to refer to them via their longer names like Puppet::Util::Windows::Registry::KEY_READ | Puppet::Util::Windows::Registry::KEY_WOW64_64KEY

Preferable to include them in their long-form in each spot or to set a single MODE constant at the top?

I only removed them because we were using the same values as the default.

Actually, I think because you've done an extend, the constants are in scope in their short form like KEY_READ

They were available in short form even with the include, iirc.

To clarify: Go back and add them in to each call to open(), such that it reads like open(HKLM, 'some\path', KEY_READ | KEY64)?

Yeah, that sounds good.

Hmm, was necessary to include in addition to extend to get the variables in short form.

Iristyle · 2017-10-25T21:30:34Z

lib/puppet_x/sqlserver/features.rb

+            open(HKLM, "#{instance_root}\\#{instance_type}") do |instance|
+              each_value(instance) do |short_name, _, long_name|
+                root = "Software\\Microsoft\\Microsoft SQL Server\\#{long_name}"
+                if discovered[short_name].nil?


discovered[short_name] ||= { 'name' => short_name, 'reg_root' => [], 'version' => open(HKLM, "#{root}\\MSSQLServer\\CurrentVersion") { |r| values(r)['CurrentVersion'] }, 'version_friendly' => friendly_version } discovered[short_name]['reg_root'].push(root)

ooo... forgot about that shorthand

Iristyle · 2017-10-25T21:31:06Z

lib/puppet_x/sqlserver/features.rb

        installed = reg_key_feat_hash.select do |subkey, feat_name|
          begin
-            hklm = Win32::Registry::HKEY_LOCAL_MACHINE
-            hklm.open("#{key_name}\\#{subkey}", KEY_READ | KEY_WOW64_64KEY) do |feat_key|
+            open(HKLM, "#{key_name}\\#{subkey}") do |feat_key|


Same comment about the constants here...

Iristyle · 2017-10-25T21:35:24Z

lib/puppet_x/sqlserver/features.rb

+        feats = []
+        sql_instance['reg_root'].each do |reg_root|
+          feats.concat(get_instance_features(reg_root, sql_instance['name']))
+        end


feats = sql_instance['reg_root'].map do |reg_root| get_instance_features(reg_root, sql_instance['name']) end

Iristyle · 2017-10-25T22:44:33Z

lib/puppet_x/sqlserver/features.rb

-            instances = get_instance_names_by_ver(version)
-              .map { |name| [ name, get_instance_info(version, name) ] }
+            instances = []
+            Array(get_reg_instance_info(version)).each do |instance|


Now that get_reg_intance_info always returns an enumerable (empty array or populated array), I think you can remove the Array bit.

And you should be able to switch back to a .map as well

glennsarti

Would be nice to have some unit tests on this now...but perhaps leave that for a separate PR.

glennsarti · 2017-10-27T22:30:50Z

lib/puppet_x/sqlserver/features.rb

@@ -1,3 +1,5 @@
+require 'puppet/util/windows'


Do we need to guard this for Windows OS only?

It looks like the module itself is guarded, so... no?

this will still attempt to load on linux :-(

e.g. facter

Gah, well, I guess we can guard like

require 'puppet/util/windows' if Puppet::Util::Platform.windows?

?

But even so, won't all of everything explode if facter runs for SQLServer on linux?

True. It's in a puppet_x ruby file so its not entirely guarded by Windows checks

It's no big deal to add the guard, just concerned about a false sense of security.

Without this commit, the logic to discover instances and features relies on WMI calls and fails to discover instances unless the SQL Engine is installed; this meant that Puppet would never discover instances with only analytics services or reporting services installed, causing each puppet run to attempt to reinstall them, even though they exist. This commit refactors the logic for discovering SQL instances to use registry instead of WMI. In order to do so, we use the Puppet Util Windows Registry module and ensure that SQL instances with valid names outside the current code page can be discovered. This ensures that we are able to discover SQL instances regardless of which features they have installed. This commit also removes code supporting calls to WMI to discover SQL instances.

This commit removes logic from the features file in the acceptance tests suite which existed only to support Puppet 3. This logic is no longer needed as Puppet 3 is no longer supported. Additionally, the way the logic was written caused the tests designed for Puppet 3 to run against Puppet 5. This commit simplifies the maintenance and execution of the suite of acceptance tests.

Iristyle reviewed Oct 16, 2017

View reviewed changes

glennsarti reviewed Oct 16, 2017

View reviewed changes

Iristyle reviewed Oct 16, 2017

View reviewed changes

Iristyle reviewed Oct 20, 2017

View reviewed changes

Iristyle reviewed Oct 25, 2017

View reviewed changes

michaeltlombardi changed the title ~~(WIP) (MODULES-5566) Rewrite Instance Discovery~~ (MODULES-5566) Rewrite Instance Discovery Oct 26, 2017

glennsarti reviewed Oct 27, 2017

View reviewed changes

michaeltlombardi and others added 2 commits November 7, 2017 13:47

Iristyle merged commit 5dcef02 into puppetlabs:master Nov 8, 2017

		return nil if version.nil?
		sql_instance = get_wmi_instance_info(version, instance_name)

(MODULES-5566) Rewrite Instance Discovery #241

(MODULES-5566) Rewrite Instance Discovery #241

Conversation

michaeltlombardi commented Oct 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaeltlombardi Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Iristyle commented Oct 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glennsarti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaeltlombardi commented Oct 16, 2017 •

edited

Loading

michaeltlombardi Oct 23, 2017 •

edited

Loading

Iristyle commented Oct 20, 2017 •

edited

Loading