Skip to content

cls/rbd: use default values for non-decoded fields in test instances#63933

Merged
idryomov merged 1 commit intoceph:mainfrom
tchaikov:wip-rbd-cls_rbd_snap
Jun 17, 2025
Merged

cls/rbd: use default values for non-decoded fields in test instances#63933
idryomov merged 1 commit intoceph:mainfrom
tchaikov:wip-rbd-cls_rbd_snap

Conversation

@tchaikov
Copy link
Contributor

@tchaikov tchaikov commented Jun 14, 2025

Previously, test instances for cls_rbd_snap used non-default values for the "parent" field, which is ignored during decoding. The check-generated.sh test passed because they reused the same instance for re-encoding, preserving undecoded fields.

An upcoming change will allocate new instances for each encode/decode verification instead of reusing instances. This will expose discrepancies between original test instances and re-encoded values when fields contain non-default values but aren't decoded.

This change sets ignored fields to their default values in test instances, ensuring consistency between encoding and decoding operations regardless of the verification approach used.

Since the incompatibility of cls_rbd_snap's on-disk format was introduced in 32b14ed, which was introduced Ceph v14, we will mark this version the first incompatible version in ceph-object-corpus in the sense that the re-encoded cls_rbd_snap with v8 struct version is different from the original copy if its parent field is set with < v8 struct version.

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)
Show available Jenkins commands

@tchaikov tchaikov requested a review from a team as a code owner June 14, 2025 14:36
@github-actions github-actions bot added the rbd label Jun 14, 2025
@tchaikov
Copy link
Contributor Author

jenkins test make check

@tchaikov
Copy link
Contributor Author

see also ceph/ceph-object-corpus#24, which marks cls_rbd_snap as forward incompatible since nautilus

Previously, test instances for cls_rbd_snap used non-default values
for the "parent" field, which is ignored during decoding. The
check-generated.sh test passed because they reused the same instance
for re-encoding, preserving undecoded fields.

An upcoming change will allocate new instances for each encode/decode
verification instead of reusing instances. This will expose
discrepancies between original test instances and re-encoded values
when fields contain non-default values but aren't decoded.

This change sets ignored fields to their default values in test
instances, ensuring consistency between encoding and decoding
operations regardless of the verification approach used.

Since the incompatibility of cls_rbd_snap's on-disk format was
introduced in 32b14ed, which was introduced Ceph v14, we will
mark this version the first incompatible version in ceph-object-corpus
in the sense that the re-encoded cls_rbd_snap with v8 struct version
is different from the original copy if its parent field is set with
< v8 struct version.

Signed-off-by: Kefu Chai <tchaikov@gmail.com>
@tchaikov tchaikov force-pushed the wip-rbd-cls_rbd_snap branch from a60e4e9 to a329b00 Compare June 17, 2025 14:39
@tchaikov
Copy link
Contributor Author

changelog:

  • incorporated the suggested formatting change .

@tchaikov tchaikov requested a review from idryomov June 17, 2025 14:41
@idryomov
Copy link
Contributor

The check-generated.sh test passed because they reused the same instance for re-encoding, preserving undecoded fields.

Is there any indication of this being done on purpose, perhaps to allow dump() methods to include fields that aren't persisted on-disk? I don't think many people realize that dump() is part of ceph-dencoder interface and is used for round-trip tests of the kind of that check-generated.sh does.

Asking just of of curiosity -- I actually prefer the "allocate new instances for each encode/decode verification instead of reusing instances" approach.

@tchaikov
Copy link
Contributor Author

tchaikov commented Jun 17, 2025

The check-generated.sh test passed because they reused the same instance for re-encoding, preserving undecoded fields.

Is there any indication of this being done on purpose, perhaps to allow dump() methods to include fields that aren't persisted on-disk?

hmm, if it was done on purpose. i'd argue that it's a wrong decision which defeats the whole purpose of the check-generated.sh test. not to mention it was implemented in a weird way and could prevent us from identifying buggy encoders and decoders, if we really wanted to preserve this behavior.

I don't think many people realize that dump() is part of ceph-dencoder interface and is used for round-trip tests of the kind of that check-generated.sh does.

Asking just of of curiosity -- I actually prefer the "allocate new instances for each encode/decode verification instead of reusing instances" approach.

@idryomov
Copy link
Contributor

jenkins test make check

@idryomov idryomov merged commit 548e252 into ceph:main Jun 17, 2025
13 checks passed
@tchaikov tchaikov deleted the wip-rbd-cls_rbd_snap branch June 18, 2025 00:35
@idryomov idryomov added the needs-tentacle-backport PRs that need tentacle back-port label Jun 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants