ceph-dencoder: OSD - Add missing types by NitzanMordhai · Pull Request #52871 · ceph/ceph

NitzanMordhai · 2023-08-08T06:27:16Z

Wrong forked repo mentioned in #52482 that PR is fixing it

Currently, ceph-dencoder lacks certain osd type, preventing us from accurately checking the ceph corpus for encode-decode mismatches. This pull request aims to address this issue by adding the missing types to ceph-dencoder.

To successfully incorporate these types into ceph-dencoder, we need to introduce the necessary dump and generate_test_instances functions that was missing in some types. These functions are essential for proper encode and decode of the added types.

This PR will enhance the functionality of ceph-dencoder by including the missing types, enabling a comprehensive analysis of encode-decode consistency. With the addition of these types, we can ensure the robustness and correctness of the ceph corpus.

This update will significantly contribute to improving the overall reliability and accuracy of ceph-dencoder. It allows for a more comprehensive assessment of the encode-decode behavior, leading to enhanced data integrity and stability within the ceph ecosystem.

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

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)
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

NitzanMordhai · 2023-08-08T06:28:33Z

@Matan-B please continue review here

Matan-B

#52482 can be closed now.

make check fails with failed to decode.

NitzanMordhai · 2023-08-09T05:34:43Z

make check fails with failed to decode.

it won't resolve until ceph/ceph-object-corpus#16 will be merged

NitzanMordhai · 2023-08-09T11:24:43Z

@rzarzynski please review my changes in that PR

rzarzynski

Basically LGTM; just nits.

src/include/cephfs/types.h

src/mgr/OSDPerfMetricTypes.cc

src/mgr/OSDPerfMetricTypes.h

rzarzynski · 2023-08-09T12:21:50Z

src/mon/MgrMap.h

+      f->dump_unsigned("gid", gid);
+      f->dump_string("name", name);
+      f->open_array_section("available_modules");
+      for (auto& i : available_modules) {


nit: const auto&.

using encode_json will reduce the number of lines

src/mon/MonCommand.h

rzarzynski · 2023-08-09T12:22:28Z

src/mon/MonMap.cc

+  f->dump_int("priority", priority);
+  f->dump_float("weight", weight);
+  f->open_object_section("crush_locations");
+  for (auto& p : crush_loc) {


nit: const auto&.

Converted to encode_json

src/os/Transaction.h

src/os/bluestore/bluestore_types.h

rzarzynski

My only concern is about breaking the format abstraction with encode_json(). Apart of that just nits.

Otherwise LGTM!

rzarzynski · 2023-08-23T17:08:22Z

src/mon/MonMap.cc

+  f->dump_stream("addr") << public_addrs;
+  f->dump_int("priority", priority);
+  f->dump_float("weight", weight);
+  encode_json("crush_location", crush_loc, f);


I'm not sure reusing the encode_json() is the right thing. The problem is that the dump() methods are supposed to format-agnostic – they are getting abstract Formatter.

However, the idea to employ a helper function is nice. Maybe Formatter already offers a facility for dumping containers?

@rzarzynski it will end up almost duplicating the code in ceph_json, i'll need to create a template dump function and then functions for each type. and that will duplicate for all other formats. ceph_json will work for xml and other types (even though ceph dencoder dumping only json)

rzarzynski · 2023-08-23T17:09:49Z

src/mon/MgrMap.h

      }
      DECODE_FINISH(p);
    }
+    void dump(ceph::Formatter *f) const {


nit: let's be consistent with this {.

Done, also fixed all other dump function that already had inconsistent use of {

src/os/Transaction.h

rzarzynski · 2023-08-23T17:11:06Z

src/os/bluestore/bluestore_types.h

 };
 WRITE_CLASS_DENC(bluestore_extent_ref_map_t)
-
+WRITE_CLASS_DENC(bluestore_extent_ref_map_t::record_t);


nit: unneeded ;.

rzarzynski · 2023-08-23T17:12:51Z

src/tools/ceph-dencoder/osd_types.h

 #include "os/kstore/kstore_types.h"
 TYPE(kstore_cnode_t)
 TYPE(kstore_onode_t)
+


nit:unneeded \n.

NitzanMordhai · 2023-09-07T05:53:26Z

@rzarzynski all checks pass now, please review

github-actions · 2023-12-13T16:22:12Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

ljflores · 2023-12-19T19:00:54Z

Rados approved: https://tracker.ceph.com/projects/rados/wiki/MAIN#httpstrellocomck9RNURve1897-wip-yuri3-testing-2023-12-07-0727-old-wip-yuri3-testing-wip-neorados-learning-from-experience

ljflores · 2023-12-19T19:01:05Z

@NitzanMordhai looks like this branch needs to be rebased.

yuriw · 2023-12-19T22:56:49Z

@NitzanMordhai pls rebase and merge
ref: https://trello.com/c/tUEWtLfq

Currently, ceph-dencoder lacks certain osd types, preventing us from accurately checking the ceph corpus for encode-decode mismatches. This pull request aims to address this issue by adding the missing types to ceph-dencoder. To successfully incorporate these types into ceph-dencoder, we need to introduce the necessary `dump` and `generate_test_instances` functions that was missing in some types. These functions are essential for proper encode and decode of the added types. This PR will enhance the functionality of ceph-dencoder by including the missing types, enabling a comprehensive analysis of encode-decode consistency. With the addition of these types, we can ensure the robustness and correctness of the ceph corpus. This update will significantly contribute to improving the overall reliability and accuracy of ceph-dencoder. It allows for a more comprehensive assessment of the encode-decode behavior, leading to enhanced data integrity and stability within the ceph ecosystem. Fixes: https://tracker.ceph.com/issues/61788 Signed-off-by: Nitzan Mordechai <nmordech@redhat.com>

NitzanMordhai requested review from a team as code owners August 8, 2023 06:27

github-actions bot added bluestore build/ops cephfs Ceph File System core crimson documentation mgr mon rgw labels Aug 8, 2023

NitzanMordhai changed the base branch from wip-nitzan-ceph-dencoder-extend-osd-type-available to main August 8, 2023 06:27

Matan-B reviewed Aug 8, 2023

View reviewed changes

NitzanMordhai mentioned this pull request Aug 9, 2023

ceph-dencoder: OSD - Add missing types #52482

Closed

14 tasks

NitzanMordhai force-pushed the wip-nitzan-ceph-dencoder-extend-osd-type-available branch from 4b484e6 to f190a1a Compare August 9, 2023 11:23

NitzanMordhai requested a review from rzarzynski August 9, 2023 11:23

rzarzynski approved these changes Aug 9, 2023

View reviewed changes

NitzanMordhai force-pushed the wip-nitzan-ceph-dencoder-extend-osd-type-available branch from f190a1a to b8e4eac Compare August 10, 2023 05:09

NitzanMordhai requested a review from Matan-B August 14, 2023 05:29

rzarzynski reviewed Aug 23, 2023

View reviewed changes

NitzanMordhai force-pushed the wip-nitzan-ceph-dencoder-extend-osd-type-available branch from b8e4eac to 3dd16d4 Compare September 5, 2023 10:05

NitzanMordhai requested a review from rzarzynski September 5, 2023 10:16

NitzanMordhai force-pushed the wip-nitzan-ceph-dencoder-extend-osd-type-available branch from 3dd16d4 to 83173f9 Compare September 6, 2023 06:04

NitzanMordhai requested review from a team as code owners September 6, 2023 06:04

NitzanMordhai requested review from avanthakkar and cloudbehl and removed request for a team September 6, 2023 06:04

github-actions bot added ceph-volume cephadm dashboard pybind tests labels Sep 6, 2023

NitzanMordhai force-pushed the wip-nitzan-ceph-dencoder-extend-osd-type-available branch 3 times, most recently from 728b87a to 1c4013c Compare September 6, 2023 08:28

rzarzynski approved these changes Nov 14, 2023

View reviewed changes

rzarzynski added the needs-qa label Nov 14, 2023

ljflores added the wip-yuri7-testing label Nov 17, 2023

github-actions bot added the needs-rebase label Dec 13, 2023

yuriw added TESTED ready-to-merge and removed wip-yuri7-testing labels Dec 19, 2023

NitzanMordhai force-pushed the wip-nitzan-ceph-dencoder-extend-osd-type-available branch from 2c81e09 to 899276a Compare December 20, 2023 06:07

github-actions bot removed the needs-rebase label Dec 20, 2023

NitzanMordhai merged commit 7eab324 into ceph:main Dec 20, 2023

NitzanMordhai deleted the wip-nitzan-ceph-dencoder-extend-osd-type-available branch December 20, 2023 07:43

cbodley mentioned this pull request Jan 25, 2024

mon: zero-initialize MonCommand::flags #55315

Merged

Conversation

NitzanMordhai commented Aug 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

NitzanMordhai commented Aug 8, 2023

Uh oh!

Matan-B left a comment

Choose a reason for hiding this comment

Uh oh!

NitzanMordhai commented Aug 9, 2023

Uh oh!

NitzanMordhai commented Aug 9, 2023

Uh oh!

rzarzynski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NitzanMordhai Aug 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rzarzynski 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!

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!

NitzanMordhai commented Sep 7, 2023

Uh oh!

github-actions bot commented Dec 13, 2023

Uh oh!

ljflores commented Dec 19, 2023

Uh oh!

ljflores commented Dec 19, 2023

Uh oh!

yuriw commented Dec 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

NitzanMordhai commented Aug 8, 2023 •

edited

Loading

NitzanMordhai Aug 10, 2023 •

edited

Loading