Merge pull request #42403 from BlaineEXE/ceph-volume-use-safer-check-for-has-bluestore-label

ceph-volume: use safer check for bluestore label

Reviewed-by: Sébastien Han <seb@redhat.com>
Reviewed-by: Dimitri Savineau <dsavinea@redhat.com>

src/ceph-volume/ceph_volume/tests/util/test_arg_validators.py

@@ -80,11 +80,11 @@ class TestValidDevice(object):
     def setup(self):
         self.validator = arg_validators.ValidDevice()
 
-    def test_path_is_valid(self, fake_call):
+    def test_path_is_valid(self, fake_call, patch_bluestore_label):
         result = self.validator('/')
         assert result.abspath == '/'
 
-    def test_path_is_invalid(self, fake_call):
+    def test_path_is_invalid(self, fake_call, patch_bluestore_label):
         with pytest.raises(argparse.ArgumentError):
             self.validator('/device/does/not/exist')
 
@@ -117,4 +117,3 @@ class TestValidFraction(object):
     def test_fraction_is_greater_one(self, fake_call):
         with pytest.raises(argparse.ArgumentError):
             self.validator('1.1')
-

src/ceph-volume/ceph_volume/tests/util/test_device.py

@@ -2,6 +2,7 @@ import pytest
 from copy import deepcopy
 from ceph_volume.util import device
 from ceph_volume.api import lvm as api
+from mock.mock import patch, mock_open
 
 
 class TestDevice(object):
@@ -258,6 +259,14 @@ class TestDevice(object):
         assert not disk.available
         assert "Has BlueStore device label" in disk.rejected_reasons
 
+    def test_reject_device_with_oserror(self, monkeypatch, patch_bluestore_label, device_info):
+        patch_bluestore_label.side_effect = OSError('test failure')
+        lsblk = {"TYPE": "disk"}
+        device_info(lsblk=lsblk)
+        disk = device.Device("/dev/sda")
+        assert not disk.available
+        assert "Failed to determine if device is BlueStore" in disk.rejected_reasons
+
     @pytest.mark.usefixtures("device_info_not_ceph_disk_member",
                              "disable_kernel_queries")
     def test_is_not_ceph_disk_member_lsblk(self, patch_bluestore_label):
@@ -348,6 +357,16 @@ class TestDevice(object):
         disk = device.Device("/dev/sda")
         assert disk._get_device_id() == 'ID_VENDOR_ID_MODEL_ID_SCSI_SERIAL'
 
+    def test_has_bluestore_label(self):
+        # patch device.Device __init__ function to do nothing since we want to only test the
+        # low-level behavior of has_bluestore_label
+        with patch.object(device.Device, "__init__", lambda self, path, with_lsm=False: None):
+            disk = device.Device("/dev/sda")
+            disk.abspath = "/dev/sda"
+            with patch('builtins.open', mock_open(read_data=b'bluestore block device\n')):
+                assert disk.has_bluestore_label
+            with patch('builtins.open', mock_open(read_data=b'not a bluestore block device\n')):
+                assert not disk.has_bluestore_label
 
 
 class TestDeviceEncryption(object):

src/ceph-volume/ceph_volume/util/device.py

@@ -1,13 +1,18 @@
 # -*- coding: utf-8 -*-
 
+import logging
 import os
 from functools import total_ordering
-from ceph_volume import sys_info, process
+from ceph_volume import sys_info
 from ceph_volume.api import lvm
 from ceph_volume.util import disk, system
 from ceph_volume.util.lsmdisk import LSMDisk
 from ceph_volume.util.constants import ceph_disk_guids
 
+
+logger = logging.getLogger(__name__)
+
+
 report_template = """
 {dev:<25} {size:<12} {rot!s:<7} {available!s:<9} {model}"""
 
@@ -350,12 +355,17 @@ class Device(object):
 
     @property
     def has_bluestore_label(self):
-        out, err, ret = process.call([
-            'ceph-bluestore-tool', 'show-label',
-            '--dev', self.abspath], verbose_on_failure=False)
-        if ret:
-            return False
-        return True
+        isBluestore = False
+        bluestoreDiskSignature = 'bluestore block device' # 22 bytes long
+
+        # throws OSError on failure
+        with open(self.abspath, "rb") as fd:
+            # read first 22 bytes looking for bluestore disk signature
+            signature = fd.read(22)
+            if signature.decode('ascii', 'replace') == bluestoreDiskSignature:
+                isBluestore = True
+
+        return isBluestore
 
     @property
     def is_mapper(self):
@@ -478,8 +488,16 @@ class Device(object):
             rejected.append("Device type is not acceptable. It should be raw device or partition")
         if self.is_ceph_disk_member:
             rejected.append("Used by ceph-disk")
-        if self.has_bluestore_label:
-            rejected.append('Has BlueStore device label')
+
+        try:
+            if self.has_bluestore_label:
+                rejected.append('Has BlueStore device label')
+        except OSError as e:
+            # likely failed to open the device. assuming it is BlueStore is the safest option
+            # so that a possibly-already-existing OSD doesn't get overwritten
+            logger.error('failed to determine if device {} is Bluestore. device should not be used to avoid false negatives. err: {}'.format(self.abspath, e))
+            rejected.append('Failed to determine if device is BlueStore')
+
         if self.has_gpt_headers:
             rejected.append('Has GPT headers')
         return rejected