[Service] Alternative location for volumes (#1105)

## Problem

The storage proposal allows to specify what to do in order to allocate
any volume:

- Use the default location (new partition or new LV)
- Create it in a given disk as a new partition
- Create it as a new LV in a new dedicated VG on top of a given disk
- Reformat an existing device
- Mount an existing device

Everything is possible, no matter whether the default target is a disk
or a new LVM VG.

So far, it was not possible to use the Agama D-Bus settings to specify
that to the proposal, so all volumes always used the default location.

## Solution

This adds two new attributes to the D-Bus definition of a volume:
`Target` and `TargetDevice`.

Internally, this is represented by a new class `VolumeLocation` with two
attributes:
  - `target`: which specifies how to allocate the volume
- `device`: the name of the target device, the exact meaning depends on
`target`

The new class and the possible targets follow the same approach of
`SpaceSettings` and its policies.

## Testing

- Added several unit tests
- Tested manually by hacking the web UI a bit (see screenshots below)

## Screenshots

Hacking the web UI to set Target to `filesystem` and TargetDevice to
`/dev/vdb5` (which contains a Btrfs file system).


![mount-new](https://github.com/openSUSE/agama/assets/3638289/9a8d83ea-b946-4fe6-b850-3058fdd353ca)

Hacking the web UI to set Target to `new_vg` and TargetDevice to
`/dev/vdc`.


![vg-new](https://github.com/openSUSE/agama/assets/3638289/ed90a87d-ac8e-45d0-85cf-4eff9f62f890)

doc/dbus/org.opensuse.Agama.Storage1.Proposal.Calculator.doc.xml

@@ -18,8 +18,8 @@
       <!--
         MountPath               s
         MountOptions            as
-        TargetDevice            s
-        TargetVG                s
+        Target                  s
+        TargetDevice            s (only makes sense if Target is not default)
         FsType                  s
         MinSize                 t (bytes)
         MaxSize                 t (bytes. Optional, max size is considered as unlimited if omitted)

doc/dbus/org.opensuse.Agama.Storage1.Proposal.doc.xml

@@ -16,8 +16,8 @@
       Each volume has the following properties:
         MountPath               s
         MountOptions            as
-        TargetDevice            s
-        TargetVG                s
+        Target                  s
+        TargetDevice            s (only makes sense if Target is not default)
         FsType                  s
         MinSize                 t (bytes)
         MaxSize                 t (bytes. Optional, max size is considered as unlimited if omitted)

service/lib/agama/dbus/storage/volume_conversion/from_dbus.rb

@@ -20,6 +20,7 @@
 # find current contact information at www.suse.com.
 
 require "agama/storage/volume"
+require "agama/storage/volume_location"
 require "agama/storage/volume_templates_builder"
 require "y2storage/disk_size"
 require "y2storage/filesystems/type"
@@ -67,8 +68,8 @@ def convert
           CONVERSIONS = {
             "MountPath"    => :mount_path_conversion,
             "MountOptions" => :mount_options_conversion,
+            "Target"       => :target_conversion,
             "TargetDevice" => :target_device_conversion,
-            "TargetVG"     => :target_vg_conversion,
             "FsType"       => :fs_type_conversion,
             "MinSize"      => :min_size_conversion,
             "MaxSize"      => :max_size_conversion,
@@ -92,13 +93,16 @@ def mount_options_conversion(target, value)
           # @param target [Agama::Storage::Volume]
           # @param value [String]
           def target_device_conversion(target, value)
-            target.device = value
+            target.location.device = value
           end
 
           # @param target [Agama::Storage::Volume]
           # @param value [String]
-          def target_vg_conversion(target, value)
-            target.separate_vg_name = value
+          def target_conversion(target, value)
+            target_value = value.downcase.to_sym
+            return unless Agama::Storage::VolumeLocation.targets.include?(target_value)
+
+            target.location.target = target_value
           end
 
           # @param target [Agama::Storage::Volume]

service/lib/agama/dbus/storage/volume_conversion/to_dbus.rb

@@ -37,8 +37,8 @@ def convert
             {
               "MountPath"     => volume.mount_path.to_s,
               "MountOptions"  => volume.mount_options,
-              "TargetDevice"  => volume.device.to_s,
-              "TargetVG"      => volume.separate_vg_name.to_s,
+              "Target"        => volume.location.target.to_s,
+              "TargetDevice"  => volume.location.device.to_s,
               "FsType"        => volume.fs_type&.to_human_string || "",
               "MinSize"       => volume.min_size&.to_i,
               "AutoSize"      => volume.auto_size?,

service/lib/agama/storage/proposal_settings_conversion/to_y2storage.rb

@@ -83,7 +83,8 @@ def lvm_conversion(target)
           lvm = settings.lvm.enabled?
 
           target.lvm = lvm
-          target.separate_vgs = lvm
+          # Activate support for dedicated volume groups
+          target.separate_vgs = true
           # Prevent VG reuse
           target.lvm_vg_reuse = false
         end
@@ -170,7 +171,7 @@ def find_max_size_fallback(mount_path)
         def all_devices
           devices = [settings.boot_device] +
             settings.lvm.system_vg_devices +
-            settings.volumes.map(&:device)
+            settings.volumes.map(&:location).reject(&:reuse_device?).map(&:device)
 
           devices.compact.uniq.map { |d| device_or_partitions(d) }.flatten
         end

service/lib/agama/storage/volume.rb

@@ -23,6 +23,7 @@
 require "y2storage/disk_size"
 require "agama/storage/btrfs_settings"
 require "agama/storage/volume_outline"
+require "agama/storage/volume_location"
 
 module Agama
   module Storage
@@ -58,15 +59,10 @@ class Volume
       # @return [Array<String>]
       attr_accessor :mount_options
 
-      # Used to locate the volume in a separate disk
+      # Location of the volume
       #
-      # @return [String, nil]
-      attr_accessor :device
-
-      # Used to locate the volume in a separate VG
-      #
-      # @return [String, nil]
-      attr_accessor :separate_vg_name
+      # @return [VolumeLocation]
+      attr_accessor :location
 
       # Min size for the volume
       #
@@ -98,6 +94,7 @@ def initialize(mount_path)
         @max_size = Y2Storage::DiskSize.unlimited
         @btrfs = BtrfsSettings.new
         @outline = VolumeOutline.new
+        @location = VolumeLocation.new
       end
 
       def_delegators :outline,

service/lib/agama/storage/volume_conversion/from_y2storage.rb

@@ -41,13 +41,12 @@ def convert
           volume = VolumeTemplatesBuilder.new_from_config(config).for(spec.mount_point || "")
 
           volume.tap do |target|
-            target.device = spec.device
-            target.separate_vg_name = spec.separate_vg_name
             target.mount_options = spec.mount_options
             target.fs_type = spec.fs_type
 
             sizes_conversion(target)
             btrfs_conversion(target)
+            location_conversion(target)
           end
         end
 
@@ -83,6 +82,17 @@ def btrfs_conversion(target)
           target.btrfs.read_only = spec.btrfs_read_only
         end
 
+        # @param target [Agama::Storage::Volume]
+        def location_conversion(target)
+          if spec.reuse?
+            target.location.target = spec.reformat? ? :device : :filesystem
+            target.location.device = spec.reuse_name
+          elsif !!spec.device
+            target.location.target = spec.separate_vg? ? :new_vg : :new_partition
+            target.location.device = spec.device
+          end
+        end
+
         # Planned device for the given mount path.
 
         # @param mount_path [String]

service/lib/agama/storage/volume_conversion/to_y2storage.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-# Copyright (c) [2023] SUSE LLC
+# Copyright (c) [2023-2024] SUSE LLC
 #
 # All Rights Reserved.
 #
@@ -37,8 +37,6 @@ def initialize(volume)
         # @return [Y2Storage::VolumeSpecification]
         def convert # rubocop:disable Metrics/AbcSize
           Y2Storage::VolumeSpecification.new({}).tap do |target|
-            target.device = volume.device
-            target.separate_vg_name = volume.separate_vg_name
             target.mount_point = volume.mount_path
             target.mount_options = volume.mount_options.join(",")
             target.proposed = true
@@ -50,6 +48,7 @@ def convert # rubocop:disable Metrics/AbcSize
 
             sizes_conversion(target)
             btrfs_conversion(target)
+            location_conversion(target)
           end
         end
 
@@ -88,6 +87,29 @@ def btrfs_conversion(target)
           target.btrfs_default_subvolume = volume.btrfs.default_subvolume
           target.btrfs_read_only = volume.btrfs.read_only?
         end
+
+        # @param target [Y2Storage::VolumeSpecification]
+        def location_conversion(target)
+          location = volume.location
+          return if location.default?
+
+          if location.reuse_device?
+            target.reuse_name = location.device
+            target.reformat = location.target == :device
+            return
+          end
+
+          target.device = location.device
+          target.separate_vg_name = vg_name(target) if location.target == :new_vg
+        end
+
+        # Name to be used as separate_vg_name for the given Y2Storage volume
+        #
+        # @param target [Y2Storage::VolumeSpecification]
+        def vg_name(target)
+          mount_point = target.root? ? "root" : target.mount_point.sub(%r{^/}, "")
+          "vg-#{mount_point.tr("/", "_")}"
+        end
       end
     end
   end

service/lib/agama/storage/volume_location.rb

@@ -0,0 +1,78 @@
+# frozen_string_literal: true
+
+# Copyright (c) [2024] 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.
+
+module Agama
+  module Storage
+    # Settings specifying what device should be used for a given Volume and how
+    class VolumeLocation
+      # @see .targets
+      TARGETS = [:default, :new_partition, :new_vg, :device, :filesystem].freeze
+      private_constant :TARGETS
+
+      # What to do to allocate the volume
+      #
+      # @return [Symbol] see {.targets}
+      attr_reader :target
+
+      # Concrete device to allocate the volume, the exact meaning depends on {#target}
+      #
+      # @return [String, nil]
+      attr_accessor :device
+
+      # All possible values for #target:
+      #
+      #   - :default new partition or logical volume in the default device
+      #   - :new_partition new partition at the disk pointed by {#device}
+      #   - :new_vg new LV in a new dedicated VG created at a the disk pointed  by {#device}
+      #   - :device the existing block device specified by {#device} is used and reformatted
+      #   - :filesystem: the existing filesystem on the device specified by {#device} is mounted
+      #
+      # @return [Array<Symbol>]
+      def self.targets
+        TARGETS
+      end
+
+      # Constructor
+      def initialize
+        @target = :default
+      end
+
+      # Sets the value of {#target} ensuring it is valid
+      def target=(value)
+        return unless TARGETS.include?(value)
+
+        @target = value
+      end
+
+      # @return [Boolean]
+      def default?
+        target == :default
+      end
+
+      # Whether the chosen target implies reusing an existing device (formatting it or not)
+      #
+      # @return [Boolean]
+      def reuse_device?
+        [:device, :filesystem].include?(target)
+      end
+    end
+  end
+end

service/package/rubygem-agama-yast.changes

@@ -1,3 +1,10 @@
+-------------------------------------------------------------------
+Thu Mar 21 10:35:09 UTC 2024 - Ancor Gonzalez Sosa <[email protected]>
+
+- Extend the storage D-Bus API: new attributes for the volumes
+  (Target and TargetDevice) to decide where to locate each of them
+  (gh#openSUSE/agama#1105).
+
 -------------------------------------------------------------------
 Tue Mar 19 14:09:54 UTC 2024 - José Iván López González <[email protected]>
 

service/test/agama/dbus/storage/proposal_settings_conversion/to_dbus_test.rb

@@ -80,7 +80,7 @@
             "MountPath"     => "/test",
             "MountOptions"  => [],
             "TargetDevice"  => "",
-            "TargetVG"      => "",
+            "Target"        => "default",
             "FsType"        => "",
             "MinSize"       => 0,
             "AutoSize"      => false,

service/test/agama/dbus/storage/volume_conversion/from_dbus_test.rb

@@ -67,7 +67,7 @@
         "MountPath"    => "/test",
         "MountOptions" => ["rw", "default"],
         "TargetDevice" => "/dev/sda",
-        "TargetVG"     => "/dev/system",
+        "Target"       => "new_vg",
         "FsType"       => "Ext4",
         "MinSize"      => 1024,
         "MaxSize"      => 2048,
@@ -89,8 +89,8 @@
       expect(volume).to be_a(Agama::Storage::Volume)
       expect(volume.mount_path).to eq("/test")
       expect(volume.mount_options).to contain_exactly("rw", "default")
-      expect(volume.device).to eq("/dev/sda")
-      expect(volume.separate_vg_name).to eq("/dev/system")
+      expect(volume.location.device).to eq("/dev/sda")
+      expect(volume.location.target).to eq(:new_vg)
       expect(volume.fs_type).to eq(Y2Storage::Filesystems::Type::EXT4)
       expect(volume.auto_size?).to eq(false)
       expect(volume.min_size.to_i).to eq(1024)
@@ -107,8 +107,7 @@
         expect(volume).to be_a(Agama::Storage::Volume)
         expect(volume.mount_path).to eq("/test")
         expect(volume.mount_options).to contain_exactly("data=ordered")
-        expect(volume.device).to be_nil
-        expect(volume.separate_vg_name).to be_nil
+        expect(volume.location.target).to eq :default
         expect(volume.fs_type).to eq(Y2Storage::Filesystems::Type::BTRFS)
         expect(volume.auto_size?).to eq(false)
         expect(volume.min_size.to_i).to eq(5 * (1024**3))
@@ -243,5 +242,20 @@
         expect(volume.btrfs.snapshots?).to eq(false)
       end
     end
+
+    context "when the D-Bus settings provide a Target that makes no sense" do
+      let(:dbus_volume) do
+        {
+          "MountPath" => "/test",
+          "Target"    => "new_disk"
+        }
+      end
+
+      it "ignores the Target value provided from D-Bus and uses :default" do
+        volume = subject.convert
+
+        expect(volume.location.target).to eq :default
+      end
+    end
   end
 end

service/test/agama/dbus/storage/volume_conversion/to_dbus_test.rb

@@ -47,8 +47,8 @@
       volume.btrfs.snapshots = true
       volume.btrfs.read_only = true
       volume.mount_options = ["rw", "default"]
-      volume.device = "/dev/sda"
-      volume.separate_vg_name = "/dev/system"
+      volume.location.device = "/dev/sda"
+      volume.location.target = :new_partition
       volume.min_size = Y2Storage::DiskSize.new(1024)
       volume.max_size = Y2Storage::DiskSize.new(2048)
       volume.auto_size = true
@@ -61,7 +61,7 @@
         "MountPath"     => "/test",
         "MountOptions"  => [],
         "TargetDevice"  => "",
-        "TargetVG"      => "",
+        "Target"        => "default",
         "FsType"        => "",
         "MinSize"       => 0,
         "AutoSize"      => false,
@@ -82,7 +82,7 @@
         "MountPath"     => "/test",
         "MountOptions"  => ["rw", "default"],
         "TargetDevice"  => "/dev/sda",
-        "TargetVG"      => "/dev/system",
+        "Target"        => "new_partition",
         "FsType"        => "Ext4",
         "MinSize"       => 1024,
         "MaxSize"       => 2048,