Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove creation of unused partitions, implement recommendation #1188

Closed
wants to merge 1 commit into from
+36 −39
Closed

Remove creation of unused partitions, implement recommendation #1188

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 20 additions & 17 deletions srv/salt/_modules/osd.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -898,14 +898,16 @@ def _bluestore_partitions(self):
log.warning(("WAL size is unsupported for same device of "
"{}".format(self.osd.device)))
else:
# Create wal of wal_size on wal device
self.create(self.osd.wal, [('wal', self.osd.wal_size)])
log.info(("WAL will reside on same device {} as db - "
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps I missed something but I don't see the difference in handling following two cases:

  1. wal == db
  2. wal != db
    IMO both are getting here but there is no wal creation for case 2)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, the wal would not be created in either case, but the reason is different for the admin. The first message is that ceph-disk does not support specifying a wal on the same device as the OSD. The second is that it's unnecessary.

"recommend removing the WAL entry from the "
"configuration for device "
"{}").format(self.osd.db, self.osd.device))
else:
# pylint: disable=line-too-long
log.warning("No size specified for wal {}. Using default sizes.".format(self.osd.wal))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated to this PR but just in case - are this line and similar handling for absent db_size relevant? It looks like respective devices wouldn't be created at all rather than "using default sizes".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a size is not specified, we cannot create a partition. However, ceph-disk will accept a device without the partition and create partitions itself. So, the "default sizes" are for what ceph-disk will use.


if self.osd.db_size:
if self.osd.wal == self.osd.device:
if self.osd.db == self.osd.device:
# pylint: disable=line-too-long
log.warning("DB size is unsupported for same device of {}".format(self.osd.device))
else:
Expand All @@ -914,25 +916,22 @@ def _bluestore_partitions(self):
else:
log.warning("No size specified for db {}. Using default sizes".format(self.osd.db))
else:
# This situation seems unintentional - use faster media for
# the wal or db but not the other. Help newbies out by
# putting wal and db on same device
# Only WAL
if self.osd.wal:
if self.osd.wal_size:
if self.osd.wal == self.osd.device:
log.warning(("WAL size is unsupported for same device of "
"{}".format(self.osd.device)))
else:
log.warning("Setting db to same device {} as wal".format(self.osd.wal))
log.info("DB will reside on device {}".format(self.osd.device))
# Create wal of wal_size on wal device
# Create db on wal device
self.create(self.osd.wal, [('wal', self.osd.wal_size),
('db', self._halve(self.osd.wal_size))])
self.create(self.osd.wal, [('wal', self.osd.wal_size)])
else:
if self.osd.wal_size:
log.warning(("WAL size is unsupported for same device of "
"{}".format(self.osd.device)))

# Only DB
if self.osd.db:
if self.osd.db_size:
if self.osd.db == self.osd.device:
Expand All @@ -941,9 +940,7 @@ def _bluestore_partitions(self):
else:
log.warning("Setting wal to same device {} as db".format(self.osd.db))
# Create db of db_size on db device
# Create wal on db device
self.create(self.osd.db, [('wal', self._double(self.osd.db_size)),
('db', self.osd.db_size)])
self.create(self.osd.db, [('db', self.osd.db_size)])
else:
if self.osd.db_size:
log.warning(("DB size is unsupported for same device of "
Expand Down Expand Up @@ -1173,11 +1170,17 @@ def _bluestore_args(self):

# WAL cornercase with sizes
if self.osd.wal and self.osd.wal_size and self.osd.wal != self.osd.device:
_partition = self.highest_partition(self.osd.wal, 'wal')
if _partition:
args += "--block.wal {}{} ".format(self.osd.wal, _partition)
if self.osd.db:
log.warning(("Ignoring WAL setting - "
"No need for two partitions, "
"WAL will use same device as DB "
"{}").format(self.osd.db))
else:
args += "--block.wal {} ".format(self.osd.wal)
_partition = self.highest_partition(self.osd.wal, 'wal')
if _partition:
args += "--block.wal {}{} ".format(self.osd.wal, _partition)
else:
args += "--block.wal {} ".format(self.osd.wal)

# DB cornercase with sizes
if self.osd.db and self.osd.db_size and self.osd.db != self.osd.device:
Expand Down
38 changes: 16 additions & 22 deletions tests/unit/_modules/test_osd.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -814,8 +814,7 @@ def test_bluestore_partitions_wal_and_db_all_size_no_eq(self, create_mock):
And I have a wal_size
And I have a db_size
And wal is not equivalent to the device
Expect to call create('/dev/sddb', [('db', db_size)])
Expect to call create('/dev/sdwal', [('wal', wal_size)])
Expect to call create('/dev/sddb', [('db', db_size)]) only
"""
kwargs = {'format': 'bluestore',
'wal': '/dev/sdwal',
Expand All @@ -826,8 +825,8 @@ def test_bluestore_partitions_wal_and_db_all_size_no_eq(self, create_mock):
test_module = helper_specs(module=DEFAULT_MODULE)()
obj = test_module.OSDPartitions(osd_config)
obj._bluestore_partitions()
create_mock.assert_any_call('/dev/sdwal', [('wal', 'walsize')])
create_mock.assert_any_call('/dev/sddb', [('db', 'dbsize')])
assert create_mock.call_count == 1

@mock.patch('srv.salt._modules.osd.OSDPartitions.create')
@mock.patch('srv.salt._modules.osd.log')
Expand Down Expand Up @@ -860,7 +859,8 @@ def test_bluestore_partitions_wal_and_db_wal_size_no_eq(self, mock_log, create_m
And I have a wal_size
And I do not have a db_size
And wal is not equivalent to the device
Expect to call create('/dev/sdx', [('wal', wal_size)])

Expect WAL to reside on DB device
"""
kwargs = {'format': 'bluestore',
'wal': '/dev/sdwal',
Expand All @@ -871,7 +871,7 @@ def test_bluestore_partitions_wal_and_db_wal_size_no_eq(self, mock_log, create_m
test_module = helper_specs(module=DEFAULT_MODULE)()
obj = test_module.OSDPartitions(osd_config)
obj._bluestore_partitions()
create_mock.assert_any_call('/dev/sdwal', [('wal', 'walsize')])
create_mock.call_count == 0

@mock.patch('srv.salt._modules.osd.log')
def test_bluestore_partitions_wal_and_db_no_sizes_no_eq(self, mock_log):
Expand Down Expand Up @@ -913,16 +913,12 @@ def test_bluestore_partitions_no_waldb_only_wal_and_size(self, mock_log):
obj._bluestore_partitions()
mock_log.warning.assert_any_call('WAL size is unsupported for same device of /dev/sdx')

@mock.patch('srv.salt._modules.osd.OSDPartitions._halve')
@mock.patch('srv.salt._modules.osd.OSDPartitions.create')
@mock.patch('srv.salt._modules.osd.log')
def test_bluestore_partitions_no_waldb_only_wal_and_size_no_eq(self, mock_log, create_mock, halve_mock):
def test_bluestore_partitions_no_waldb_only_wal_and_size_no_eq(self, create_mock):
"""
Given I defined only wal
And I have a wal_size
And wal is not equivalent to the device
Expect to call log()
Expect to call _halve()
Expect to call create()
"""
kwargs = {'format': 'bluestore',
Expand All @@ -934,9 +930,7 @@ def test_bluestore_partitions_no_waldb_only_wal_and_size_no_eq(self, mock_log, c
test_module = helper_specs(module=DEFAULT_MODULE)()
obj = test_module.OSDPartitions(osd_config)
obj._bluestore_partitions()
mock_log.warning.assert_called_with('Setting db to same device /dev/sdwal as wal')
create_mock.assert_called_with('/dev/sdwal', [('wal', 100000), ('db', halve_mock('100000'))])
halve_mock.assert_called_with('100000')
create_mock.assert_called_with('/dev/sdwal', [('wal', 100000)])

@mock.patch('srv.salt._modules.osd.log')
def test_bluestore_partitions_no_waldb_only_wal_and_no_size(self, mock_log):
Expand All @@ -956,8 +950,9 @@ def test_bluestore_partitions_no_waldb_only_wal_and_no_size(self, mock_log):
obj._bluestore_partitions()
mock_log.warning.assert_called_with('WAL size is unsupported for same device of /dev/sdx')

@mock.patch('srv.salt._modules.osd.OSDPartitions.create')
@mock.patch('srv.salt._modules.osd.log')
def test_bluestore_partitions_no_waldb_only_db_and_size_eq_log(self, mock_log):
def test_bluestore_partitions_no_waldb_only_db_and_size_eq_log(self, mock_log, create_mock):
"""
Given I haven't defined wal but a db
And I have a db_size
Expand All @@ -968,17 +963,16 @@ def test_bluestore_partitions_no_waldb_only_db_and_size_eq_log(self, mock_log):
'wal': '/dev/sdx', # temp fix until #340 is merged
'db': '/dev/sddb',
'wal_size': None,
'db_size': 'dbsize'}
'db_size': 100000}
osd_config = OSDConfig(**kwargs)
test_module = helper_specs(module=DEFAULT_MODULE)()
obj = test_module.OSDPartitions(osd_config)
obj._bluestore_partitions()
mock_log.warning.assert_called_with('DB size is unsupported for same device of /dev/sdx')
create_mock.assert_called_with('/dev/sddb', [('db', 100000)])

@mock.patch('srv.salt._modules.osd.OSDPartitions._double')
@mock.patch('srv.salt._modules.osd.OSDPartitions.create')
@mock.patch('srv.salt._modules.osd.log')
def test_bluestore_partitions_no_waldb_only_db_and_size_no_eq_create(self, mock_log, create_mock, double_mock):
def test_bluestore_partitions_no_waldb_only_db_and_size_no_eq_create(self, mock_log, create_mock):
"""
Given I haven't defined wal but a db
And I have a db_size
Expand All @@ -996,8 +990,7 @@ def test_bluestore_partitions_no_waldb_only_db_and_size_no_eq_create(self, mock_
obj = test_module.OSDPartitions(osd_config)
obj._bluestore_partitions()
mock_log.warning.assert_called_with('Setting wal to same device /dev/sddb as db')
create_mock.assert_called_with('/dev/sddb', [('wal', double_mock(100000)), ('db', 100000)])
double_mock.assert_called_with(100000)
create_mock.assert_called_with('/dev/sddb', [('db', 100000)])

@mock.patch('srv.salt._modules.osd.log')
def test_bluestore_partitions_no_waldb_no_db_log(self, mock_log):
Expand Down Expand Up @@ -1591,8 +1584,9 @@ def test_bluestore_args_1(self, ip_mock, hp_mock, osdc_o):
And and the device is a NVME
Expect args to be:

--block.wal /dev/sdwal1 --block.db /dev/sddb1 /dev/nvme0n1p1
--block.db /dev/sddb1 /dev/nvme0n1p1

since the WAL will use the DB
"""
kwargs = {'wal': '/dev/sdwal',
'wal_size': '2G',
Expand All @@ -1604,7 +1598,7 @@ def test_bluestore_args_1(self, ip_mock, hp_mock, osdc_o):
ip_mock.side_effect = [True]
hp_mock.side_effect = [1, 1]
ret = obj._bluestore_args()
assert ret == "--block.wal /dev/sdwal1 --block.db /dev/sddb1 /dev/nvme0n1p1"
assert ret == "--block.db /dev/sddb1 /dev/nvme0n1p1"

@mock.patch('srv.salt._modules.osd.OSDCommands.highest_partition')
@mock.patch('srv.salt._modules.osd.OSDCommands.is_partitioned')
Expand Down