Merge pull request #763 from joseivanlopez/lvm-multidisk

Configure system volume group devices

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

@@ -29,7 +29,7 @@
   <interface name="org.opensuse.Agama.Storage1.Proposal">
     <property type="s" name="BootDevice" access="read"/>
     <property type="b" name="LVM" access="read"/>
-    <property type="aas" name="SystemVGDevices" access="read"/>
+    <property type="as" name="SystemVGDevices" access="read"/>
     <property type="s" name="EncryptionPassword" access="read"/>
     <property type="s" name="EncryptionMethod" access="read"/>
     <property type="s" name="EncryptionPBKDFunction" access="read"/>

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

@@ -6,7 +6,7 @@
   <interface name="org.opensuse.Agama.Storage1.Proposal">
     <property type="s" name="BootDevice" access="read"/>
     <property type="b" name="LVM" access="read"/>
-    <property type="aas" name="SystemVGDevices" access="read"/>
+    <property type="as" name="SystemVGDevices" access="read"/>
     <property type="s" name="EncryptionPassword" access="read"/>
     <property type="s" name="EncryptionMethod" access="read"/>
     <property type="s" name="EncryptionPBKDFunction" access="read"/>

service/lib/agama/dbus/storage/proposal.rb

@@ -48,7 +48,7 @@ def initialize(backend, logger)
 
           dbus_reader :lvm, "b", dbus_name: "LVM"
 
-          dbus_reader :system_vg_devices, "aas", dbus_name: "SystemVGDevices"
+          dbus_reader :system_vg_devices, "as", dbus_name: "SystemVGDevices"
 
           dbus_reader :encryption_password, "s"
 

service/lib/agama/storage/proposal.rb

@@ -85,10 +85,13 @@ def settings
           proposal.settings,
           config: config
         ).tap do |settings|
-          # FIXME: Currently, the conversion from Y2Storage cannot infer the space policy. Copying
-          #   space settings from the original settings.
+          # FIXME: The conversion from Y2Storage cannot infer the space policy. Copying space
+          #   settings from the original settings.
           settings.space.policy = original_settings.space.policy
           settings.space.actions = original_settings.space.actions
+          # FIXME: The conversion from Y2Storage cannot reliably infer the system VG devices in all
+          #   cases. Copying system VG devices from the original settings.
+          settings.lvm.system_vg_devices = original_settings.lvm.system_vg_devices
         end
       end
 

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

@@ -64,8 +64,11 @@ def boot_device_conversion(target)
         def lvm_conversion(target)
           target.lvm.enabled = settings.lvm
 
-          # Only assign system VG devices if candidate devices contains any device different to the
-          # root device.
+          # FIXME: The candidate devices list represents the system VG devices if it contains any
+          #   device different to the root device. If the candidate devices only contains the root
+          #   device, then there is no way to know whether the root device was explicitly assigned
+          #   as system VG device. Note that candidate devices will also contain the root device
+          #   when the system VG devices list was empty.
           candidate_devices = settings.candidate_devices || []
           return unless candidate_devices.reject { |d| d == settings.root_device }.any?
 

service/package/rubygem-agama.changes

@@ -1,3 +1,9 @@
+-------------------------------------------------------------------
+Wed Sep 27 12:12:59 UTC 2023 - José Iván López González <[email protected]>
+
+- Fix D-Bus type for SystemVGDevices and restore system VG devices
+  from previous settings (gh#openSUSE/agama#763).
+
 -------------------------------------------------------------------
 Tue Sep 26 15:57:08 UTC 2023 - Imobach Gonzalez Sosa <[email protected]>
 

service/test/agama/storage/proposal_test.rb

@@ -164,6 +164,55 @@
           )
         )
       end
+
+      # Checking system VG devices explicitly here because the settings converter cannot infer the
+      # system VG devices from the Y2Storage settings in all cases. The system VG devices are
+      # directly recovered from the original settings passed to #calculate.
+      context "system VG devices" do
+        let(:settings) do
+          achievable_settings.tap do |settings|
+            settings.lvm.system_vg_devices = system_vg_devices
+          end
+        end
+
+        context "if no devices were assigned as system VG devices" do
+          let(:system_vg_devices) { [] }
+
+          it "returns settings containing no system VG devices " do
+            expect(subject.settings).to have_attributes(
+              lvm: an_object_having_attributes(
+                system_vg_devices: be_empty
+              )
+            )
+          end
+        end
+
+        context "if only boot device was assigned as system VG device" do
+          let(:system_vg_devices) { ["/dev/sdb"] }
+
+          # This case cannot be inferred by conversion from Y2Storage, so the test does not pass if
+          # system VG devices are not copied from the settings passed to #calculate.
+          it "returns settings containing only boot device as system VG device" do
+            expect(subject.settings).to have_attributes(
+              lvm: an_object_having_attributes(
+                system_vg_devices: contain_exactly("/dev/sdb")
+              )
+            )
+          end
+        end
+
+        context "if several devices were assigned as system VG devices" do
+          let(:system_vg_devices) { ["/dev/sdb", "/dev/sdc"] }
+
+          it "returns settings containing the system VG devices" do
+            expect(subject.settings).to have_attributes(
+              lvm: an_object_having_attributes(
+                system_vg_devices: contain_exactly("/dev/sdb", "/dev/sdc")
+              )
+            )
+          end
+        end
+      end
     end
   end
 

web/package/cockpit-agama.changes

@@ -1,3 +1,9 @@
+-------------------------------------------------------------------
+Wed Sep 27 12:15:13 UTC 2023 - José Iván López González <[email protected]>
+
+- Allow to select the devices for the system volume group
+  (gh#openSUSE/agama#763).
+
 -------------------------------------------------------------------
 Tue Sep 26 15:57:21 UTC 2023 - Imobach Gonzalez Sosa <[email protected]>
 

web/src/assets/styles/blocks.scss

@@ -259,9 +259,8 @@ span.notification-mark[data-variant="sidebar"] {
   }
 }
 
-.device-selector {
+.device-list {
   [role="option"] {
-    cursor: pointer;
     padding: var(--spacer-normal);
     border: 2px solid var(--color-gray-dark);
     background: var(--color-gray-light);

web/src/assets/styles/patternfly-overrides.scss

@@ -170,3 +170,8 @@ table td > .pf-c-empty-state {
 .password-toggler span.pf-c-button__icon {
   display: flex;
 }
+
+.pf-c-toggle-group__button.pf-m-selected {
+  --pf-c-toggle-group__button--m-selected--BackgroundColor: var(--color-primary);
+  --pf-c-toggle-group__button--Color: var(--color-gray-light);
+}

web/src/assets/styles/utilities.scss

@@ -151,3 +151,7 @@
   inline-size: 95dvw;
   max-inline-size: calc(var(--ui-max-inline-size) + var(--spacer-large))
 }
+
+.cursor-pointer {
+  cursor: pointer;
+}

web/src/client/storage.js

@@ -226,8 +226,9 @@ class ProposalManager {
   /**
    * @typedef {object} ProposalSettings
    * @property {string} bootDevice
-   * @property {boolean} lvm
    * @property {string} encryptionPassword
+   * @property {boolean} lvm
+   * @property {string[]} systemVGDevices
    * @property {Volume[]} volumes
    *
    * @typedef {object} Volume
@@ -322,6 +323,7 @@ class ProposalManager {
         settings: {
           bootDevice: proxy.BootDevice,
           lvm: proxy.LVM,
+          systemVGDevices: proxy.SystemVGDevices,
           encryptionPassword: proxy.EncryptionPassword,
           volumes: proxy.Volumes.map(this.buildVolume),
         },
@@ -338,7 +340,7 @@ class ProposalManager {
    * @param {ProposalSettings} settings
    * @returns {Promise<number>} 0 on success, 1 on failure
    */
-  async calculate({ bootDevice, encryptionPassword, lvm, volumes }) {
+  async calculate({ bootDevice, encryptionPassword, lvm, systemVGDevices, volumes }) {
     const dbusVolume = (volume) => {
       return removeUndefinedCockpitProperties({
         MountPath: { t: "s", v: volume.mountPath },
@@ -354,6 +356,7 @@ class ProposalManager {
       BootDevice: { t: "s", v: bootDevice },
       EncryptionPassword: { t: "s", v: encryptionPassword },
       LVM: { t: "b", v: lvm },
+      SystemVGDevices: { t: "as", v: systemVGDevices },
       Volumes: { t: "aa{sv}", v: volumes?.map(dbusVolume) }
     });
 

web/src/client/storage.test.js

@@ -153,6 +153,7 @@ const contexts = {
     cockpitProxies.proposal = {
       BootDevice: "/dev/sda",
       LVM: true,
+      SystemVGDevices: ["/dev/sda", "/dev/sdb"],
       EncryptionPassword: "00000",
       Volumes: [
         {
@@ -694,7 +695,7 @@ describe("#proposal", () => {
       client = new StorageClient();
     });
 
-    it.only("returns the list of product mount points", async () => {
+    it("returns the list of product mount points", async () => {
       const mount_points = await client.proposal.getProductMountPoints();
       expect(mount_points).toEqual(["/", "swap", "/home"]);
     });
@@ -814,6 +815,7 @@ describe("#proposal", () => {
         expect(settings).toStrictEqual({
           bootDevice: "/dev/sda",
           lvm: true,
+          systemVGDevices: ["/dev/sda", "/dev/sdb"],
           encryptionPassword: "00000",
           volumes: [
             {
@@ -877,6 +879,7 @@ describe("#proposal", () => {
         bootDevice: "/dev/vdb",
         encryptionPassword: "12345",
         lvm: true,
+        systemVGDevices: ["/dev/sdc"],
         volumes: [
           {
             mountPath: "/test1",
@@ -897,6 +900,7 @@ describe("#proposal", () => {
         BootDevice: { t: "s", v: "/dev/vdb" },
         EncryptionPassword: { t: "s", v: "12345" },
         LVM: { t: "b", v: true },
+        SystemVGDevices: { t: "as", v: ["/dev/sdc"] },
         Volumes: {
           t: "aa{sv}",
           v: [