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-16251 cart: Ignore expired bulks and rpcs #14971

Open
wants to merge 39 commits into
base: master
Choose a base branch
from
Open

DAOS-16251 cart: Ignore expired bulks and rpcs #14971

wants to merge 39 commits into from

Conversation

frostedcmos
Copy link
Contributor

  • Switch rpc headers to transfer deadline instead of a timeout
  • Add checks at the start and end of bulk transfer to ensure deadline has not expired.
  • Add deadline expiration checks in all places where rpc_priv timeout is initialized

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.

@frostedcmos
DAOS-16251 cart: Ignore expired bulks and rpcs
389b22e 
- Switch rpc headers to transfer deadline instead of a timeout
- Add checks at the start and end of bulk transfer to ensure deadline has not expired.
- Add deadline expiration checks in all places where rpc_priv timeout is initialized

Required-githooks: true

Signed-off-by: Alexander A Oganezov <[email protected]>
@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 20, 2024 •
edited
Loading

Ticket title is 'DAOS 2.4.2-4: Errored DAOS engine 0 exited unexpectedly on daos_user'
Status is 'Awaiting Verification'
Labels: 'ALCF,pre_acceptance_issues,scrubbed_2.8'
https://daosio.atlassian.net/browse/DAOS-16251

@frostedcmos
- additional fixes
5a651ce 
Required-githooks: true

Signed-off-by: Alexander A Oganezov <[email protected]>
@frostedcmos
style
e6e93e9 
Required-githooks: true

Signed-off-by: Alexander A Oganezov <[email protected]>
@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-14971/3/testReport/

@daosbuild1
Copy link
Collaborator

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

@frostedcmos
- update unittest
cd1a062 
Required-githooks: true

Signed-off-by: Alexander A Oganezov <[email protected]>
@frostedcmos
- retrigger and mark to allow next tests for debug
cd36584 
Allow-unstable-test: true

Required-githooks: true

Signed-off-by: Alexander A Oganezov <[email protected]>
@frostedcmos
retrigger
bfdb9ec 
Allow-unstable-test: true

Required-githooks: true

Signed-off-by: Alexander A Oganezov <[email protected]>
soumagne
soumagne reviewed Aug 21, 2024
View reviewed changes
src/cart/crt_bulk.c Outdated
if (crt_bulk_desc_expired(bulk_desc))
D_GOTO(out, rc = -DER_DEADLINE_EXPIRED);

D_ALLOC_PTR(verify_arg);
Copy link
Collaborator

Choose a reason for hiding this comment

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

you should avoid allocating anything here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, this is for now for experimentation, not happy with this alloc either, but cant think of cleaner way

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Incorporated verify_cb into the existing cb info structs, avoiding extra allocations on this path

soumagne
soumagne reviewed Aug 21, 2024
View reviewed changes
src/cart/crt_hg_proc.c Outdated
@@ -583,7 +583,7 @@ crt_proc_in_common(crt_proc_t proc, crt_rpc_input_t *data)
);
hdr->cch_dst_tag = rpc_priv->crp_pub.cr_ep.ep_tag;

hdr->cch_src_timeout = rpc_priv->crp_timeout_sec;
hdr->cch_src_deadline_sec = timeout_to_deadline(rpc_priv->crp_timeout_sec);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think in general we would want to use half of the timeout value, based on what Sean had mentioned in the past, ie. client timeout value is double the server timeout.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i am not sure using half is the right approach. i could see using "deadline - [small amount]" to account for the response delay, but for example, expiring a "60second timeout" rpc in 30 seconds (vs say 59 seconds on the receiver) would cause a lot of test issues and weird behaviors.

@frostedcmos
Skip nlt in hopes of debugging this further
93d2bdc 
Skip-nlt: true

Required-githooks: true

Signed-off-by: Alexander A Oganezov <[email protected]>
@daosbuild1
Copy link
Collaborator

Test stage Test RPMs on EL 8.6 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-14971/7/execution/node/1053/log

@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-14971/7/execution/node/1141/log

@frostedcmos
- bugfix
75f28a9 
Required-githooks: true

Signed-off-by: Alexander A Oganezov <[email protected]>
@frostedcmos
Skip-nlt: true
c7f6b57 
Required-githooks: true

Signed-off-by: Alexander A Oganezov <[email protected]>
@frostedcmos
Debug pr
ac169fd 
Skip-nlt: true

Required-githooks: true

Signed-off-by: Alexander A Oganezov <[email protected]>
@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-14971/10/execution/node/1104/log

@frostedcmos
- Add debug to trace deadline to timeout conversion and back
f79a883 
Required-githooks: true
Skip-nlt: true

Signed-off-by: Alexander A Oganezov <[email protected]>
@frostedcmos
- style
70163f8 
Skip-nlt: true

Required-githooks: true

Signed-off-by: Alexander A Oganezov <[email protected]>
@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-14971/12/execution/node/1105/log

@frostedcmos
- change clock for deadlines to be realtime
fd7786e 
Skin-nlt: true

Required-githooks: true

Signed-off-by: Alexander A Oganezov <[email protected]>
@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-14971/13/execution/node/1183/log

@frostedcmos
- experiment to avoid situation when deadline is calculated at the edge
6a5ac90 
of the second, causing rpc to expire prematurely.

Required-githooks: true

Signed-off-by: Alexander A Oganezov <[email protected]>
@frostedcmos
Skip-nlt: true
44a2b59 
Allow-unstable-test: true

Required-githooks: true

Signed-off-by: Alexander A Oganezov <[email protected]>
@frostedcmos
Merge branch 'master' into aaoganez/rpc-bulk-deadlines
f264640
@frostedcmos
DAOS-16251 cart: Deadlines
6047e82 
- add crp_deadline_sec to rpc_priv. rpc header is now populated from it,
instead of timeout being converted to deadline at the time of the encoding.

- deadline is now set at crt_req_send() time. for collective
calls, if deadline is already set prior to crt_req_send(), then
deadline is left alone. Otherwise a timeout is converted to the deadline.

This is done for children of collective rpcs to properly inherit the parents
deadline.

- colletive rpcs will also set local expiration time now based on deadline, instead
of previously incorrect behavior with each child having its own timeout starting from 'now'.

- 1 second test timeouts expanded to 3 seconds, to allow for room due to 1 sec expiration
granularity.

- fix: realtime clock is used now in bulk expiration handling similar to
rpc deadline handling

Required-githooks: true

Signed-off-by: Alexander A Oganezov <[email protected]>
@frostedcmos
Merge branch 'master' into aaoganez/rpc-bulk-deadlines
2b63894 
Required-githooks: true
@daosbuild1
Copy link
Collaborator

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

@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-14971/17/execution/node/1206/log

@frostedcmos
Merge branch 'master' into aaoganez/rpc-bulk-deadlines
7973d58
@frostedcmos
- cleanup
7ef9d46 
Required-githooks: true

Signed-off-by: Alexander A Oganezov <[email protected]>
@frostedcmos
- retrigger
252c0f4 
Required-githooks: true

Signed-off-by: Alexander A Oganezov <[email protected]>
@daosbuild1
Copy link
Collaborator

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

@frostedcmos frostedcmos requested a review from soumagne October 10, 2024 16:54
@frostedcmos frostedcmos marked this pull request as ready for review October 23, 2024 01:41
@frostedcmos frostedcmos requested review from a team as code owners October 23, 2024 01:41
@frostedcmos frostedcmos requested review from liw and jolivier23 October 23, 2024 01:41
@frostedcmos
- tread -DER_DEADLINE_EXPIRED same as DER_TIMEDOUT for some situations
172abcb 
Required-githooks: true

Signed-off-by: Alexander A Oganezov <[email protected]>
daltonbohning
daltonbohning reviewed Oct 23, 2024
View reviewed changes
Copy link
Contributor

@daltonbohning daltonbohning left a comment

Choose a reason for hiding this comment

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

ftest LGTM

@frostedcmos
Copy link
Contributor Author

ping reviewers

johannlombardi
johannlombardi previously approved these changes Nov 28, 2024
View reviewed changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daltonbohning daltonbohning daltonbohning left review comments

@johannlombardi johannlombardi johannlombardi left review comments

@soumagne soumagne Awaiting requested review from soumagne

@liw liw Awaiting requested review from liw

@jolivier23 jolivier23 Awaiting requested review from jolivier23

@gnailzenh gnailzenh Awaiting requested review from gnailzenh

@wangdi1 wangdi1 Awaiting requested review from wangdi1

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@frostedcmos @daosbuild1 @johannlombardi @soumagne @daltonbohning