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

NitzanMordhai · 2023-07-17T12:34:22Z

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

rzarzynski · 2023-07-24T13:55:52Z

src/test/encoding/readble.py

@@ -0,0 +1,160 @@
+#!/usr/bin/env python3


This doesn't look like a mds- or osd-targetting change. Maybe we have it (+ directly related parts) in a separated commit?

thanks, need to delete it from that commit

rzarzynski · 2023-07-24T13:56:54Z

src/test/encoding/readble.py

+
+    print("Passed {} tests.".format(numtests))
+
+if __name__ == "__main__":


Who's going to execute readable.py? I can't find a caller in this PR.

rzarzynski · 2023-07-24T13:57:36Z

src/tools/ceph-dencoder/osd_types.h

+TYPE_FEATUREFUL(pool_opts_t)
+TYPE_FEATUREFUL(pg_missing_item)
+TYPE(eversion_t)
+//TYPE(compact_interval_t) decliered in .cc


src/tools/ceph-dencoder/osd_types.h

Matan-B

Overall looks good (and useful), left a few comments

Do we have a test that iterates through all the dencoder supported types? Might be useful before merging

Matan-B · 2023-08-03T09:01:19Z

src/osd/osd_types.h

 template <bool TrackChanges>
 class pg_missing_set : public pg_missing_const_i {
-  using item = pg_missing_item;
+  using item = pg_missing_item; 


nit extra space

Matan-B · 2023-08-03T09:09:43Z

src/tools/ceph-dencoder/denc_plugin.h

 };

 #define TYPE(t) plugin->emplace<DencoderImplNoFeature<t>>(#t, false, false);
+#define TYPE_VARARGS(t, ...) plugin->emplace<DencoderImplNoFeature<t>>(#t, false, false, ##__VA_ARGS__);


Does this definition have any users?

Not yet, but for future use with arguments

Matan-B · 2023-08-03T09:12:29Z

src/mgr/OSDPerfMetricTypes.h


 #include "include/denc.h"
 #include "include/stringify.h"
+#include "common/Formatter.h"


Why do we need to include Formatter here?

moving it to .cc file

Matan-B · 2023-08-03T09:13:34Z

src/mon/CreatingPGs.h

    }
    void dump(ceph::Formatter *f) const {
-      f->dump_unsigned("create_epoch", create_epoch);
+      f->dump_unsigned("create_epoch1", create_epoch);


yep.. removing

Matan-B · 2023-08-03T09:19:02Z

src/osd/SnapMapper.h

+    void dump(ceph::Formatter *f) const {
+      f->dump_unsigned("snap", snap);
+      f->dump_stream("hoid") << hoid;
+    }
+    static void generate_test_instances(std::list<Mapping*>& o) {
+      o.push_back(new Mapping);
+      o.push_back(new Mapping);
+      o.back()->snap = 1;
+      o.back()->hoid = hobject_t(object_t("objname"), "key", 123, 456, 0, "");
+    }


SnapMapper support was already useful for my local branches 👍

Matan-B · 2023-08-03T09:19:51Z

src/osd/osd_types.cc


 void PastIntervals::pg_interval_t::encode(ceph::buffer::list& bl) const
 {
+


Matan-B · 2023-08-03T09:20:01Z

src/osd/osd_types.cc

 }

-void PastIntervals::pg_interval_t::decode(ceph::buffer::list::const_iterator& bl)
+void PastIntervals::pg_interval_t::decode(ceph::buffer::list::const_iterator& bl) 


nit extra space

Matan-B · 2023-08-03T09:20:56Z

src/osd/osd_types.cc

 }

-WRITE_CLASS_ENCODER(PastIntervals::pg_interval_t)
+


nit extra new line

Matan-B · 2023-08-03T09:26:22Z

src/tools/ceph-dencoder/osd_types.h

+TYPE(PastIntervals::pg_interval_t)
+using pg_interval_t = PastIntervals::pg_interval_t;
+TYPE(pg_interval_t)


Why do we need both the namespace alias and the original namespaces declared?
I noticed that this was the case also with ceph::os::Transaction, MgrMap::StandbyInfo, etc.

ceph-dencoder doesn't know how to find type when it doesn't have its namespace, and since we dumped the type without namespace we will "lose" checks for older version, but yes, i'll change that to skip those types, but will add dump for the full namespace

Matan-B · 2023-08-03T09:34:45Z

#52623 #52482 #52481 #52210 are all sharing (roughly) the same title.
May I suggest the following title format?
ceph-dencoder: < component > - Add missing types

NitzanMordhai · 2023-08-06T08:15:10Z

Overall looks good (and useful), left a few comments

Do we have a test that iterates through all the dencoder supported types? Might be useful before merging

Running it for each PR change ../src/test/encoding/check-generated.sh

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>

Matan-B · 2023-08-06T08:57:52Z

src/tools/ceph-dencoder/osd_types.h

+TYPE(bluestore_onode_t::shard_info)
+using shard_info = bluestore_onode_t::shard_info;
+TYPE(shard_info)


full namespace can be used exclusively for consistency

Matan-B · 2023-08-06T09:05:40Z

check-generated.sh is failing with few failed to decode

error: void MonCommand::decode(ceph::buffer::list::const_iterator &) decode past end of struct encoding: Malformed input [buffer:3]
        /home/jenkins-build/build/workspace/ceph-pull-requests/ceph-object-corpus/archive/0.80-rc1-35-g4812150/objects/SnapMapper::object_snaps
**** failed to decode /home/jenkins-build/build/workspace/ceph-pull-requests/ceph-object-corpus/archive/0.80-rc1-35-g4812150/objects/MonCommand/6cde3066e02a9ab7fce3758724e719bb ****

NitzanMordhai · 2023-08-07T11:25:20Z

jenkins test make check

rzarzynski

Overall LGTM! Just nits and few questions.

rzarzynski · 2023-08-08T13:20:06Z

src/mgr/MetricTypes.h

+
+  void dump(ceph::Formatter *f) const {
+    f->open_array_section("report");
+    for (auto& i : report) {


C++17's structured bindings could useful to provide names for the std::pair's members (query & report?).

rzarzynski · 2023-08-08T13:21:11Z

src/cls/fifo/cls_fifo.cc


 #include "include/buffer.h"
 #include "include/types.h"
+#include "common/Formatter.h"


nit: should be sorted alphabetically (c precedes i).

rzarzynski · 2023-08-08T13:21:22Z

src/mgr/DaemonHealthMetric.h

 #include <cstdint>
 #include <ostream>
 #include "include/denc.h"
+#include "common/Formatter.h"


nit: should be sorted alphabetically.

rzarzynski · 2023-08-08T13:22:38Z

src/mgr/OSDPerfMetricTypes.cc


 #include "mgr/OSDPerfMetricTypes.h"
-
+#include "common/Formatter.h"


Our includes should appear after any standard ones. I know it's non-new but it would be really nice if we can fix that.

rzarzynski · 2023-08-08T13:22:45Z

src/mgr/OSDPerfMetricTypes.h

 #include "include/denc.h"
 #include "include/stringify.h"
-
+#include "common/ceph_json.h"


nit: should be sorted alphabetically.

rzarzynski · 2023-08-08T13:29:58Z

src/os/Transaction.h

  static void generate_test_instances(std::list<Transaction*>& o);
 };
-WRITE_CLASS_ENCODER(Transaction)
+WRITE_CLASS_ENCODER(ceph::os::Transaction)


Why this is needed? Just one line below we still have Transaction::TransactionData.

rzarzynski · 2023-08-08T13:32:31Z

src/osd/osd_types.cc

 {
-  o.push_back(new pg_notify_t(shard_id_t(3), shard_id_t::NO_SHARD, 1, 1,
-			      pg_info_t(), PastIntervals()));
-  o.push_back(new pg_notify_t(shard_id_t(0), shard_id_t(0), 3, 10,


What's the reason behind stripping these test instances?

i missed that change, hit some segfault and didn't place it back, added it back

rzarzynski · 2023-08-08T13:33:42Z

src/osd/osd_types.h

 */
 struct pg_missing_item {
  eversion_t need, have;
+


nit: do we need this blank?

Done, please continue review #52871

rzarzynski · 2023-08-08T13:34:48Z

src/tools/ceph-dencoder/osd_types.h

+TYPE_FEATUREFUL(pg_missing_item)
+TYPE(eversion_t)
+//TYPE(compact_interval_t) declared in .cc
+//TYPE(pg_missing_t::item)


i left it there just as a note, we currently can't include those types.

rzarzynski · 2023-08-08T13:35:12Z

src/tools/ceph-dencoder/osd_types.h

+//TYPE(compact_interval_t) declared in .cc
+//TYPE(pg_missing_t::item)
+
+


nit: a doubled newline.

Done, please continue review #52871

NitzanMordhai requested review from a team as code owners July 17, 2023 12:34

github-actions bot added bluestore cephfs Ceph File System core mgr mon rgw tests labels Jul 17, 2023

NitzanMordhai force-pushed the wip-nitzan-ceph-dencoder-extend-osd-type-available branch 4 times, most recently from 664f0cb to 45e7bac Compare July 20, 2023 11:46

rzarzynski reviewed Jul 24, 2023

View reviewed changes

NitzanMordhai force-pushed the wip-nitzan-ceph-dencoder-extend-osd-type-available branch from 45e7bac to d650b0d Compare July 25, 2023 12:16

NitzanMordhai changed the title ~~ceph-dencoder: Add missing osd and mds types to ceph-dencoder for ac…~~ ceph-dencoder: Add missing osd types to ceph-dencoder for acurate encode-decode comparison Jul 25, 2023

NitzanMordhai requested a review from rzarzynski July 25, 2023 12:18

Matan-B reviewed Aug 3, 2023

View reviewed changes

NitzanMordhai force-pushed the wip-nitzan-ceph-dencoder-extend-osd-type-available branch from d650b0d to fb11471 Compare August 6, 2023 08:16

NitzanMordhai changed the title ~~ceph-dencoder: Add missing osd types to ceph-dencoder for acurate encode-decode comparison~~ ceph-dencoder: OSD - Add missing types Aug 6, 2023

Matan-B approved these changes Aug 6, 2023

View reviewed changes

NitzanMordhai mentioned this pull request Aug 8, 2023

ceph-dencoder: OSD - Add missing types #52871

Merged

14 tasks

rzarzynski reviewed Aug 8, 2023

View reviewed changes

NitzanMordhai closed this Aug 9, 2023


		print("Passed {} tests.".format(numtests))

		if __name__ == "__main__":


		void PastIntervals::pg_interval_t::encode(ceph::buffer::list& bl) const
		{


		#include "mgr/OSDPerfMetricTypes.h"

		#include "common/Formatter.h"

		//TYPE(compact_interval_t) declared in .cc
		//TYPE(pg_missing_t::item)

		}

		WRITE_CLASS_ENCODER(PastIntervals::pg_interval_t)

Conversation

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

Uh oh!

Contribution Guidelines

Checklist

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!

Uh oh!

Matan-B left a comment • 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!

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!

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 Aug 3, 2023

Uh oh!

NitzanMordhai commented Aug 6, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Matan-B commented Aug 6, 2023

Uh oh!

NitzanMordhai commented Aug 7, 2023

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NitzanMordhai commented Jul 17, 2023 •

edited

Loading

Matan-B left a comment •

edited

Loading