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

qat,e2e: add heartbeat and auto-reset validation #1839

Merged
merged 2 commits into from
Oct 4, 2024
Merged

qat,e2e: add heartbeat and auto-reset validation #1839

merged 2 commits into from
Oct 4, 2024

Conversation

hj-johannes-lee
Copy link
Contributor

This is draft for suggesting an idea and getting opinions.!

@hj-johannes-lee hj-johannes-lee marked this pull request as draft September 17, 2024 11:21
@hj-johannes-lee hj-johannes-lee force-pushed the PR-2024-007 branch 2 times, most recently from 208171e to d1875c4 Compare September 17, 2024 19:47
@hj-johannes-lee
Copy link
Contributor Author

@tkatila @mythi pleaes give me your opinions on the first draft when you have time.!

@mythi
Copy link
Contributor

mythi commented Sep 17, 2024

@tkatila @mythi pleaes give me your opinions on the first draft when you have time.!

it's great to have a test case for this available! the implementation cannot rely on the Go code running on the same system that runs the plugin so some additional work is still needed. do you think it's useful to run the test as part of JustBeforeEach?

@tkatila
Copy link
Contributor

tkatila commented Sep 18, 2024 •
edited
Loading

I'd move the test code to a "Context" that specifically tests the autoreset functionality. With the SKIP/FOCUS feature we can then enable it when the e2e nodes are upgraded to a kernel with autoreset. At least the SPR node doesn't seem to support it now.
edit: It to Context.

@hj-johannes-lee
Copy link
Contributor Author

hj-johannes-lee commented Sep 19, 2024 •
edited
Loading

do you think it's useful to run the test as part of JustBeforeEach?

I at first thought to put in Context.It, but had no idea for putting a tag for that since others are all like [App: something].

Would it make sense to replace [App:noapp]?

@tkatila
Copy link
Contributor

tkatila commented Sep 19, 2024

Would it make sense to replace [App:noapp]?

I'd add a new Context for this specific test and run one App inside of it (e.g. openssl). You could add a new tag to it, like "[Functionality:auto_reset]". It doesn't necessary fit with the other Contexts or Its but I'd be fine with it.

@hj-johannes-lee
Copy link
Contributor Author

hj-johannes-lee commented Sep 19, 2024 •
edited
Loading

@tkatila I see, but i would try in already existing contexts with new It()s..
New context anyway needs the same process of creating configmap BeforeEach in existing contexts. so,, it somewhat does not make sense to me that we create a new context.
But, [Functionality: auto_reset] is a good idea.! Thanks!

run one App inside of it (e.g. openssl)

Umm, is this functionality affect running an app? I am confused.

@tkatila
Copy link
Contributor

tkatila commented Sep 19, 2024

run one App inside of it (e.g. openssl)

Umm, is this functionality affect running an app? I am confused.

Nevermind. My though was that instead of just testing for the error recovery, running a workload after the recovery would make sure that the device is actually recovered. Not just appearing to be.

@hj-johannes-lee
Copy link
Contributor Author

@tkatila Aha, that actually makes sense. Let me add it also.

@hj-johannes-lee hj-johannes-lee force-pushed the PR-2024-007 branch 4 times, most recently from 5d8c87e to 3506ae2 Compare September 19, 2024 19:06
@hj-johannes-lee hj-johannes-lee marked this pull request as ready for review September 19, 2024 19:45
mythi
mythi reviewed Sep 20, 2024
View reviewed changes
demo/qat-auto-reset/qat-auto-reset-test.sh Outdated Show resolved Hide resolved
demo/qat-auto-reset/qat-auto-reset-test.sh Outdated Show resolved Hide resolved
@hj-johannes-lee hj-johannes-lee marked this pull request as draft September 26, 2024 22:06
@hj-johannes-lee hj-johannes-lee force-pushed the PR-2024-007 branch 3 times, most recently from 06c717e to 76e18d7 Compare September 27, 2024 20:29
mythi
mythi requested changes Sep 30, 2024
View reviewed changes
test/e2e/qat/qatplugin_dpdk.go Outdated Show resolved Hide resolved
test/e2e/qat/qatplugin_dpdk.go Outdated Show resolved Hide resolved
.github/workflows/lib-e2e.yaml Outdated Show resolved Hide resolved
test/e2e/qat/qatplugin_dpdk.go Outdated Show resolved Hide resolved
@hj-johannes-lee hj-johannes-lee force-pushed the PR-2024-007 branch 2 times, most recently from 20f554b to 7e04d7e Compare September 30, 2024 19:46
@hj-johannes-lee hj-johannes-lee marked this pull request as ready for review September 30, 2024 22:59
@hj-johannes-lee
Copy link
Contributor Author

@mythi @tkatila May I ask if there is any reason why this is not getting merged...?

@mythi
Copy link
Contributor

mythi commented Oct 1, 2024

May I ask if there is any reason why this is not getting merged...?

it was only moved away from draft 15 hours ago and at least I haven't had the change to review it properly

mythi
mythi requested changes Oct 2, 2024
View reviewed changes
test/e2e/qat/qatplugin_dpdk.go Outdated Show resolved Hide resolved
test/e2e/qat/qatplugin_dpdk.go Outdated Show resolved Hide resolved
test/e2e/qat/qatplugin_dpdk.go Outdated Show resolved Hide resolved
test/e2e/qat/qatplugin_dpdk.go Outdated Show resolved Hide resolved
test/e2e/qat/qatplugin_dpdk.go Show resolved Hide resolved
@hj-johannes-lee hj-johannes-lee force-pushed the PR-2024-007 branch 2 times, most recently from 5ee6f7d to 96a6b6b Compare October 2, 2024 16:17
@hj-johannes-lee
Copy link
Contributor Author

hj-johannes-lee commented Oct 2, 2024 •
edited
Loading

@mythi @tkatila Thank you for your reviews.! I guess there have been quite many changes and improved a lot.!
I think it's at least much clearer and cleaner now. If there are still things that should be improved, please comment!

tkatila
tkatila reviewed Oct 3, 2024
View reviewed changes
Copy link
Contributor

@tkatila tkatila left a comment

Choose a reason for hiding this comment

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

Great job! Code looks much better now.

test/e2e/qat/qatplugin_dpdk.go Show resolved Hide resolved
test/e2e/qat/qatplugin_dpdk.go Outdated Show resolved Hide resolved
test/e2e/qat/qatplugin_dpdk.go Outdated Show resolved Hide resolved
@hj-johannes-lee
qat,e2e: add heartbeat and auto-reset validations
3b47d27 
Signed-off-by: Hyeongju Johannes Lee <[email protected]>
@hj-johannes-lee
ci, e2e: skip qat e2e auto reset validation & make targetjob clear
7d3f3f3 
The validation requires its runner to have specific kernel config.
Until it is set, skip the e2e validation.

When Resource:(cy/dc) is present in FOCUS, Functionality tests are
automatically skipped. To make it look clear, remove it, and add
Functionality in SKIP.

Signed-off-by: Hyeongju Johannes Lee <[email protected]>
mythi
mythi approved these changes Oct 3, 2024
View reviewed changes
Copy link
Contributor

@mythi mythi left a comment

Choose a reason for hiding this comment

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

LGTM.

@tkatila tkatila merged commit 53efaae into intel:main Oct 4, 2024
75 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eero-t eero-t eero-t left review comments

@tkatila tkatila tkatila approved these changes

@mythi mythi mythi approved these changes

@bart0sh bart0sh Awaiting requested review from bart0sh bart0sh is a code owner

@ozhuraki ozhuraki Awaiting requested review from ozhuraki ozhuraki is a code owner

@kad kad Awaiting requested review from kad kad is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hj-johannes-lee @mythi @tkatila @eero-t