From c4b57bcdb4f6803afb6b6c35e6af3ffb06f2e651 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Iv=C3=A1n=20L=C3=B3pez=20Gonz=C3=A1lez?= Date: Fri, 26 Jan 2024 15:16:52 +0000 Subject: [PATCH 1/5] [service] Export partitions on D-Bus --- service/lib/agama/dbus/storage/device.rb | 13 +++++++-- .../lib/agama/dbus/storage/devices_tree.rb | 29 ++++++++++++++----- .../agama/dbus/storage/interfaces/block.rb | 10 ++++++- .../storage/interfaces/partition_table.rb | 12 ++++---- 4 files changed, 45 insertions(+), 19 deletions(-) diff --git a/service/lib/agama/dbus/storage/device.rb b/service/lib/agama/dbus/storage/device.rb index dd47e40c98..0b2661c50f 100644 --- a/service/lib/agama/dbus/storage/device.rb +++ b/service/lib/agama/dbus/storage/device.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -# Copyright (c) [2023] SUSE LLC +# Copyright (c) [2023-2024] SUSE LLC # # All Rights Reserved. # @@ -39,11 +39,13 @@ class Device < BaseObject # # @param storage_device [Y2Storage::Device] Storage device # @param path [::DBus::ObjectPath] Path for the D-Bus object + # @param tree [DevicesTree] D-Bus tree in which the device is exported # @param logger [Logger, nil] - def initialize(storage_device, path, logger: nil) + def initialize(storage_device, path, tree, logger: nil) super(path, logger: logger) @storage_device = storage_device + @tree = tree add_interfaces end @@ -52,6 +54,9 @@ def initialize(storage_device, path, logger: nil) # @return [Y2Storage::Device] attr_reader :storage_device + # @return [DevicesTree] + attr_reader :tree + # Adds the required interfaces according to the storage object def add_interfaces # rubocop:disable Metrics/CyclomaticComplexity interfaces = [] @@ -82,7 +87,9 @@ def drive? # # @return [Boolean] def partition_table? - storage_device.is?(:blk_device) && storage_device.partition_table? + storage_device.is?(:blk_device) && + storage_device.respond_to?(:partition_table?) && + storage_device.partition_table? end end end diff --git a/service/lib/agama/dbus/storage/devices_tree.rb b/service/lib/agama/dbus/storage/devices_tree.rb index c0bb3814c4..5c57e49544 100644 --- a/service/lib/agama/dbus/storage/devices_tree.rb +++ b/service/lib/agama/dbus/storage/devices_tree.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -# Copyright (c) [2023] SUSE LLC +# Copyright (c) [2023-2024] SUSE LLC # # All Rights Reserved. # @@ -67,23 +67,27 @@ def update(devicegraph) # @return [Logger] attr_reader :logger - # Exports a D-Bus object for each storage device + # Exports a D-Bus object for each storage device. + # + # Right now, only the required information for calculating a proposal is exported, that is: + # * Information about the potential candidate devices (i.e., disk devices, MDs). + # * Information about the partitions of the candidate devices in order to indicate how to + # find free space. + # + # TODO: export LVM VGs and file systems of directly formatted devices. # # @param devicegraph [Y2Storage::Devicegraph] def export_devices(devicegraph) - # TODO: Right now, the goal of exporting the storage devices on D-Bus is to provide the - # required information of the available devices for calculating a proposal. For that - # reason, only the potential candidate diks are exported (i.e., disk devices and MDs). - # Note that partitons, LVM, etc are not exported yet. devices = devicegraph.disk_devices + devicegraph.software_raids - devices.each { |d| export_device(d) } + + (devices + partitions_from(devices)).each { |d| export_device(d) } end # Exports a D-Bus object for the given device # # @param device [Y2Storage::Device] def export_device(device) - dbus_node = Device.new(device, path_for(device), logger: logger) + dbus_node = Device.new(device, path_for(device), self, logger: logger) service.export(dbus_node) end @@ -101,6 +105,15 @@ def dbus_objects root.descendant_objects end + + # All partitions of the given devices. + # + # @param devices [Array] + # @return [Array] + def partitions_from(devices) + devices.select { |d| d.is?(:blk_device) && d.respond_to?(:partitions) } + .flat_map(&:partitions) + end end end end diff --git a/service/lib/agama/dbus/storage/interfaces/block.rb b/service/lib/agama/dbus/storage/interfaces/block.rb index 11e1b8bb02..84648443f0 100644 --- a/service/lib/agama/dbus/storage/interfaces/block.rb +++ b/service/lib/agama/dbus/storage/interfaces/block.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -# Copyright (c) [2023] SUSE LLC +# Copyright (c) [2023-2024] SUSE LLC # # All Rights Reserved. # @@ -67,6 +67,13 @@ def block_size storage_device.size.to_i end + # Size of the space that could be theoretically reclaimed by shrinking the device. + # + # @return [Integer] + def block_recoverable_size + storage_device.recoverable_size.to_i + end + # Name of the currently installed systems # # @return [Array] @@ -85,6 +92,7 @@ def self.included(base) dbus_reader :block_udev_ids, "as", dbus_name: "UdevIds" dbus_reader :block_udev_paths, "as", dbus_name: "UdevPaths" dbus_reader :block_size, "t", dbus_name: "Size" + dbus_reader :block_recoverable_size, "t", dbus_name: "RecoverableSize" dbus_reader :block_systems, "as", dbus_name: "Systems" end end diff --git a/service/lib/agama/dbus/storage/interfaces/partition_table.rb b/service/lib/agama/dbus/storage/interfaces/partition_table.rb index eb56696f62..635a331984 100644 --- a/service/lib/agama/dbus/storage/interfaces/partition_table.rb +++ b/service/lib/agama/dbus/storage/interfaces/partition_table.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -# Copyright (c) [2023] SUSE LLC +# Copyright (c) [2023-2024] SUSE LLC # # All Rights Reserved. # @@ -39,20 +39,18 @@ def partition_table_type storage_device.partition_table.type.to_s end - # Name of the partitions + # Paths of the D-Bus objects representing the partitions. # - # TODO: return the path of the partition objects once the partitions are exported. - # - # @return [Array] + # @return [Array<::DBus::ObjectPath>] def partition_table_partitions - storage_device.partition_table.partitions.map(&:name) + storage_device.partition_table.partitions.map { |p| tree.path_for(p) } end def self.included(base) base.class_eval do dbus_interface PARTITION_TABLE_INTERFACE do dbus_reader :partition_table_type, "s", dbus_name: "Type" - dbus_reader :partition_table_partitions, "as", dbus_name: "Partitions" + dbus_reader :partition_table_partitions, "ao", dbus_name: "Partitions" end end end From a17cf3f7968222d6061f879fa8157fdf320e1734 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Iv=C3=A1n=20L=C3=B3pez=20Gonz=C3=A1lez?= Date: Fri, 26 Jan 2024 15:48:08 +0000 Subject: [PATCH 2/5] [service] Inherit from BaseTree class --- service/lib/agama/dbus/storage/device.rb | 25 +++++- .../lib/agama/dbus/storage/devices_tree.rb | 79 ++++++------------- 2 files changed, 48 insertions(+), 56 deletions(-) diff --git a/service/lib/agama/dbus/storage/device.rb b/service/lib/agama/dbus/storage/device.rb index 0b2661c50f..5001dd25e0 100644 --- a/service/lib/agama/dbus/storage/device.rb +++ b/service/lib/agama/dbus/storage/device.rb @@ -35,6 +35,9 @@ module Storage # # The D-Bus object includes the required interfaces for the storage object that it represents. class Device < BaseObject + # @return [Y2Storage::Device] + attr_reader :storage_device + # Constructor # # @param storage_device [Y2Storage::Device] Storage device @@ -49,10 +52,26 @@ def initialize(storage_device, path, tree, logger: nil) add_interfaces end - private + # Sets the represented storage device. + # + # @note A properties changed signal is emitted for each interface. + # @raise [RuntimeError] If the given device has a different sid. + # + # @param value [Y2Storage::Device] + def storage_device=(value) + if value.sid != storage_device.sid + raise "Cannot update the D-Bus object because the given device has a different sid: " \ + "#{value} instead of #{storage_device.sid}" + end - # @return [Y2Storage::Device] - attr_reader :storage_device + @storage_device = value + + interfaces_and_properties.each do |interface, properties| + dbus_properties_changed(interface, properties, []) + end + end + + private # @return [DevicesTree] attr_reader :tree diff --git a/service/lib/agama/dbus/storage/devices_tree.rb b/service/lib/agama/dbus/storage/devices_tree.rb index 5c57e49544..c1b395dffe 100644 --- a/service/lib/agama/dbus/storage/devices_tree.rb +++ b/service/lib/agama/dbus/storage/devices_tree.rb @@ -19,25 +19,15 @@ # To contact SUSE LLC about this file by physical or electronic mail, you may # find current contact information at www.suse.com. -require "dbus/object_path" +require "agama/dbus/base_tree" require "agama/dbus/storage/device" +require "dbus/object_path" module Agama module DBus module Storage # Class representing a storage devices tree exported on D-Bus - class DevicesTree - # Constructor - # - # @param service [::DBus::ObjectServer] - # @param root_path [::DBus::ObjectPath] - # @param logger [Logger, nil] - def initialize(service, root_path, logger: nil) - @service = service - @root_path = root_path - @logger = logger - end - + class DevicesTree < BaseTree # Object path for the D-Bus object representing the given device # # @param device [Y2Storage::Device] @@ -48,62 +38,45 @@ def path_for(device) # Updates the D-Bus tree according to the given devicegraph # - # The current D-Bus nodes are all unexported. - # # @param devicegraph [Y2Storage::Devicegraph] def update(devicegraph) - unexport_devices - export_devices(devicegraph) + self.objects = devices(devicegraph) end private - # @return [::DBus::ObjectServer] - attr_reader :service + # @see BaseTree + # @param device [Y2Storage::Device] + def create_dbus_object(device) + Device.new(device, path_for(device), self, logger: logger) + end - # @return [::DBus::ObjectPath] - attr_reader :root_path + # @see BaseTree + # @param dbus_object [Device] + # @param device [Y2Storage::Device] + def update_dbus_object(dbus_object, device) + dbus_object.storage_device = device + end - # @return [Logger] - attr_reader :logger + # @see BaseTree + # @param dbus_object [Device] + # @param device [Y2Storage::Device] + def dbus_object?(dbus_object, device) + dbus_object.storage_device.sid == device.sid + end - # Exports a D-Bus object for each storage device. + # Devices to be exported. # # Right now, only the required information for calculating a proposal is exported, that is: - # * Information about the potential candidate devices (i.e., disk devices, MDs). - # * Information about the partitions of the candidate devices in order to indicate how to - # find free space. + # * Potential candidate devices (i.e., disk devices, MDs). + # * Partitions of the candidate devices in order to indicate how to find free space. # # TODO: export LVM VGs and file systems of directly formatted devices. # # @param devicegraph [Y2Storage::Devicegraph] - def export_devices(devicegraph) + def devices(devicegraph) devices = devicegraph.disk_devices + devicegraph.software_raids - - (devices + partitions_from(devices)).each { |d| export_device(d) } - end - - # Exports a D-Bus object for the given device - # - # @param device [Y2Storage::Device] - def export_device(device) - dbus_node = Device.new(device, path_for(device), self, logger: logger) - service.export(dbus_node) - end - - # Unexports the currently exported D-Bus objects - def unexport_devices - dbus_objects.each { |n| service.unexport(n) } - end - - # All exported D-Bus objects - # - # @return [Array] - def dbus_objects - root = service.get_node(root_path, create: false) - return [] unless root - - root.descendant_objects + devices + partitions_from(devices) end # All partitions of the given devices. From 94ab50f50ea8a0657d2fc260af5f819bc9154514 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Iv=C3=A1n=20L=C3=B3pez=20Gonz=C3=A1lez?= Date: Mon, 29 Jan 2024 13:05:23 +0000 Subject: [PATCH 3/5] [service] Unit tests --- .../test/agama/dbus/storage/device_test.rb | 45 +++++++++- .../agama/dbus/storage/devices_tree_test.rb | 84 ++++++++++++++----- .../dbus/storage/interfaces/block_examples.rb | 12 +++ .../interfaces/partition_table_examples.rb | 7 +- 4 files changed, 122 insertions(+), 26 deletions(-) diff --git a/service/test/agama/dbus/storage/device_test.rb b/service/test/agama/dbus/storage/device_test.rb index 5a99fe26c0..6fa03e64d4 100644 --- a/service/test/agama/dbus/storage/device_test.rb +++ b/service/test/agama/dbus/storage/device_test.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -# Copyright (c) [2023] SUSE LLC +# Copyright (c) [2023-2024] SUSE LLC # # All Rights Reserved. # @@ -28,6 +28,7 @@ require_relative "./interfaces/md_examples" require_relative "./interfaces/partition_table_examples" require "agama/dbus/storage/device" +require "agama/dbus/storage/devices_tree" require "dbus" describe Agama::DBus::Storage::Device do @@ -44,7 +45,11 @@ end end - subject { described_class.new(device, "/test") } + subject { described_class.new(device, "/test", tree) } + + let(:tree) { Agama::DBus::Storage::DevicesTree.new(service, "/agama/devices") } + + let(:service) { instance_double(::DBus::ObjectServer) } before do mock_storage(devicegraph: scenario) @@ -136,4 +141,40 @@ include_examples "Block interface" include_examples "PartitionTable interface" + + describe "#storage_device=" do + before do + allow(subject).to receive(:dbus_properties_changed) + end + + let(:scenario) { "partitioned_md.yml" } + let(:device) { devicegraph.find_by_name("/dev/sda") } + + context "if the given device has a different sid" do + let(:new_device) { devicegraph.find_by_name("/dev/sdb") } + + it "raises an error" do + expect { subject.storage_device = new_device } + .to raise_error(RuntimeError, /Cannot update the D-Bus object/) + end + end + + context "if the given device has the same sid" do + let(:new_device) { devicegraph.find_by_name("/dev/sda") } + + it "sets the new device" do + subject.storage_device = new_device + + expect(subject.storage_device).to equal(new_device) + end + + it "emits a properties changed signal for each interface" do + subject.interfaces_and_properties.each_key do |interface| + expect(subject).to receive(:dbus_properties_changed).with(interface, anything, anything) + end + + subject.storage_device = new_device + end + end + end end diff --git a/service/test/agama/dbus/storage/devices_tree_test.rb b/service/test/agama/dbus/storage/devices_tree_test.rb index 84c4ee0815..2f38bd69fe 100644 --- a/service/test/agama/dbus/storage/devices_tree_test.rb +++ b/service/test/agama/dbus/storage/devices_tree_test.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -# Copyright (c) [2023] SUSE LLC +# Copyright (c) [2023-2024] SUSE LLC # # All Rights Reserved. # @@ -38,6 +38,16 @@ failure_message do |_| "The object #{object_path} is not exported." end + + match_when_negated do |service| + expect(service).to receive(:export) do |dbus_object| + expect(dbus_object.path).to_not eq(object_path) + end + end + + failure_message_when_negated do |_| + "The object #{object_path} is exported." + end end RSpec::Matchers.define(:unexport_object) do |object_path| @@ -77,43 +87,75 @@ mock_storage(devicegraph: scenario) allow(service).to receive(:get_node).with(root_path, anything).and_return(root_node) + allow(root_node).to receive(:descendant_objects).and_return(dbus_objects) + allow(service).to receive(:export) allow(service).to receive(:unexport) - allow(root_node).to receive(:descendant_objects).and_return(dbus_objects) + allow_any_instance_of(::DBus::Object).to receive(:interfaces_and_properties).and_return({}) + allow_any_instance_of(::DBus::Object).to receive(:dbus_properties_changed) end let(:scenario) { "partitioned_md.yml" } let(:root_node) { instance_double(::DBus::Node) } - let(:dbus_objects) do - [ - instance_double(Agama::DBus::Storage::Device, path: "#{root_path}/1001"), - instance_double(Agama::DBus::Storage::Device, path: "#{root_path}/1002") - ] + let(:devicegraph) { Y2Storage::StorageManager.instance.probed } + + context "if a device is not exported yet" do + let(:dbus_objects) { [] } + + it "exports a D-Bus object" do + sda = devicegraph.find_by_name("/dev/sda") + sdb = devicegraph.find_by_name("/dev/sdb") + md0 = devicegraph.find_by_name("/dev/md0") + sda1 = devicegraph.find_by_name("/dev/sda1") + sda2 = devicegraph.find_by_name("/dev/sda2") + md0p1 = devicegraph.find_by_name("/dev/md0p1") + + expect(service).to export_object("#{root_path}/#{sda.sid}") + expect(service).to export_object("#{root_path}/#{sdb.sid}") + expect(service).to export_object("#{root_path}/#{md0.sid}") + expect(service).to export_object("#{root_path}/#{sda1.sid}") + expect(service).to export_object("#{root_path}/#{sda2.sid}") + expect(service).to export_object("#{root_path}/#{md0p1.sid}") + expect(service).to_not receive(:export) + + subject.update(devicegraph) + end end - let(:devicegraph) { Y2Storage::StorageManager.instance.probed } + context "if a device is already exported" do + let(:dbus_objects) { [dbus_object1] } + let(:dbus_object1) { Agama::DBus::Storage::Device.new(sda, subject.path_for(sda), subject) } + let(:sda) { devicegraph.find_by_name("/dev/sda") } - it "unexports the current objects" do - expect(service).to unexport_object("#{root_path}/1001") - expect(service).to unexport_object("#{root_path}/1002") + it "does not export a D-Bus object" do + expect(service).to_not export_object("#{root_path}/#{sda.sid}") + + subject.update(devicegraph) + end - subject.update(devicegraph) + it "updates the D-Bus object" do + expect(dbus_object1.storage_device).to equal(sda) + + subject.update(devicegraph) + + expect(dbus_object1.storage_device).to_not equal(sda) + expect(dbus_object1.storage_device.sid).to equal(sda.sid) + end end - it "exports an object for each storage device" do - sda = devicegraph.find_by_name("/dev/sda") - sdb = devicegraph.find_by_name("/dev/sdb") - md0 = devicegraph.find_by_name("/dev/md0") + context "if an exported D-Bus object does not represent any of the current devices" do + let(:dbus_objects) { [dbus_object1] } + let(:dbus_object1) { Agama::DBus::Storage::Device.new(sdd, subject.path_for(sdd), subject) } + let(:sdd) { instance_double(Y2Storage::Disk, sid: 1, is?: false) } - expect(service).to export_object("#{root_path}/#{sda.sid}") - expect(service).to export_object("#{root_path}/#{sdb.sid}") - expect(service).to export_object("#{root_path}/#{md0.sid}") - expect(service).to_not receive(:export) + it "unexports the D-Bus object" do + expect(service).to unexport_object("#{root_path}/1") - subject.update(devicegraph) + subject.update(devicegraph) + end end end end diff --git a/service/test/agama/dbus/storage/interfaces/block_examples.rb b/service/test/agama/dbus/storage/interfaces/block_examples.rb index 1a8e37b624..5922038a3f 100644 --- a/service/test/agama/dbus/storage/interfaces/block_examples.rb +++ b/service/test/agama/dbus/storage/interfaces/block_examples.rb @@ -79,6 +79,18 @@ end end + describe "#block_recoverable_size" do + before do + allow(device).to receive(:recoverable_size).and_return(size) + end + + let(:size) { Y2Storage::DiskSize.new(1024) } + + it "returns the recoverable size in bytes" do + expect(subject.block_recoverable_size).to eq(1024) + end + end + describe "#block_systems" do let(:filesystem1) { instance_double(Y2Storage::Filesystems::Base, is?: false) } let(:filesystem2) { instance_double(Y2Storage::Filesystems::Base, is?: false) } diff --git a/service/test/agama/dbus/storage/interfaces/partition_table_examples.rb b/service/test/agama/dbus/storage/interfaces/partition_table_examples.rb index 362ed88eca..a902e3dbda 100644 --- a/service/test/agama/dbus/storage/interfaces/partition_table_examples.rb +++ b/service/test/agama/dbus/storage/interfaces/partition_table_examples.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -# Copyright (c) [2023] SUSE LLC +# Copyright (c) [2023-2024] SUSE LLC # # All Rights Reserved. # @@ -34,8 +34,9 @@ end describe "#partition_table_partitions" do - it "returns the name of the partitions" do - expect(subject.partition_table_partitions).to contain_exactly("/dev/md0p1") + it "returns the path of the partitions" do + md0p1 = devicegraph.find_by_name("/dev/md0p1") + expect(subject.partition_table_partitions).to contain_exactly(tree.path_for(md0p1)) end end end From b76b2d58d372f0cdeedf74d9fca6922ef5dee955 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Iv=C3=A1n=20L=C3=B3pez=20Gonz=C3=A1lez?= Date: Mon, 29 Jan 2024 13:52:12 +0000 Subject: [PATCH 4/5] [service] Changelog --- service/package/rubygem-agama.changes | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/service/package/rubygem-agama.changes b/service/package/rubygem-agama.changes index 686902495f..df8922d318 100644 --- a/service/package/rubygem-agama.changes +++ b/service/package/rubygem-agama.changes @@ -1,3 +1,8 @@ +------------------------------------------------------------------- +Mon Jan 29 13:51:30 UTC 2024 - José Iván López González + +- Export partitions on D-Bus (gh#openSUSE/agama#1016). + ------------------------------------------------------------------- Thu Jan 18 14:55:36 UTC 2024 - José Iván López González From d48d43a4b43ec953571787ac6b7ac1867500adc9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Iv=C3=A1n=20L=C3=B3pez=20Gonz=C3=A1lez?= Date: Tue, 30 Jan 2024 14:10:26 +0000 Subject: [PATCH 5/5] [service] Document return type --- service/lib/agama/dbus/storage/devices_tree.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/service/lib/agama/dbus/storage/devices_tree.rb b/service/lib/agama/dbus/storage/devices_tree.rb index c1b395dffe..c06501bd1e 100644 --- a/service/lib/agama/dbus/storage/devices_tree.rb +++ b/service/lib/agama/dbus/storage/devices_tree.rb @@ -74,6 +74,7 @@ def dbus_object?(dbus_object, device) # TODO: export LVM VGs and file systems of directly formatted devices. # # @param devicegraph [Y2Storage::Devicegraph] + # @return [Array] def devices(devicegraph) devices = devicegraph.disk_devices + devicegraph.software_raids devices + partitions_from(devices)