Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions rust/agama-lib/share/examples/storage/drives.json
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,7 @@
"deleteIfNeeded": true
},
{
"search": {
"ifNotFound": "skip"
},
"search": "*",
"delete": true
},
{
Expand Down
8 changes: 8 additions & 0 deletions rust/agama-lib/share/profile.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -1029,6 +1029,11 @@
"type": "string",
"examples": ["/dev/vda", "/dev/disk/by-id/ata-WDC_WD3200AAKS-75L9"]
},
"searchAll": {
"title": "Search all devices",
"description": "Shortcut to match all devices if there is any (equivalent to specify no conditions and to skip the entry if no device is found).",
"const": "*"
},
"searchByName": {
"title": "Search by name condition",
"type": "object",
Expand All @@ -1042,6 +1047,9 @@
},
"search": {
"anyOf": [
{
"$ref": "#/$defs/searchAll"
},
{
"$ref": "#/$defs/searchName"
},
Expand Down
3 changes: 3 additions & 0 deletions service/lib/agama/storage/config_conversions.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@ module Agama
module Storage
# Conversions for the storage config.
module ConfigConversions
# Reserved string for Configs::Search meaning 'match all devices if there is any'
SEARCH_ANYTHING_STRING = "*"
private_constant :SEARCH_ANYTHING_STRING
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -41,27 +41,20 @@ def convert
# @see Base#conversions
# @return [Hash]
def conversions
return convert_string if search_json.is_a?(String)

{
name: convert_name,
if_not_found: convert_not_found
name: search_json.dig(:condition, :name),
max: search_json[:max],
if_not_found: search_json[:ifNotFound]&.to_sym
}
end

# @return [String, nil]
def convert_name
return search_json if search_json.is_a?(String)

search_json.dig(:condition, :name)
end

# @return [Symbol, nil]
def convert_not_found
return if search_json.is_a?(String)

value = search_json[:ifNotFound]
return unless value
# @return [String]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NP: I think we can follow the same implementation pattern as the rest of conversions:

          # @see Base#conversions
          # @return [Hash]
          def conversions
            {
              name:         convert_name,
              max:          search_json[:max],
              if_not_found: convert_not_found
            }
          end

          # @return [String, nil]
          def convert_name
            return if search_json == SEARCH_ANYTHING_STRING
            return search_json if search_json.is_a?(String)

            search_json.dig(:condition, :name)
          end

          # @return [Symbol, nil]
          def convert_not_found
            return :skip if search_json == SEARCH_ANYTHING_STRING
            return if search_json.is_a?(String)
            
            search_json[:ifNotFound]&.to_sym          
          end

@ancorgs ancorgs Oct 24, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find that much harder to follow.

We typically handle a hash except an special case in which a string can be used as shortcut.

With the implementation in this PR, that special case is handled at its own method.

With the implementation you suggest, the management is spread over several methods, all of them with its own exception for the string case. And the more attributes we add, the more methods we have with a dedicated if at the beginning. I don't see the point.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we see it differently. In the conversions I prefer to focus on each attribute and assign a value according to the value of the search_json. But it is a NP anyway, so let keep your implementation.

def convert_string
return { if_not_found: :skip } if search_json == SEARCH_ANYTHING_STRING

value.to_sym
{ name: search_json }
end
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,21 @@ def self.config_type
Configs::Search
end

# @see Base#convert
def convert
return SEARCH_ANYTHING_STRING if config.all_if_any?

super
end

private

# @see Base#conversions
def conversions
{
condition: convert_condition,
ifNotFound: config.if_not_found.to_s
ifNotFound: config.if_not_found.to_s,
max: config.max
}
end

Expand Down
102 changes: 76 additions & 26 deletions service/lib/agama/storage/config_search_solver.rb
Original file line number Diff line number Diff line change
Expand Up @@ -36,22 +36,7 @@ def initialize(devicegraph)
# @param config [Agama::Storage::Config]
def solve(config)
@sids = []
config.drives.each do |drive_config|
device = find_drive(drive_config.search)
drive_config.search.solve(device)

add_found(drive_config)

next unless drive_config.found_device && drive_config.partitions?

drive_config.partitions.each do |partition_config|
next unless partition_config.search

partition = find_partition(partition_config.search, drive_config.found_device)
partition_config.search.solve(partition)
add_found(partition_config)
end
end
config.drives = config.drives.flat_map { |d| solve_drive(d) }
end

private
Expand All @@ -62,24 +47,87 @@ def solve(config)
# @return [Array<Integer>] SIDs of the devices that are already associated to another search.
attr_reader :sids

# Finds a drive matching the given search config.
# @see #solve
#
# @note The given drive object can be modified
#
# @param original_drive [Configs::Drive]
# @return [Configs::Drive, Array<Configs::Drive>]
def solve_drive(original_drive)
devices = find_drives(original_drive.search)
return without_device(original_drive) if devices.empty?

devices.map do |device|
drive_copy(original_drive, device)
end
end

# Marks the search of the given config object as solved
#
# @note The config object is modified.
#
# @param config [Configs::Drive, Configs::Partition]
# @return [Configs::Drive, Configs::Partition]
def without_device(config)
config.search.solve
config
end

# see #solve_drive
def drive_copy(original_drive, device)
drive_config = original_drive.copy
drive_config.search.solve(device)
add_found(drive_config)

return drive_config unless drive_config.partitions?

drive_config.partitions = drive_config.partitions.flat_map do |partition_config|
solve_partition(partition_config, device)
end

drive_config
end

# see #solve_drive
#
# @note The given partition object can be modified
#
# @param original_partition [Configs::Partition]
# @param drive_device [Y2Storage::Partitionable]
# @return [Configs::Partition, Array<Configs::Partition>]
def solve_partition(original_partition, drive_device)
return original_partition unless original_partition.search

partitions = find_partitions(original_partition.search, drive_device)
return without_device(original_partition) if partitions.empty?

partitions.map do |partition|
partition_config = original_partition.copy
partition_config.search.solve(partition)
add_found(partition_config)

partition_config
end
end

# Finds the drives matching the given search config.
#
# @param search_config [Agama::Storage::Configs::Search]
# @return [Y2Storage::Device, nil]
def find_drive(search_config)
def find_drives(search_config)
candidates = candidate_devices(search_config, default: devicegraph.blk_devices)
candidates.select! { |d| d.is?(:disk_device, :stray_blk_device) }
next_unassigned_device(candidates)
next_unassigned_devices(candidates, search_config)
end

# Finds a partitions matching the given search config.
# Finds the partitions matching the given search config, if any
#
# @param search_config [Agama::Storage::Configs::Search]
# @return [Y2Storage::Device, nil]
def find_partition(search_config, device)
def find_partitions(search_config, device)
candidates = candidate_devices(search_config, default: device.partitions)
candidates.select! { |d| d.is?(:partition) }
next_unassigned_device(candidates)
next_unassigned_devices(candidates, search_config)
end

# Candidate devices for the given search config.
Expand All @@ -89,7 +137,7 @@ def find_partition(search_config, device)
# conditions.
# @return [Array<Y2Storage::Device>]
def candidate_devices(search_config, default: [])
return default if search_config.any_device?
return default if search_config.always_match?

[find_device(search_config)].compact
end
Expand All @@ -102,14 +150,16 @@ def find_device(search_config)
devicegraph.find_by_any_name(search_config.name)
end

# Next unassigned device from the given list.
# Next unassigned devices from the given list.
#
# @param devices [Array<Y2Storage::Device>]
# @param search [Config::Search]
# @return [Y2Storage::Device, nil]
def next_unassigned_device(devices)
def next_unassigned_devices(devices, search)
devices
.reject { |d| sids.include?(d.sid) }
.min_by(&:name)
.sort_by(&:name)
.first(search.max || devices.size)
end

# @see #search
Expand Down
2 changes: 1 addition & 1 deletion service/lib/agama/storage/configs/drive.rb
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ class Drive
def initialize
@partitions = []
# All drives are expected to match a real device in the system, so let's ensure a search.
@search = Search.new
@search = Search.new.tap { |s| s.max = 1 }
end

# Whether the drive definition contains partition definitions
Expand Down
15 changes: 14 additions & 1 deletion service/lib/agama/storage/configs/search.rb
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,12 @@ class Search
# @return [:create, :skip, :error]
attr_accessor :if_not_found

# Optional max number of devices to match
#
# return [Integer, nil] nil means no limit, ie. all devices that meet the condition are
# matched
attr_accessor :max
Comment thread
ancorgs marked this conversation as resolved.

# Constructor
def initialize
@solved = false
Expand All @@ -61,10 +67,17 @@ def solve(device = nil)
# Whether the search does not define any specific condition.
#
# @return [Boolean]
def any_device?
def always_match?
name.nil?
end

# Whether the search matches all the available devices, skipping if none is found
#
# @return [Boolean]
def all_if_any?
always_match? && max.nil? && if_not_found == :skip
end

# Whether the section containing the search should be skipped
#
# @return [Boolean]
Expand Down
8 changes: 8 additions & 0 deletions service/lib/agama/storage/configs/with_search.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,14 @@ module WithSearch
def found_device
search&.device
end

# Creates a deep copy of the config element
#
# Needed when a search returns multiple devices and the configuration needs to be replicated
# for each one.
def copy

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for myself: use Copyable mixing introduced by #1692.

Marshal.load(Marshal.dump(self))
end
end
end
end
Expand Down
4 changes: 2 additions & 2 deletions service/lib/y2storage/proposal/agama_vg_planner.rb
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,8 @@ def planned_vg(vg_config, config)
# @param config [Agama::Storage::Config]
# @return [Array<String>]
def devices_for_pvs(vg_config, config)
drives = vg_config.physical_volumes_devices.map do |dev_alias|
config.drives.find { |d| d.alias?(dev_alias) }
drives = vg_config.physical_volumes_devices.flat_map do |dev_alias|
config.drives.select { |d| d.alias?(dev_alias) }
end.compact

drives.map { |d| d.found_device.name }
Expand Down
6 changes: 6 additions & 0 deletions service/package/rubygem-agama-yast.changes
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
-------------------------------------------------------------------
Thu Oct 24 13:07:50 UTC 2024 - Ancor Gonzalez Sosa <ancor@suse.com>

- Storage: support to match several devices with every 'search'
section (gh#agama-project/agama#1691).

-------------------------------------------------------------------
Tue Oct 22 09:48:57 UTC 2024 - José Iván López González <jlopez@suse.com>

Expand Down
6 changes: 4 additions & 2 deletions service/test/agama/storage/config_checker_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -244,8 +244,8 @@

before do
mock_storage(devicegraph: scenario)
# To speed-up the tests
allow(Y2Storage::EncryptionMethod::TPM_FDE)
# To speed-up the tests. Use #allow_any_instance because #allow introduces marshaling problems
allow_any_instance_of(Y2Storage::EncryptionMethod::TpmFde)
.to(receive(:possible?))
.and_return(true)
end
Expand All @@ -255,6 +255,8 @@
# Solves the config before checking.
devicegraph = Y2Storage::StorageManager.instance.probed

allow(Y2Storage::BlkDevice).to receive(:find_by_any_name)

Agama::Storage::ConfigSolver
.new(devicegraph, product_config)
.solve(config)
Expand Down
32 changes: 31 additions & 1 deletion service/test/agama/storage/config_conversions/from_json_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@

require_relative "../../../test_helper"
require "agama/config"
require "agama/storage/config_conversions/from_json"
require "agama/storage/config_conversions"
require "y2storage/encryption_method"
require "y2storage/filesystems/mount_by_type"
require "y2storage/filesystems/type"
Expand Down Expand Up @@ -107,6 +107,18 @@
end
end

context "with an asterisk" do
let(:search) { "*" }

it "sets #search to the expected value" do
config = config_proc.call(subject.convert)
expect(config.search).to be_a(Agama::Storage::Configs::Search)
expect(config.search.name).to be_nil
expect(config.search.if_not_found).to eq(:skip)
expect(config.search.max).to be_nil
end
end

context "with a search section" do
let(:search) do
{
Expand All @@ -120,6 +132,24 @@
expect(config.search).to be_a(Agama::Storage::Configs::Search)
expect(config.search.name).to eq("/dev/vda1")
expect(config.search.if_not_found).to eq(:skip)
expect(config.search.max).to be_nil
end
end

context "with a search section including a max" do
let(:search) do
{
ifNotFound: "error",
max: 3
}
end

it "sets #search to the expected value" do
config = config_proc.call(subject.convert)
expect(config.search).to be_a(Agama::Storage::Configs::Search)
expect(config.search.name).to be_nil
expect(config.search.if_not_found).to eq(:error)
expect(config.search.max).to eq 3
end
end
end
Expand Down
Loading