Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DAOS-16687 control: Handle missing PCIe caps in storage query usage #15296

Merged
merged 7 commits into from
Oct 23, 2024

Conversation

tanabarr
Copy link
Contributor

@tanabarr tanabarr commented Oct 11, 2024

Missing PCIe capabilities when querying a NVMe SSD's configuration
space is unusual but should be handled gracefully by the control-plane
and shouldn't cause a failure to return usage statistics when calling
dmg storage query usage.

Update so that pciutils lib is only called when attempting to display
health stats via dmg and not when fetching usage info. Improve clarity
of workflow to ease maintenance and add test coverage for updates.
Enable continued functionality when NVMe device doesn't return any
extended capabilities in PCIe configuration space data by adding
sentinel error to library for such a case.

Features: control
Required-githooks: true

Before requesting gatekeeper:

  • Two review approvals and any prior change requests have been resolved.
  • Testing is complete and all tests passed or there is a reason documented in the PR why it should be force landed and forced-landing tag is set.
  • Features: (or Test-tag*) commit pragma was used or there is a reason documented that there are no appropriate tags for this PR.
  • Commit messages follows the guidelines outlined here.
  • Any tests skipped by the ticket being addressed have been run and passed in the PR.

Gatekeeper:

  • You are the appropriate gatekeeper to be landing the patch.
  • The PR has 2 reviews by people familiar with the code, including appropriate owners.
  • Githooks were used. If not, request that user install them and check copyright dates.
  • Checkpatch issues are resolved. Pay particular attention to ones that will show up on future PRs.
  • All builds have passed. Check non-required builds for any new compiler warnings.
  • Sufficient testing is done. Check feature pragmas and test tags and that tests skipped for the ticket are run and now pass with the changes.
  • If applicable, the PR has addressed any potential version compatibility issues.
  • Check the target branch. If it is master branch, should the PR go to a feature branch? If it is a release branch, does it have merge approval in the JIRA ticket.
  • Extra checks if forced landing is requested
    • Review comments are sufficiently resolved, particularly by prior reviewers that requested changes.
    • No new NLT or valgrind warnings. Check the classic view.
    • Quick-build or Quick-functional is not used.
  • Fix the commit message upon landing. Check the standard here. Edit it to create a single commit. If necessary, ask submitter for a new summary.

@tanabarr tanabarr added the control-plane work on the management infrastructure of the DAOS Control Plane label Oct 11, 2024
@tanabarr tanabarr requested review from mjmac, kjacque and knard38 October 11, 2024 12:48
@tanabarr tanabarr self-assigned this Oct 11, 2024
@tanabarr tanabarr requested review from a team as code owners October 11, 2024 12:48
Copy link

Ticket title is 'dmg storage query usage fails if no PCIe capabilities'
Status is 'In Review'
https://daosio.atlassian.net/browse/DAOS-16687

@tanabarr tanabarr force-pushed the tanabarr/control-pciutils-fix-nopciecaps branch from b8cdd72 to 9ec6cc3 Compare October 11, 2024 12:56
@daosbuild1
Copy link
Collaborator

Test stage Functional Hardware Medium completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15296/2/execution/node/1431/log

knard38
knard38 previously approved these changes Oct 14, 2024
Copy link
Contributor

@knard38 knard38 left a comment

Choose a reason for hiding this comment

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

LGTM for what I understand.

Features: control
Required-githooks: true

Signed-off-by: Tom Nabarro <[email protected]>
Features: control
Required-githooks: true

Signed-off-by: Tom Nabarro <[email protected]>
@tanabarr tanabarr force-pushed the tanabarr/control-pciutils-fix-nopciecaps branch from 9ec6cc3 to e2c41bf Compare October 14, 2024 14:09
@tanabarr tanabarr requested a review from knard38 October 15, 2024 20:49
@tanabarr tanabarr added the release-2.6.2 Targeted for release 2.6.2 label Oct 15, 2024
src/control/server/ctl_smd_rpc_test.go Outdated Show resolved Hide resolved
src/control/server/instance_storage_rpc.go Outdated Show resolved Hide resolved
src/control/server/instance_storage_rpc.go Outdated Show resolved Hide resolved
src/control/server/instance_storage_rpc.go Show resolved Hide resolved
src/control/server/instance_storage_rpc.go Show resolved Hide resolved
src/control/server/instance_storage_rpc.go Outdated Show resolved Hide resolved
@tanabarr tanabarr requested a review from kjacque October 17, 2024 14:38
Features: control
Required-githooks: true

Signed-off-by: Tom Nabarro <[email protected]>
…iutils-fix-nopciecaps

Features: control
Signed-off-by: Tom Nabarro <[email protected]>
knard38
knard38 previously approved these changes Oct 18, 2024
…iutils-fix-nopciecaps

Features: control
Skip-nlt: true
Required-githooks: true

Signed-off-by: Tom Nabarro <[email protected]>
@tanabarr tanabarr added the forced-landing The PR has known failures or has intentionally reduced testing, but should still be landed. label Oct 21, 2024
Copy link
Contributor

@kjacque kjacque left a comment

Choose a reason for hiding this comment

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

Good fixes, thanks. The one issue remaining open for me is the assumption about the interaction between the incoming flags (meta vs. health). IMO needing to make an assumption like that is a sign that we need more granular flags coming in.

Features: control
Required-githooks: true

Signed-off-by: Tom Nabarro <[email protected]>
…iutils-fix-nopciecaps

Features: control
Signed-off-by: Tom Nabarro <[email protected]>
@tanabarr tanabarr requested review from kjacque and knard38 October 21, 2024 21:40
@tanabarr tanabarr requested a review from a team October 23, 2024 13:40
@phender phender merged commit 7a70c1d into master Oct 23, 2024
56 checks passed
@phender phender deleted the tanabarr/control-pciutils-fix-nopciecaps branch October 23, 2024 14:58
tanabarr added a commit that referenced this pull request Oct 24, 2024
…15296)

Missing PCIe capabilities when querying a NVMe SSD's configuration
space is unusual but should be handled gracefully by the control-plane
and shouldn't cause a failure to return usage statistics when calling
dmg storage query usage.

Update so that pciutils lib is only called when attempting to display
health stats via dmg and not when fetching usage info. Improve clarity
of workflow to ease maintenance and add test coverage for updates.
Enable continued functionality when NVMe device doesn't return any
extended capabilities in PCIe configuration space data by adding
sentinel error to library for such a case.

Features: control
Required-githooks: true

Signed-off-by: Tom Nabarro <[email protected]>
tanabarr added a commit that referenced this pull request Oct 24, 2024
…15296)

Missing PCIe capabilities when querying a NVMe SSD's configuration
space is unusual but should be handled gracefully by the control-plane
and shouldn't cause a failure to return usage statistics when calling
dmg storage query usage.

Update so that pciutils lib is only called when attempting to display
health stats via dmg and not when fetching usage info. Improve clarity
of workflow to ease maintenance and add test coverage for updates.
Enable continued functionality when NVMe device doesn't return any
extended capabilities in PCIe configuration space data by adding
sentinel error to library for such a case.

Features: control
Required-githooks: true

Signed-off-by: Tom Nabarro <[email protected]>
mjmac pushed a commit that referenced this pull request Oct 28, 2024
…15296) (#15392)

Missing PCIe capabilities when querying a NVMe SSD's configuration
space is unusual but should be handled gracefully by the control-plane
and shouldn't cause a failure to return usage statistics when calling
dmg storage query usage.

Update so that pciutils lib is only called when attempting to display
health stats via dmg and not when fetching usage info. Improve clarity
of workflow to ease maintenance and add test coverage for updates.
Enable continued functionality when NVMe device doesn't return any
extended capabilities in PCIe configuration space data by adding
sentinel error to library for such a case.

Signed-off-by: Tom Nabarro <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
control-plane work on the management infrastructure of the DAOS Control Plane forced-landing The PR has known failures or has intentionally reduced testing, but should still be landed. release-2.6.2 Targeted for release 2.6.2
Development

Successfully merging this pull request may close these issues.

5 participants