-
Notifications
You must be signed in to change notification settings - Fork 338
DAOS-17535 chk: misc improvements for CR logic #17329
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
base: master
Are you sure you want to change the base?
Conversation
|
Ticket title is 'DAOS checker cannot completed on Aurora after some engines excluded' |
|
Test stage NLT on EL 8.8 completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-17329/1/testReport/ |
8e4ad6a to
639a8ec
Compare
|
Test stage Functional Hardware Medium MD on SSD completed with status FAILURE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net//job/daos-stack/job/daos/view/change-requests/job/PR-17329/1/execution/node/1388/log |
|
Test stage NLT on EL 8.8 completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-17329/2/testReport/ |
639a8ec to
78579dd
Compare
|
Test stage Functional on EL 8.8 completed with status FAILURE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net//job/daos-stack/job/daos/view/change-requests/job/PR-17329/2/execution/node/1076/log |
aa39da7 to
476d0f9
Compare
|
Test stage NLT on EL 8.8 completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-17329/5/testReport/ |
|
Test stage Functional Hardware Medium MD on SSD completed with status FAILURE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net//job/daos-stack/job/daos/view/change-requests/job/PR-17329/5/execution/node/1324/log |
Include the followings: 1. When create CHK IV namespace, make the secondary group to be same as the primary group. Otherwise, CHK logic may hit DER_NONEXIST trouble when communicate via IV. 2. Integrate CHK IV namespace create and destroy API, cleanup related logic, redefine the version. 3. Get ranks list and IV namespace version from CHK leader when rejoin. Adjust CHK_REJOIN RPC for related changes. 4. Remove unsupported functionality for checking the specified 'phase'. 5. Add new test for case of lost some engine(s) before start checker. Test-tag: recovery Signed-off-by: Fan Yong <[email protected]>
476d0f9 to
09aaf91
Compare
|
Test stage NLT on EL 8.8 completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-17329/6/testReport/ |
NLT failure for DAOS-17435, not related with the patch. All required CR tests passed. |
|
Ping reviewers, thanks! |
kjacque
left a comment
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.
Overall looks good, just a couple minor comments/questions.
| if (unlikely(ins->ci_ranks == NULL)) | ||
| D_GOTO(out, rc = -DER_NOTAPPLICABLE); |
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.
This seems like a strange case where it would be good to log something. What could cause the ranks not to be fetched, if an error wasn't returned?
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.
This may happen when chk_prop_fetch() got empty rank_list if checker has never run before or related CR property was removed for some reason. Anyway, it is not fatal for current operation since we expect to remove the specified rank from the rank_list.
| uint32_t ci_start_flags; | ||
| uint32_t ci_ns_ver; |
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.
minor - indent is strange here. If the formatter did it, feel free to disregard.
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.
Yes, it is the clang-format required, honestly, I also do not like such style;)
| /* Let secondary rank == primary rank. */ | ||
| rc = crt_group_secondary_modify(ins->ci_iv_group, ins->ci_ranks, ins->ci_ranks, | ||
| CRT_GROUP_MOD_OP_REPLACE, ns_ver); |
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.
Since this function call is repeated in a few places with the same comment, you could put it in a simple (maybe even inline) wrapper function that takes only ins and ns_ver as params. Also, I think it would be good to add detail to the comment as to why the primary and secondary groups must be the same for the checker.
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 idea. I will do that when need to refresh the patch next time, or in subsequent CR related PR.
Include the followings:
When create CHK IV namespace, make the secondary group to be same as
the primary group. Otherwise, CHK logic may hit DER_NONEXIST trouble
when communicate via IV.
Integrate CHK IV namespace create and destroy API, cleanup related
logic, redefine the version.
Get ranks list and IV namespace version from CHK leader when rejoin.
Adjust CHK_REJOIN RPC for related changes.
Remove unsupported functionality for checking the specified 'phase'.
Add new test for case of lost some engine(s) before start checker.
Test-tag: recovery
Signed-off-by: Fan Yong [email protected]
Steps for the author:
After all prior steps are complete: