fabrics: propagate connect-all/discover errors to the exit status - #3957
Open
prabhakarpujeri wants to merge 1 commit into
Open
fabrics: propagate connect-all/discover errors to the exit status#3957prabhakarpujeri wants to merge 1 commit into
prabhakarpujeri wants to merge 1 commit into
Conversation
nvme-cli 2.x discards almost every error in the auto-discovery paths, so 'nvme connect-all' exits 0 even when the discovery controller or an individual entry fails to connect. With nvmf-autoconnect.service this breaks Restart=on-failure setups. This is a regression from 1.x, which accumulated errors from do_discover() over each discovery.conf entry and returned the last non-zero one. Restore the accumulation at the four places that drop errors: - __discover(): record per-entry connect failures (excluding ENVME_CONNECT_ALREADY, which is informational) and errors from the recursive discovery calls; - discover_from_conf_file(): keep the return value of __discover() instead of overwriting it with the disconnect result, and treat a failure to create the discovery controller as an error, mirroring the argv path in nvmf_discover(); - _discover_from_json_config_file(): same treatment; its caller already aggregates. The explicit single-target path (nvmf_discover with -t/-a/-s) already returns the error, and 3.x does this via the fabrics rework (eedb1c1); this restores the behavior on the 2.x branch. Tested: discovery.conf with '-t tcp -a 127.0.0.1 -s 18080'; 'nvme connect-all --no-nbft' exits 1 with the fix and 0 without. Fixes: linux-nvme#2800 Signed-off-by: Prabhakar Pujeri <prabhakar.pujeri@dell.com>
Collaborator
|
I've deleted my previous comment because I thought it was for master, this PR is for 2.x |
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
2 participants
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.
Fixes #2800 on the 2.x branch.
connect-allon 2.x drops every error from__discover()and friends:discover_from_conf_file()ignores the__discover()return, overwritesretwith the disconnect result, and silently skips creation failures of the discovery controller (goto next)__discover()returns 0 even when per-entry connects fail (only the "already connected" case was handled)_discover_from_json_config_file()drops errors the same waySo with several targets in
discovery.confand one down, exit status is 0 andnvmf-autoconnect.service(Restart=on-failure) never retries — exactly the regression igaw analyzed on the issue (1.x accumulated the error per discovery.conf entry; 2.x ignored__discover's return).Note: 3.x already has correct aggregation through the fabrics rework (
eedb1c15d); this is the targeted 2.x fix, keeping last-error-wins semantics matching 1.x.Test (local, 2.x branch)
discovery.conf:
-t tcp -a 127.0.0.1 -s 18080(refused instantly)meson testsuite: 39 pass, 0 fail (2 pre-existing expected failures). The NBFT path was exercised only with --no-nbft in this test loop; NBFT on the 2.x tip has a separate crash, see the other PR.