-
Notifications
You must be signed in to change notification settings - Fork 298
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-13777 control: Invalid storage information with MD-on-SSD #12485
Conversation
With storage query usage, only take into account NVMe devices which are used to store data. Features: control Required-githooks: true Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@intel.com>
Bug-tracker data: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. No errors found by checkpatch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable, but I'd like to see some unit test coverage that exercises the changes.
Test stage Functional Hardware Medium completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-12485/1/execution/node/1286/log |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, notwithstanding the comment about a unit test case added to verify the change
…/daos-13777 Required-githooks: true
Integrate revivewers comments. Features: control Required-githooks: true Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@intel.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. No errors found by checkpatch.
@@ -1266,6 +1266,11 @@ func processSCMSpaceStats(log logging.Logger, filterRank filterRankFn, scmNamesp | |||
func processNVMeSpaceStats(log logging.Logger, filterRank filterRankFn, nvmeControllers storage.NvmeControllers, rankNVMeFreeSpace rankFreeSpaceMap) error { | |||
for _, nvmeController := range nvmeControllers { | |||
for _, smdDevice := range nvmeController.SmdDevices { | |||
if !smdDevice.Roles.IsEmpty() && (smdDevice.Roles.OptionBits&storage.BdevRoleData) == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good change for ctl_storage_rpc_test.go, but I was hoping to see a test case for this one as well, showing that we now skip the non-data SSDs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Add unit tests for
processNVMeSpaceStats()
function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the remark.
The unit test allowed me to fix a potential issue.
Indeed, the device was not skipped: forgot the continue statement.
However, as the usable size was reset to zero in the control plane it was not detected with the functional tests.
With future update of the control plane, this last action could have not been done any more and a bug would arise.
Test stage Functional Hardware Medium completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-12485/2/execution/node/1286/log |
…/daos-13777 Required-githooks: true
Integrate revivewers comments: * Add unit tests for processNVMeSpaceStats() function #12485 (comment) Features: control Required-githooks: true Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@intel.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. No errors found by checkpatch.
Test stage Functional on EL 8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-12485/3/execution/node/1096/log |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. No errors found by checkpatch.
…/daos-13777 Test-tags: pr control,-version_number,-test_daos_control_config_basic,-test_daos_agent_config_basic Required-githooks: true
Resubmit to CI with skipping functional tests related to the JIRA ticket DAOS-13886 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. No errors found by checkpatch.
Test stage Functional Hardware Medium completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-12485/11/execution/node/1286/log |
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-12485/11/execution/node/1241/log |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. No errors found by checkpatch.
Test stage Functional Hardware Medium completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-12485/12/execution/node/1270/log |
PR Build #11 failure:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. No errors found by checkpatch.
Test stage Unit Test on EL 8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-12485/13/testReport/ |
…/daos-13777 Test-tag: pr control,-version_number,-test_daos_control_config_basic,-test_daos_agent_config_basic Required-githooks: true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. No errors found by checkpatch.
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-12485/14/execution/node/1241/log |
…/daos-13777 Test-tags: pr control,-version_number,-test_daos_control_config_basic,-test_daos_agent_config_basic Required-githooks: true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. No errors found by checkpatch.
Merge remote-tracking branch 'origin/master' into ckochhof/fix/master/daos-13777 Required-githooks: true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. No errors found by checkpatch.
Test stage Functional Hardware Medium Verbs Provider completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-12485/16/testReport/ |
The only test failing is a known CI issue:
|
Description
With storage query usage, only take into account NVMe devices which are used to store data.
Before requesting gatekeeper:
Features:
(orTest-tag*
) commit pragma was used or there is a reason documented that there are no appropriate tags for this PR.Gatekeeper: