Merge pull request #650 from joseivanlopez/zfcp-improvements

zFCP improvements

service/lib/agama/storage/zfcp/manager.rb

@@ -21,6 +21,7 @@
 
 require "yast"
 require "y2s390/zfcp"
+require "y2storage/storage_manager"
 require "agama/storage/zfcp/controller"
 require "agama/storage/zfcp/disk"
 
@@ -54,14 +55,20 @@ def on_disks_change(&block)
 
         # Probes zFCP
         #
-        # Callbacks are called at the end, see {#on_probe}.
+        # Callbacks {#on_probe} are called after probing, and callbacks {#on_disks_change} are
+        # called if there is any change in the disks.
         def probe
           logger.info "Probing zFCP"
 
+          previous_disks = disks
           yast_zfcp.probe_controllers
           yast_zfcp.probe_disks
 
           @on_probe_callbacks&.each { |c| c.call(controllers, disks) }
+
+          return unless disks_changed?(previous_disks)
+
+          @on_disks_change_callbacks&.each { |c| c.call(disks) }
         end
 
         # zFCP controllers
@@ -91,13 +98,22 @@ def allow_lun_scan?
 
         # Activates the controller with the given channel id
         #
-        # @note: If "allow_lun_scan" is active, then all LUNs are automatically activated.
+        # @note: If "allow_lun_scan" is active, then all its LUNs are automatically activated.
         #
         # @param channel [String]
         # @return [Integer] Exit code of the chzdev command (0 on success)
         def activate_controller(channel)
           output = yast_zfcp.activate_controller(channel)
-          update_disks if output["exit"] == 0
+          if output["exit"] == 0
+            # LUNs activation could delay after activating the controller. This usually happens when
+            # activating a controller for first time because some SCSI initialization. Probing the
+            # disks should be done after all disks are activated.
+            #
+            # FIXME: waiting 2 seconds should be enough, but there is no guarantee that all the
+            # disks are actually activated.
+            sleep(2)
+            probe
+          end
           output["exit"]
         end
 
@@ -110,7 +126,7 @@ def activate_controller(channel)
         # @return [Integer] Exit code of the chzdev command (0 on success)
         def activate_disk(channel, wwpn, lun)
           output = yast_zfcp.activate_disk(channel, wwpn, lun)
-          update_disks if output["exit"] == 0
+          probe if output["exit"] == 0
           output["exit"]
         end
 
@@ -125,7 +141,7 @@ def activate_disk(channel, wwpn, lun)
         # @return [Integer] Exit code of the chzdev command (0 on success)
         def deactivate_disk(channel, wwpn, lun)
           output = yast_zfcp.deactivate_disk(channel, wwpn, lun)
-          update_disks if output["exit"] == 0
+          probe if output["exit"] == 0
           output["exit"]
         end
 
@@ -154,15 +170,28 @@ def find_luns(channel, wwpn)
         # @return [Logger]
         attr_reader :logger
 
-        # Probes and runs the on_disk_change callbacsk if disks have changed
-        def update_disks
-          disk_records = yast_zfcp.disks.sort_by { |d| d["dev_name"] }
-          probe
-          new_disk_records = yast_zfcp.disks.sort_by { |d| d["dev_name"] }
+        # Whether threre is any change in the disks
+        #
+        # Checks whether any of the removed disks is still probed or whether any of the current
+        # disks is not probed yet.
+        #
+        # @param previous_disks [Array<Disk>] Previously activated disks (before zFCP (re)probing)
+        # @return [Booelan]
+        def disks_changed?(previous_disks)
+          removed_disks = previous_disks - disks
+
+          return true if removed_disks.any? { |d| exist_disk?(d) }
+          return true if disks.any? { |d| !exist_disk?(d) }
 
-          return if disk_records == new_disk_records
+          false
+        end
 
-          @on_disks_change_callbacks&.each { |c| c.call(disks) }
+        # Whether the given disk is probed
+        #
+        # @param disk [Disk]
+        # @return [Booelan]
+        def exist_disk?(disk)
+          !Y2Storage::StorageManager.instance.probed.find_by_name(disk.name).nil?
         end
 
         # Creates a zFCP controller from YaST data

service/package/rubygem-agama.changes

@@ -1,3 +1,10 @@
+-------------------------------------------------------------------
+Wed Jul  5 14:02:23 UTC 2023 - José Iván López González <jlopez@suse.com>
+
+- Delay zFCP probing after activating a controller and ensure the
+  system is marked as deprecated if needed after probing zFCP
+  (gh#openSUSE/agama#650).
+
 -------------------------------------------------------------------
 Wed Jun 14 15:11:56 UTC 2023 - José Iván López González <jlopez@suse.com>
 

service/test/agama/storage/zfcp/manager_test.rb

@@ -23,6 +23,9 @@
 require "agama/storage/zfcp/manager"
 require "agama/storage/zfcp/controller"
 require "agama/storage/zfcp/disk"
+require "y2storage/storage_manager"
+require "y2storage/devicegraph"
+require "y2storage/disk"
 
 # yast2-s390 is not available for all architectures. Defining Y2S390::ZFCP constant here in order to
 # make possible to mock that class independently on the architecture. Note that Y2S390 classes are
@@ -38,6 +41,9 @@ module Y2S390
 
   before do
     allow(Y2S390::ZFCP).to receive(:new).and_return(yast_zfcp)
+    allow(Y2Storage::StorageManager).to receive(:instance).and_return(storage_manager)
+    allow(storage_manager).to receive(:probed).and_return(devicegraph)
+    allow(devicegraph).to receive(:find_by_name).and_return(storage_disk)
     allow(yast_zfcp).to receive(:probe_controllers)
     allow(yast_zfcp).to receive(:probe_disks)
     allow(yast_zfcp).to receive(:controllers).and_return(*controller_records)
@@ -46,6 +52,12 @@ module Y2S390
 
   let(:yast_zfcp) { double(Y2S390::ZFCP) }
 
+  let(:storage_manager) { instance_double(Y2Storage::StorageManager) }
+
+  let(:devicegraph) { instance_double(Y2Storage::Devicegraph) }
+
+  let(:storage_disk) { instance_double(Y2Storage::Disk) }
+
   let(:controller_records) { [[]] }
 
   let(:disk_records) { [[]] }
@@ -80,15 +92,55 @@ module Y2S390
       subject.probe
     end
 
-    let(:callback) { proc { |_, _| } }
-
     it "runs the on_probe callbacks" do
+      callback = proc { |_, _| }
       subject.on_probe(&callback)
 
       expect(callback).to receive(:call).with([], [])
 
       subject.probe
     end
+
+    context "if any zFCP disk was deactivated" do
+      let(:disk_records) { [[sda_record, sdb_record], [sda_record]] }
+
+      it "runs the on_disks_change callbacks" do
+        callback = proc { |_| }
+        subject.on_disks_change(&callback)
+
+        expect(callback).to receive(:call)
+
+        subject.probe
+      end
+    end
+
+    context "if any new zFCP disk was activated" do
+      let(:disk_records) { [[sda_record], [sda_record, sdb_record]] }
+
+      let(:storage_disk) { nil }
+
+      it "runs the on_disks_change callbacks" do
+        callback = proc { |_| }
+        subject.on_disks_change(&callback)
+
+        expect(callback).to receive(:call)
+
+        subject.probe
+      end
+    end
+
+    context "if the zFCP disks have not changed" do
+      let(:disk_records) { [[sda_record, sdb_record], [sda_record, sdb_record]] }
+
+      it "does not run the on_disks_change callbacks" do
+        callback = proc { |_| }
+        subject.on_disks_change(&callback)
+
+        expect(callback).to_not receive(:call)
+
+        subject.probe
+      end
+    end
   end
 
   describe "#controllers" do
@@ -179,14 +231,11 @@ module Y2S390
   describe "#activate_controller" do
     before do
       allow(yast_zfcp).to receive(:activate_controller).and_return(output)
-
-      subject.on_disks_change(&callback)
+      allow(subject).to receive(:sleep)
     end
 
     let(:output) { { "exit" => 3 } }
 
-    let(:callback) { proc { |_| } }
-
     it "tries to activate the controller with the given channel id" do
       expect(yast_zfcp).to receive(:activate_controller).with("0.0.fa00")
 
@@ -207,26 +256,6 @@ module Y2S390
 
         subject.activate_controller("0.0.fa00")
       end
-
-      context "and the zFCP disks have changed" do
-        let(:disk_records) { [[sda_record], [sdb_record]] }
-
-        it "runs the on_disks_change callbacks" do
-          expect(callback).to receive(:call)
-
-          subject.activate_controller("0.0.fa00")
-        end
-      end
-
-      context "and the zFCP disks have not changed" do
-        let(:disk_records) { [[sda_record], [sda_record]] }
-
-        it "does not run the on_disks_change callbacks" do
-          expect(callback).to_not receive(:call)
-
-          subject.activate_controller("0.0.fa00")
-        end
-      end
     end
 
     context "if the controller was not activated" do
@@ -237,26 +266,16 @@ module Y2S390
 
         subject.activate_controller("0.0.fa00")
       end
-
-      it "does not run the on_disks_change callbacks" do
-        expect(callback).to_not receive(:call)
-
-        subject.activate_controller("0.0.fa00")
-      end
     end
   end
 
   describe "#activate_disk" do
     before do
       allow(yast_zfcp).to receive(:activate_disk).and_return(output)
-
-      subject.on_disks_change(&callback)
     end
 
     let(:output) { { "exit" => 3 } }
 
-    let(:callback) { proc { |_| } }
-
     it "tries to activate the zFCP diks with the given channel id, WWPN and LUN" do
       expect(yast_zfcp)
         .to receive(:activate_disk).with("0.0.fa00", "0x500507630708d3b3", "0x0013000000000000")
@@ -278,26 +297,6 @@ module Y2S390
 
         subject.activate_disk("0.0.fa00", "0x500507630708d3b3", "0x0013000000000000")
       end
-
-      context "and the zFCP disks have changed" do
-        let(:disk_records) { [[sda_record], [sdb_record]] }
-
-        it "runs the on_disks_change callbacks" do
-          expect(callback).to receive(:call)
-
-          subject.activate_disk("0.0.fa00", "0x500507630708d3b3", "0x0013000000000000")
-        end
-      end
-
-      context "and the zFCP disks have not changed" do
-        let(:disk_records) { [[sda_record], [sda_record]] }
-
-        it "does not run the on_disks_change callbacks" do
-          expect(callback).to_not receive(:call)
-
-          subject.activate_disk("0.0.fa00", "0x500507630708d3b3", "0x0013000000000000")
-        end
-      end
     end
 
     context "if the zFCP disk was not activated" do
@@ -308,26 +307,16 @@ module Y2S390
 
         subject.activate_disk("0.0.fa00", "0x500507630708d3b3", "0x0013000000000000")
       end
-
-      it "does not run the on_disks_change callbacks" do
-        expect(callback).to_not receive(:call)
-
-        subject.activate_disk("0.0.fa00", "0x500507630708d3b3", "0x0013000000000000")
-      end
     end
   end
 
   describe "#deactivate_disk" do
     before do
       allow(yast_zfcp).to receive(:deactivate_disk).and_return(output)
-
-      subject.on_disks_change(&callback)
     end
 
     let(:output) { { "exit" => 3 } }
 
-    let(:callback) { proc { |_| } }
-
     it "tries to deactivate the zFCP diks with the given channel id, WWPN and LUN" do
       expect(yast_zfcp)
         .to receive(:deactivate_disk).with("0.0.fa00", "0x500507630708d3b3", "0x0013000000000000")
@@ -349,26 +338,6 @@ module Y2S390
 
         subject.deactivate_disk("0.0.fa00", "0x500507630708d3b3", "0x0013000000000000")
       end
-
-      context "and the zFCP disks have changed" do
-        let(:disk_records) { [[sda_record], [sdb_record]] }
-
-        it "runs the on_disks_change callbacks" do
-          expect(callback).to receive(:call)
-
-          subject.deactivate_disk("0.0.fa00", "0x500507630708d3b3", "0x0013000000000000")
-        end
-      end
-
-      context "and the zFCP disks have not changed" do
-        let(:disk_records) { [[sda_record], [sda_record]] }
-
-        it "does not run the on_disks_change callbacks" do
-          expect(callback).to_not receive(:call)
-
-          subject.deactivate_disk("0.0.fa00", "0x500507630708d3b3", "0x0013000000000000")
-        end
-      end
     end
 
     context "if the zFCP disk was not deactivated" do
@@ -379,12 +348,6 @@ module Y2S390
 
         subject.deactivate_disk("0.0.fa00", "0x500507630708d3b3", "0x0013000000000000")
       end
-
-      it "does not run the on_disks_change callbacks" do
-        expect(callback).to_not receive(:call)
-
-        subject.deactivate_disk("0.0.fa00", "0x500507630708d3b3", "0x0013000000000000")
-      end
     end
   end
 

web/package/cockpit-agama.changes

@@ -1,3 +1,9 @@
+-------------------------------------------------------------------
+Wed Jul  5 13:59:58 UTC 2023 - José Iván López González <jlopez@suse.com>
+
+- Add info about deactivated zFCP auto_lun_scan and sort members in
+  device selector (gh#openSUSE/agama#650).
+
 -------------------------------------------------------------------
 Mon Jul  3 10:18:32 UTC 2023 - José Iván López González <jlopez@suse.com>