crimson: Set device class during spawn of a crimson osd by mohit84 · Pull Request #60747 · ceph/ceph

mohit84 · 2024-11-15T05:13:24Z

Implement a wrapper for different backend storage to set device_class during spawn of a process

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

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. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.
When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an x between the brackets: [x]. Spaces and capitalization matter when checking off items this way.

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)
Component impact
- Affects Dashboard, opened tracker ticket
- Affects Orchestrator, opened tracker ticket
- No impact that needs to be tracked
Documentation (select at least one)
- Updates relevant documentation
- No doc update is appropriate
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
jenkins test rook e2e

mohit84 · 2024-11-15T05:14:57Z

jenkins test make check

Matan-B

looks good so far, left a few comments.
I would also suggest trying to use coroutines in OSD::_add_device_class() as it can make the implementation easier to read.

Matan-B · 2024-11-17T09:53:26Z

src/crimson/os/cyanstore/cyan_store.cc

+  //TODO In case of classic memstore we are always
+  // returning hdd so for the timebeing we are skipping


Let's return empty string, no need for TODO with CyanStore for now.

Matan-B · 2024-11-17T09:57:10Z

src/crimson/osd/osd.cc

+seastar::future<> OSD::_add_device_class()
+{
+  LOG_PREFIX(OSD::_add_device_class);
+  if (!local_conf().get_val<bool>("osd_crush_update_on_start")) {


osd_class_update_on_start should be used here

Matan-B · 2024-11-17T10:10:15Z

src/crimson/osd/osd.cc

+    }).handle_exception([FNAME](std::exception_ptr e) {
+      ERROR("Failed to get device class: ", e);
+      return seastar::make_ready_future<std::string>("");
+    });


Are we expecting an thrown exception here?
For expected errors in Crimson we use errorator, see dev/crimson/error-handling.

I have changed the code now, please let me know if it is ok now.

Matan-B · 2024-11-17T10:11:37Z

src/crimson/osd/osd.cc

+    string cmd = string("{\"prefix\": \"osd crush set-device-class\", ") +
+                 string("\"class\": \"") + device_class + string("\", ") +
+                 string("\"ids\": [\"") + stringify(whoami) + string("\"]}");


Let's switch to fmt::format here

Matan-B · 2024-11-17T10:12:47Z

src/crimson/osd/osd.cc

+    });
+  };
+
+  return get_device_class().then([FNAME, this](auto device_class) {


We should check that device_class is not an empty string before using it

Matan-B · 2024-11-17T10:13:47Z

src/crimson/osd/osd.cc

+      [[maybe_unused]] auto [code, message, out] = std::move(command_result);
+      if (code) {
+        WARN("fail to set device_class : {} ({})", message, code);
+        throw std::runtime_error("fail to add to crush");


"fail to set device_class"

Matan-B · 2024-11-17T10:19:41Z

makecheck:

/home/jenkins-build/build/workspace/ceph-pull-requests/src/crimson/osd/osd.cc:621:58: error: lambda capture 'this' is not used [-Werror,-Wunused-lambda-capture]
    return store.get_default_device_class().then([FNAME, this](std::string device_class) {
                                                  ~~~~~~~^~~~
/home/jenkins-build/build/workspace/ceph-pull-requests/src/crimson/osd/osd.cc:623:26: error: variable 'FNAME' cannot be implicitly captured in a lambda with no capture-default specified
    }).handle_exception([FNAME](std::exception_ptr e) {

mohit84 · 2024-11-18T09:56:24Z

jenkins test make check

mohit84 · 2024-11-18T11:49:09Z

jenkins test make check

Matan-B · 2024-11-18T13:43:27Z

src/crimson/osd/osd.cc

+  auto get_device_class = [FNAME, this]() -> seastar::future<std::string> {
+    try {
+      std::string device_class = co_await store.get_default_device_class();
+      co_return device_class;
+    } catch (const std::exception& e) {
+      ERROR("Failed to get device class: ", e.what());
+      co_return "";
+    }
+  };


There's no need for try/catch we discarding exceptions as we handle error with Crimson's errorator concept.
See TEST_F(coroutine_test_t, test_ertr_coroutine_error)

We can skip the get_device_class wrapper:

std::string device_class = co_await store.get_default_device_class();

Matan-B · 2024-11-18T13:44:21Z

src/crimson/osd/osd.cc

+  } catch (const std::exception& e) {
+    ERROR("Failed to add device class: ", e.what());
+    throw;
+  }


I have changed the code

Matan-B · 2024-11-18T13:45:06Z

src/crimson/osd/osd.cc

+      device_class, stringify(whoami)
+    );
+
+    auto [code, message, out] = co_await monc->run_command(std::move(cmd), {});


Great usage!

Matan-B · 2024-11-18T13:49:49Z

src/crimson/osd/osd.cc

+    auto [code, message, out] = co_await monc->run_command(std::move(cmd), {});
+      if (code) {
+        WARN("fail to set device_class : {} ({})", message, code);
+        throw std::runtime_error("fail to set device_class");


nit:

// to be caught by crimson/osd/main.cc throw std::runtime_error("fail to set device_class");

Matan-B

lgtm! Left final comments

Matan-B · 2024-11-19T09:07:13Z

src/crimson/osd/osd.cc

+    co_return;
+  }
+
+  INFO("device_class is {} whoami: {}", device_class, whoami);


The osd id would be printed as a prefix, whoami is redundant

Matan-B · 2024-11-19T09:11:30Z

src/crimson/osd/osd.cc

+    WARN("fail to set device_class : {} ({})", message, code);
+    throw std::runtime_error("fail to set device_class");
+  } else {
+    INFO("newly added to crush: {}", message);


"device_class was set: {}"

Implement a wrapper for different backend storage to set device_class during spawn of a process. Fixes: https://tracker.ceph.com/issues/66627 Signed-off-by: Mohit Agrawal <moagrawa@redhat.com>

mohit84 · 2024-11-19T09:33:39Z

jenkins test make check

mohit84 · 2024-11-22T05:01:40Z

@Matan-B
https://pulpito.ceph.com/moagrawa-2024-11-21_08:55:14-crimson-rados-wip-mohit-crimson-device_class-distro-crimson-smithi/

Failures
8003283 https://tracker.ceph.com/issues/67972
8003305 https://tracker.ceph.com/issues/66852

Matan-B · 2024-11-24T09:42:41Z

jenkins test make check

Matan-B · 2024-11-24T09:44:46Z

@Matan-B https://pulpito.ceph.com/moagrawa-2024-11-21_08:55:14-crimson-rados-wip-mohit-crimson-device_class-distro-crimson-smithi/

Failures 8003283 https://tracker.ceph.com/issues/67972 8003305 https://tracker.ceph.com/issues/66852

Please also include "dead" jobs.
8003264 from perf suite failed as well, unrelated since a similar failure was seen in recent main run:
https://pulpito.ceph.com/teuthology-2024-11-23_20:56:02-crimson-rados-main-distro-crimson-smithi/

Merging, thank you!

mohit84 · 2024-11-25T04:45:21Z

@Matan-B https://pulpito.ceph.com/moagrawa-2024-11-21_08:55:14-crimson-rados-wip-mohit-crimson-device_class-distro-crimson-smithi/

Failures 8003283 https://tracker.ceph.com/issues/67972 8003305 https://tracker.ceph.com/issues/66852

Dead job links, it seems both jobs are dead due to some issue at infra level.
https://pulpito.ceph.com/teuthology-2024-11-23_20:56:02-crimson-rados-main-distro-crimson-smithi/8005981/
https://pulpito.ceph.com/teuthology-2024-11-23_20:56:02-crimson-rados-main-distro-crimson-smithi/8005998/

Matan-B · 2024-11-25T08:57:09Z

jenkins test make check

Matan-B · 2024-11-25T08:58:32Z

Dead job links, it seems both jobs are dead due to some issue at infra level. https://pulpito.ceph.com/teuthology-2024-11-23_20:56:02-crimson-rados-main-distro-crimson-smithi/8005981/ https://pulpito.ceph.com/teuthology-2024-11-23_20:56:02-crimson-rados-main-distro-crimson-smithi/8005998/

There seems to be something off with Crimson and cbt (perf suite). As mentioned earlier these failures also appear in main so I'll merge once CI passes.

Matan-B · 2024-11-26T10:20:56Z

jenkins test make check

mohit84 requested a review from a team as a code owner November 15, 2024 05:13

github-actions bot added the crimson label Nov 15, 2024

Matan-B reviewed Nov 17, 2024

View reviewed changes

mohit84 force-pushed the crimson_device_class branch from ea5684f to 29d8312 Compare November 18, 2024 11:47

Matan-B reviewed Nov 18, 2024

View reviewed changes

mohit84 force-pushed the crimson_device_class branch from 29d8312 to 39305b2 Compare November 19, 2024 08:53

Matan-B approved these changes Nov 19, 2024

View reviewed changes

Matan-B added the needs-crimson-qa label Nov 19, 2024

crimson: Set device class during spawn of a crimson osd

d2ee4c1

Implement a wrapper for different backend storage to set device_class during spawn of a process. Fixes: https://tracker.ceph.com/issues/66627 Signed-off-by: Mohit Agrawal <moagrawa@redhat.com>

mohit84 force-pushed the crimson_device_class branch from 39305b2 to d2ee4c1 Compare November 19, 2024 09:33

Matan-B added TESTED ready-to-merge labels Nov 24, 2024

Matan-B merged commit 3caa542 into ceph:main Nov 26, 2024

		//TODO In case of classic memstore we are always
		// returning hdd so for the timebeing we are skipping

Conversation

mohit84 commented Nov 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

mohit84 commented Nov 15, 2024

Uh oh!

Matan-B left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Matan-B commented Nov 17, 2024

Uh oh!

mohit84 commented Nov 18, 2024

Uh oh!

mohit84 commented Nov 18, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Matan-B left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mohit84 commented Nov 19, 2024

Uh oh!

mohit84 commented Nov 22, 2024

Uh oh!

Matan-B commented Nov 24, 2024

Uh oh!

Matan-B commented Nov 24, 2024

Uh oh!

mohit84 commented Nov 25, 2024

Uh oh!

Matan-B commented Nov 25, 2024

Uh oh!

Matan-B commented Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

mohit84 commented Nov 15, 2024 •

edited

Loading

Matan-B commented Nov 25, 2024 •

edited

Loading