Skip to content

os/bluestore: use default value for non-decoded field in test instances#63935

Merged
tchaikov merged 1 commit intoceph:mainfrom
tchaikov:wip-bluestore-exclude-unencoded-field-in-dump
Jul 9, 2025
Merged

os/bluestore: use default value for non-decoded field in test instances#63935
tchaikov merged 1 commit intoceph:mainfrom
tchaikov:wip-bluestore-exclude-unencoded-field-in-dump

Conversation

@tchaikov
Copy link
Contributor

@tchaikov tchaikov commented Jun 15, 2025 •
edited
Loading

The sbid field added in commit a7f8e23 is set during construction but not persisted to disk. Including it in dump() output causes discrepancies between original and re-encoded instances, leading to test failures in readable.sh and check-generated.sh.

Current tests pass because they reuse the same instance for re-encoding, preserving non-persisted fields. An upcoming change will allocate fresh instances for each decode operation, which will expose this issue and break tests.

Use default value for sbid when generating test instances to ensure consistency between encoding and decoding operations.

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 ifed01 June 15, 2025 05:56
@tchaikov tchaikov requested a review from a team as a code owner June 15, 2025 05:56
@github-actions github-actions bot added the core label Jun 15, 2025
@tchaikov
Copy link
Contributor Author

tchaikov commented Jun 15, 2025

@ifed01 hi Igor, could you please help review this change?

@tchaikov tchaikov mentioned this pull request Jun 15, 2025
Merged
14 tasks
@aclamk aclamk self-requested a review June 18, 2025 09:04
aclamk
aclamk previously requested changes Jun 18, 2025
View reviewed changes
Copy link
Contributor

@aclamk aclamk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should not remove "sbid" from dump.

@tchaikov
os/bluestore: use default value for non-decoded field in test instances
2426c11 
The sbid field added in commit a7f8e23 is set during construction
but not persisted to disk. Including it in dump() output causes
discrepancies between original and re-encoded instances, leading
to test failures in readable.sh and check-generated.sh.

Current tests pass because they reuse the same instance for
re-encoding, preserving non-persisted fields. An upcoming change
will allocate fresh instances for each decode operation, which
will expose this issue and break tests.

Use default value for sbid when generating test instances to ensure
consistency between encoding and decoding operations.

Signed-off-by: Kefu Chai <tchaikov@gmail.com>
@tchaikov tchaikov force-pushed the wip-bluestore-exclude-unencoded-field-in-dump branch from 1009cda to 2426c11 Compare June 23, 2025 14:03
@tchaikov tchaikov changed the title os/bluestore: exclude sbid field from bluestore_shared_blob_t dump os/bluestore: use default value for non-decoded field in test instances Jun 23, 2025
@tchaikov
Copy link
Contributor Author

tchaikov commented Jun 23, 2025

jenkins test make check

@tchaikov
Copy link
Contributor Author

tchaikov commented Jun 23, 2025

jenkins test api

@tchaikov
Copy link
Contributor Author

tchaikov commented Jun 24, 2025

jenkins test make check

@tchaikov
Copy link
Contributor Author

tchaikov commented Jun 24, 2025

jenkins test make check arm64

@tchaikov
Copy link
Contributor Author

tchaikov commented Jun 24, 2025

jenkins test make check

@tchaikov
Copy link
Contributor Author

tchaikov commented Jun 24, 2025

jenkins test make check arm64

@tchaikov
Copy link
Contributor Author

tchaikov commented Jun 24, 2025

jenkins test windows

@tchaikov
Copy link
Contributor Author

tchaikov commented Jun 24, 2025

changelog

  • use default value for sbid instead of not printing it in dump()

@aclamk hi Adam, could you please take another look?

@tchaikov tchaikov requested a review from aclamk June 24, 2025 23:37
@tchaikov
Copy link
Contributor Author

tchaikov commented Jun 29, 2025

@aclamk ping?

@tchaikov
Copy link
Contributor Author

tchaikov commented Jul 7, 2025

@aclamk ping?

@tchaikov tchaikov dismissed aclamk’s stale review July 9, 2025 02:37

tried to contact Adam multiple times -- on slack and on github. no responses so far, and the change was confirmed by him over slack three weeks ago. so dismissing his "request for change".

@tchaikov
Copy link
Contributor Author

tchaikov commented Jul 9, 2025

@aclamk sorry, Adam, after trying to contact you multiple times -- on slack and on github, no responses so far. i understand there are lots of things going on. but i made the change requested by you half a month ago. so i believe that your concern should be addressed in the latest change. since Igor approved the change, and this change only impacts the encoding related tests which are exercised by CI. so i am jumping the gun, and merging this change without your approval in order to unblock #63910.

@tchaikov tchaikov merged commit 388cff0 into ceph:main Jul 9, 2025
19 checks passed
@tchaikov tchaikov deleted the wip-bluestore-exclude-unencoded-field-in-dump branch July 9, 2025 02:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ifed01 ifed01 ifed01 approved these changes

@aclamk aclamk Awaiting requested review from aclamk

Assignees

No one assigned

Labels

bluestore core

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tchaikov @ifed01 @aclamk