From 67d9a7247aa3dd950c8ad5ab806c19e3508f0567 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, 28 Aug 2023 15:40:33 +0100 Subject: [PATCH] Improvements from review --- ...Agama.Storage1.Proposal.Calculator.doc.xml | 4 ++-- ...g.opensuse.Agama.Storage1.Proposal.doc.xml | 4 ++-- service/etc/agama.yaml | 4 ++-- service/lib/agama/dbus/storage/manager.rb | 8 +++++-- service/lib/agama/dbus/storage/proposal.rb | 2 +- .../proposal_settings_conversion/to_dbus.rb | 2 +- .../storage/volume_conversion/from_dbus.rb | 8 ++++--- .../lib/agama/storage/encryption_settings.rb | 4 +++- service/lib/agama/storage/lvm_settings.rb | 6 +---- service/lib/agama/storage/proposal.rb | 3 ++- .../lib/agama/storage/proposal_settings.rb | 2 +- .../to_y2storage.rb | 6 ++--- .../volume_conversion/from_y2storage.rb | 9 ++++++- .../storage/volume_conversion/to_y2storage.rb | 5 ++++ service/lib/agama/storage/volume_outline.rb | 24 ++++++++++++++----- 15 files changed, 60 insertions(+), 31 deletions(-) diff --git a/doc/dbus/org.opensuse.Agama.Storage1.Proposal.Calculator.doc.xml b/doc/dbus/org.opensuse.Agama.Storage1.Proposal.Calculator.doc.xml index 5a973c8df4..216b35650f 100644 --- a/doc/dbus/org.opensuse.Agama.Storage1.Proposal.Calculator.doc.xml +++ b/doc/dbus/org.opensuse.Agama.Storage1.Proposal.Calculator.doc.xml @@ -21,8 +21,8 @@ TargetDevice s TargetVG s FsType s - MinSize t - MaxSize t (optinal, max size is considered as unlimited if omitted) + MinSize t (bytes) + MaxSize t (bytes. Optinal, max size is considered as unlimited if omitted) AutoSize b Snapshots b Outline a{sv} diff --git a/doc/dbus/org.opensuse.Agama.Storage1.Proposal.doc.xml b/doc/dbus/org.opensuse.Agama.Storage1.Proposal.doc.xml index 290dfbea93..cf79a5855d 100644 --- a/doc/dbus/org.opensuse.Agama.Storage1.Proposal.doc.xml +++ b/doc/dbus/org.opensuse.Agama.Storage1.Proposal.doc.xml @@ -19,8 +19,8 @@ TargetDevice s TargetVG s FsType s - MinSize t - MaxSize t (optinal, max size is considered as unlimited if omitted) + MinSize t (bytes) + MaxSize t (bytes. Optinal, max size is considered as unlimited if omitted) AutoSize b Snapshots b Outline a{sv} diff --git a/service/etc/agama.yaml b/service/etc/agama.yaml index 39a0c28a05..ef44acbd72 100644 --- a/service/etc/agama.yaml +++ b/service/etc/agama.yaml @@ -41,7 +41,7 @@ ALP-Dolomite: - package: device-mapper # Apparently needed if devices at /dev/mapper are used at boot (eg. FDE) - package: fde-tools # Needed for FDE with TPM, hardcoded here temporarily archs: aarch64, x86_64 - - package: libtss2-tcti-device0 # Same than fde-tools + - package: libtss2-tcti-device0 - package: ppc64-diag # Needed for hardware-based installations archs: ppc64 optional_packages: null @@ -255,7 +255,7 @@ Leap16: - package: device-mapper # Apparently needed if devices at /dev/mapper are used at boot (eg. FDE) - package: fde-tools # Needed for FDE with TPM, hardcoded here temporarily archs: aarch64, x86_64 - - package: libtss2-tcti-device0 # Same than fde-tools + - package: libtss2-tcti-device0 optional_packages: null base_product: Leap16 diff --git a/service/lib/agama/dbus/storage/manager.rb b/service/lib/agama/dbus/storage/manager.rb index 2d327465ac..d820fc280a 100644 --- a/service/lib/agama/dbus/storage/manager.rb +++ b/service/lib/agama/dbus/storage/manager.rb @@ -145,9 +145,13 @@ def default_volume(mount_path) # @param dbus_settings [Hash] # @return [Integer] 0 success; 1 error def calculate_proposal(dbus_settings) - logger.info("Calculating storage proposal from D-Bus settings: #{dbus_settings}") - settings = ProposalSettingsConversion.from_dbus(dbus_settings, config: config) + logger.info( + "Calculating storage proposal from D-Bus.\n " \ + "D-Bus settings: #{dbus_settings}\n" \ + "Agama settings: #{settings}" + ) + success = proposal.calculate(settings) success ? 0 : 1 diff --git a/service/lib/agama/dbus/storage/proposal.rb b/service/lib/agama/dbus/storage/proposal.rb index 4ef7b8b8e8..73e4fd24b1 100644 --- a/service/lib/agama/dbus/storage/proposal.rb +++ b/service/lib/agama/dbus/storage/proposal.rb @@ -65,7 +65,7 @@ def initialize(backend, logger) # Device used as boot device by the storage proposal # - # @return [String] + # @return [String] Empty string if no device has been selected yet. def boot_device dbus_settings.fetch("BootDevice", "") end diff --git a/service/lib/agama/dbus/storage/proposal_settings_conversion/to_dbus.rb b/service/lib/agama/dbus/storage/proposal_settings_conversion/to_dbus.rb index edb3782238..8d641f0b1b 100644 --- a/service/lib/agama/dbus/storage/proposal_settings_conversion/to_dbus.rb +++ b/service/lib/agama/dbus/storage/proposal_settings_conversion/to_dbus.rb @@ -38,7 +38,7 @@ def initialize(settings) def convert # rubocop:disable Metrics/AbcSize { "BootDevice" => settings.boot_device.to_s, - "LVM" => settings.lvm.enabled, + "LVM" => settings.lvm.enabled?, "SystemVGDevices" => settings.lvm.system_vg_devices, "EncryptionPassword" => settings.encryption.password.to_s, "EncryptionMethod" => settings.encryption.method.id.to_s, diff --git a/service/lib/agama/dbus/storage/volume_conversion/from_dbus.rb b/service/lib/agama/dbus/storage/volume_conversion/from_dbus.rb index 503517b1bb..3499c599fc 100644 --- a/service/lib/agama/dbus/storage/volume_conversion/from_dbus.rb +++ b/service/lib/agama/dbus/storage/volume_conversion/from_dbus.rb @@ -104,8 +104,10 @@ def target_vg_conversion(target, value) # @param target [Agama::Storage::Volume] # @param value [String] def fs_type_conversion(target, value) + downcase_value = value.downcase + fs_type = target.outline.filesystems.find do |type| - type.to_human_string.downcase == value.downcase + type.to_human_string.downcase == downcase_value end return unless fs_type @@ -114,13 +116,13 @@ def fs_type_conversion(target, value) end # @param target [Agama::Storage::Volume] - # @param value [Integer] + # @param value [Integer] Size in bytes. def min_size_conversion(target, value) target.min_size = Y2Storage::DiskSize.new(value) end # @param target [Agama::Storage::Volume] - # @param value [Integer] + # @param value [Integer] Size in bytes. def max_size_conversion(target, value) target.max_size = Y2Storage::DiskSize.new(value) end diff --git a/service/lib/agama/storage/encryption_settings.rb b/service/lib/agama/storage/encryption_settings.rb index 034ef57d00..5e308a6398 100644 --- a/service/lib/agama/storage/encryption_settings.rb +++ b/service/lib/agama/storage/encryption_settings.rb @@ -36,7 +36,9 @@ class EncryptionSettings # @return [Y2Storage::EncryptionMethod::Base] attr_accessor :method - # @return [Y2Storage::PbkdFunction, nil] + # PBKD function to use for LUKS2 + # + # @return [Y2Storage::PbkdFunction, nil] Can be nil if using LUKS1. attr_accessor :pbkd_function def initialize diff --git a/service/lib/agama/storage/lvm_settings.rb b/service/lib/agama/storage/lvm_settings.rb index 6840fab5ea..245ad27b45 100644 --- a/service/lib/agama/storage/lvm_settings.rb +++ b/service/lib/agama/storage/lvm_settings.rb @@ -27,6 +27,7 @@ class LvmSettings # # @return [Boolean] attr_accessor :enabled + alias_method :enabled?, :enabled # Devices to use for the system LVM volume group # @@ -37,11 +38,6 @@ def initialize @enabled = false @system_vg_devices = [] end - - # @return [Boolean] - def enabled? - !!@enabled - end end end end diff --git a/service/lib/agama/storage/proposal.rb b/service/lib/agama/storage/proposal.rb index 69f9d3688f..3bfbd48306 100644 --- a/service/lib/agama/storage/proposal.rb +++ b/service/lib/agama/storage/proposal.rb @@ -101,7 +101,8 @@ def calculate(settings) # Note that this settings might differ from the {#original_settings}. For example, the sizes # of some volumes could be adjusted if auto size is set. # - # @return [ProposalSettings, nil] + # @return [ProposalSettings, nil] nil if no proposal has been calculated yet or the proposal + # was invalidated. def settings return nil unless calculated? diff --git a/service/lib/agama/storage/proposal_settings.rb b/service/lib/agama/storage/proposal_settings.rb index 14a83cebe1..c0e237de50 100644 --- a/service/lib/agama/storage/proposal_settings.rb +++ b/service/lib/agama/storage/proposal_settings.rb @@ -45,7 +45,7 @@ class ProposalSettings # Device name of the disk that will be used for booting the system and also to allocate all # the partitions, except those that have been explicitly assigned to other disk(s). # - # @return [String, nil] + # @return [String, nil] nil if no device has been selected yet. attr_accessor :boot_device # Set of volumes to create diff --git a/service/lib/agama/storage/proposal_settings_conversion/to_y2storage.rb b/service/lib/agama/storage/proposal_settings_conversion/to_y2storage.rb index 5407a60601..7955858f57 100644 --- a/service/lib/agama/storage/proposal_settings_conversion/to_y2storage.rb +++ b/service/lib/agama/storage/proposal_settings_conversion/to_y2storage.rb @@ -136,13 +136,13 @@ def fallbacks_conversion(target) end end - # @param mount_path [String] + # @param mount_path [String, nil] nil if not found def find_min_size_fallback(mount_path) volume = settings.volumes.find { |v| v.min_size_fallback_for.include?(mount_path) } volume&.mount_path end - # @param mount_path [String] + # @param mount_path [String, nil] nil if not found def find_max_size_fallback(mount_path) volume = settings.volumes.find { |v| v.max_size_fallback_for.include?(mount_path) } volume&.mount_path @@ -167,7 +167,7 @@ def all_devices # @return [String, Array] def device_or_partitions(device) partitions = partitions(device) - partitions.any? ? partitions : device + partitions.empty? ? device : partitions end # @param device [String] diff --git a/service/lib/agama/storage/volume_conversion/from_y2storage.rb b/service/lib/agama/storage/volume_conversion/from_y2storage.rb index cccafd7808..364531699c 100644 --- a/service/lib/agama/storage/volume_conversion/from_y2storage.rb +++ b/service/lib/agama/storage/volume_conversion/from_y2storage.rb @@ -61,8 +61,15 @@ def convert # @param target [Agama::Storage::Volume] def sizes_conversion(target) - target.auto_size = !(spec.ignore_fallback_sizes? && spec.ignore_snapshots_sizes?) + target.auto_size = !spec.ignore_fallback_sizes? || !spec.ignore_snapshots_sizes? + # The volume specification contains the min and max sizes for the volume. But the final + # range of sizes used by the Y2Storage proposal depends on the fallback sizes (if this + # volume is fallback for other volume) and the size for snapshots (if snapshots is + # active). The planned device contains the real range of sizes used by the proposal. + # + # From Agama point of view, this is the way of recovering the range of sizes used by + # Y2Storage when a volume is set to have auto size. planned = planned_device_for(spec.mount_point) target.min_size = planned&.min || spec.min_size target.max_size = planned&.max || spec.max_size diff --git a/service/lib/agama/storage/volume_conversion/to_y2storage.rb b/service/lib/agama/storage/volume_conversion/to_y2storage.rb index 0eef8128bf..059733b89a 100644 --- a/service/lib/agama/storage/volume_conversion/to_y2storage.rb +++ b/service/lib/agama/storage/volume_conversion/to_y2storage.rb @@ -65,6 +65,11 @@ def sizes_conversion(target) target.ignore_fallback_sizes = !auto target.ignore_snapshots_sizes = !auto + # The range of sizes is defined by the volume outline in case of auto size (mix and max + # sizes cannot be configured if auto size is set). + # And note that the inal range of sizes used by the Y2Storage proposal is calculated by + # Y2Storage according the range configured here and other sizes like fallback sizes or + # the size for snapshots. target.min_size = auto ? volume.outline.base_min_size : volume.min_size target.max_size = auto ? volume.outline.base_max_size : volume.max_size end diff --git a/service/lib/agama/storage/volume_outline.rb b/service/lib/agama/storage/volume_outline.rb index 15734ed547..8214f99088 100644 --- a/service/lib/agama/storage/volume_outline.rb +++ b/service/lib/agama/storage/volume_outline.rb @@ -41,13 +41,13 @@ class VolumeOutline attr_accessor :filesystems # Base value to calculate the min size for the volume (if #auto_size is set to true - # for that final volume) or to use as default value (if #auto_size is false) + # for that final volume) # # @return [Y2Storage::DiskSize] attr_accessor :base_min_size # Base value to calculate the max size for the volume (if #auto_size is set to true - # for that final volume) or to use as default value (if #auto_size is false) + # for that final volume) # # @return [Y2Storage::DiskSize] attr_accessor :base_max_size @@ -56,10 +56,22 @@ class VolumeOutline attr_accessor :adjust_by_ram alias_method :adjust_by_ram?, :adjust_by_ram - # @return [Array] mount paths of other volumes + # Lists the mount paths of the volumes for which this volume is a min size fallback. + # + # Being a min size fallback means that the min size of the volume would be increased by the + # min size of other volumes, if any of that other volumes is not used for the proposal. + # + # For example, let's say the root volume is a fallback for the min size of /home and /var + # (root.min_size_fallback_for => ["/home", "/var"]). And a proposal is calculated with only + # root and /home. In that case, the min size of /var is added to the min size of root. The + # same would happen for /home if the proposal does not include it. + # + # @return [Array] attr_accessor :min_size_fallback_for - # @return [Array] mount paths of other volumes + # The same as {#min_size_fallback_for}, but for the max size of the volume. + # + # @return [Array] attr_accessor :max_size_fallback_for # Whether snapshots option can be configured @@ -70,12 +82,12 @@ class VolumeOutline # Size required for snapshots # - # @return [Y2Storage::DiskSize, nil] + # @return [Y2Storage::DiskSize, nil] nil if no extra size for snapshots. attr_accessor :snapshots_size # Percentage of space required for snapshots # - # @return [Integer, nil] + # @return [Integer, nil] nil if no extra size for snapshots. attr_accessor :snapshots_percentage def initialize