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.

Sign up for GitHub

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

Jump to bottom

DAOS-14442 control: Make NVMe auto-faulty configurable #13548

Merged
merged 9 commits into from
Jan 24, 2024
Merged

DAOS-14442 control: Make NVMe auto-faulty configurable #13548

merged 9 commits into from
Jan 24, 2024

Conversation

tanabarr
Copy link
Contributor

@tanabarr tanabarr commented Jan 1, 2024 •
edited
Loading

SSD auto faulty is enabled by default and criteria set through
environment variables. Make criteria settable through the server
configuration file instead of environment variables. bdev_auto_faulty
parameter can be used to set enable, max_io_errs and max_csum_errs.

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 watchers.
  • 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 requested a review from a team as a code owner January 1, 2024 14:48
@tanabarr tanabarr requested review from mjmac and kjacque and removed request for a team January 1, 2024 14:48
@tanabarr tanabarr self-assigned this Jan 1, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 1, 2024 •
edited
Loading

Bug-tracker data:
Ticket title is 'Make SSD auto faulty configurable'
Status is 'In Review'
Labels: 'hotplug'
https://daosio.atlassian.net/browse/DAOS-14442

@tanabarr tanabarr force-pushed the tanabarr/control-auto-faulty-config branch from 8315958 to 3b377ce Compare January 3, 2024 00:27
@tanabarr tanabarr marked this pull request as draft January 3, 2024 00:27
wangshilong
wangshilong previously approved these changes Jan 3, 2024
View reviewed changes
@tanabarr tanabarr marked this pull request as ready for review January 8, 2024 17:43
@tanabarr tanabarr requested a review from a team as a code owner January 8, 2024 23:16
@daosbuild1
Copy link
Collaborator

Test stage Functional on EL 8.8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-13548/5/execution/node/1112/log

wangshilong
wangshilong previously approved these changes Jan 9, 2024
View reviewed changes
@daosbuild1
Copy link
Collaborator

Test stage Functional Hardware Large completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-13548/6/execution/node/694/log

@daosbuild1
Copy link
Collaborator

Test stage Functional Hardware Medium Verbs Provider completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-13548/6/execution/node/648/log

@tanabarr
Copy link
Contributor Author

I don't think it's worth running through CI again for these indentation check warnings: https://github.com/daos-stack/daos/actions/runs/7454353436/job/20281518852?pr=13548 I will fix in a subsequent PR

@tanabarr
Copy link
Contributor Author

CI run https://build.hpdd.intel.com/blue/organizations/jenkins/daos-stack%2Fdaos/detail/PR-13548/6/tests/ failed with unrelated known issues:

  • DFS_Parallel_DTX_cmocka
  • EcodOnlineRebuildMdtest
  • test_daos_rebuild_ec

@tanabarr tanabarr added the forced-landing The PR has known failures or has intentionally reduced testing, but should still be landed. label Jan 10, 2024
@daosbuild1
Copy link
Collaborator

Test stage Functional Hardware Medium completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-13548/7/testReport/

@daosbuild1
Copy link
Collaborator

Test stage Functional Hardware Medium Verbs Provider completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-13548/7/execution/node/1418/log

@daosbuild1
Copy link
Collaborator

Test stage Functional Hardware Large completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-13548/7/execution/node/1526/log

wangshilong
wangshilong previously approved these changes Jan 11, 2024
View reviewed changes
@tanabarr tanabarr removed the forced-landing The PR has known failures or has intentionally reduced testing, but should still be landed. label Jan 12, 2024
@daosbuild1
Copy link
Collaborator

Test stage Unit Test on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-13548/8/testReport/

@tanabarr
DAOS-14442 control: Make NVMe auto-faulty configurable
f8595bd 
Required-githooks: true

Signed-off-by: Tom Nabarro <tom.nabarro@intel.com>
@daosbuild1
Copy link
Collaborator

Test stage Functional Hardware Medium completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-13548/9/testReport/

@daosbuild1
Copy link
Collaborator

Test stage Functional Hardware Large completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-13548/9/execution/node/1480/log

@tanabarr
Copy link
Contributor Author

tanabarr commented Jan 13, 2024 •
edited
Loading

CI run https://build.hpdd.intel.com/blue/organizations/jenkins/daos-stack%2Fdaos/detail/PR-13548/9/tests/ failed on the following issues:

  • DAOS-14845 online_rebuild_mdtest.py Timeout for mdtest after killing one rank
  • DAOS-14884 dmg failure with scm-size zero

@tanabarr
fix codespell issue
58642d8 
Required-githooks: true

Signed-off-by: Tom Nabarro <tom.nabarro@intel.com>
@tanabarr
Merge remote-tracking branch 'origin/master' into tanabarr/control-au…
cb4b3ae 
…to-faulty-config

Features: control
Signed-off-by: Tom Nabarro <tom.nabarro@intel.com>
@daosbuild1
Copy link
Collaborator

Test stage Functional Hardware Medium completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-13548/10/testReport/

@daosbuild1
Copy link
Collaborator

Test stage Functional Hardware Medium Verbs Provider completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-13548/10/execution/node/1433/log

@daosbuild1
Copy link
Collaborator

Test stage Functional Hardware Medium completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-13548/11/testReport/

@daosbuild1
Copy link
Collaborator

Test stage Functional Hardware Medium Verbs Provider completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-13548/11/execution/node/437/log

NiuYawei
NiuYawei approved these changes Jan 16, 2024
View reviewed changes
Copy link
Contributor

@NiuYawei NiuYawei left a comment

Choose a reason for hiding this comment

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

The C changes look good to me.

@tanabarr tanabarr added the forced-landing The PR has known failures or has intentionally reduced testing, but should still be landed. label Jan 16, 2024
@tanabarr
Copy link
Contributor Author

CI results for run no. 11 with control and pool features failed for the following known issues:

Requesting forced landing

knard-intel
knard-intel approved these changes Jan 16, 2024
View reviewed changes
Copy link
Contributor

@knard-intel knard-intel left a comment

Choose a reason for hiding this comment

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

Mostly OK for me.
Just minor remarks which could be fix if repush is needed.

utils/config/daos_server.yml
Comment on lines +205 to +210
## Optional parameter that should only be set if overriding the automatically calculated value is #
## #necessary. Specifies the number (not size) of hugepages to allocate for use by NVMe through
## #SPDK. For optimum performance each target requires 1 GiB of hugepage space. The provided value
## should be calculated by dividing the total amount of hugepages memory required for all targets
## across all engines on a host by the system hugepage size. If not set here, the value will be
## automatically calculated based on the number of targets (using the default system hugepage size).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
## Optional parameter that should only be set if overriding the automatically calculated value is #
## #necessary. Specifies the number (not size) of hugepages to allocate for use by NVMe through
## #SPDK. For optimum performance each target requires 1 GiB of hugepage space. The provided value
## should be calculated by dividing the total amount of hugepages memory required for all targets
## across all engines on a host by the system hugepage size. If not set here, the value will be
## automatically calculated based on the number of targets (using the default system hugepage size).
## Optional parameter that should only be set if overriding the automatically calculated value is
## necessary. Specifies the number (not size) of hugepages to allocate for use by NVMe through
## SPDK. For optimum performance each target requires 1 GiB of hugepage space. The provided value
## should be calculated by dividing the total amount of hugepages memory required for all targets
## across all engines on a host by the system hugepage size. If not set here, the value will be
## automatically calculated based on the number of targets (using the default system hugepage size).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will fix if repushed

src/bio/bio_config.c
D_INFO("NVMe auto faulty is %s. Criteria: max_io_errs:%u, max_csum_errs:%u\n",
*enable ? "enabled" : "disabled", *max_io_errs, *max_csum_errs);

if (cfg.method != NULL)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit, not needed to test as D_FREE will do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will fix if repushed

@tanabarr tanabarr requested a review from a team January 16, 2024 14:23
@tanabarr
Merge remote-tracking branch 'origin/master' into tanabarr/control-au…
9e2efb6 
…to-faulty-config

Features: control
Required-githooks: true

Signed-off-by: Tom Nabarro <tom.nabarro@intel.com>
@tanabarr
Copy link
Contributor Author

@daos-stack/daos-gatekeeper please can this PR be force landed now landings are open on master?

@mchaarawi
Copy link
Contributor

since there is a control test failure on the latest run, please merge latest master and add gatekeeper when testing completes.

@mchaarawi mchaarawi removed the request for review from a team January 22, 2024 17:23
@tanabarr tanabarr removed the forced-landing The PR has known failures or has intentionally reduced testing, but should still be landed. label Jan 22, 2024
@tanabarr tanabarr requested a review from a team January 24, 2024 16:12
@tanabarr tanabarr added forced-landing The PR has known failures or has intentionally reduced testing, but should still be landed. and removed forced-landing The PR has known failures or has intentionally reduced testing, but should still be landed. labels Jan 24, 2024
@tanabarr
Copy link
Contributor Author

clang-format failures to be addressed in a subsequent PR

mjmac
mjmac approved these changes Jan 24, 2024
View reviewed changes
Copy link
Contributor

@mjmac mjmac left a comment

Choose a reason for hiding this comment

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

The code changes seem fine to me, but I have some concerns about the design.

In an ideal world, I think that this configuration would be done as system properties, as I don't see any valid use case for allowing per-engine permutations in behavior. We don't currently have a way to propagate system-level configuration settings out to engines as they join, so I understand that we need to use the server yaml. That having been said, it would probably be better to set these in a top-level entry which is then applied to all engines, similar to how we set the provider and other parameters that should be the same for all engines in a configuration.

I wouldn't block on making this change, but I recommend that it be made sooner rather than later so that you don't have to support legacy configuration styles.

src/bio/bio_xstream.c
@@ -242,6 +236,14 @@ bio_nvme_init(const char *nvme_conf, int numa_node, unsigned int mem_size,
goto free_mutex;
}

glb_criteria.fc_enabled = true;
glb_criteria.fc_max_io_errs = 10;
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: I know they weren't before this patch, but it seems like these should be named constant values.

@tanabarr
Copy link
Contributor Author

The code changes seem fine to me, but I have some concerns about the design.

In an ideal world, I think that this configuration would be done as system properties, as I don't see any valid use case for allowing per-engine permutations in behavior. We don't currently have a way to propagate system-level configuration settings out to engines as they join, so I understand that we need to use the server yaml. That having been said, it would probably be better to set these in a top-level entry which is then applied to all engines, similar to how we set the provider and other parameters that should be the same for all engines in a configuration.

I wouldn't block on making this change, but I recommend that it be made sooner rather than later so that you don't have to support legacy configuration styles.

noted

@tanabarr
Copy link
Contributor Author

@mjmac can you land the PR please?

@mchaarawi mchaarawi merged commit f128f07 into master Jan 24, 2024
49 of 50 checks passed
@mchaarawi mchaarawi deleted the tanabarr/control-auto-faulty-config branch January 24, 2024 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knard-intel knard-intel knard-intel approved these changes

@NiuYawei NiuYawei NiuYawei approved these changes

@mjmac mjmac mjmac approved these changes

@kjacque kjacque Awaiting requested review from kjacque kjacque is a code owner automatically assigned from daos-stack/go-watchers

@Michael-Hennecke Michael-Hennecke Awaiting requested review from Michael-Hennecke

@wangshilong wangshilong Awaiting requested review from wangshilong

Assignees

@tanabarr tanabarr

Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@tanabarr @daosbuild1 @mchaarawi @mjmac @wangshilong @NiuYawei @knard-intel