Add per OSD crush_device_class definition #49555

fmount · 2022-12-23T14:35:59Z

This patch introduces a per osd crush_device_class definition in the DriveGroup spec. The Device object is extended to support a crush_device_class parameter which is processed by ceph-volume when drives are prepared in batch mode. According to the per osd defined crush device classes, drives are collected and grouped in a dict that is used to produce a set of ceph-volume commands that eventually apply (if defined) the right device class. The test_drive_group unit tests are also extended to make sure we're not breaking compatibility with the default definition and the new syntax is validated, raising an exception if it's violated.

Fixes: https://tracker.ceph.com/issues/58184

Signed-off-by: Francesco Pantano fpantano@redhat.com

Contribution Guidelines

To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "pacific"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

Checklist

Tracker (select at least one)
- References tracker ticket
- Very recent bug; references commit where it was introduced
- New feature (ticket optional)
- Doc update (no ticket needed)
- Code cleanup (no ticket needed)
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

Show available Jenkins commands

jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows

fmount · 2022-12-23T15:17:00Z

@adk3798 can you check this when you have the chance? It's something we already discussed at the orch meeting and it basically solves the tracker in $subject.
With this change you can either have the form:

service_type: osd
service_id: testing_drivegroup
placement:
  host_pattern: hostname
data_devices:
  paths:
  - /dev/sda
  - /dev/sdb

or specify a per disk crush_device_class like the following:

service_type: osd
service_id: testing_drivegroup
placement:
  host_pattern: hostname
crush_device_class: hdd
data_devices:
  paths:
  - data: /dev/sda
    crush_device_class: ssd
  - data: /dev/sdb
    crush_device_class: hdd
  - data: /dev/sdc
    crush_device_class: nvme
  - data: /dev/sdd

If no crush_device_class is defined within paths, the OSD can use the global one (the value present in the spec), and be collected in the right dict entry.
If nothing is specified (neither within the paths spec nor at global level, then ceph-volume won't append any --crush-device-class to the command.
A few examples that cover the combinations we have can be found here [1].

[1] https://paste.opendev.org/show/bEZJEMD67J1i1oPFj5mf/

fmount · 2022-12-23T15:18:51Z

@adk3798 I suspect the failing tests [1] are unrelated to this patch, right?

[1] https://jenkins.ceph.com/job/ceph-pull-requests/108756/console

anthonyeleven · 2022-12-27T16:30:47Z

doc/cephadm/services/osd.rst

+        - data: /dev/sdb
+        - crush_device_class: ssd
+        - data: /dev/sdc
+        - crush_device_class: nvme


Is this the right YAML structure? I expected something more like:

data_devices: paths: - data: /dev/sdb crush_device_class: ssd - data: /dev/sdc crush_device_class: nvme

You're right, I had an issue in the previous change. The right structure is the following:

... ... spec: data_devices: paths: - data: /dev/sdb crush_device_class: ssd - data: /dev/sdc crush_device_class: nvme db_devices: ...

where paths is translated into a json like paths: [ { "data": "/dev/sdb", "crush_device_class": "ssd" }, { "data": "/dev/sdc", "crush_device_class": "nvme" } ] which is easy to process (and validate) from cephadm.
Thanks, nice catch on that issue I had with the yaml definition, just updated the doc change!

YAML! I hate YAML! Even with strawberries!
;)

anthonyeleven

Docs LGTM, no code or overall PR approval should be inferred.

adk3798 · 2023-01-05T19:53:36Z

@adk3798 I suspect the failing tests [1] are unrelated to this patch, right?

[1] https://jenkins.ceph.com/job/ceph-pull-requests/108756/console

they look unrelated, yeah

adk3798 · 2023-01-05T19:53:47Z

jenkins retest this please

adk3798

Generally LGTM, some minor things/cleanup

src/python-common/ceph/deployment/drive_group.py

src/python-common/ceph/deployment/translate.py

src/python-common/ceph/deployment/drive_group.py

adk3798 · 2023-01-05T21:02:10Z

src/python-common/ceph/tests/test_drive_group.py

    dg = DriveGroupSpec.from_json(yaml.safe_load(test_input))
    assert dg.service_id == 'testing_drivegroup'
    assert all([isinstance(x, Device) for x in dg.data_devices.paths])
-    assert dg.data_devices.paths[0].path == '/dev/sda'
+    if isinstance(dg.data_devices.paths[0].path, str):


was mypy complaining without this extra isinstance? It seems like the path attribute for the Device class is still just type str so it feels like this shouldn't be necessary.

Yes, this change just solved that problem. I'll add a stacktrace to show you the issue, but definitely having this check (that doesn't change the nature of the test) helped to make it happy

fmount · 2023-01-19T12:57:02Z

src/python-common/ceph/deployment/translate.py

+
+        # For this use case we don't apply any custom crush_device_classes
+        # Note that filestore is not supported anymore by the DriveGroupSpec
+        if self.spec.objectstore == 'filestore':


@adk3798 I'm not sure we want to keep L121-L138 (the filestore use case). This code is actually never reached, and if filestore is added to the spec:

test_input11 = """service_type: osd service_id: testing_drivegroup placement: host_pattern: hostname objectstore: filestore data_devices: paths: - path: /dev/ceph_vg/ceph_lv_data crush_device_class: ssd - path: /dev/ceph_vg1/ceph_lv_data1 - path: /dev/ceph_vg1/ceph_lv_data2 - path: /dev/ceph_vg1/ceph_lv_data3 - path: /dev/ceph_vg1/ceph_lv_data4 """

the spec validation will fail with the following error:

raise DriveGroupValidationError(self.service_id, ceph.deployment.drive_group.DriveGroupValidationError: Failed to validate OSD spec "testing_drivegroup": filestore is not supported. Must be one of ('bluestore')

which is actually expected from [1].
We can leave this block here (and it's out of the for loop), but it's just something we want to clean up (maybe in a follow up change?)

[1] https://github.com/ceph/ceph/blob/main/src/python-common/ceph/deployment/drive_group.py#L328-L331

I'll have to check if this is getting used by anything else somehow that could make use of that. Maybe something for a follow up PR, yeah

fwiw, filestore is going to be deprecated and removed soon

src/python-common/ceph/deployment/translate.py

fmount · 2023-01-19T17:08:17Z

src/python-common/ceph/tests/test_drive_group.py

+  - path: /dev/sdb
+    crush_device_class: hdd
+  - path: /dev/sdc
+    crush_device_class: ssd


Note that test_input6 != test_input5 as here we have multiple device classes

src/python-common/ceph/tests/test_drive_group.py

This patch introduces a per osd crush_device_class definition in the DriveGroup spec. The Device object is extended to support a crush_device_class parameter which is processed by ceph-volume when drives are prepared in batch mode. According to the per osd defined crush device classes, drives are collected and grouped in a dict that is used to produce a set of ceph-volume commands that eventually apply (if defined) the right device class. The test_drive_group unit tests are also extended to make sure we're not breaking compatibility with the default definition and the new syntax is validated, raising an exception if it's violated. Fixes: https://tracker.ceph.com/issues/58184 Signed-off-by: Francesco Pantano <fpantano@redhat.com>

adk3798

LGTM

adk3798 · 2023-01-22T16:12:02Z

https://pulpito.ceph.com/adking-2023-01-21_05:38:21-orch:cephadm-wip-adk-testing-2023-01-20-1359-distro-default-smithi/

Lots of failures (13) but all accounted for

3 instances of 'list' object has no attribute 'get'. Failure was on new test for setting mon crush location introduced by a PR in the run. That PR can't be merged, but this shouldn't block merging anything else.
2 instances of test_cephadm.sh test failing, caused by the same mon crush location PR. Failed with local variable 'config_json' referenced before assignment
1 instance of https://tracker.ceph.com/issues/49287, known issue
2 instances of https://tracker.ceph.com/issues/58526, known issue
2 instances of https://tracker.ceph.com/issues/58535, known issue
1 instance of https://tracker.ceph.com/issues/57771, known issue
1 instance of https://tracker.ceph.com/issues/57755, known issue
1 failed basic smoke test that timed out waiting for all 8 OSDs to come up. Did 5 reruns of this test with the same build (https://pulpito.ceph.com/adking-2023-01-22_15:28:20-orch:cephadm-wip-adk-testing-2023-01-20-1359-distro-default-smithi/) all of which passed. My guess is an I/O error creating the last OSD, which I've seen happen (infrequently) when we make a bunch of OSDs on the split up nvme drive we use for having multiple OSDs on these machines. Considering it a 1 off for now, and not blocking merging over it.

Overall, PRs in the run should be okay to merge other than the mon crush location one causing failures. Will start to try and clean up the test suite now that we're able to make builds and run tests again.

adk3798 · 2023-01-23T13:16:00Z

https://pulpito.ceph.com/adking-2023-01-22_23:00:58-orch:cephadm-wip-adk-testing-2023-01-22-1523-distro-default-smithi/

lots of failures from infra stuff (re-imaging machines, installing things pre-test) so did a rerun of all failed and dead jobs, resulting in

https://pulpito.ceph.com/adking-2023-01-23_04:36:45-orch:cephadm-wip-adk-testing-2023-01-22-1523-distro-default-smithi/

Leaving us with 6 failures.

3 failed jobs were the new test_set_mon_crush_locations test introduced in one of the PRs in the run. The test just isn't quite right yet, but it's not relevant in terms of whether we can merge other PRs
2 instances of the staggered upgrade test issue (known issue) https://tracker.ceph.com/issues/58535
1 instance of failing to install container selinux package (known issue) https://tracker.ceph.com/issues/57771

Overall, nothing that would block merging for anything other than the mon crush location PR.

fmount requested a review from a team as a code owner December 23, 2022 14:36

github-actions bot added ceph-volume cephadm pybind labels Dec 23, 2022

fmount force-pushed the drive_group_crush branch from 1be85ee to ce80830 Compare December 27, 2022 14:45

fmount requested a review from a team as a code owner December 27, 2022 14:45

github-actions bot added the documentation label Dec 27, 2022

anthonyeleven reviewed Dec 27, 2022

View reviewed changes

fmount force-pushed the drive_group_crush branch from ce80830 to b929930 Compare December 28, 2022 11:06

anthonyeleven reviewed Dec 28, 2022

View reviewed changes

adk3798 reviewed Jan 5, 2023

View reviewed changes

fmount force-pushed the drive_group_crush branch 2 times, most recently from 07f59ca to be8f4bc Compare January 19, 2023 12:44

fmount commented Jan 19, 2023

View reviewed changes

fmount requested a review from adk3798 January 19, 2023 12:58

adk3798 reviewed Jan 19, 2023

View reviewed changes

src/python-common/ceph/deployment/translate.py Show resolved Hide resolved

fmount force-pushed the drive_group_crush branch from be8f4bc to f4e9630 Compare January 19, 2023 17:03

fmount commented Jan 19, 2023

View reviewed changes

fmount force-pushed the drive_group_crush branch from f4e9630 to d774803 Compare January 19, 2023 17:10

fmount commented Jan 19, 2023

View reviewed changes

src/python-common/ceph/tests/test_drive_group.py Show resolved Hide resolved

fmount force-pushed the drive_group_crush branch from d774803 to 6c6cb2f Compare January 19, 2023 21:01

adk3798 approved these changes Jan 20, 2023

View reviewed changes

adk3798 added the wip-adk-testing label Jan 20, 2023

adk3798 merged commit 45813ea into ceph:main Feb 13, 2023
11 of 12 checks passed

fmount mentioned this pull request Feb 17, 2023

Remove the filestore section from ceph-volume #50149

Merged

14 tasks

adk3798 mentioned this pull request Mar 8, 2023

quincy: Add per OSD crush_device_class definition #50444

Merged

Add per OSD crush_device_class definition #49555

Add per OSD crush_device_class definition #49555

fmount commented Dec 23, 2022 •

edited

fmount commented Dec 23, 2022

fmount commented Dec 23, 2022

anthonyeleven Dec 27, 2022

fmount Dec 28, 2022

anthonyeleven Dec 28, 2022

anthonyeleven left a comment

adk3798 commented Jan 5, 2023

adk3798 commented Jan 5, 2023

adk3798 left a comment

adk3798 Jan 5, 2023

fmount Jan 8, 2023

fmount Jan 19, 2023 •

edited

adk3798 Jan 19, 2023

guits Jan 20, 2023

fmount Jan 19, 2023

adk3798 left a comment

adk3798 commented Jan 22, 2023 •

edited

adk3798 commented Jan 23, 2023

Add per OSD crush_device_class definition #49555

Add per OSD crush_device_class definition #49555

Conversation

fmount commented Dec 23, 2022 • edited

Contribution Guidelines

Checklist

fmount commented Dec 23, 2022

fmount commented Dec 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anthonyeleven left a comment

Choose a reason for hiding this comment

adk3798 commented Jan 5, 2023

adk3798 commented Jan 5, 2023

adk3798 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmount Jan 19, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adk3798 left a comment

Choose a reason for hiding this comment

adk3798 commented Jan 22, 2023 • edited

adk3798 commented Jan 23, 2023

fmount commented Dec 23, 2022 •

edited

fmount Jan 19, 2023 •

edited

adk3798 commented Jan 22, 2023 •

edited