Merge pull request #1209 from ancorgs/systemd_remote_master

Use the driver to decide whether _netdev is needed

package/yast2-storage-ng.changes

@@ -1,3 +1,10 @@
+-------------------------------------------------------------------
+Wed Feb 25 16:26:55 UTC 2021 - Ancor Gonzalez Sosa <ancor@suse.com>
+
+- Improved mechanism to detect whether _netdev is needed for a
+  given disk: use its driver as extra criterion (bsc#1176140).
+- 4.3.46
+
 -------------------------------------------------------------------
 Thu Feb 25 09:20:13 UTC 2021 - Ancor Gonzalez Sosa <ancor@suse.com>
 

package/yast2-storage-ng.spec

@@ -16,7 +16,7 @@
 #
 
 Name:           yast2-storage-ng
-Version:        4.3.45
+Version:        4.3.46
 Release:        0
 Summary:        YaST2 - Storage Configuration
 License:        GPL-2.0-only OR GPL-3.0-only

src/lib/y2storage/blk_device.rb

@@ -382,6 +382,30 @@ def mount_point
       filesystem.mount_point
     end
 
+    # Whether this block device is at the top of the hierarchy of block devices
+    # in the devicegraph
+    #
+    # This should only return true for disks, DASDs and stray block devices
+    #
+    # @return [Boolean] false if this is a descendant of any other block device
+    def root_blk_device?
+      ancestors.none? { |i| i.is?(:blk_device) }
+    end
+
+    # Block devices that are at the top of the list of ancestors for the current one
+    #
+    # For block devices that are already at the top of their own hierarchy, this
+    # returns an array with the device as only element.
+    #
+    # @return [Array<BlkDevice>] a list of disks, DASD and stray block devices
+    def root_blk_devices
+      if root_blk_device?
+        [self]
+      else
+        ancestors.select { |i| i.is?(:blk_device) && i.root_blk_device? }
+      end
+    end
+
     # LVM physical volume defined on top of the device, either directly or
     # through an encryption layer.
     #
@@ -617,9 +641,7 @@ def hwinfo
     #
     # @return [String, nil] nil if vendor is unknown
     def vendor
-      return nil if hwinfo.nil?
-
-      hwinfo.vendor
+      hwinfo&.vendor
     end
 
     # Device model
@@ -628,9 +650,7 @@ def vendor
     #
     # @return [String, nil] nil if model is unknown
     def model
-      return nil if hwinfo.nil?
-
-      hwinfo.model
+      hwinfo&.model
     end
 
     # Device bus (IDE, SATA, etc)
@@ -639,9 +659,18 @@ def model
     #
     # @return [String, nil] nil if bus is unknown
     def bus
-      return nil if hwinfo.nil?
+      hwinfo&.bus
+    end
+
+    # Kernel drivers used by this device
+    #
+    # @see #hwinfo
+    #
+    # @return [Array<String>] empty if the driver is unknown
+    def driver
+      return [] if hwinfo.nil?
 
-      hwinfo.bus
+      hwinfo.driver || []
     end
 
     # Size of the space that could be theoretically reclaimed by shrinking the
@@ -674,7 +703,28 @@ def journal?
     #
     # @return [Boolean] true if this is a network-based disk or depends on one
     def in_network?
-      false
+      if root_blk_device?
+        false
+      else
+        root_blk_devices.any?(&:in_network?)
+      end
+    end
+
+    # Whether the block device must be considered remote regarding how and when
+    # things are mounted during the systemd boot process
+    #
+    # Remote mount units have dependencies on some systemd targets like
+    # remote-fs-pre, remote-fs, network and network-online.
+    #
+    # See bsc#1176140
+    #
+    # @return [Boolean]
+    def systemd_remote?
+      if root_blk_device?
+        in_network?
+      else
+        root_blk_devices.any?(&:systemd_remote?)
+      end
     end
 
     # Whether the block device fulfills conditions to be used for a Windows system

src/lib/y2storage/disk.rb

@@ -76,6 +76,25 @@ def in_network?
       transport.network?
     end
 
+    # @see #systemd_remote?
+    SYSTEMD_REMOTE_DRIVERS = ["iscsi-tcp", "bnx2i", "qedi", "fcoe", "bnx2fc", "qedf"].freeze
+    private_constant :SYSTEMD_REMOTE_DRIVERS
+
+    # @see BlkDevice#systemd_remote?
+    def systemd_remote?
+      # For local devices we don't need to check the driver from the hwinfo data
+      return false unless in_network?
+
+      # Some iSCSI and FCoE disk are accessible to systemd without any need to wait for systemd
+      # to initialize the network. For example, those using drivers like qla4xxx, be2iscsi, cxgbi,
+      # fnic and csiostor keep all the configuration within the HBA NVRAM and will start
+      # presenting devices once the driver is loaded. Thus, this method only returns true if the
+      # disk uses a driver that is known to require a daemon to be started in order to make the
+      # device available. See bsc#1176140.
+      log.info "systemd_remote? for #{name}: checking driver - #{driver}"
+      (SYSTEMD_REMOTE_DRIVERS & driver).any?
+    end
+
     # Default partition table type for newly created partition tables
     # @see Partitionable#default_ptable_type
     #

src/lib/y2storage/encryption.rb

@@ -329,11 +329,6 @@ def ensure_suitable_mount_by
       self.mount_by = Filesystems::MountByType.best_for(blk_device, suitable_mount_bys)
     end
 
-    # @see BlkDevice#in_network?
-    def in_network?
-      blk_device.in_network?
-    end
-
     # Options that must be propagated from the fstab entries of the mount points
     # to the crypttab entries of the corresponding device
     OPTIONS_TO_PROPAGATE = {

src/lib/y2storage/filesystems/blk_filesystem.rb

@@ -212,6 +212,13 @@ def in_network?
         blk_devices.any?(&:in_network?)
       end
 
+      # @see BlkDevice#systemd_remote?
+      #
+      # @return [Boolean]
+      def systemd_remote?
+        blk_devices.any?(&:systemd_remote?)
+      end
+
       # Option used in the fstab file for devices that require network
       NETWORK_OPTION = "_netdev".freeze
       private_constant :NETWORK_OPTION
@@ -369,7 +376,7 @@ def path_for_mount_by(mount_by)
       def needs_network_mount_options?
         # Adding "_netdev" and similar options in fstab for / should not be necessary
         # and it confuses (or used to confuse) systemd. See bsc#1165937.
-        in_network? && !root?
+        systemd_remote? && !root?
       end
 
       # @see Device#is?

src/lib/y2storage/lvm_lv.rb

@@ -198,13 +198,6 @@ def resize(new_size)
       log.info "Size of #{name} set to #{size}"
     end
 
-    # @see BlkDevice#in_network?
-    #
-    # @return [Boolean]
-    def in_network?
-      lvm_vg.lvm_pvs.map(&:blk_device).any?(&:in_network?)
-    end
-
     protected
 
     def types_for_is

src/lib/y2storage/multi_disk_device.rb

@@ -28,13 +28,6 @@ module Y2Storage
   # Mixin for devices that, from the libstorage point of view, are basically
   # an aggregation of several disks. I.e. Multipath I/O or BIOS RAID.
   module MultiDiskDevice
-    # Checks whether this is a network device.
-    #
-    # @return [Boolean] true if any of disks is network-based
-    def in_network?
-      parents.any? { |i| i.respond_to?(:in_network?) && i.in_network? }
-    end
-
     # Checks whether some of the disks of the device are connected through USB.
     #
     # Although that is obviously very unlikely, this method is offered for
@@ -43,7 +36,7 @@ def in_network?
     #
     # @return [Boolean]
     def usb?
-      parents.any? { |i| i.respond_to?(:usb?) && i.usb? }
+      any_parent?(:usb?)
     end
 
     # Default partition table type for newly created partition tables
@@ -56,5 +49,13 @@ def default_ptable_type
       # We always suggest GPT
       PartitionTables::Type::GPT
     end
+
+    # Checks whether any of the parent devices returns true for the given method
+    #
+    # @param method [Symbol] name of the method to be checked in all parents
+    # @return [Boolean]
+    def any_parent?(method)
+      parents.any? { |i| i.respond_to?(method) && i.send(method) }
+    end
   end
 end

src/lib/y2storage/partition.rb

@@ -274,11 +274,6 @@ def efi_system?
       id.is?(:esp) && formatted_as?(:vfat)
     end
 
-    # @see BlkDevice#in_network?
-    def in_network?
-      partitionable.in_network?
-    end
-
     # Whether the partition fulfills conditions to be used for a Windows system
     #
     # @return [Boolean]

test/y2storage/encryption_test.rb

@@ -185,6 +185,7 @@
     let(:scenario) { "encrypted_partition.xml" }
     let(:disk) { devicegraph.find_by_name("/dev/sda") }
     let(:partition) { devicegraph.find_by_name(partition_name) }
+    let(:hwinfo) { OpenStruct.new }
 
     def create_btrfs(blk_dev)
       fs = blk_dev.create_filesystem(Y2Storage::Filesystems::Type::BTRFS)
@@ -215,7 +216,10 @@ def prepare_create_fs__encrypt__mount
       fs.mount_point.mount_options |= mount_options
     end
 
-    before { allow(disk.transport).to receive(:network?).and_return network }
+    before do
+      allow(disk.transport).to receive(:network?).and_return network
+      allow_any_instance_of(Y2Storage::Disk).to receive(:hwinfo).and_return(hwinfo)
+    end
 
     RSpec.shared_examples "netdev for network device" do
       context "within a network disk" do
@@ -225,8 +229,21 @@ def prepare_create_fs__encrypt__mount
 
         before { prepare_fs.call }
 
-        it "makes sure #crypt_options include _netdev" do
-          expect(encryption.crypt_options).to include("_netdev")
+        context "which uses a driver that needs an extra systemd service" do
+          let(:hwinfo) { OpenStruct.new(driver: ["sg", "bnx2i"]) }
+
+          it "makes sure #crypt_options include _netdev" do
+            expect(encryption.crypt_options).to include("_netdev")
+          end
+        end
+
+        context "which does not demand any extra systemd service in order to be used" do
+          # Test with fnic, based on what happened at bsc#1176140
+          let(:hwinfo) { OpenStruct.new(driver: ["sg", "fnic"]) }
+
+          it "makes no changes to #crypt_options" do
+            expect(encryption.crypt_options).to be_empty
+          end
         end
       end
 

test/y2storage/filesystems/blk_filesystem_test.rb

@@ -377,6 +377,32 @@
         expect(filesystem.in_network?).to eq true
       end
     end
+
+    context "when the filesystem is in a logical volume of an encrypted LVM" do
+      let(:scenario) { "complex-lvm-encrypt" }
+      let(:dev_name) { "/dev/vg0/lv1" }
+      let(:disk) { Y2Storage::BlkDevice.find_by_name(fake_devicegraph, "/dev/sdd") }
+
+      before do
+        allow(disk.transport).to receive(:network?).and_return network_transport
+      end
+
+      context "and the underlying disk is in the network" do
+        let(:network_transport) { true }
+
+        it "returns true" do
+          expect(filesystem.in_network?).to eq true
+        end
+      end
+
+      context "and the underlying disk is local" do
+        let(:network_transport) { false }
+
+        it "returns false" do
+          expect(filesystem.in_network?).to eq false
+        end
+      end
+    end
   end
 
   describe "#match_fstab_spec?" do

test/y2storage/mount_point_test.rb

@@ -545,7 +545,7 @@
       end
     end
 
-    context "in a local disk" do
+    context "in a network disk" do
       let(:blk_device) { Y2Storage::BlkDevice.find_by_name(fake_devicegraph, dev_name) }
       let(:mountable) { blk_device.filesystem }
 
@@ -593,14 +593,28 @@
       context "for a non-root filesystem" do
         before do
           mount_point.path = "/home"
+          allow_any_instance_of(Y2Storage::Disk).to receive(:hwinfo).and_return(hwinfo)
         end
 
+        let(:hwinfo) { OpenStruct.new }
+
         context "for a Btrfs file-system" do
           let(:dev_name) { "/dev/sda2" }
 
-          it "sets #mount_options to an array containing only '_netdev'" do
-            mount_point.set_default_mount_options
-            expect(mount_point.mount_options).to eq ["_netdev"]
+          context "if the disk uses a driver that depends on a systemd service" do
+            let(:hwinfo) { OpenStruct.new(driver: ["fcoe"]) }
+
+            it "sets #mount_options to an array containing only '_netdev'" do
+              mount_point.set_default_mount_options
+              expect(mount_point.mount_options).to eq ["_netdev"]
+            end
+          end
+
+          context "if the disk driver does not depend on any systemd service" do
+            it "sets #mount_options to an empty array" do
+              mount_point.set_default_mount_options
+              expect(mount_point.mount_options).to eq []
+            end
           end
         end
 
@@ -619,9 +633,22 @@
         context "for an Ext3 file-system " do
           let(:dev_name) { "/dev/sda3" }
 
-          it "sets #mount_options to an array containing the 'data' and '_netdev' options" do
-            mount_point.set_default_mount_options
-            expect(mount_point.mount_options).to contain_exactly("data=ordered", "_netdev")
+          context "if the disk uses a driver that depends on a systemd service" do
+            let(:hwinfo) { OpenStruct.new(driver: ["iscsi-tcp"]) }
+
+            it "sets #mount_options to an array containing the 'data' and '_netdev' options" do
+              mount_point.set_default_mount_options
+              expect(mount_point.mount_options).to contain_exactly("data=ordered", "_netdev")
+            end
+          end
+
+          context "if the disk driver does not depend on any systemd service" do
+            let(:hwinfo) { OpenStruct.new(driver: ["qla4xxx"]) }
+
+            it "sets #mount_options to an array containing only the data option" do
+              mount_point.set_default_mount_options
+              expect(mount_point.mount_options).to eq ["data=ordered"]
+            end
           end
         end
       end