Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add released updates. #17

Merged
merged 1 commit into from
Dec 16, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions package/yast2-sap-ha.changes
Original file line number Diff line number Diff line change
@@ -1,3 +1,51 @@
-------------------------------------------------------------------
Tue Sep 6 10:06:08 UTC 2022 - Peter Varkoly <varkoly@suse.com>

- YaST2 HA Setup for SAP Products - cannot input several instance numbers
(bsc#1202979)
- 1.0.15

-------------------------------------------------------------------
Mon Jun 13 14:32:45 UTC 2022 - Peter Varkoly <varkoly@suse.com>

- YaST2 sap_ha tool does not allow digits at the beginning of site names
(bsc#1200427)
- 1.0.14

-------------------------------------------------------------------
Sun May 22 18:09:27 UTC 2022 - Peter Varkoly <varkoly@suse.com>

- Introduce a new function refresh_all_proposals.
This reads the proposal for the modules watchdog and fence.
This is neccessary when reading an earlier configuration.
- Use .gsub instead of File.basename to find all modules files.
Replace tab with spaces.
(bsc#1197290)
- 1.0.13

-------------------------------------------------------------------
Fri May 13 12:09:33 UTC 2022 - Peter Varkoly <varkoly@suse.com>

- system/watchdog.rb searches watchdog modules with .ko extension
but we ship .ko.xz (bsc#1197290)
- 1.0.12

-------------------------------------------------------------------
Tue May 3 13:58:15 UTC 2022 - Peter Varkoly <varkoly@suse.com>

- softdog missing in Yast while configuring HA for SAP Products
(bsc#1199029)
- 1.0.11

-------------------------------------------------------------------
Tue Oct 12 16:08:26 UTC 2021 - Peter Varkoly <varkoly@suse.com>

- "SUSE SAP HA Yast wizard for HANA doesn´t configure the HANA hooks.
(bsc#1190774)
Add SAPHanaSR via global.ini as proposed in
https://documentation.suse.com/sbp/all/html/SLES4SAP-hana-sr-guide-PerfOpt-15/index.html#id-1.10.6.6"
- 1.0.10

-------------------------------------------------------------------
Wed Dec 11 11:07:47 UTC 2019 - Peter Varkoly <varkoly@suse.com>

Expand Down
2 changes: 1 addition & 1 deletion package/yast2-sap-ha.spec
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@


Name: yast2-sap-ha
Version: 1.0.9
Version: 1.0.15
Release: 0

BuildArch: noarch
Expand Down
1 change: 1 addition & 0 deletions src/clients/sap_ha.rb
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,7 @@ def scenario_selection
log.debug "--- called #{self.class}.#{__callee__}:: ret is #{selection.class} ---"
if selection.is_a?(SapHA::HAConfiguration)
@config = selection
@config.refresh_all_proposals
return :next
end
selection
Expand Down
7 changes: 7 additions & 0 deletions src/lib/sap_ha/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,13 @@ def initialize(role = :master)
@platform = SapHA::Helpers.platform_check
end

# Function to refresh the proposals of some modules. This is neccessary when
# loading an old configuration to detect new hardware.
def refresh_all_proposals
@watchdog.refresh_proposals
@fencing.read_system
end

# Product ID setter. Raises an ScenarioNotFoundException if the ID was not found
# @param [String] value product ID
def set_product_id(value)
Expand Down
1 change: 0 additions & 1 deletion src/lib/sap_ha/configuration/hana.rb
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,6 @@ def apply(role)
primary_host_name, @instance, @replication_mode, @operation_mode)
if @additional_instance # cost-optimized scenario
SapHA::System::Hana.hdb_stop(@np_system_id)
SapHA::System::Hana.write_sr_hook(@system_id, @hook_script)
SapHA::System::Hana.adjust_production_system(@system_id,
@hook_script_parameters.merge(@production_constraints))
# SapHA::System::Hana.adjust_non_production_system(@np_system_id)
Expand Down
4 changes: 4 additions & 0 deletions src/lib/sap_ha/configuration/watchdog.rb
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,10 @@ def initialize(global_config)
@proposals = System::Watchdog.list_watchdogs
end

def refresh_proposals
@proposals = System::Watchdog.list_watchdogs
end

def configured?
!@loaded.empty? || !@configured.empty? || !@to_install.empty?
end
Expand Down
18 changes: 11 additions & 7 deletions src/lib/sap_ha/semantic_checks.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,13 @@ class SemanticChecks
attr_accessor :silent
attr_reader :checks_passed

IDENTIFIER_REGEXP = Regexp.new('^[_a-zA-Z][_a-zA-Z0-9]{0,30}$')
#Site identifier regexp. That is what SAP allows. All ASCII charactest from 33 until 125
#expect of '*' and '/'. The identifier can be 256 character long
#However, for security and technical reasons, we only allow alphanumeric characters as well as '-' and '_'.
#The identifier must not be longer than 30 characters and it must be minimum 2 long.
IDENTIFIER_REGEXP = Regexp.new('^[a-zA-Z0-9][a-zA-Z0-9_\-]{1,29}$')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note: do not forget that in Ruby ^ means "start of line", not "start of the string" as usually...

SAP_SID_REGEXP = Regexp.new('^[A-Z][A-Z0-9]{2}$')
SAP_INST_NUM_REGEX = Regexp.new('^[0-9]{2}$')
RESERVED_SAP_SIDS = %w(ADD ALL AND ANY ASC COM DBA END EPS FOR GID IBM INT KEY LOG MON NIX
NOT OFF OMS RAW ROW SAP SET SGA SHG SID SQL SYS TMP UID USR VAR).freeze

Expand Down Expand Up @@ -235,19 +240,18 @@ def integer_in_range(value, low, high, message = '', field_name = '')
def sap_sid(value, message = '', field_name = '')
message = "A valid SAP System ID consists of three characters, starts with a letter, and "\
" must not collide with one of the reserved IDs" if message.nil? || message.empty?
flag = !SAP_SID_REGEXP.match(value).nil?
flag &= !RESERVED_SAP_SIDS.include?(value)
flag = SAP_SID_REGEXP.match?(value) && !RESERVED_SAP_SIDS.include?(value)
report_error(flag, message, field_name, value)
end

# Check if the provided value is a correct SAP Instance number
# @param value [String]
# @param message [String] custom error message
# @param field_name [String] name of the related field in the form
def sap_instance_number(value, message, field_name)
return report_error(false, 'The SAP Instance number must be a string of exactly two digits',
field_name, value) unless value.is_a?(::String) && value.length == 2
integer_in_range(value, 0, 99, message, field_name)
def sap_instance_number(value, message = '', field_name = 'Instant Number')
message = "The SAP Instance number must be a string of exactly two digits " if message.nil? || message.empty?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Is the trailing space in the message OK?
  • Quoting is inconsistent, most of the code uses single quotes (`) , here are double quotes (")
    (In YaST we use double quotes, but I'm fine with single quotes, it just should be consistent.)

flag = SAP_INST_NUM_REGEX.match?(value)
report_error(flag, message, field_name, value)
end

# Check if the provided value is a non-empty string
Expand Down
40 changes: 8 additions & 32 deletions src/lib/sap_ha/system/hana.rb
Original file line number Diff line number Diff line change
Expand Up @@ -213,35 +213,6 @@ def set_secute_store(system_id, key_name, env, user_name, password)
)
end

# Write the takeover hook script
# @param system_id [String] HANA System ID (production)
# @param hook_script [String] HANA takeover hook script
def write_sr_hook(system_id, hook_script)
log.info "--- called #{self.class}.#{__callee__}(#{system_id}, #{hook_script.inspect}) ---"
hook_dir = "/hana/shared/#{system_id.upcase}/srHook"
begin
Dir.mkdir(hook_dir) unless Dir.exist?(hook_dir)
rescue StandardError => e
log.error "Error creating directory #{hook_dir}: #{e.message}"
NodeLogger.error("Could not create directory #{hook_dir}")
NodeLogger.output(e.message)
return false
end
begin
hook_path = File.join(hook_dir, 'srTakeover.py')
File.open(hook_path, 'wb') do |fh|
fh.write(hook_script)
end
rescue StandardError => e
log.error "Error writing file #{hook_path}: #{e.message}"
NodeLogger.error("Could not write the system replication hook script to #{hook_path}")
NodeLogger.output(e.message)
return false
end
NodeLogger.info("Wrote system replication hook script to #{hook_path}")
true
end

# Implement adjustments to the production system, so that a non-production
# HANA could be run along it
# @param system_id [String] HANA System ID (production)
Expand All @@ -258,11 +229,12 @@ def adjust_production_system(system_id, options = {})
begin
global_ini = CFA::GlobalIni.new(global_ini_path)
global_ini.load
global_ini.set_config('ha_dr_provider_srTakeover', 'provider', 'srTakeover')
global_ini.set_config('ha_dr_provider_srTakeover', 'path', "/hana/shared/#{system_id.upcase}/srHook/srTakeover.py")
global_ini.set_config('ha_dr_provider_srTakeover', 'execution_order', options[:execution_order])
global_ini.set_config('memorymanager', 'global_allocation_limit', options[:global_alloc_limit])
global_ini.set_config('system_replication', 'preload_column_tables', options[:preload_column_tables])
global_ini.set_config('ha_dr_provider_SAPHanaSR', 'provider', 'SAPHanaSR')
global_ini.set_config('ha_dr_provider_SAPHanaSR', 'path', '/usr/share/SAPHanaSR')
global_ini.set_config('ha_dr_provider_SAPHanaSR', 'execution_order', '1')
global_ini.set_config('trace', 'ha_dr_saphanasr', 'info')
global_ini.save
rescue StandardError => e
NodeLogger.error "Could not adjust global.ini for the production system:"
Expand Down Expand Up @@ -290,6 +262,10 @@ def adjust_non_production_system(system_id, options = {})
global_ini = CFA::GlobalIni.new(global_ini_path)
global_ini.load
global_ini.set_config('memorymanager', 'global_allocation_limit', options[:global_alloc_limit])
global_ini.set_config('ha_dr_provider_SAPHanaSR', 'provider', 'SAPHanaSR')
global_ini.set_config('ha_dr_provider_SAPHanaSR', 'path', '/usr/share/SAPHanaSR')
global_ini.set_config('ha_dr_provider_SAPHanaSR', 'execution_order', '1')
global_ini.set_config('trace', 'ha_dr_saphanasr', 'info')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a lot of duplication from the previous method, maybe it would make sense to share the common part. But as this is for a maintenance update it would be more safe to do this only in the master branch:

def adjust_non_production_system(system_id, options = {})
  log.info "--- called #{self.class}.#{__callee__}(#{system_id}, #{options.inspect}) ---"
  adjust_system(system_id, options, 'non-production')
end

def adjust_production_system(system_id, options = {})
  log.info "--- called #{self.class}.#{__callee__}(#{system_id}, #{options.inspect}) ---"
  adjust_system(system_id, options, 'production') do |ini|
    ini.set_config('system_replication', 'preload_column_tables', options[:preload_column_tables])
  end
end

def adjust_system(system_id, options = {}, system_type = "", &block)
  # Change the global.ini
  global_ini_path = HANA_GLOBAL_INI % system_id.upcase
  unless File.exist?(global_ini_path)
    NodeLogger.error "Could not adjust global.ini for the #{system_type} system:"
    NodeLogger.output "File #{global_ini_path} does not exist"
    return false
  end
  begin
    global_ini = CFA::GlobalIni.new(global_ini_path)
    global_ini.load
    global_ini.set_config('memorymanager', 'global_allocation_limit', options[:global_alloc_limit])
    global_ini.set_config('ha_dr_provider_SAPHanaSR', 'provider', 'SAPHanaSR')
    global_ini.set_config('ha_dr_provider_SAPHanaSR', 'path', '/usr/share/SAPHanaSR')
    global_ini.set_config('ha_dr_provider_SAPHanaSR', 'execution_order', '1')
    global_ini.set_config('trace', 'ha_dr_saphanasr', 'info')

    block.call(global_ini) if block_given?

    global_ini.save
  rescue StandardError => e
    NodeLogger.error "Could not adjust global.ini for the #{system_type} system:"
    NodeLogger.output e.message
    return false
  else
    NodeLogger.info "Successfully adjusted global.ini for the #{system_type} system #{system_id}"
    true
  end
end

You can make the adjust_system method private.

Untested! Quick Hack! Please test it well!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've applied your suggestions in master.

global_ini.save
rescue StandardError => e
NodeLogger.error "Could not adjust global.ini for the non-production system:"
Expand Down
6 changes: 3 additions & 3 deletions src/lib/sap_ha/system/network.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,15 +41,15 @@ def interfaces
# Get local machine's IPv4 addresses excluding the loopback iface
def ip_addresses
interfaces = Socket.getifaddrs.select do |iface|
iface.addr.ipv4? && !iface.addr.ipv4_loopback?
!iface.addr.nil? && iface.addr.ipv4? && !iface.addr.ipv4_loopback?
end
interfaces.map { |iface| iface.addr.ip_address }
end

# Get a list of network addresses on the local node's interface
def network_addresses
interfaces = Socket.getifaddrs.select do |iface|
iface.addr.ipv4? && !iface.addr.ipv4_loopback?
!iface.addr.nil? && iface.addr.ipv4? && !iface.addr.ipv4_loopback?
end
interfaces.map do |iface|
IPAddr.new(iface.addr.ip_address).mask(iface.netmask.ip_address).to_s
Expand All @@ -59,7 +59,7 @@ def network_addresses
# Get a list of network addresses along with the CIDR mask
def network_addresses_cidr
interfaces = Socket.getifaddrs.select do |iface|
iface.addr.ipv4? && !iface.addr.ipv4_loopback?
!iface.addr.nil? && iface.addr.ipv4? && !iface.addr.ipv4_loopback?
end
interfaces.map do |iface|
IPAddr.new(iface.addr.ip_address).mask(iface.netmask.ip_address).to_s + '/' +
Expand Down
3 changes: 2 additions & 1 deletion src/lib/sap_ha/system/watchdog.rb
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,8 @@ def list_watchdogs
log.error "Could not find the kernel modules source directory #{MODULES_PATH}"
return []
end
Dir.glob(MODULES_PATH + '/*.ko').map { |path| File.basename(path, '.ko') }
wmods = Dir.glob(MODULES_PATH + '/*.ko*').map { |path| File.basename(path).gsub(/\.ko[\.\S+]*$/,'') }
wmods
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wmods is unused, you can remove that

end

# Look into the /etc/modules-load.d and list all of the modules
Expand Down
33 changes: 33 additions & 0 deletions test/semantic_checks_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -139,4 +139,37 @@
expect(subject.ipv4_in_network_cidr('192.168.100.16', '192.168.100.0/28')).to eq false
end
end

describe 'SAP naming tests' do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some notes:

  • describe should be used for methods, if you need to group some tests use context. (Technically it is irrelevant, one of them is just an alias to the other one, but it makes the tests better structured and more readable.)
  • describe should use a method name if possible/makes sense
  • you can put the repeating parts into before block
  • it should describe the action and the result

You can find more suggestions at https://www.betterspecs.org/

Proposal:

context 'SAP naming tests' do
  before do
    subject.silent = true
  end

  describe "#identifier" do
    it "returns true for valid IDs" do
      expect(subject.identifier('1Nuremberg')).to eq true
      expect(subject.identifier('1Nuremberg-A')).to eq true
      expect(subject.identifier('1Nuremberg_BA')).to eq true
      expect(subject.identifier('SuSE-1_2')).to eq true
    end

    it "returns false for invalid IDs" do
      expect(subject.identifier('Nürnberg')).to eq false
      expect(subject.identifier('-Nuremberg')).to eq false
      expect(subject.identifier('_Nuremberg')).to eq false
      expect(subject.identifier('*WalDorf-1_2/')).to eq false
      end
  end

  describe "#sap_sid" do
    it "returns true for valid IDs" do
      expect(subject.sap_sid('X12')).to eq true
    end

    it "returns false for invalid IDs" do
      expect(subject.sap_sid('123')).to eq false
      expect(subject.sap_sid('NOT')).to eq false
    end
  end

  describe "#sap_instance_number" do
    it "returns true for valid numbers" do
      expect(subject.sap_instance_number('05')).to eq true
      expect(subject.sap_instance_number('09')).to eq true
      expect(subject.sap_instance_number('10')).to eq true
      expect(subject.sap_instance_number('99')).to eq true
      end

    it "returns false for invalid numbers" do
      expect(subject.sap_instance_number('1')).to eq false
      expect(subject.sap_instance_number('1A')).to eq false
      expect(subject.sap_instance_number('999')).to eq false
      end
  end
end

Again, quick hack, please test it!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot!
I've applied your suggestions in master.

it 'reports if valid SID and site names will be allowed' do
subject.silent = true
expect(subject.identifier('1Nuremberg')).to eq true
expect(subject.identifier('1Nuremberg-A')).to eq true
expect(subject.identifier('1Nuremberg_BA')).to eq true
expect(subject.identifier('SuSE-1_2')).to eq true
expect(subject.sap_sid('X12')).to eq true
end
it 'reports if invalid SID and site names will be found' do
subject.silent = true
expect(subject.identifier('Nürnberg')).to eq false
expect(subject.identifier('-Nuremberg')).to eq false
expect(subject.identifier('_Nuremberg')).to eq false
expect(subject.identifier('*WalDorf-1_2/')).to eq false
expect(subject.sap_sid('123')).to eq false
expect(subject.sap_sid('NOT')).to eq false
end
it 'reports if valid instance number will be allowed' do
subject.silent = true
expect(subject.sap_instance_number('05')).to eq true
expect(subject.sap_instance_number('09')).to eq true
expect(subject.sap_instance_number('10')).to eq true
expect(subject.sap_instance_number('99')).to eq true
end
it 'reports if invalid instance number will be found' do
subject.silent = true
expect(subject.sap_instance_number('1')).to eq false
expect(subject.sap_instance_number('1A')).to eq false
expect(subject.sap_instance_number('999')).to eq false
end
end
end
33 changes: 0 additions & 33 deletions test/src/lib/sap_ha/system/hana_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -346,39 +346,6 @@
end
end

describe '#write_sr_hook' do
context 'given all prerequisites are satisfied' do
it 'works' do
io = StringIO.new
expect(Dir).to receive(:mkdir).with('/hana/shared/XXX/srHook').and_return(0)
expect(File).to receive(:open)
.with('/hana/shared/XXX/srHook/srTakeover.py', 'wb')
.and_yield(io)
flag = SapHA::System::Hana.write_sr_hook('XXX', 'hook string')
expect(flag).to eq(true), SapHA::NodeLogger.text
expect(io.string).to eq 'hook string'
end
end

context 'given the directory cannot be created' do
it 'fails to write the hook file' do
expect(Dir).to receive(:mkdir).with('/hana/shared/XXX/srHook').and_raise(Errno::ENOENT)
flag = SapHA::System::Hana.write_sr_hook('XXX', 'hook string')
expect(flag).to eq(false)
end
end

context 'given that the file could not be opened' do
it 'fails to write the hook file' do
expect(Dir).to receive(:mkdir).with('/hana/shared/XXX/srHook').and_return(0)
expect(File).to receive(:open).with('/hana/shared/XXX/srHook/srTakeover.py', 'wb')
.and_raise(Errno::ENOENT)
flag = SapHA::System::Hana.write_sr_hook('XXX', 'hook string')
expect(flag).to eq(false)
end
end
end

# # TODO:
# describe '#adjust_production_system' do
# context 'all is good' do
Expand Down
62 changes: 47 additions & 15 deletions test/test_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,43 @@
# Set the paths
ENV['Y2DIR'] = File.expand_path('../../src', __FILE__)

if ENV['COVERAGE']
require 'simplecov'
SimpleCov.start
RSpec.configure do |config|
config.mock_with :rspec do |mocks|
# If you misremember a method name both in code and in tests,
# will save you.
# https://relishapp.com/rspec/rspec-mocks/v/3-0/docs/verifying-doubles/partial-doubles
#
# With graceful degradation for RSpec 2
mocks.verify_partial_doubles = true if mocks.respond_to?(:verify_partial_doubles=)
end
end

if ENV["COVERAGE"]
require "simplecov"
SimpleCov.start do
add_filter "/test/"
end

src_location = File.expand_path("../src", __dir__)
# track all ruby files under src
SimpleCov.track_files("#{src_location}/**/*.rb")

# additionally use the LCOV format for on-line code coverage reporting at CI
if ENV["CI"] || ENV["COVERAGE_LCOV"]
require "simplecov-lcov"

SimpleCov::Formatter::LcovFormatter.config do |c|
c.report_with_single_file = true
# this is the default Coveralls GitHub Action location
# https://github.com/marketplace/actions/coveralls-github-action
c.single_report_path = "coverage/lcov.info"
end

SimpleCov.formatter = SimpleCov::Formatter::MultiFormatter[
SimpleCov::Formatter::HTMLFormatter,
SimpleCov::Formatter::LcovFormatter
]
end
end

require 'yast'
Expand Down Expand Up @@ -95,20 +129,18 @@ def _make_basic_ha_config(config, product_id, scenario_name, options = {})
backup_file: options.fetch(:backup_file, 'mybackupfile2'),
perform_backup: options.fetch(:perform_backup, false)
)
ntp_cfg = { "synchronize_time" => false,
"sync_interval" => 5,
"start_at_boot" => true,
"start_in_chroot" => false,
"ntp_policy" => "auto",
"peers" => [{ "type" => "server",
"address" => "ntp.local",
"options" => " iburst",
"comment" => "# key (6) for accessing server variables\n" }],
"restricts" => []
}
ntp_cfg = {
"ntp_sync"=>"systemd",
"ntp_policy"=>"auto",
"ntp_servers"=>[
{"address"=>"0.suse.pool.ntp.org", "iburst"=>true, "offline"=>false},
{"address"=>"1.suse.pool.ntp.org", "iburst"=>true, "offline"=>false},
{"address"=>"2.suse.pool.ntp.org", "iburst"=>true, "offline"=>false},
{"address"=>"3.suse.pool.ntp.org", "iburst"=>true, "offline"=>false}]
}
config.ntp.import(
config: ntp_cfg,
used_servers: ['ntp.local']
used_servers: ['2.suse.pool.ntp.org']
)
config
end