You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(satellite): record only hex generations - #225
The GI record took any non-empty string from the observer and read back any non-empty token as a generation. Now only a non-zero hex UUID, the way drbdsetup and drbdmeta print it without 0x, gets recorded. A zero GI is skipped quietly, since it is DRBD's "none yet". Anything else is skipped with a warning.
On read, a line whose generation is not such a UUID counts as damaged, so the repair refuses the record instead of trusting it, same as for the other damaged lines.
Tests: TestOnlyHexGenerationsAreRecorded, plus two more cases in TestUnreadableGIRecordFailsClosed.
Summary by CodeRabbit
Bug Fixes
Generation identifiers are recorded only when they contain exactly 16 hexadecimal digits and are not all zero. Invalid nonzero identifiers trigger a warning; the zero-GI value is ignored.
Invalid identifiers in stored generation history are now treated as damaged records rather than accepted.
File: No configuration file from this repository applied
Commit: Unavailable
Layers: defaults
Review profile: CHILL
Plan: Advanced
Run ID: 4449ffdf-7170-4004-a354-01ef653e9b62
Commits
Reviewing files that changed from the base of the PR and between 65ad6e0 and 1b5720c.
Files selected for processing: 2
pkg/drbd/dumpmd_test.go
pkg/satellite/createmd_test.go
No actionable comments were generated in the recent review.
Walkthrough
The change adds drbd.IsGenerationUUID, which accepts exactly 16 hexadecimal digits when at least one digit is nonzero. Metadata parsing and GI history recording and reading now use this validation. Tests cover invalid and accepted generations.
IsGenerationUUID accepts exactly 16 hexadecimal digits with at least one nonzero digit. Metadata parsing rejects UUID statements that do not meet these requirements. Tests cover short and overlong values.
Validate GI history generations pkg/satellite/gi_history.go, pkg/satellite/createmd_test.go
GI history recording skips invalid values and warns for invalid nonzero values. Reading rejects stored generations that fail validation. Tests cover malformed, zero, lowercase, and zero-padded generations.
cozystack/blockstor#220: Introduced GI history recording and the observer callback that supplies generation identifiers.
Merge Risk: Minimal · previous review 65ad6
The change tightens generation validation and rejects damaged history records. No actionable merge-blocking risk is established; merge after normal checks pass.
Pre-merge checks: 5
Check name
Status
Explanation
Description Check
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
The title clearly summarizes the main change: recording only valid hexadecimal generation UUIDs in satellite handling.
Docstring Coverage
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
Explanation
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 6 functions across 4 files.
Finishing Touches
Generate docstrings
Commit to this branch
Create a new PR
Generate unit tests (beta)
Commit to this branch
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
File: No configuration file from this repository applied
Commit: Unavailable
Layers: defaults
Review profile: CHILL
Plan: Advanced
Run ID: 37e2d98f-0fc4-4e9b-a56a-85817b3f8889
Commits
Reviewing files that changed from the base of the PR and between 4c6324e and 65ad6e0.
Files selected for processing: 4
pkg/drbd/dumpmd.go
pkg/drbd/dumpmd_test.go
pkg/satellite/createmd_test.go
pkg/satellite/gi_history.go
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
The reason will be displayed to describe this comment to others. Learn more.
Andrei Kvapil (@kvaps) NOT LGTM: damaged hex values of the wrong length still reach metadata re-creation.
DRBD prints these UUIDs as 16 hex digits. IsGenerationUUID also accepts ABCD and the 17-digit 16E3A5D0B2C1F4A96, so the new read guard trusts values the tools do not print.
Evidence: I wrote each value as volume 0's only history entry and ran both metadata paths with an unclean probe and a valid dump after apply-al. Both returned success and ran create-md --force because the bad entry matched none of the real UUIDs. Adding len(s) == 16 makes the fail-closed reproducer pass.
Please reject short and overlong values on read and write, and cover them in the recording and repair tests.
The reason will be displayed to describe this comment to others. Learn more.
Andrei Kvapil (@kvaps) NOT LGTM: damaged hex values of the wrong length still reach metadata re-creation.
DRBD prints these UUIDs as 16 hex digits. IsGenerationUUID also accepts ABCD and the 17-digit 16E3A5D0B2C1F4A96, so the new read guard trusts values the tools do not print.
Evidence: I wrote each value as volume 0's only history entry and ran both metadata paths with an unclean probe and a valid dump after apply-al. Both returned success and ran create-md --force because the bad entry matched none of the real UUIDs. Adding len(s) == 16 makes the fail-closed reproducer pass.
Please reject short and overlong values on read and write, and cover them in the recording and repair tests.
The generation record took any non-empty string the observer handed it,
and read back any non-empty token as a generation. A value that is not
a GI would sit in the record as one, and a zero GI identifies nothing.
Record only a non-zero hex UUID, and treat any other value in the file
as a damaged line, which the readers already refuse.
Assisted-by: LLM
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
drbdsetup and drbdmeta print a generation UUID zero-padded to 16 hex
digits. Any hex of another length was accepted on both sides, so a
damaged history line matched none of the real UUIDs and the repair went
on to create-md. A value of another length is now damage, in the record
and in the metadata dump alike.
Assisted-by: LLM
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Fixed in 65ad6e0, a generation has to be exactly 16 hex digits now on read and write, and the dump parser uses the same check. Short and long values are in the recording test and in the repair test on both paths.
Every non-hex fixture is also the wrong length. Evidence: I removed only isHex(s) from IsGenerationUUID, and the full suites for the three affected packages still passed. On that mutant, a separate test with the 16-character 6E3A5D0B2C1F4A9G ran create-md --force on both metadata paths. The current implementation rejects it correctly.
Please add a 16-character non-hex value to the recording and damaged-history cases so the hex check is tested independently of the length check.
Every non-hex value the tests used was also the wrong length, so the
hex check could go without any test noticing. A 16-character value
with a non-hex digit is now refused on recording, on reading the
history back, and in a metadata dump.
Assisted-by: LLM
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
The reason will be displayed to describe this comment to others. Learn more.
Andrei Kvapil (@kvaps) LGTM. I repeated the hex-only mutation with the committed tests and it fails on recording, damaged history on both metadata paths, and metadata dumps. The width and zero mutations fail too.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Aleksei Sviridkin (lexfrei)
left a comment
Follow-up to #220.
The GI record took any non-empty string from the observer and read back any non-empty token as a generation. Now only a non-zero hex UUID, the way
drbdsetupanddrbdmetaprint it without0x, gets recorded. A zero GI is skipped quietly, since it is DRBD's "none yet". Anything else is skipped with a warning.On read, a line whose generation is not such a UUID counts as damaged, so the repair refuses the record instead of trusting it, same as for the other damaged lines.
Tests:
TestOnlyHexGenerationsAreRecorded, plus two more cases inTestUnreadableGIRecordFailsClosed.Summary by CodeRabbit