-
Notifications
You must be signed in to change notification settings - Fork 4.8k
TRT-2292: Add support for test isolation #30478
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: main
Are you sure you want to change the base?
Conversation
|
@xueqzhan: This pull request references TRT-2292 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@xueqzhan: This pull request references TRT-2292 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: xueqzhan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
stbenjam
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.
Looks really good, just a couple comments
109ff7f to
745cf1d
Compare
|
Scheduling required tests: |
|
Scheduling required tests: |
|
/retest-required |
|
Job Failure Risk Analysis for sha: df32893
|
|
/retest-required |
pkg/test/ginkgo/test_suite.go
Outdated
| // isolation defines conflict groups, mode, taints, and tolerations for test isolation | ||
| isolation extensiontests.Isolation |
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.
Do we need to copy this or could we just access it via spec?
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.
Doesn't seem that we are copying spec in extensionTestSpecsToOriginTestCases. I can and use the spec instead if that is preferred.
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.
We definitely should, we have a field for it. Retry copies it -- looks like maybe an oversight that it wasn't copied in the original creation of them
| // getTestConflictGroup returns the conflict group for a test. | ||
| // Conflicts are only checked within the same conflict group. | ||
| func getTestConflictGroup(test *testCase) string { | ||
| return "default" |
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 needs to get replaced from the content in the spec right?
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.
spec doesn't define group. Group is designed to support mode.
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.
Sorry I'm not quite understanding what this function does. Isolation has two fields relevant, Conflicts and Mode. I think we agreed to only support mode=exec for now, but shouldn't we get the Conflicts out of the Isolation struct?
Why does this always return default?
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.
The idea was to have a framework to support mode in the future. So conflict is only checked within a conflictGroup. Right now all tests belong to default group and therefore work like mode=exec. But just in case another mode is needed, more conflictGroup will be created for that purpose.
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.
Is there a reason not to call it mode here instead of conflict group, and use the name we're implementing ("exec")? It's not clear to me "group" is linked to the "mode"
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.
Mode is not clear to indicate a grouping mechanism. But conflictGroup is clear about its functionality of grouping. But configGroup can be used to implement mode. So I think conflictGroup is still better.
This PR adds a testScheduler that
If taint/toleration function looks ok, I will add the structure in the extension library and adjust here accordingly.
/hold for review and further test