From 651b28f2e3cb39dbe9c7038cd677a01523f08821 Mon Sep 17 00:00:00 2001 From: Blaine Gardner Date: Mon, 19 Jul 2021 11:57:09 -0600 Subject: [PATCH] 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 --- .../tests/util/test_arg_validators.py | 5 ++- .../ceph_volume/tests/util/test_device.py | 19 ++++++++++ src/ceph-volume/ceph_volume/util/device.py | 36 ++++++++++++++----- 3 files changed, 48 insertions(+), 12 deletions(-) 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 c4181d7678f..b76f8f6e417 100644 --- 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,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') - 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 0df5ac61e9e..8c4d0153c35 100644 --- 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 3dcb434184d..a7716af31bc 100644 --- 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}""" @@ -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