Skip to content

feat(storage): add search conditions#2338

Merged
joseivanlopez merged 12 commits intoagama-project:masterfrom
joseivanlopez:search-conditions
May 14, 2025
Merged

feat(storage): add search conditions#2338
joseivanlopez merged 12 commits intoagama-project:masterfrom
joseivanlopez:search-conditions

Conversation

@joseivanlopez
Copy link
Contributor

@joseivanlopez joseivanlopez commented May 7, 2025

Extend storage schema with more search conditions:

  • Drives and MD RAIDs: by name, by size (equal, greater or less).
  • Partitions: by name, by size (equal, greater or less), by partition number.

Requires yast/yast-storage-ng#1410.

@joseivanlopez joseivanlopez force-pushed the search-conditions branch 15 times, most recently from de72de3 to 7d83831 Compare May 13, 2025 12:03
@joseivanlopez joseivanlopez marked this pull request as ready for review May 14, 2025 08:57
@joseivanlopez
Copy link
Contributor Author

Changelogs will be updated once the PR is approved to avoid fixing conflicts with master.

new.tap { |c| c.if_not_found = :skip }
end

# Search by name.
Copy link
Contributor

Choose a reason for hiding this comment

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

This design of Configs::Search with separate explicit attributes for name, size and partition_number doesn't look too flexible for the future. How will it work in the future to combine conditions? I mean stuff like this (not sure about the syntax yet):

{
  "partitions": [
    {
      "search": {
        "if_not_found": "create",
        "condition": {
          { "or": [
              { "number": 2 },
              { "and": [
                  { "size": { "greater": "1 MiB" } },
                  { "size": { "less": "10 MiB" } }
                ]
              }
            ]
          }
        }
      }
    }
  ] 
}

Copy link
Contributor Author

@joseivanlopez joseivanlopez May 14, 2025

Choose a reason for hiding this comment

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

I did the minimal changes to Configs::Search to support the current needs. It will evolve according to what we want to offer. I think in the fututure we will have somehing like Configs::Search#condition and Configs::SearchCondition, or something similar.

#
# @return [Array<Y2Storage::Drive, Y2Storage::StrayBlkDevice>]
def candidate_drives
disk_analyzer = Y2Storage::DiskAnalyzer.new(devicegraph)
Copy link
Contributor

Choose a reason for hiding this comment

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

One of the main features/goals of DiskAnalyzer is to cache all the intermediate and final results that are expensive (eg. need to lookup into the system, to mount something, etc.).

In other words, disk analyzers are more designed to be shared together with its devicegraph than to be created over and over.

I feel kind of uncomfortable creating instances of DiskAnalyzer for every search (or for every kind of search). Have we checked it's not a potential performance problem?

Copy link
Contributor Author

@joseivanlopez joseivanlopez May 14, 2025

Choose a reason for hiding this comment

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

I haven't checked the performance. I will pass the disk analyzer to avoid some potential penalty. Anyway, it would be nice to have DiskAnalyzer#devicegraph as a public method. So we don't have to pass a devicegraph-analyzer pair.

@joseivanlopez joseivanlopez force-pushed the search-conditions branch 2 times, most recently from 7e6522f to 8ef3c6e Compare May 14, 2025 12:12
Copy link
Contributor

@ancorgs ancorgs left a comment

Choose a reason for hiding this comment

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

LGTM (except, of course, the lack of a changelog).

@ancorgs
Copy link
Contributor

ancorgs commented May 14, 2025

Now that I realize, should we update the dependency on yast2-storage-ng?

@joseivanlopez
Copy link
Contributor Author

Now that I realize, should we update the dependency on yast2-storage-ng?

Yes, it is updated, see https://github.com/agama-project/agama/pull/2338/files#diff-ceee56f39d11b19309aec78a4c4b31b91f100062d85d7548a3d1cba14972cfa2.

@joseivanlopez joseivanlopez merged commit 2181bcb into agama-project:master May 14, 2025
2 of 4 checks passed
@imobachgs imobachgs mentioned this pull request May 26, 2025
imobachgs added a commit that referenced this pull request May 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants