Merge pull request #476 from yast/dasd_progress2

Report progress of DASD formatting to the D-Bus interface

doc/dbus_api.md

@@ -133,6 +133,9 @@ Service for managing storage devices.
   .DInstaller.Storage1.ISCSI.Node
 /DInstaller/Storage1/dasds/[0-9]+ (Only available on s390 systems)
   .DInstaller.Storage1.DASD.device
+/DInstaller/Storage1/jobs/[0-9]+
+  .DInstaller.Storage1.Job
+  .DInstaller.Storage1.DASD.Format
 ~~~
 
 ### D-Bus Objects
@@ -184,6 +187,16 @@ Objects representing iSCSI nodes are dynamically exported when a successful iSCS
 
 Objects representing DASDs are dynamically exported when a successful probing is performed by the `DASD.manager` interface of the main storage object, see `.org.opensuse.DInstaller.Storage1.DASD.manager`.
 
+#### `/org/opensuse/DInstaller/Storage1/jobs/[0-9]+` Objects
+
+~~~
+/DInstaller/Storage1/jobs/[0-9]+
+  .DInstaller.Storage1.Job
+  .DInstaller.Storage1.DASD.Format
+~~~
+
+Objects representing long-running processes, like formatting of DASDs.
+
 ### D-Bus Interfaces
 
 #### `org.opensuse.DInstaller.Storage1` Interface
@@ -484,11 +497,11 @@ Disable(in  ao devices,
 SetDiag(in  ao devices,
         in  b  diag,
         out u  result)
+Format(in  ao devices,
+       out u  result,
+       out o  job)
 ~~~
 
-In addition, a `Format()` method is provided, but it's not documented here because it's going to
-change heavily in the short term.
-
 ##### Details
 
 ###### `Probe` Method
@@ -543,6 +556,23 @@ Arguments:
 * `in b diag`: new value for the flag.
 * `out u result`: `0` if `use_diag` is correctly set for all the requested DASDs. `1` if any of the given paths is invalid (ie. it does not correspond to a known DASD), `2` in case of any other error.
 
+###### `Format` Method
+
+~~~
+Format(in  ao devices,
+       out u  result,
+       out o  job)
+~~~
+
+Starts a format process for the DASDs in the given list. It creates a job to represent such a
+process.
+
+Arguments:
+
+* `in ao devices`: paths of the D-Bus objects representing the DASDs to format.
+* `out u result`: `0` if the format operation starts correctly and the job to track it is created. `1` if any of the given paths is invalid (ie. it does not correspond to a known DASD), `2` in case of any other error.
+* `out o job`: if the result is 0, path of the new job that can be used to track the formatting. Contains the string `/` (no job) if the result is not zero.
+
 #### `org.opensuse.DInstaller.Storage1.DASD.Device` Interface
 
 This interface is implemented by objects exported at the `/org/opensuse/DInstaller/Storage1/dasds/[0-9]+`
@@ -576,6 +606,47 @@ string `AccessType` could be replaced by a boolean `ReadOnly`).
 * `PartitionInfo`: Partition names (and sometimes their type) separated by commas.
   Eg. "_/dev/dasda1 (Linux native), /dev/dasda2 (Linux native)_". Empty if the information is unknown.
 
+#### `org.opensuse.DInstaller.Storage1.Job` Interface
+
+This interface is implemented by objects exported at the `/org/opensuse/DInstaller/Storage1/jobs/[0-9]+`
+paths. It provides information about a long-running process.
+
+##### Properties
+
+~~~
+Running  readable b
+ExitCode readable u
+~~~
+
+* `Running`: Whether the Job is still being executed or it has already finished.
+* `ExitCode`: Final result. Zero for running processes.
+
+##### Signals
+
+* `Finished(u exit_code)`: the Job is not longer running and the exit code has been set to its final
+  value.
+* `PropertiesChanged()`: in parallel to the mentioned `Finished` signal, the standard
+  `PropertiesChanged` signal from `org.freedesktop.DBus.Properties` is also triggered at the end of
+  the job execution to reflect the corresponding changes in the properties.
+
+#### `org.opensuse.DInstaller.Storage1.DASD.Format` Interface
+
+This interface is implemented by those Job objects used to represent the formatting of a set of DASDs.
+
+##### Properties
+
+~~~
+Summary readable a{s(uub)}
+~~~
+
+* `Summary`: A hash where each key is the path of one of the DASDs being formatted and the value is
+  the status represented by a triplet of total cylinders, cylinders already processed and a boolean
+  indicating whether the process for that particular disk is completed.
+
+##### Signals
+
+* `PropertiesChanged`, as standard from `org.freedesktop.DBus.Properties`.
+
 ## Users
 
 ### iface o.o.Installer1.Users

service/lib/dinstaller/dbus/interfaces/dasd.rb

@@ -38,6 +38,7 @@ module Dasd
         def self.included(base)
           # Require the optional dependencies only if the module is included
           require "dinstaller/dbus/storage/dasds_tree"
+          require "dinstaller/dbus/storage/jobs_tree"
           require "dinstaller/storage/dasd/manager"
 
           base.class_eval do
@@ -66,10 +67,8 @@ def self.included(base)
                 busy_while { dasd_set_diag(devs, diag) }
               end
 
-              # Formats the DASDs in the given list
-              # TODO: we plan to change this so it returns the path of a new Process object. See
-              # https://gist.github.com/ancorgs/390b064373995595a1b1dfb08d00507a#gistcomment-4502266
-              dbus_method(:Format, "in devices:ao, out result:u") do |devs|
+              # Creates a format job for the DASDs in the given list
+              dbus_method(:Format, "in devices:ao, out result:u, out job:o") do |devs|
                 busy_while { dasd_format(devs) }
               end
             end
@@ -122,9 +121,29 @@ def dasd_set_diag(paths, diag)
         # Succeeds (returns 0) if all the devices where successfully formatted.
         #
         # @param paths [Array<String>]
-        # @return [Integer]
+        # @return [Array(Integer,::DBus::ObjectPath)]
         def dasd_format(paths)
-          dasds_dbus_method(paths) { |dasds| dasd_backend.format(dasds) }
+          dasds = find_dasds(paths)
+          return [1, "/"] if dasds.nil?
+
+          # Theoreticaly, there is room for a race condition here if the callbacks 'progress' or
+          # 'finish' are called before the job is created below. But in practice it will not happen
+          # because dasd_backend#format sleeps before calling any of the callbacks and, of course,
+          # it only calls them if the formatting process effectively started.
+          #
+          # We can change the approach in the future and always create the job beforehand if we feel
+          # the risk is not acceptable. That would make the Format operation a bit less consistent
+          # with other methods in this interface. If the format process cannot be started it would
+          # still return 0 as result and would create a job in the tree with kind of meaningless
+          # progress information representing the failed execution.
+          job = nil
+          progress = proc { |statuses| job.update_format(statuses) }
+          finish = proc { |result| job.finish_format(result) }
+          initial_statuses = dasd_backend.format(dasds, on_progress: progress, on_finish: finish)
+          return [2, "/"] unless initial_statuses
+
+          job = jobs_tree.add_dasds_format(initial_statuses, dasds_tree)
+          [0, job.path]
         end
 
         # Finds the DASDs matching the given D-Bus paths and calls the given block on them
@@ -137,16 +156,28 @@ def dasd_format(paths)
         #
         # @return [Integer]
         def dasds_dbus_method(paths)
+          dasds = find_dasds(paths)
+          return 1 if dasds.nil?
+
+          return 0 if yield(dasds)
+
+          2
+        end
+
+        # Finds the DASDs matching the given D-Bus paths
+        #
+        # Returns nil (and logs an error) unless all the paths are known.
+        #
+        # @return [Array<Y2S390::Dasd>, nil]
+        def find_dasds(paths)
           dbus_dasds = dasds_tree.find_paths(paths)
           if dbus_dasds.size != paths.size
             missing = paths - dbus_dasds.map(&:path)
             logger.error "Unknown path(s) #{missing} at requested list #{paths}"
-            return 1
+            return nil
           end
 
-          return 0 if yield(dbus_dasds.map(&:dasd))
-
-          2
+          dbus_dasds.map(&:dasd)
         end
 
         # Registers the callbacks necessary to ensure accuracy of the tree of DASD objects
@@ -165,6 +196,11 @@ def dasds_tree
           @dasds_tree ||= Storage::DasdsTree.new(@service, logger: logger)
         end
 
+        # Tree with the storage jobs
+        def jobs_tree
+          @jobs_tree ||= Storage::JobsTree.new(@service, logger: logger)
+        end
+
         # DASD manager
         #
         # @return [Storage::DASD::Manager]

service/lib/dinstaller/dbus/storage/dasds_format_job.rb

@@ -0,0 +1,165 @@
+# frozen_string_literal: true
+
+# Copyright (c) [2023] SUSE LLC
+#
+# All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or modify it
+# under the terms of version 2 of the GNU General Public License as published
+# by the Free Software Foundation.
+#
+# This program is distributed in the hope that it will be useful, but WITHOUT
+# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+# FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+# more details.
+#
+# You should have received a copy of the GNU General Public License along
+# with this program; if not, contact SUSE LLC.
+#
+# To contact SUSE LLC about this file by physical or electronic mail, you may
+# find current contact information at www.suse.com.
+
+require "dbus"
+require "dinstaller/dbus/base_object"
+
+module DInstaller
+  module DBus
+    module Storage
+      # Class representing the process of formatting a set of DASDs
+      class DasdsFormatJob < BaseObject
+        # Internal class to make easier to index the status information both by DASD id and by job
+        # path, although in fact both are fully stable during the whole execution of the installer.
+        #
+        # This class helps to refresh the relationship between the DASD and its path every time a
+        # status update is sent from the backend, although as already mentioned that wouldn't be
+        # needed with the current implementation of the DASDs tree, since it keeps that relationship
+        # stable.
+        class DasdFormatInfo
+          # @return [String] channel id of the DASD
+          attr_accessor :id
+
+          # @return [String] path of the DASD in the D-Bus tree
+          attr_accessor :path
+
+          # @return [Integer] total number of cylinders reported by the format operation
+          attr_accessor :cylinders
+
+          # @return [Integer] number of cylinders already processed by the format operation
+          attr_accessor :progress
+
+          # @return [Boolean] whether the disk is already fully formatted
+          attr_accessor :done
+
+          # Constructor
+          #
+          # @param status [Y2S390::FormatStatus]
+          # @param dasds_tree [DasdsTree]
+          def initialize(status, dasds_tree)
+            @id = status.dasd.id
+            @cylinders = status.cylinders
+            @progress = status.progress
+            @done = status.done?
+
+            dbus_dasd = dasds_tree.find { |d| d.id == @id }
+            if dbus_dasd
+              @path = dbus_dasd.path
+            else
+              logger.warning "DASD is not longer in the D-BUS tree: #{status.inspect}"
+            end
+          end
+
+          # Progress representation as expected by the D-Bus API (property Summary and signal
+          # SummaryUpdated)
+          def to_dbus
+            [cylinders, progress, done]
+          end
+        end
+
+        JOB_INTERFACE = "org.opensuse.DInstaller.Storage1.Job"
+        private_constant :JOB_INTERFACE
+
+        dbus_interface JOB_INTERFACE do
+          dbus_reader(:running, "b")
+          dbus_reader(:exit_code, "u")
+          dbus_signal(:Finished, "exit_code:u")
+        end
+
+        DASD_FORMAT_INTERFACE = "org.opensuse.DInstaller.Storage1.DASD.Format"
+        private_constant :DASD_FORMAT_INTERFACE
+
+        dbus_interface DASD_FORMAT_INTERFACE do
+          dbus_reader(:summary, "a{s(uub)}")
+        end
+
+        # @return [Boolean]
+        attr_reader :running
+
+        # @return [Integer] zero if still running
+        attr_reader :exit_code
+
+        # @return [Array<Y2390::Dasd>]
+        attr_reader :dasds
+
+        # Constructor
+        #
+        # @param initial [Array<Y2S390::FormatStatus>] initial status report from the format process
+        # @param dasds_tree [DasdsTree] see #dasds_tree
+        # @param path [DBus::ObjectPath] path in which the Job object is exported
+        # @param logger [Logger, nil]
+        def initialize(initial, dasds_tree, path, logger: nil)
+          super(path, logger: logger)
+
+          @exit_code = 0
+          @running = true
+          @dasds_tree = dasds_tree
+          @infos = {}
+          update_info(initial)
+        end
+
+        # Current status, in the format described by the D-Bus API
+        def summary
+          result = {}
+          @infos.each_value { |i| result[i.path] = i.to_dbus if i.path }
+          result
+        end
+
+        # Marks the job as finished
+        #
+        # @note A Finished and a PropertiesChanged signals are always emitted.
+        #
+        # @param exit_code [Integer]
+        def finish_format(exit_code)
+          @running = false
+          @exit_code = exit_code
+          Finished(exit_code)
+          dbus_properties_changed(JOB_INTERFACE, interfaces_and_properties.slice(JOB_INTERFACE), [])
+        end
+
+        # Updates the internal status information
+        #
+        # @note A SummaryUpdated signal is always emitted
+        #
+        # @param statuses [Array<Y2S390::FormatStatus] latest status update from the format process
+        def update_format(statuses)
+          return if statuses.empty?
+
+          update_info(statuses)
+          dbus_properties_changed(DASD_FORMAT_INTERFACE, { "Summary" => summary }, [])
+        end
+
+      private
+
+        # @return [DasdsTree] D-Bus representation of the DASDs
+        attr_reader :dasds_tree
+
+        # @param statuses [Array<Y2S390::FormatStatus] latest status update from the format process
+        def update_info(statuses)
+          statuses.each do |status|
+            info = DasdFormatInfo.new(status, dasds_tree)
+            @infos[info.id] = info
+          end
+        end
+      end
+    end
+  end
+end