ceph-volume: use safer check for bluestore label

Using only the exit status of `ceph-bluestore-tool show-label` to
determine if a device is a bluestore OSD could report a false negative
if there is a system error when `ceph-bluestore-tool` opens the device.

A better check is to open the device and read the bluestore device
label (the first 22 bytes of the device) to look for the bluestore
device signature ("bluestore block device"). If ceph-volume fails to
open the device due to a system error, it is safest to assume the device
is BlueStore so that an existing OSD isn't overwritten.

Signed-off-by: Blaine Gardner <blaine.gardner@redhat.com>
(cherry picked from commit 651b28f2e3cb39dbe9c7038cd677a01523f08821)

diff --git a/src/ceph-volume/ceph_volume/tests/util/test_arg_validators.py b/src/ceph-volume/ceph_volume/tests/util/test_arg_validators.py
index d4565ef4dcb5c3f6e2edfaff331c5ada65348ceb..1e8a49e2624c246b0a2f9ff03008dc00283eee8a 100644 (file)
--- a/src/ceph-volume/ceph_volume/tests/util/test_arg_validators.py
+++ b/src/ceph-volume/ceph_volume/tests/util/test_arg_validators.py
@@ -80,10 +80,10 @@ 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')

diff --git a/src/ceph-volume/ceph_volume/tests/util/test_device.py b/src/ceph-volume/ceph_volume/tests/util/test_device.py
index 0df5ac61e9ea319f79f27bc3e6cf8ce69d0691cb..8c4d0153c35d13d97f38a23a261618aedc68f55c 100644 (file)
--- a/src/ceph-volume/ceph_volume/tests/util/test_device.py
+++ b/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):

diff --git a/src/ceph-volume/ceph_volume/util/device.py b/src/ceph-volume/ceph_volume/util/device.py
index deed06436d8ed1b85ba753563d99917807bdb24e..4503f6500218effc80e78b8d32a761e94a9499b9 100644 (file)
--- a/src/ceph-volume/ceph_volume/util/device.py
+++ b/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}"""
 
@@ -348,12 +353,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):
@@ -476,8 +486,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