nvmeof: fix dhchap key-value bug in rbd metadata#6085
Merged
mergify[bot] merged 1 commit intoFeb 18, 2026
Conversation
dh-chap key-value in rbd metadata was mandatory by mistake. it failed if dh-chap key was not found (in case of not using dh-chap feature..) make it as optional. Signed-off-by: gadi-didi <gadi.didi@ibm.com>
4 tasks
There was a problem hiding this comment.
Pull request overview
Adjusts NVMe-oF RBD metadata retrieval so DH-CHAP-related metadata is treated as optional, avoiding failures when DH-CHAP is not in use.
Changes:
- Splits NVMe-oF RBD metadata keys into required vs optional sets.
- Skips missing/empty optional metadata keys instead of failing metadata retrieval.
Comment on lines
+1208
to
+1213
| value, err := rbdVol.GetMetadata(key) | ||
| if err != nil { | ||
| log.DebugLog(ctx, "Optional metadata %s not found: %v", key, err) | ||
|
|
||
| continue | ||
| } |
There was a problem hiding this comment.
In the optional-metadata loop, any error from GetMetadata() is treated as “not found” and silently ignored. That can mask real failures (e.g., inability to open the image, permission/IO issues) and the log message becomes misleading. Consider only skipping errors that are errors.Is(err, librbd.ErrNotFound) and returning other errors (similar to patterns used elsewhere in the repo, e.g. internal/rbd/qos.go).
Comment on lines
+1206
to
+1220
| // Optional metadata keys | ||
| for _, key := range optionalKeys { | ||
| value, err := rbdVol.GetMetadata(key) | ||
| if err != nil { | ||
| log.DebugLog(ctx, "Optional metadata %s not found: %v", key, err) | ||
|
|
||
| continue | ||
| } | ||
| if value == "" { | ||
| log.DebugLog(ctx, "Optional metadata %s is empty", key) | ||
|
|
||
| continue | ||
| } | ||
| metadata[key] = value | ||
| } |
There was a problem hiding this comment.
This change alters behavior to allow missing DH-CHAP/KMSID metadata. There are unit tests in this package, but none appear to exercise getNVMeoFMetadata’s handling of absent optional keys. Consider adding a focused test (or refactoring the metadata-read/parsing logic into a helper that can be unit-tested with a fake types.Volume) to prevent regressions where missing optional metadata incorrectly fails volume cleanup/unpublish flows.
nixpanic
approved these changes
Feb 18, 2026
Madhu-1
approved these changes
Feb 18, 2026
78 tasks
Contributor
Merge Queue StatusRule:
This pull request spent 38 minutes 25 seconds in the queue, including 37 minutes 37 seconds running CI. Required conditions to merge
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
dh-chap key-value in rbd metadata was mandatory by mistake. it failed if dh-chap key was not found (in case of not using dh-chap feature..)
make it as optional.
Checklist:
guidelines in the developer
guide.
Request
notes
updated with breaking and/or notable changes for the next major release.
Show available bot commands
These commands are normally not required, but in case of issues, leave any of
the following bot commands in an otherwise empty comment in this PR:
/retest ci/centos/<job-name>: retest the<job-name>after unrelatedfailure (please report the failure too!)