From 94e882d0be9252f49edf8f66f7a2980c68e1dad5 Mon Sep 17 00:00:00 2001 From: Dan Bode Date: Wed, 12 Feb 2014 21:58:32 +0000 Subject: [PATCH 1/2] Update how compellent retrieves wwns from the server This commit refactors the code related to retrieving server ids used by compellent. 1. It renames GetWWPN to ASM::WWPN the namespace of that class did not even match its location on disk, it was also called GetWWPN (a verb) instead of WWPN (a noun/object). The class contains lots of code for encrytion/decrytion that should not be in the class (this code was removed) The class was also simplified to have a single method called get. This method was simplied to be just 2 lines of code. 2. It removes compellent specific logic that was contained in process generic. Process generic is only intended for common code, component specific code should be moved to the component specific processing methods. It was also updating the file instead of updating the hash before it gets written into the file. This made the code way overly complicated and brittle. Changing it to just adjust the resource hash, cut the amount of code required by at least 5X. 3. It removed some copy paste code that did not actually seem to do anything. 4. It adds a few comments to parts of the code that I found to be unclear. --- lib/asm/GetWWPN.rb | 52 ----------- lib/asm/WWPN.rb | 19 ++++ lib/asm/service_deployment.rb | 164 ++++++++++++---------------------- 3 files changed, 74 insertions(+), 161 deletions(-) delete mode 100644 lib/asm/GetWWPN.rb create mode 100644 lib/asm/WWPN.rb diff --git a/lib/asm/GetWWPN.rb b/lib/asm/GetWWPN.rb deleted file mode 100644 index b519b72d..00000000 --- a/lib/asm/GetWWPN.rb +++ /dev/null @@ -1,52 +0,0 @@ -#!/usr/bin/env ruby - -require 'set' -require 'uri' - -#require '/etc/puppetlabs/puppet/modules/asm_lib/lib/security/encode' - -class GetWWPN - def initialize (ipaddress, username, password) - @ipaddress = ipaddress - @username = username - @password = password - @password = URI.decode(get_plain_password(@password)) - end - - def check_base64(string_input) - if string_input =~ /^([A-Za-z0-9+\/]{4})*([A-Za-z0-9+\/]{4}|[A-Za-z0-9+\/]{3}=|[A-Za-z0-9+\/]{2}==)$/ - true - else - false - end - end - - def get_plain_password(encoded_password) - # Check if the password is encoded or not, if not then try to decrypt it - if check_base64(encoded_password) - plain_password=`/opt/puppet/bin/ruby /opt/asm-deployer/lib/asm/encode_asm.rb #{encoded_password}` - plain_password=plain_password.strip - else - plain_password=encoded_password - end - return plain_password - end - - def getwwpn - wwpnumber = "" - wsmanCmdResponse = `wsman enumerate http://schemas.dmtf.org/wbem/wscim/1/cim-schema/2/root/DCIM/DCIM_FCView -h "#{@ipaddress}" -V -v -c dummy.cert -P 443 -u "#{@username}" -p "#{@password}" -j utf-8 -y basic` - data=wsmanCmdResponse.split(/\n/) - data.each do |ele| - if ele.to_s =~ /(\S+)<\/n1:VirtualWWPN>/ - vardata = $1 - if wwpnumber.length > 0 - wwpnumber = wwpnumber + ",#{vardata}" - else - wwpnumber = vardata - end - end - end - return wwpnumber - end - -end diff --git a/lib/asm/WWPN.rb b/lib/asm/WWPN.rb new file mode 100644 index 00000000..e32a564a --- /dev/null +++ b/lib/asm/WWPN.rb @@ -0,0 +1,19 @@ +# +# This class is used to get wwpns from Dell servers +# +class ASM::WWPN + + def self.get(ipaddress, username, password) + # TODO do we need to specify this external link? + # it makes it seem this has an external dependency + # on network connectivity which we know is not + # true + wsmanCmdResponse = `wsman enumerate http://schemas.dmtf.org/wbem/wscim/1/cim-schema/2/root/DCIM/DCIM_FCView -h "#{ipaddress}" -V -v -c dummy.cert -P 443 -u "#{username}" -p "#{password}" -j utf-8 -y basic` + wsmanCmdResponse.split(/\n/).collect do |ele| + if ele =~ /(\S+)<\/n1:VirtualWWPN>/ + $1 + end + end + end + +end diff --git a/lib/asm/service_deployment.rb b/lib/asm/service_deployment.rb index 4ede5b62..e3fb3e20 100755 --- a/lib/asm/service_deployment.rb +++ b/lib/asm/service_deployment.rb @@ -8,8 +8,7 @@ require 'timeout' require 'securerandom' require 'yaml' -require 'set' -require 'asm/GetWWPN' +require 'asm/WWPN' require 'fileutils' require 'asm/get_switch_information' require 'uri' @@ -288,7 +287,14 @@ def process_components() end end - def process_generic(cert_name, config, puppet_run_type, override = true, server_cert_name = nil, asm_guid=nil) + def process_generic( + cert_name, + config, + puppet_run_type, + override = true, + server_cert_name = nil, + asm_guid=nil + ) raise(Exception, 'Component has no certname') unless cert_name log("Starting processing resources for endpoint #{cert_name}") @@ -298,58 +304,9 @@ def process_generic(cert_name, config, puppet_run_type, override = true, server_ resource_file = File.join(resources_dir, "#{cert_name}.yaml") end - wwpn = "" - wwpnSet = Set.new() File.open(resource_file, 'w') do |fh| fh.write(config.to_yaml) end - if ( cert_name =~ /(.*)[C|c]ompellent(.*)/ ) - wwpnSet = process_compellent - - myfile = File.open(resource_file, "r+") - - time = Time.now.to_i - tempfileloc = "/tmp/temp#{time}.txt" - mytempfile = File.open(tempfileloc, "w") - - myfile.each do |line| - if ( line =~ /(.*)wwn\:(.*)/ ) - data = line.scan(/wwn\:(.*)/) - temp = data[0] - temp1 = temp[0] - temp2=temp1.strip - wwpnstring = temp2.split(',') - #for each_wwpnstring in wwpnstring - wwpnstring.each do | each_wwpnstring | - if each_wwpnstring.length > 0 - wwpnSet.add("#{each_wwpnstring}") - end - end - else - mytempfile.puts("#{line}") - end - end - - myfile.close - mytempfile.close - - wwpnSet.each do |wwpndata| - wwpndata = wwpndata.gsub(/:/, '') - if wwpn.to_s.strip.length == 0 - wwpn = "#{wwpndata}" - else - if wwpndata.to_s.strip.length > 4 - wwpn.concat( ",#{wwpndata}") - end - end - end - - File.open(tempfileloc,"a") do |tempfile| - tempfile.puts(" wwn: '#{wwpn}'") - tempfile.close - end - FileUtils.mv(tempfileloc, resource_file) - end override_opt = override ? "--always-override " : "" noop_opt = @noop ? '--noop' : '' @@ -439,62 +396,27 @@ def massage_asm_server_params(serial_number, params) end end - def process_compellent() + # + # This method is used for collecting server wwpn to + # provide to compellent for it's processing + # + def get_dell_server_wwpns log("Processing server component for compellent") - - wwpnSet = Set.new if components = @components_by_type['SERVER'] components.collect do |comp| - cert_name = comp['id'] - - resource_hash = {} - deviceconf = nil - inventory = nil - resource_hash = ASM::Util.build_component_configuration(comp) - if resource_hash['asm::idrac'] - deviceconf ||= ASM::Util.parse_device_config(cert_name) - inventory ||= ASM::Util.fetch_server_inventory(cert_name) - end - - (resource_hash['asm::server'] || {}).each do |title, params| - if params['rule_number'] - raise(Exception, "Did not expect rule_number in asm::server") - else - params['rule_number'] = rule_number - end - - # In the case of Dell servers the title should contain - # the service tag and we retrieve it here - service_tag = cert_name_to_service_tag(title) - if service_tag - params['serial_number'] = service_tag - else - params['serial_number'] = title - end - - params['policy_name'] = "policy-#{params['serial_number']}-#{@id}" - - # TODO: if present this should go in kickstart - params.delete('custom_script') + cert_name = comp['id'] + dell_service_tag = cert_name_to_service_tag(cert_name) + # service_tag is only set for Dell servers + if dell_service_tag + deviceconf = ASM::Util.parse_device_config(cert_name) + ASM::WWPN.get( + deviceconf[:host], + deviceconf[:user], + deviceconf[:password] + ) end - - ## get list off WWPN - - (resource_hash['asm::idrac'] || {}).each do |title, params| - ipaddress = deviceconf[:host] - username = deviceconf[:user] - password = deviceconf[:password] - getWWPNData = GetWWPN.new(ipaddress, username, password) - getWWPNd = getWWPNData.getwwpn - res = getWWPNd.split(",") - res.each do |setdata| - wwpnSet.add(setdata) - end - - end - end + end.compact.flatten.uniq end - return wwpnSet end def process_test(component) @@ -504,8 +426,29 @@ def process_test(component) def process_storage(component) log("Processing storage component: #{component['id']}") - config = ASM::Util.build_component_configuration(component) - process_generic(component['id'], config, 'device', true, nil, component['asmGUID']) + + resource_hash = ASM::Util.build_component_configuration(component) + + wwpns = nil + (resource_hash['compellent::createvol'] || {}).each do |title, params| + + # TODO this can't be right, it should not be all servers, but + # just those that are related components + wwpns ||= (get_dell_server_wwpns || []) + + new_wwns = params['wwn'].split(',') + wwpns + + resource_hash['compellent::createvol'][title]['wwn'] = new_wwns + end + + process_generic( + component['id'], + resource_hash, + 'device', + true, + nil, + component['asmGUID'] + ) end def process_tor(component) @@ -733,9 +676,12 @@ def get_portchannel(interfaceList) return portchannellist end - # If certificate name is of the form bladeserver-SERVICETAG - # or rackserver-SERVICETAG, return the service tag; - # otherwise return the certificate name + # + # Dell specific servers have the service tag in + # the certificate name. This method returns a service + # tag for certificate names of Dell servers and + # returns nil for non-dell servers + # def cert_name_to_service_tag(title) match = /^(bladeserver|rackserver)-(.*)$/.match(title) if match @@ -925,7 +871,7 @@ def get_server_macaddress(dracipaddress,dracusername,dracpassword,certname,serve end end end - elsif servermodel.downcase == "m620" || servermodel.downcase == "m420" || servermodel.downcase == "m820" || + elsif servermodel.downcase == "m620" || servermodel.downcase == "m420" || servermodel.downcase == "m820" macAddress = "" resp.split("\n").each do |line| line = line.to_s From d96191e1f20b264b6dc46ab7316d348a4ed2215b Mon Sep 17 00:00:00 2001 From: Dan Bode Date: Thu, 13 Feb 2014 20:15:17 +0000 Subject: [PATCH 2/2] Ensure that resource files don't conflict Currently, calling puppet is synchronized per certname, but the creation of the resource file was not. This commit does two things: 1. moves resource file creation into the sync block 2. adds code to ensure that all resource files from a run are maintained (that they don't overwrite each other) --- lib/asm/service_deployment.rb | 46 +++++++++++++++++++----- spec/unit/asm/service_deployment_spec.rb | 29 +++++++++++++++ 2 files changed, 67 insertions(+), 8 deletions(-) diff --git a/lib/asm/service_deployment.rb b/lib/asm/service_deployment.rb index e3fb3e20..f8fd57df 100755 --- a/lib/asm/service_deployment.rb +++ b/lib/asm/service_deployment.rb @@ -304,17 +304,10 @@ def process_generic( resource_file = File.join(resources_dir, "#{cert_name}.yaml") end - File.open(resource_file, 'w') do |fh| - fh.write(config.to_yaml) - end - - override_opt = override ? "--always-override " : "" - noop_opt = @noop ? '--noop' : '' - cmd = "sudo puppet asm process_node --filename #{resource_file} --run_type #{puppet_run_type} --statedir #{resources_dir} #{noop_opt} #{override_opt}#{cert_name}" if @debug logger.info("[DEBUG MODE] execution skipped for '#{cmd}'") + return nil else - puppet_out = File.join(deployment_dir, "#{cert_name}.out") begin # The timeout to obtain the device lock was originally 5 # minutes. However, the equallogic module currently takes > @@ -334,6 +327,15 @@ def process_generic( while(yet_to_run_command) if ASM.block_certname(cert_name) yet_to_run_command = false + puppet_out = File.join(deployment_dir, "#{cert_name}.out") + # synchronize creation of file counter + resource_file = iterate_resource_file(resource_file) + File.open(resource_file, 'w') do |fh| + fh.write(config.to_yaml) + end + override_opt = override ? "--always-override " : "" + noop_opt = @noop ? '--noop' : '' + cmd = "sudo puppet asm process_node --debug --trace --filename #{resource_file} --run_type #{puppet_run_type} --statedir #{resources_dir} #{noop_opt} #{override_opt}#{cert_name}" logger.debug "Executing the command" ASM::Util.run_command(cmd, puppet_out) if puppet_run_type == 'device' @@ -372,6 +374,34 @@ def process_generic( end end + # + # occassionally, the same certificate is re-used by multiple + # components in the same service deployment. This code checks + # if a given filename already exists, and creates a different + # resource file by appending a counter to the end of the + # resource file name. + # + # NOTE : This method is not thread safe. I expects it's calling + # method to invoke it in a way that is thread safe + # + def iterate_resource_file(resource_file) + if File.exists?(resource_file) + # search for all files that match our pattern, increment us! + base_name = File.basename(resource_file, '.yaml') + dir = File.dirname(resource_file) + matching_files = File.join(dir, "#{base_name}___*") + i = 1 + Dir[matching_files].each do |file| + f_split = File.basename(file, '.yaml').split('___') + num = Integer(f_split.last) + i = num > i ? num : i + end + resource_file = File.join(dir, "#{base_name}___#{i + 1}.yaml") + else + resource_file + end + end + def massage_asm_server_params(serial_number, params) if params['rule_number'] raise(Exception, "Did not expect rule_number in asm::server") diff --git a/spec/unit/asm/service_deployment_spec.rb b/spec/unit/asm/service_deployment_spec.rb index 7976f4fb..c0cd68b5 100644 --- a/spec/unit/asm/service_deployment_spec.rb +++ b/spec/unit/asm/service_deployment_spec.rb @@ -270,4 +270,33 @@ end + describe 'dealing with duplicate certs in the same deployment' do + before do + @counter_files = [File.join(@tmp_dir, 'existing_file.yaml')] + end + after do + @counter_files.each do |f| + File.delete(f) if File.exists?(f) + end + end + def write_counter_files + @counter_files.each do |f| + File.open(f, 'w') do |fh| + fh.write('stuff') + end + end + end + it 'should be able to create file counters labeled 2 when files exist' do + write_counter_files + @sd.iterate_resource_file(@counter_files.first).should == File.join(@tmp_dir, 'existing_file___2.yaml') + end + it 'should increment existing counter files' do + @counter_files.push(File.join(@tmp_dir, 'existing_file___4.yaml')) + write_counter_files + @sd.iterate_resource_file(@counter_files.first).should == File.join(@tmp_dir, 'existing_file___5.yaml') + end + it 'should return passed in file when no file exists' do + @sd.iterate_resource_file(@counter_files.first).should == File.join(@tmp_dir, 'existing_file.yaml') + end + end end