From 5ac94c5509e368aac834709c50af26573d135375 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Imobach=20Gonz=C3=A1lez=20Sosa?= Date: Wed, 25 Mar 2020 15:48:12 +0000 Subject: [PATCH 01/14] Extend DASDController.Write tests --- test/dasd_controller_test.rb | 122 ++++++++++++++++++++++++++++++----- 1 file changed, 106 insertions(+), 16 deletions(-) diff --git a/test/dasd_controller_test.rb b/test/dasd_controller_test.rb index 5227e99d..0fc8f5d1 100755 --- a/test/dasd_controller_test.rb +++ b/test/dasd_controller_test.rb @@ -143,8 +143,10 @@ describe "#Write" do let(:data) do { "devices" => [{ "channel" => "0.0.0100", "diag" => false, - "format" => true }], "format_unformatted" => true } + "format" => format }], "format_unformatted" => format_unformatted } end + let(:format_unformatted) { false } + let(:format) { true } before do allow(Yast::SCR).to receive(:Execute).with(path(".target.bash_output"), @@ -160,25 +162,93 @@ allow(Yast::DASDController).to receive(:GetDeviceName).and_return("/dev/dasda") end - it "writes the dasd settings to the target (formating disks)" do - # bnc 928388 - expect(Yast::SCR).to receive(:Execute).with(path(".process.start_shell"), - "/sbin/dasdfmt -Y -P 1 -b 4096 -y -r 10 -m 10 -f '/dev/dasda'") + context "during autoinstallation" do + before do + Yast::DASDController.Import(data) + end - expect(Yast::DASDController.Import(data)).to eq(true) - expect(Yast::DASDController.Write).to eq(true) - end + it "activates the disk" do + allow(Yast::DASDController).to receive(:FormatDisks) + expect(Yast::DASDController).to receive(:ActivateDisk).with("0.0.0100", false) + Yast::DASDController.Write + end - it "does not format disk for FBA disk" do - allow(Yast::SCR).to receive(:Execute).with(path(".target.bash_output"), - /\/sbin\/dasdview/) - .and_return("exitstatus" => 0, "stdout" => load_file("dasdview_fba.txt"), "stderr" => "") + context "when 'format' is sets to true" do + let(:format) { true } + + it "formats the disk" do + # bnc 928388 + expect(Yast::SCR).to receive(:Execute).with(path(".process.start_shell"), + "/sbin/dasdfmt -Y -P 1 -b 4096 -y -r 10 -m 10 -f '/dev/dasda'") + expect(Yast::DASDController.Write).to eq(true) + end + end + + context "when 'format' is set to false" do + let(:format) { false } + + it "does not format the disk" do + expect(Yast::DASDController).to_not receive(:FormatDisks) + Yast::DASDController.Write + end + end + + context "when the activated device is not formatted" do + NOT_FORMATTED_CODE = 8 # means that the device is not formatted + + let(:format) { false } + + before do + allow(Yast::DASDController).to receive(:ActivateDisk).with("0.0.0100", false) + .and_return(NOT_FORMATTED_CODE) + end + + context "and 'format_unformatted' is set to 'true'" do + let(:format_unformatted) { true } + + it "formats the device" do + expect(Yast::DASDController).to receive(:FormatDisks).with(["/dev/dasda"], anything) + Yast::DASDController.Write + end + end + + context "and 'format_unformatted' is set to 'false'" do + let(:format_unformatted) { false } + + it "does not format the device" do + expect(Yast::DASDController).to_not receive(:FormatDisks) + Yast::DASDController.Write + end + end + + context "and 'format' is set to 'true'" do + let(:format) { true } + let(:format_unformatted) { false } - expect(Yast::SCR).to_not receive(:Execute).with(path(".process.start_shell"), - /dasdfmt.*\/dev\/dasda/) + it "formats the device" do + expect(Yast::DASDController).to receive(:FormatDisks).with(["/dev/dasda"], anything) + Yast::DASDController.Write + end - expect(Yast::DASDController.Import(data)).to eq(true) - expect(Yast::DASDController.Write).to eq(true) + it "reactivates the disk" do + expect(Yast::DASDController).to receive(:FormatDisks) + expect(Yast::DASDController).to receive(:ActivateDisk).twice + Yast::DASDController.Write + end + end + end + + it "does not format disk for FBA disk" do + allow(Yast::SCR).to receive(:Execute).with(path(".target.bash_output"), + /\/sbin\/dasdview/) + .and_return("exitstatus" => 0, "stdout" => load_file("dasdview_fba.txt"), "stderr" => "") + + expect(Yast::SCR).to_not receive(:Execute).with(path(".process.start_shell"), + /dasdfmt.*\/dev\/dasda/) + + expect(Yast::DASDController.Import(data)).to eq(true) + expect(Yast::DASDController.Write).to eq(true) + end end end @@ -242,6 +312,7 @@ "device" => "DASD", "dev_name" => "/dev/dasda", "sysfs_bus_id" => "0.0.0150", + "sysfs_id" => "/class/block/dasda", "resource" => { "io" => ["active" => true] } @@ -262,6 +333,25 @@ expect(subject.devices.size).to eq 1 expect(subject.devices.values.first["formatted"]).to eq false end + + context "when the 'use_diag' file exists" do + let(:diag_path) { "/sys//class/block/dasda/device/use_diag" } + + before do + allow(Yast::FileUtils).to receive(:Exists).with(diag_path) + .and_return(true) + allow(Yast::SCR).to receive(:Read) + .with(Yast::Path.new(".target.string"), diag_path) + .and_return("1") + end + + it "reads its value" do + subject.ProbeDisks + device = subject.devices.values.first + expect(device["diag"]).to eq(true) + expect(subject.diag).to eq("0.0.0150" => true) + end + end end end From 0297433076b1402d1ee3f8e893ad7592461aba56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Imobach=20Gonz=C3=A1lez=20Sosa?= Date: Thu, 26 Mar 2020 09:20:25 +0000 Subject: [PATCH 02/14] DASDController: extract the logic to find disks to another method --- src/modules/DASDController.rb | 142 ++++++++++++++++++---------------- 1 file changed, 77 insertions(+), 65 deletions(-) diff --git a/src/modules/DASDController.rb b/src/modules/DASDController.rb index 19a20cef..8d80b1e9 100644 --- a/src/modules/DASDController.rb +++ b/src/modules/DASDController.rb @@ -381,71 +381,7 @@ def ProbeDisks # popup label UI.OpenDialog(Label(_("Reading Configured DASD Disks"))) - disks = Convert.convert( - SCR.Read(path(".probe.disk")), - from: "any", - to: "list >" - ) - disks = Builtins.filter(disks) do |d| - Builtins.tolower(Ops.get_string(d, "device", "")) == "dasd" - end - - disks.sort_by! { |disk| FormatChannel(disk.fetch("sysfs_bus_id", "0.0.0000")) } - - disks = Builtins.maplist(disks) do |d| - channel = Ops.get_string(d, "sysfs_bus_id", "") - Ops.set(d, "channel", channel) - active = Ops.get_boolean(d, ["resource", "io", 0, "active"], false) - if active - device = Ops.get_string(d, "dev_name", "") - scr_out = Convert.to_map( - SCR.Execute( - path(".target.bash_output"), - Builtins.sformat("/sbin/dasdview --extended '%1' | grep formatted", device) - ) - ) - formatted = false - if Ops.get_integer(scr_out, "exit", 0) == 0 - out = Ops.get_string(scr_out, "stdout", "") - formatted = !Builtins.regexpmatch( - Builtins.toupper(out), - "NOT[ \t]*FORMATTED" - ) - end - Ops.set(d, "formatted", formatted) - - Ops.set(d, "partition_info", GetPartitionInfo(device)) if formatted - - diag_file = Builtins.sformat( - "/sys/%1/device/use_diag", - Ops.get_string(d, "sysfs_id", "") - ) - if FileUtils.Exists(diag_file) - use_diag = Convert.to_string( - SCR.Read(path(".target.string"), diag_file) - ) - Ops.set(d, "diag", Builtins.substring(use_diag, 0, 1) == "1") - Ops.set(@diag, channel, Builtins.substring(use_diag, 0, 1) == "1") - end - end - d = Builtins.filter(d) do |k, _v| - Builtins.contains( - [ - "channel", - "diag", - "resource", - "formatted", - "partition_info", - "dev_name", - "detail", - "device_id", - "sub_device_id" - ], - k - ) - end - deep_copy(d) - end + disks = find_disks index = -1 @devices = Builtins.listmap(disks) do |d| @@ -453,6 +389,10 @@ def ProbeDisks { index => d } end + disks.each do |dev| + @diag[dev["channel"]] = dev["diag"] if dev["diag"] + end + Builtins.y2milestone("probed DASD devices %1", @devices) UI.CloseDialog @@ -851,6 +791,78 @@ def report_error(headline, message, details) Yast2::Popup.show(message, headline: headline, details: details) end end + + # Returns the DASD disks + # + # Probes and returns the found DASD disks ordered by channel. + # + # @return [Array] Found DASD disks + def find_disks + disks = Convert.convert( + SCR.Read(path(".probe.disk")), + from: "any", + to: "list >" + ) + disks = Builtins.filter(disks) do |d| + Builtins.tolower(Ops.get_string(d, "device", "")) == "dasd" + end + + disks.sort_by! { |disk| FormatChannel(disk.fetch("sysfs_bus_id", "0.0.0000")) } + + disks = Builtins.maplist(disks) do |d| + channel = Ops.get_string(d, "sysfs_bus_id", "") + Ops.set(d, "channel", channel) + active = Ops.get_boolean(d, ["resource", "io", 0, "active"], false) + if active + device = Ops.get_string(d, "dev_name", "") + scr_out = Convert.to_map( + SCR.Execute( + path(".target.bash_output"), + Builtins.sformat("/sbin/dasdview --extended '%1' | grep formatted", device) + ) + ) + formatted = false + if Ops.get_integer(scr_out, "exit", 0) == 0 + out = Ops.get_string(scr_out, "stdout", "") + formatted = !Builtins.regexpmatch( + Builtins.toupper(out), + "NOT[ \t]*FORMATTED" + ) + end + Ops.set(d, "formatted", formatted) + + Ops.set(d, "partition_info", GetPartitionInfo(device)) if formatted + + diag_file = Builtins.sformat( + "/sys/%1/device/use_diag", + Ops.get_string(d, "sysfs_id", "") + ) + if FileUtils.Exists(diag_file) + use_diag = Convert.to_string( + SCR.Read(path(".target.string"), diag_file) + ) + Ops.set(d, "diag", Builtins.substring(use_diag, 0, 1) == "1") + end + end + d = Builtins.filter(d) do |k, _v| + Builtins.contains( + [ + "channel", + "diag", + "resource", + "formatted", + "partition_info", + "dev_name", + "detail", + "device_id", + "sub_device_id" + ], + k + ) + end + deep_copy(d) + end + end end DASDController = DASDControllerClass.new From e2969b55fda90461d1920a68b15b9be46bf8c2be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Imobach=20Gonz=C3=A1lez=20Sosa?= Date: Thu, 26 Mar 2020 13:10:32 +0000 Subject: [PATCH 03/14] Do not activate an already active DASD disk --- src/modules/DASDController.rb | 33 +++++++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(-) diff --git a/src/modules/DASDController.rb b/src/modules/DASDController.rb index 8d80b1e9..40b6e3e2 100644 --- a/src/modules/DASDController.rb +++ b/src/modules/DASDController.rb @@ -143,7 +143,7 @@ def Write channel = Ops.get_string(device, "channel", "") format = Ops.get_boolean(device, "format", false) do_diag = Ops.get_boolean(device, "diag", false) - act_ret = ActivateDisk(channel, do_diag) + act_ret = activate_disk_if_needed(channel, do_diag) # FIXME: general activation error handling - also in sync with below # for AutoInstall, format unformatted disks later at once # even disks manually selected for formatting must be reactivated @@ -381,7 +381,7 @@ def ProbeDisks # popup label UI.OpenDialog(Label(_("Reading Configured DASD Disks"))) - disks = find_disks + disks = find_disks(true) index = -1 @devices = Builtins.listmap(disks) do |d| @@ -792,12 +792,37 @@ def report_error(headline, message, details) end end + # Activates a disk if its not activated + # + # When the disk is already activated, it returns '8' if the + # disk is unformatted or '0' otherwise. The idea is to mimic + # the same API that ActivateDisk. + # + # @return [Boolean] Returns an error code (8 means 'unformatted'). + def activate_disk_if_needed(channel, diag) + disk = find_disks.find { |d| d["channel"] == channel } + return ActivateDisk(channel, diag) unless disk && active_disk?(disk) + disk["formatted"] ? 0 : 8 + end + + # Determines whether the disk is activated or not + # + # @param disk [Hash] + # @return [Boolean] + def active_disk?(disk) + io = disk.fetch("resource", {}).fetch("io", []).first + !!(io && io["active"]) + end + # Returns the DASD disks # # Probes and returns the found DASD disks ordered by channel. + # It caches the found disks. # + # @param force_probing [Boolean] Ignore the cached values and probes again. # @return [Array] Found DASD disks - def find_disks + def find_disks(force_probing = false) + return @disks if @disks && !force_probing disks = Convert.convert( SCR.Read(path(".probe.disk")), from: "any", @@ -809,7 +834,7 @@ def find_disks disks.sort_by! { |disk| FormatChannel(disk.fetch("sysfs_bus_id", "0.0.0000")) } - disks = Builtins.maplist(disks) do |d| + @disks = Builtins.maplist(disks) do |d| channel = Ops.get_string(d, "sysfs_bus_id", "") Ops.set(d, "channel", channel) active = Ops.get_boolean(d, ["resource", "io", 0, "active"], false) From e09ae80e2b0bf9770ea4364a542df97c533970b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Imobach=20Gonz=C3=A1lez=20Sosa?= Date: Thu, 26 Mar 2020 15:50:12 +0000 Subject: [PATCH 04/14] ZFCPController: extract controller activation to a separate method --- src/modules/ZFCPController.rb | 47 ++++++++++++++++++++--------------- test/zfcp_controller_test.rb | 46 ++++++++++++++++++++++++++++------ 2 files changed, 65 insertions(+), 28 deletions(-) diff --git a/src/modules/ZFCPController.rb b/src/modules/ZFCPController.rb index 880704af..6ffcc032 100644 --- a/src/modules/ZFCPController.rb +++ b/src/modules/ZFCPController.rb @@ -578,31 +578,38 @@ def ReportControllerActivationError(channel, ret) nil end + # Activates the controller unless it was already activated + # + # @param channel [String] channel + def activate_controller(channel) + return if @activated_controllers[channel] + + command2 = Builtins.sformat( + "/sbin/zfcp_host_configure '%1' %2", + channel, + 1 + ) + Builtins.y2milestone("Running command \"%1\"", command2) + ret2 = Convert.to_integer(SCR.Execute(path(".target.bash"), command2)) + Builtins.y2milestone( + "Command \"%1\" returned with exit code %2", + command2, + ret2 + ) + + if ret2 != 0 + ReportControllerActivationError(channel, ret2) + else + Ops.set(@activated_controllers, channel, true) + end + end + # Activate a disk # @param [String] channel string channel # @param [String] wwpn string wwpn (hexa number) # @param [String] lun string lun (hexa number) def ActivateDisk(channel, wwpn, lun) - if !Ops.get(@activated_controllers, channel, false) - command2 = Builtins.sformat( - "/sbin/zfcp_host_configure '%1' %2", - channel, - 1 - ) - Builtins.y2milestone("Running command \"%1\"", command2) - ret2 = Convert.to_integer(SCR.Execute(path(".target.bash"), command2)) - Builtins.y2milestone( - "Command \"%1\" returned with exit code %2", - command2, - ret2 - ) - - if ret2 != 0 - ReportControllerActivationError(channel, ret2) - else - Ops.set(@activated_controllers, channel, true) - end - end + activate_controller(channel) if wwpn != "" || lun != "" # we are not using allow_lun_scan command = Builtins.sformat( diff --git a/test/zfcp_controller_test.rb b/test/zfcp_controller_test.rb index 0cc1b8b7..838a6fca 100755 --- a/test/zfcp_controller_test.rb +++ b/test/zfcp_controller_test.rb @@ -5,22 +5,52 @@ Yast.import "ZFCPController" describe "Yast::ZFCPController" do + before do + Yast::ZFCPController.main + end describe "#ActivateDisk" do it "Activates the given disk" do - expect(Yast::SCR).to receive(:Execute).with(anything, /\/sbin\/zfcp_host_configure '1' 1/).and_return(0) - expect(Yast::ZFCPController).to_not receive(:ReportControllerActivationError) - Yast::ZFCPController.ActivateDisk(1, "", "") + expect(Yast::ZFCPController).to receive(:activate_controller).with('0.0.fc00') + expect(Yast::SCR).to receive(:Execute) + .with(anything, /\/sbin\/zfcp_disk_configure '0.0.fc00' '0x500' '0x401' 1/) + .and_return(0) + Yast::ZFCPController.ActivateDisk("0.0.fc00", "0x500", "0x401") end end - describe "#GetControllers" do - after do - # workaround: the GetControllers() result is cached, force reset - # after each test - Yast::ZFCPController.instance_eval("@controllers = nil") + describe "#activate_controller" do + let(:channel) { "0.0.fc00" } + + it "activates the given controller" do + expect(Yast::SCR).to receive(:Execute) + .with(anything, /\/sbin\/zfcp_host_configure '0.0.fc00' 1/).and_return(0) + expect(Yast::ZFCPController).to_not receive(:ReportControllerActivationError) + Yast::ZFCPController.activate_controller(channel) + end + + it "does not activate a controller twice" do + expect(Yast::SCR).to receive(:Execute) + .with(anything, /\/sbin\/zfcp_host_configure '0.0.fc00' 1/).once.and_return(0) + Yast::ZFCPController.activate_controller(channel) + Yast::ZFCPController.activate_controller(channel) end + context "when the activation fails" do + before do + allow(Yast::SCR).to receive(:Execute) + .with(anything, /\/sbin\/zfcp_host_configure '0.0.fc00' 1/).and_return(1) + end + + it "reports the error" do + expect(Yast::ZFCPController).to receive(:ReportControllerActivationError) + .with('0.0.fc00', 1) + Yast::ZFCPController.activate_controller(channel) + end + end + end + + describe "#GetControllers" do it "Returns all controllers" do allow(Yast::Arch).to receive(:is_zkvm).and_return(false) expect(Yast::SCR).to receive(:Read).with(Yast.path(".probe.storage")).once From e64ee9161df19ff9d4373cb0b7048d7565ac0fca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Imobach=20Gonz=C3=A1lez=20Sosa?= Date: Thu, 26 Mar 2020 16:32:08 +0000 Subject: [PATCH 05/14] zFCP: do not activate again already activated controllers --- src/modules/ZFCPController.rb | 39 ++++++++++++++++++++++++++---- test/data/probe_storage.yml | 2 +- test/zfcp_controller_test.rb | 45 ++++++++++++++++++++++++++++------- 3 files changed, 72 insertions(+), 14 deletions(-) diff --git a/src/modules/ZFCPController.rb b/src/modules/ZFCPController.rb index 6ffcc032..a6836c3e 100644 --- a/src/modules/ZFCPController.rb +++ b/src/modules/ZFCPController.rb @@ -52,7 +52,7 @@ def main @controllers = nil - @activated_controllers = {} + @activated_controllers = nil @disk_configured = false @@ -338,7 +338,7 @@ def GetControllers @controllers = Builtins.maplist(@controllers) do |c| Builtins.filter(c) do |k, _v| - Builtins.contains(["sysfs_bus_id"], k) + Builtins.contains(["sysfs_bus_id", "resource"], k) end end @@ -582,7 +582,7 @@ def ReportControllerActivationError(channel, ret) # # @param channel [String] channel def activate_controller(channel) - return if @activated_controllers[channel] + return if activated_controller?(channel) command2 = Builtins.sformat( "/sbin/zfcp_host_configure '%1' %2", @@ -600,7 +600,7 @@ def activate_controller(channel) if ret2 != 0 ReportControllerActivationError(channel, ret2) else - Ops.set(@activated_controllers, channel, true) + register_as_activated(channel) end end @@ -727,6 +727,37 @@ def GetLUNs(busid, wwpn) publish function: :DeactivateDisk, type: "void (string, string, string)" publish function: :GetWWPNs, type: "list (string)" publish function: :GetLUNs, type: "list (string, string)" + + private + + # Finds the activated controllers + # + # Initially, it reads the activated controllers from hwinfo. + # + # @return [Array] List of controller channels + def activated_controllers + return @activated_controllers if @activated_controllers + ctrls = GetControllers().select do |ctrl| + io = ctrl.fetch("resource", {}).fetch("io", []).first + io && io["active"] + end + @activated_controllers = ctrls.map { |c| c["sysfs_bus_id"] } + end + + # Mark a controller as activated + # + # @param channel [String] Channel + def register_as_activated(channel) + @activated_controllers << channel + end + + # Determines whether a controller is activated or not + # + # @param channel [String] Channel + # @return [Boolean] + def activated_controller?(channel) + activated_controllers.include?(channel) + end end ZFCPController = ZFCPControllerClass.new diff --git a/test/data/probe_storage.yml b/test/data/probe_storage.yml index f1d8a42f..bdfba895 100644 --- a/test/data/probe_storage.yml +++ b/test/data/probe_storage.yml @@ -108,7 +108,7 @@ old_unique_key: _AAN.6czr7zOIMz1 resource: io: - - active: true + - active: false length: 3 mode: rw start: 64512 diff --git a/test/zfcp_controller_test.rb b/test/zfcp_controller_test.rb index 838a6fca..5b0a277f 100755 --- a/test/zfcp_controller_test.rb +++ b/test/zfcp_controller_test.rb @@ -11,7 +11,7 @@ describe "#ActivateDisk" do it "Activates the given disk" do - expect(Yast::ZFCPController).to receive(:activate_controller).with('0.0.fc00') + expect(Yast::ZFCPController).to receive(:activate_controller).with("0.0.fc00") expect(Yast::SCR).to receive(:Execute) .with(anything, /\/sbin\/zfcp_disk_configure '0.0.fc00' '0x500' '0x401' 1/) .and_return(0) @@ -22,6 +22,19 @@ describe "#activate_controller" do let(:channel) { "0.0.fc00" } + before do + allow(Yast::Arch).to receive(:is_zkvm).and_return(false) + allow(Yast::SCR).to receive(:Read).with(Yast.path(".probe.storage")).once + .and_return(load_data("probe_storage.yml")) + + # Removing all fcp devices from blacklist + allow(Yast::SCR).to receive(:Execute).with(anything, /\/sbin\/vmcp q v fcp/).and_return( + "exit" => 0, + "stdout" => "FCP F800 ON FCP F807 CHPID 1C SUBCHANNEL = 000B\n F800 TOKEN = 0000000362A42C00" + ) + allow(Yast::SCR).to receive(:Execute).with(anything, /\/sbin\/cio_ignore -r f800/).and_return(0) + end + it "activates the given controller" do expect(Yast::SCR).to receive(:Execute) .with(anything, /\/sbin\/zfcp_host_configure '0.0.fc00' 1/).and_return(0) @@ -44,7 +57,21 @@ it "reports the error" do expect(Yast::ZFCPController).to receive(:ReportControllerActivationError) - .with('0.0.fc00', 1) + .with("0.0.fc00", 1) + Yast::ZFCPController.activate_controller(channel) + end + end + + context "when the controller is already activated" do + let(:channel) { "0.0.fa00" } + + before do + allow(Yast::SCR).to receive(:Read).with(Yast.path(".probe.storage")).once + .and_return(load_data("probe_storage.yml")) + end + + it "does not activate the controller" do + expect(Yast::SCR).to_not receive(:Execute).with(anything, /\/sbin\/zfcp_host_configure/) Yast::ZFCPController.activate_controller(channel) end end @@ -63,14 +90,14 @@ ) expect(Yast::SCR).to receive(:Execute).with(anything, /\/sbin\/cio_ignore -r f800/).and_return(0) - expect(Yast::ZFCPController.GetControllers).to eq( - [ - { "sysfs_bus_id"=>"0.0.f800" }, - { "sysfs_bus_id"=>"0.0.f900" }, - { "sysfs_bus_id"=>"0.0.fa00" }, - { "sysfs_bus_id"=>"0.0.fc00" } - ] + ctrls = Yast::ZFCPController.GetControllers + expect(ctrls).to contain_exactly( + hash_including("sysfs_bus_id" => "0.0.f800"), + hash_including("sysfs_bus_id" => "0.0.f900"), + hash_including("sysfs_bus_id" => "0.0.fa00"), + hash_including("sysfs_bus_id" => "0.0.fc00") ) + expect(ctrls.first["resource"]).to be_a(Hash) end context "no ZFCP controller found" do From f1262c105a05617c149e385fba47da399b7028cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Imobach=20Gonz=C3=A1lez=20Sosa?= Date: Tue, 31 Mar 2020 13:44:42 +0100 Subject: [PATCH 06/14] Do not try to activate zFCP disks twice --- src/modules/ZFCPController.rb | 103 +++++++++++++++++++++++----------- test/zfcp_controller_test.rb | 25 +++++++-- 2 files changed, 91 insertions(+), 37 deletions(-) diff --git a/src/modules/ZFCPController.rb b/src/modules/ZFCPController.rb index a6836c3e..79dbcf56 100644 --- a/src/modules/ZFCPController.rb +++ b/src/modules/ZFCPController.rb @@ -365,38 +365,8 @@ def ProbeDisks # popup label UI.OpenDialog(Label(_("Reading Configured ZFCP Devices"))) - disks = Convert.convert( - SCR.Read(path(".probe.disk")), - from: "any", - to: "list >" - ) - disks = Builtins.filter(disks) do |d| - d["driver"] == "zfcp" - end - - tapes = Convert.convert( - SCR.Read(path(".probe.tape")), - from: "any", - to: "list >" - ) - tapes = Builtins.filter(tapes) do |d| - Ops.get_string(d, "bus", "") == "SCSI" - end - - disks_tapes = Convert.convert( - Builtins.merge(disks, tapes), - from: "list", - to: "list >" - ) - - disks_tapes = Builtins.maplist(disks_tapes) do |d| - Builtins.filter(d) do |k, _v| - Builtins.contains(["dev_name", "detail", "vendor", "device"], k) - end - end - index = -1 - @devices = Builtins.listmap(disks_tapes) do |d| + @devices = Builtins.listmap(find_disks(true)) do |d| index = Ops.add(index, 1) { index => d } end @@ -609,9 +579,14 @@ def activate_controller(channel) # @param [String] wwpn string wwpn (hexa number) # @param [String] lun string lun (hexa number) def ActivateDisk(channel, wwpn, lun) - activate_controller(channel) + disk = find_disk(channel, wwpn, lun) + if disk + log.info "Disk #{channel}:#{wwpn}:#{lun} is already active. Skipping activation." + else + activate_controller(channel) + end - if wwpn != "" || lun != "" # we are not using allow_lun_scan + if disk.nil? && (wwpn != "" || lun != "") # we are not using allow_lun_scan command = Builtins.sformat( "/sbin/zfcp_disk_configure '%1' '%2' '%3' %4", channel, @@ -758,6 +733,68 @@ def register_as_activated(channel) def activated_controller?(channel) activated_controllers.include?(channel) end + + # Returns the zFCP disks + # + # Probes and returns the found zFCP . It caches the found disks. + # + # @param force_probing [Boolean] Ignore the cached values and probes again. + # @return [Array] Found zFCP disks + def find_disks(force_probing = false) + return @disks if @disks && !force_probing + disks = Convert.convert( + SCR.Read(path(".probe.disk")), + from: "any", + to: "list >" + ) + disks = Builtins.filter(disks) do |d| + d["driver"] == "zfcp" + end + + tapes = Convert.convert( + SCR.Read(path(".probe.tape")), + from: "any", + to: "list >" + ) + tapes = Builtins.filter(tapes) do |d| + Ops.get_string(d, "bus", "") == "SCSI" + end + + disks_tapes = Convert.convert( + Builtins.merge(disks, tapes), + from: "list", + to: "list >" + ) + + @disks = Builtins.maplist(disks_tapes) do |d| + Builtins.filter(d) do |k, _v| + Builtins.contains(["dev_name", "detail", "vendor", "device", "io"], k) + end + end + end + + # Determines whether the disk is activated or not + # + # @param disk [Hash] + # @return [Boolean] + def active_disk?(disk) + io = disk.fetch("resource", {}).fetch("io", []).first + !!(io && io["active"]) + end + + # Finds a disk + # + # @param [String] channel string channel + # @param [String] wwpn string wwpn (hexa number) + # @param [String] lun string lun (hexa number) + # @return [Hash,nil] Disk information is found; nil is the disk is not found + def find_disk(channel, wwpn, lun) + find_disks.find do |d| + detail = d["detail"] + next unless detail + detail["controller_id"] == channel && detail["wwpn"] == wwpn && detail["fcp_lun"] == lun + end + end end ZFCPController = ZFCPControllerClass.new diff --git a/test/zfcp_controller_test.rb b/test/zfcp_controller_test.rb index 5b0a277f..88e98217 100755 --- a/test/zfcp_controller_test.rb +++ b/test/zfcp_controller_test.rb @@ -10,12 +10,29 @@ end describe "#ActivateDisk" do - it "Activates the given disk" do - expect(Yast::ZFCPController).to receive(:activate_controller).with("0.0.fc00") + before do + allow(Yast::Arch).to receive(:is_zkvm).and_return(false) + allow(Yast::SCR).to receive(:Read).with(Yast.path(".probe.storage")).once + .and_return(load_data("probe_storage.yml")) + allow(Yast::SCR).to receive(:Read).with(Yast.path(".probe.disk")).once + .and_return(load_data("probe_disk.yml")) + allow(Yast::SCR).to receive(:Read).with(Yast.path(".probe.tape")).once.and_return([]) + end + + it "activates the controller and the given disk" do + expect(Yast::ZFCPController).to receive(:activate_controller).with("0.0.fa00") expect(Yast::SCR).to receive(:Execute) - .with(anything, /\/sbin\/zfcp_disk_configure '0.0.fc00' '0x500' '0x401' 1/) + .with(anything, /\/sbin\/zfcp_disk_configure '0.0.fa00' '0x500\d+' '0x401\d+' 1/) .and_return(0) - Yast::ZFCPController.ActivateDisk("0.0.fc00", "0x500", "0x401") + Yast::ZFCPController.ActivateDisk("0.0.fa00", "0x5000000000000000", "0x4010400000000000") + end + + context "when the disk is already active" do + it "does not try to active the given disk" do + allow(Yast::ZFCPController).to receive(:activate_controller).with("0.0.fa00") + expect(Yast::SCR).to_not receive(:Execute) + Yast::ZFCPController.ActivateDisk("0.0.fa00", "0x500507630500873a", "0x4010400000000000") + end end end From 84140c274c26c9103d075767677775ba6db458e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Imobach=20Gonz=C3=A1lez=20Sosa?= Date: Tue, 31 Mar 2020 13:08:28 +0100 Subject: [PATCH 07/14] Log already activated zFCP and DASD devices --- src/modules/DASDController.rb | 7 +++++-- src/modules/ZFCPController.rb | 3 ++- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/modules/DASDController.rb b/src/modules/DASDController.rb index 40b6e3e2..99b2e583 100644 --- a/src/modules/DASDController.rb +++ b/src/modules/DASDController.rb @@ -801,8 +801,11 @@ def report_error(headline, message, details) # @return [Boolean] Returns an error code (8 means 'unformatted'). def activate_disk_if_needed(channel, diag) disk = find_disks.find { |d| d["channel"] == channel } - return ActivateDisk(channel, diag) unless disk && active_disk?(disk) - disk["formatted"] ? 0 : 8 + if disk && active_disk?(disk) + log.info "Disk #{channel} is already active. Skipping the activation." + return disk["formatted"] ? 0 : 8 + end + ActivateDisk(channel, diag) unless disk && active_disk?(disk) end # Determines whether the disk is activated or not diff --git a/src/modules/ZFCPController.rb b/src/modules/ZFCPController.rb index 79dbcf56..309471b0 100644 --- a/src/modules/ZFCPController.rb +++ b/src/modules/ZFCPController.rb @@ -581,7 +581,7 @@ def activate_controller(channel) def ActivateDisk(channel, wwpn, lun) disk = find_disk(channel, wwpn, lun) if disk - log.info "Disk #{channel}:#{wwpn}:#{lun} is already active. Skipping activation." + log.info "Disk #{channel}:#{wwpn}:#{lun} is already active. Skipping the activation." else activate_controller(channel) end @@ -716,6 +716,7 @@ def activated_controllers io = ctrl.fetch("resource", {}).fetch("io", []).first io && io["active"] end + log.info "Already activated controllers: #{ctrls}" @activated_controllers = ctrls.map { |c| c["sysfs_bus_id"] } end From 3b45db04dfa228efe0f17639401f8a14b37249c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Imobach=20Gonz=C3=A1lez=20Sosa?= Date: Tue, 31 Mar 2020 14:54:53 +0100 Subject: [PATCH 08/14] Bump version and update changes file --- package/yast2-s390.changes | 7 +++++++ package/yast2-s390.spec | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/package/yast2-s390.changes b/package/yast2-s390.changes index 2d8bf20c..ed2e42c2 100644 --- a/package/yast2-s390.changes +++ b/package/yast2-s390.changes @@ -1,3 +1,10 @@ +------------------------------------------------------------------- +Tue Mar 31 13:52:28 UTC 2020 - Imobach Gonzalez Sosa + +- Do not try to activate DASD and zFCP devices that are already + active (bsc#1163149). Related to SLE-7396. +- 4.2.5 + ------------------------------------------------------------------- Mon Mar 23 13:56:13 UTC 2020 - Imobach Gonzalez Sosa diff --git a/package/yast2-s390.spec b/package/yast2-s390.spec index d565eeb6..b9b9d587 100644 --- a/package/yast2-s390.spec +++ b/package/yast2-s390.spec @@ -17,7 +17,7 @@ Name: yast2-s390 -Version: 4.2.4 +Version: 4.2.5 Release: 0 Group: System/YaST License: GPL-2.0-only From 510e2910678085fdbf9ac19e7b7b770e533df40d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Imobach=20Gonz=C3=A1lez=20Sosa?= Date: Tue, 31 Mar 2020 16:04:16 +0100 Subject: [PATCH 09/14] Update from code review --- package/yast2-s390.changes | 2 +- src/modules/DASDController.rb | 15 +++++++++------ src/modules/ZFCPController.rb | 8 ++++---- 3 files changed, 14 insertions(+), 11 deletions(-) diff --git a/package/yast2-s390.changes b/package/yast2-s390.changes index ed2e42c2..4ba355d8 100644 --- a/package/yast2-s390.changes +++ b/package/yast2-s390.changes @@ -2,7 +2,7 @@ Tue Mar 31 13:52:28 UTC 2020 - Imobach Gonzalez Sosa - Do not try to activate DASD and zFCP devices that are already - active (bsc#1163149). Related to SLE-7396. + active (bsc#1163149). Related to jsc#SLE-7396. - 4.2.5 ------------------------------------------------------------------- diff --git a/src/modules/DASDController.rb b/src/modules/DASDController.rb index 99b2e583..e5b8e45c 100644 --- a/src/modules/DASDController.rb +++ b/src/modules/DASDController.rb @@ -381,7 +381,7 @@ def ProbeDisks # popup label UI.OpenDialog(Label(_("Reading Configured DASD Disks"))) - disks = find_disks(true) + disks = find_disks(force_probing: true) index = -1 @devices = Builtins.listmap(disks) do |d| @@ -796,20 +796,23 @@ def report_error(headline, message, details) # # When the disk is already activated, it returns '8' if the # disk is unformatted or '0' otherwise. The idea is to mimic - # the same API that ActivateDisk. + # the same API than ActivateDisk. # - # @return [Boolean] Returns an error code (8 means 'unformatted'). + # @return [Integer] Returns an error code (8 means 'unformatted'). def activate_disk_if_needed(channel, diag) disk = find_disks.find { |d| d["channel"] == channel } if disk && active_disk?(disk) - log.info "Disk #{channel} is already active. Skipping the activation." + log.info "Disk #{disk.inspect} is already active. Skipping the activation." return disk["formatted"] ? 0 : 8 end - ActivateDisk(channel, diag) unless disk && active_disk?(disk) + ActivateDisk(channel, diag) end # Determines whether the disk is activated or not # + # Since any of its IO elements in 'resource' is active, consider the device + # as 'active'. + # # @param disk [Hash] # @return [Boolean] def active_disk?(disk) @@ -824,7 +827,7 @@ def active_disk?(disk) # # @param force_probing [Boolean] Ignore the cached values and probes again. # @return [Array] Found DASD disks - def find_disks(force_probing = false) + def find_disks(force_probing: false) return @disks if @disks && !force_probing disks = Convert.convert( SCR.Read(path(".probe.disk")), diff --git a/src/modules/ZFCPController.rb b/src/modules/ZFCPController.rb index 309471b0..dbbff52b 100644 --- a/src/modules/ZFCPController.rb +++ b/src/modules/ZFCPController.rb @@ -366,7 +366,7 @@ def ProbeDisks UI.OpenDialog(Label(_("Reading Configured ZFCP Devices"))) index = -1 - @devices = Builtins.listmap(find_disks(true)) do |d| + @devices = Builtins.listmap(find_disks(force_probing: true)) do |d| index = Ops.add(index, 1) { index => d } end @@ -581,7 +581,7 @@ def activate_controller(channel) def ActivateDisk(channel, wwpn, lun) disk = find_disk(channel, wwpn, lun) if disk - log.info "Disk #{channel}:#{wwpn}:#{lun} is already active. Skipping the activation." + log.info "Disk #{disk.inspect} is already active. Skipping the activation." else activate_controller(channel) end @@ -724,7 +724,7 @@ def activated_controllers # # @param channel [String] Channel def register_as_activated(channel) - @activated_controllers << channel + activated_controllers.push(channel) end # Determines whether a controller is activated or not @@ -741,7 +741,7 @@ def activated_controller?(channel) # # @param force_probing [Boolean] Ignore the cached values and probes again. # @return [Array] Found zFCP disks - def find_disks(force_probing = false) + def find_disks(force_probing: false) return @disks if @disks && !force_probing disks = Convert.convert( SCR.Read(path(".probe.disk")), From e4cd535484820600a741751776d686c5c07f19f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Imobach=20Gonz=C3=A1lez=20Sosa?= Date: Tue, 31 Mar 2020 16:06:05 +0100 Subject: [PATCH 10/14] Replace module names with 'subject' --- test/dasd_controller_test.rb | 56 ++++++++++++++++++------------------ test/zfcp_controller_test.rb | 46 ++++++++++++++--------------- 2 files changed, 51 insertions(+), 51 deletions(-) diff --git a/test/dasd_controller_test.rb b/test/dasd_controller_test.rb index 0fc8f5d1..732cca47 100755 --- a/test/dasd_controller_test.rb +++ b/test/dasd_controller_test.rb @@ -4,7 +4,7 @@ Yast.import "DASDController" -describe "Yast::DASDController" do +describe Yast::DASDController do subject { Yast::DASDController } describe "#DeactivateDisk" do @@ -110,7 +110,7 @@ it "returns true if .probe.disk contains DASDs" do expect(Yast::SCR).to receive(:Read).with(Yast.path(".probe.disk")).once .and_return(load_data("probe_disk_dasd.yml")) - expect(Yast::DASDController.IsAvailable()).to eq(true) + expect(subject.IsAvailable()).to eq(true) end end @@ -118,8 +118,8 @@ it "returns DASDs" do expect(Yast::SCR).to receive(:Read).with(Yast.path(".probe.disk")).once .and_return(load_data("probe_disk_dasd.yml")) - expect(Yast::DASDController.ProbeDisks()).to eq(nil) - expect(Yast::DASDController.GetDevices()).to eq( + expect(subject.ProbeDisks()).to eq(nil) + expect(subject.GetDevices()).to eq( 0 => { "detail" => { "cu_model" => 233, "dev_model" => 10, "lcss" => 0 }, "device_id" => 276880, "resource" => { "io" => [{ "active" => false, @@ -158,19 +158,19 @@ allow(Yast::Mode).to receive(:autoinst).and_return(true) # speed up the test a bit allow(Yast::Builtins).to receive(:sleep) - allow(Yast::DASDController).to receive(:ActivateDisk).and_return(0) - allow(Yast::DASDController).to receive(:GetDeviceName).and_return("/dev/dasda") + allow(subject).to receive(:ActivateDisk).and_return(0) + allow(subject).to receive(:GetDeviceName).and_return("/dev/dasda") end context "during autoinstallation" do before do - Yast::DASDController.Import(data) + subject.Import(data) end it "activates the disk" do - allow(Yast::DASDController).to receive(:FormatDisks) - expect(Yast::DASDController).to receive(:ActivateDisk).with("0.0.0100", false) - Yast::DASDController.Write + allow(subject).to receive(:FormatDisks) + expect(subject).to receive(:ActivateDisk).with("0.0.0100", false) + subject.Write end context "when 'format' is sets to true" do @@ -180,7 +180,7 @@ # bnc 928388 expect(Yast::SCR).to receive(:Execute).with(path(".process.start_shell"), "/sbin/dasdfmt -Y -P 1 -b 4096 -y -r 10 -m 10 -f '/dev/dasda'") - expect(Yast::DASDController.Write).to eq(true) + expect(subject.Write).to eq(true) end end @@ -188,8 +188,8 @@ let(:format) { false } it "does not format the disk" do - expect(Yast::DASDController).to_not receive(:FormatDisks) - Yast::DASDController.Write + expect(subject).to_not receive(:FormatDisks) + subject.Write end end @@ -199,7 +199,7 @@ let(:format) { false } before do - allow(Yast::DASDController).to receive(:ActivateDisk).with("0.0.0100", false) + allow(subject).to receive(:ActivateDisk).with("0.0.0100", false) .and_return(NOT_FORMATTED_CODE) end @@ -207,8 +207,8 @@ let(:format_unformatted) { true } it "formats the device" do - expect(Yast::DASDController).to receive(:FormatDisks).with(["/dev/dasda"], anything) - Yast::DASDController.Write + expect(subject).to receive(:FormatDisks).with(["/dev/dasda"], anything) + subject.Write end end @@ -216,8 +216,8 @@ let(:format_unformatted) { false } it "does not format the device" do - expect(Yast::DASDController).to_not receive(:FormatDisks) - Yast::DASDController.Write + expect(subject).to_not receive(:FormatDisks) + subject.Write end end @@ -226,14 +226,14 @@ let(:format_unformatted) { false } it "formats the device" do - expect(Yast::DASDController).to receive(:FormatDisks).with(["/dev/dasda"], anything) - Yast::DASDController.Write + expect(subject).to receive(:FormatDisks).with(["/dev/dasda"], anything) + subject.Write end it "reactivates the disk" do - expect(Yast::DASDController).to receive(:FormatDisks) - expect(Yast::DASDController).to receive(:ActivateDisk).twice - Yast::DASDController.Write + expect(subject).to receive(:FormatDisks) + expect(subject).to receive(:ActivateDisk).twice + subject.Write end end end @@ -246,8 +246,8 @@ expect(Yast::SCR).to_not receive(:Execute).with(path(".process.start_shell"), /dasdfmt.*\/dev\/dasda/) - expect(Yast::DASDController.Import(data)).to eq(true) - expect(Yast::DASDController.Write).to eq(true) + expect(subject.Import(data)).to eq(true) + expect(subject.Write).to eq(true) end end end @@ -357,9 +357,9 @@ describe "#ActivateDiag" do it "deactivates and reactivates dasd" do - expect(Yast::DASDController).to receive(:DeactivateDisk).ordered - expect(Yast::DASDController).to receive(:ActivateDisk).ordered - expect(Yast::DASDController.ActivateDiag("0.0.3333", true)).to eq(nil) + expect(subject).to receive(:DeactivateDisk).ordered + expect(subject).to receive(:ActivateDisk).ordered + expect(subject.ActivateDiag("0.0.3333", true)).to eq(nil) end end diff --git a/test/zfcp_controller_test.rb b/test/zfcp_controller_test.rb index 88e98217..c90e10b2 100755 --- a/test/zfcp_controller_test.rb +++ b/test/zfcp_controller_test.rb @@ -4,7 +4,7 @@ Yast.import "ZFCPController" -describe "Yast::ZFCPController" do +describe Yast::ZFCPController do before do Yast::ZFCPController.main end @@ -20,18 +20,18 @@ end it "activates the controller and the given disk" do - expect(Yast::ZFCPController).to receive(:activate_controller).with("0.0.fa00") + expect(subject).to receive(:activate_controller).with("0.0.fa00") expect(Yast::SCR).to receive(:Execute) .with(anything, /\/sbin\/zfcp_disk_configure '0.0.fa00' '0x500\d+' '0x401\d+' 1/) .and_return(0) - Yast::ZFCPController.ActivateDisk("0.0.fa00", "0x5000000000000000", "0x4010400000000000") + subject.ActivateDisk("0.0.fa00", "0x5000000000000000", "0x4010400000000000") end context "when the disk is already active" do it "does not try to active the given disk" do - allow(Yast::ZFCPController).to receive(:activate_controller).with("0.0.fa00") + allow(subject).to receive(:activate_controller).with("0.0.fa00") expect(Yast::SCR).to_not receive(:Execute) - Yast::ZFCPController.ActivateDisk("0.0.fa00", "0x500507630500873a", "0x4010400000000000") + subject.ActivateDisk("0.0.fa00", "0x500507630500873a", "0x4010400000000000") end end end @@ -55,15 +55,15 @@ it "activates the given controller" do expect(Yast::SCR).to receive(:Execute) .with(anything, /\/sbin\/zfcp_host_configure '0.0.fc00' 1/).and_return(0) - expect(Yast::ZFCPController).to_not receive(:ReportControllerActivationError) - Yast::ZFCPController.activate_controller(channel) + expect(subject).to_not receive(:ReportControllerActivationError) + subject.activate_controller(channel) end it "does not activate a controller twice" do expect(Yast::SCR).to receive(:Execute) .with(anything, /\/sbin\/zfcp_host_configure '0.0.fc00' 1/).once.and_return(0) - Yast::ZFCPController.activate_controller(channel) - Yast::ZFCPController.activate_controller(channel) + subject.activate_controller(channel) + subject.activate_controller(channel) end context "when the activation fails" do @@ -73,9 +73,9 @@ end it "reports the error" do - expect(Yast::ZFCPController).to receive(:ReportControllerActivationError) + expect(subject).to receive(:ReportControllerActivationError) .with("0.0.fc00", 1) - Yast::ZFCPController.activate_controller(channel) + subject.activate_controller(channel) end end @@ -89,7 +89,7 @@ it "does not activate the controller" do expect(Yast::SCR).to_not receive(:Execute).with(anything, /\/sbin\/zfcp_host_configure/) - Yast::ZFCPController.activate_controller(channel) + subject.activate_controller(channel) end end end @@ -107,7 +107,7 @@ ) expect(Yast::SCR).to receive(:Execute).with(anything, /\/sbin\/cio_ignore -r f800/).and_return(0) - ctrls = Yast::ZFCPController.GetControllers + ctrls = subject.GetControllers expect(ctrls).to contain_exactly( hash_including("sysfs_bus_id" => "0.0.f800"), hash_including("sysfs_bus_id" => "0.0.f900"), @@ -134,7 +134,7 @@ let(:is_zkvm) { false } it "reports a warning" do expect(Yast::Report).to receive(:Warning).with(/Cannot evaluate ZFCP controllers/) - Yast::ZFCPController.GetControllers + subject.GetControllers end end @@ -142,7 +142,7 @@ let(:is_zkvm) { true } it "does not report a warning" do expect(Yast::Report).to_not receive(:Warning).with(/Cannot evaluate ZFCP controllers/) - Yast::ZFCPController.GetControllers + subject.GetControllers end end end @@ -155,8 +155,8 @@ { "controller_id" => "0.0.f800" }, { "controller_id" => "0.0.f900" }] } - expect(Yast::ZFCPController.Import(import_data)).to eq(true) - expect(Yast::ZFCPController.GetDeviceIndex("0.0.f800", "", "")).to eq(2) + expect(subject.Import(import_data)).to eq(true) + expect(subject.GetDeviceIndex("0.0.f800", "", "")).to eq(2) end end @@ -168,8 +168,8 @@ end it "Probing disk" do - expect(Yast::ZFCPController.ProbeDisks()).to eq(nil) - expect(Yast::ZFCPController.devices).to eq(load_data("device_list.yml")) + expect(subject.ProbeDisks()).to eq(nil) + expect(subject.devices).to eq(load_data("device_list.yml")) end end @@ -181,10 +181,10 @@ { "controller_id" => "0.0.f800" }, { "controller_id" => "0.0.f900" }] } - expect(Yast::ZFCPController.Import(import_data)).to eq(true) - Yast::ZFCPController.filter_max = Yast::ZFCPController.FormatChannel("0.0.FA00") - Yast::ZFCPController.filter_min = Yast::ZFCPController.FormatChannel("0.0.f900") - expect(Yast::ZFCPController.GetFilteredDevices()).to eq( + expect(subject.Import(import_data)).to eq(true) + subject.filter_max = subject.FormatChannel("0.0.FA00") + subject.filter_min = subject.FormatChannel("0.0.f900") + expect(subject.GetFilteredDevices()).to eq( 0 => { "detail"=>{ "controller_id" => "0.0.fa00", "wwpn" => "", "fcp_lun" => "" } }, 4 => { "detail"=>{ "controller_id" => "0.0.f900", "wwpn" => "", "fcp_lun" => "" } } ) From 0e7d2184eab5cc2ccdf3cd2b8f916f3718c9d47a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Imobach=20Gonz=C3=A1lez=20Sosa?= Date: Tue, 31 Mar 2020 16:09:27 +0100 Subject: [PATCH 11/14] Use '<<' instead of 'push' --- src/modules/ZFCPController.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/modules/ZFCPController.rb b/src/modules/ZFCPController.rb index dbbff52b..8ec406fd 100644 --- a/src/modules/ZFCPController.rb +++ b/src/modules/ZFCPController.rb @@ -724,7 +724,7 @@ def activated_controllers # # @param channel [String] Channel def register_as_activated(channel) - activated_controllers.push(channel) + activated_controllers << channel end # Determines whether a controller is activated or not From fcf4088f330f0c61befd2de3c21baf3e9b5941c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Imobach=20Gonz=C3=A1lez=20Sosa?= Date: Wed, 1 Apr 2020 09:33:50 +0100 Subject: [PATCH 12/14] Extend DASDController tests --- test/dasd_controller_test.rb | 75 +++++++++++++++++++++++++++++++----- 1 file changed, 66 insertions(+), 9 deletions(-) diff --git a/test/dasd_controller_test.rb b/test/dasd_controller_test.rb index 732cca47..04dbc7f4 100755 --- a/test/dasd_controller_test.rb +++ b/test/dasd_controller_test.rb @@ -7,6 +7,46 @@ describe Yast::DASDController do subject { Yast::DASDController } + describe "#ActivateDisk" do + let(:exit_code) { 8 } + let(:channel) { "0.0.0100" } + + before do + allow(Yast::SCR).to receive(:Execute).and_return( + "exit" => exit_code + ) + end + + it "runs dasd_configure" do + expect(Yast::SCR).to receive(:Execute).with( + path(".target.bash_output"), "/sbin/dasd_configure '0.0.0100' 1 0" + ) + subject.ActivateDisk(channel, false) + end + + it "returns dasd_configure exit value" do + expect(subject.ActivateDisk(channel, false)).to eq(exit_code) + end + + context "when exit code is 7" do + let(:exit_code) { 7 } + + it "deactivates the device" do + expect(subject).to receive(:DeactivateDisk).with(channel, false) + subject.ActivateDisk(channel, false) + end + end + + context "when exit code is other than 7 or 8" do + let(:exit_code) { 1 } + + it "reports activation error" do + expect(subject).to receive(:ReportActivationError).with(channel, "exit" => exit_code) + subject.ActivateDisk(channel, false) + end + end + end + describe "#DeactivateDisk" do let(:auto) { false } let(:channel) { "0.0.0160" } @@ -140,17 +180,26 @@ end end + describe "#FormatDisks" do + it "formats the given disks using dasdfmt" do + expect(Yast::SCR).to receive(:Execute).with( + path(".process.start_shell"), "/sbin/dasdfmt -Y -P 1 -b 4096 -y -r 10 -m 10 -f '/dev/dasda'" + ) + subject.FormatDisks(["/dev/dasda"], 8) + end + end + describe "#Write" do let(:data) do - { "devices" => [{ "channel" => "0.0.0100", "diag" => false, + { "devices" => [{ "channel" => channel, "diag" => false, "format" => format }], "format_unformatted" => format_unformatted } end let(:format_unformatted) { false } let(:format) { true } + let(:channel) { "0.0.0100" } before do - allow(Yast::SCR).to receive(:Execute).with(path(".target.bash_output"), - /\/sbin\/dasdview/) + allow(Yast::SCR).to receive(:Execute).with(path(".target.bash_output"), /\/sbin\/dasdview/) .and_return("exitstatus" => 0, "stdout" => load_file("dasdview_eckd.txt"), "stderr" => "") allow(Yast::Mode).to receive(:normal).and_return(false) @@ -177,9 +226,7 @@ let(:format) { true } it "formats the disk" do - # bnc 928388 - expect(Yast::SCR).to receive(:Execute).with(path(".process.start_shell"), - "/sbin/dasdfmt -Y -P 1 -b 4096 -y -r 10 -m 10 -f '/dev/dasda'") + expect(subject).to receive(:FormatDisks).with(["/dev/dasda"], 8) expect(subject.Write).to eq(true) end end @@ -199,7 +246,7 @@ let(:format) { false } before do - allow(subject).to receive(:ActivateDisk).with("0.0.0100", false) + allow(subject).to receive(:activate_disk_if_needed).with(channel, false) .and_return(NOT_FORMATTED_CODE) end @@ -210,6 +257,12 @@ expect(subject).to receive(:FormatDisks).with(["/dev/dasda"], anything) subject.Write end + + it "reactivates the disk" do + allow(subject).to receive(:FormatDisks) + expect(subject).to receive(:ActivateDisk).with(channel, false) + subject.Write + end end context "and 'format_unformatted' is set to 'false'" do @@ -219,6 +272,11 @@ expect(subject).to_not receive(:FormatDisks) subject.Write end + + it "does not reactivate the disk" do + expect(subject).to_not receive(:ActivateDisk) + subject.Write + end end context "and 'format' is set to 'true'" do @@ -232,7 +290,7 @@ it "reactivates the disk" do expect(subject).to receive(:FormatDisks) - expect(subject).to receive(:ActivateDisk).twice + expect(subject).to receive(:ActivateDisk).with(channel, false) subject.Write end end @@ -362,5 +420,4 @@ expect(subject.ActivateDiag("0.0.3333", true)).to eq(nil) end end - end From 2b23a8de7ee4bf479bbe19e3ff70aed89ac3b7fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Imobach=20Gonz=C3=A1lez=20Sosa?= Date: Wed, 1 Apr 2020 09:52:45 +0100 Subject: [PATCH 13/14] Add tests for DASDController#activate_disk_if_needed --- src/modules/DASDController.rb | 32 ++++++++++----------- test/dasd_controller_test.rb | 52 +++++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 16 deletions(-) diff --git a/src/modules/DASDController.rb b/src/modules/DASDController.rb index e5b8e45c..3667f8d3 100644 --- a/src/modules/DASDController.rb +++ b/src/modules/DASDController.rb @@ -485,6 +485,22 @@ def ActivateDisk(channel, diag) ret["exit"] end + # Activates a disk if it is not active + # + # When the disk is already activated, it returns '8' if the + # disk is unformatted or '0' otherwise. The idea is to mimic + # the same API than ActivateDisk. + # + # @return [Integer] Returns an error code (8 means 'unformatted'). + def activate_disk_if_needed(channel, diag) + disk = find_disks.find { |d| d["channel"] == channel } + if disk && active_disk?(disk) + log.info "Disk #{disk.inspect} is already active. Skipping the activation." + return disk["formatted"] ? 0 : 8 + end + ActivateDisk(channel, diag) + end + # Deactivate disk # @param [String] channel string Name of the disk to deactivate # @param [Boolean] diag boolean Activate DIAG or not @@ -792,22 +808,6 @@ def report_error(headline, message, details) end end - # Activates a disk if its not activated - # - # When the disk is already activated, it returns '8' if the - # disk is unformatted or '0' otherwise. The idea is to mimic - # the same API than ActivateDisk. - # - # @return [Integer] Returns an error code (8 means 'unformatted'). - def activate_disk_if_needed(channel, diag) - disk = find_disks.find { |d| d["channel"] == channel } - if disk && active_disk?(disk) - log.info "Disk #{disk.inspect} is already active. Skipping the activation." - return disk["formatted"] ? 0 : 8 - end - ActivateDisk(channel, diag) - end - # Determines whether the disk is activated or not # # Since any of its IO elements in 'resource' is active, consider the device diff --git a/test/dasd_controller_test.rb b/test/dasd_controller_test.rb index 04dbc7f4..9ca38750 100755 --- a/test/dasd_controller_test.rb +++ b/test/dasd_controller_test.rb @@ -47,6 +47,58 @@ end end + describe "#activate_disk_if_needed" do + let(:channel) { "0.0.0150" } + let(:formatted) { true } + let(:active) { true } + + let(:disk) do + { + "dev_name" => "/dev/dasda", + "formatted" => formatted, + "channel" => channel, + "resource" => { + "io" => [{ "active" => active }] + } + } + end + + before do + allow(subject).to receive(:find_disks).and_return([disk]) + end + + context "when the disk is already active" do + it "does not activate the disk" do + expect(subject).to_not receive(:ActivateDisk) + subject.activate_disk_if_needed(channel, false) + end + + context "and it is not formatted" do + let(:formatted) { false } + + it "returns 8" do + expect(subject.activate_disk_if_needed(channel, false)).to eq(8) + end + end + + context "and it is formatted" do + it "returns 0" do + expect(subject.activate_disk_if_needed(channel, false)).to eq(0) + end + end + end + + context "when the disk is not active" do + let(:active) { false } + + it "activates the disk" do + expect(subject).to receive(:ActivateDisk).with(channel, false) + .and_return(0) + expect(subject.activate_disk_if_needed(channel, false)).to eq(0) + end + end + end + describe "#DeactivateDisk" do let(:auto) { false } let(:channel) { "0.0.0160" } From 04ab8545717aa94cc943fe4eed5cc4d79b3c6d83 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Imobach=20Gonz=C3=A1lez=20Sosa?= Date: Wed, 1 Apr 2020 09:52:56 +0100 Subject: [PATCH 14/14] Improve detection of 'active' devices --- src/modules/DASDController.rb | 4 ++-- src/modules/ZFCPController.rb | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/modules/DASDController.rb b/src/modules/DASDController.rb index 3667f8d3..bd4513eb 100644 --- a/src/modules/DASDController.rb +++ b/src/modules/DASDController.rb @@ -816,8 +816,8 @@ def report_error(headline, message, details) # @param disk [Hash] # @return [Boolean] def active_disk?(disk) - io = disk.fetch("resource", {}).fetch("io", []).first - !!(io && io["active"]) + io = disk.fetch("resource", {}).fetch("io", []) + io.any? { |i| i["active"] } end # Returns the DASD disks diff --git a/src/modules/ZFCPController.rb b/src/modules/ZFCPController.rb index 8ec406fd..d7c0465b 100644 --- a/src/modules/ZFCPController.rb +++ b/src/modules/ZFCPController.rb @@ -713,8 +713,8 @@ def GetLUNs(busid, wwpn) def activated_controllers return @activated_controllers if @activated_controllers ctrls = GetControllers().select do |ctrl| - io = ctrl.fetch("resource", {}).fetch("io", []).first - io && io["active"] + io = ctrl.fetch("resource", {}).fetch("io", []) + io.any? { |i| i["active"] } end log.info "Already activated controllers: #{ctrls}" @activated_controllers = ctrls.map { |c| c["sysfs_bus_id"] }