OpenShift Tests Extension Framework Initial #1676
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
| ##### OpenShift Payload Extension Binaries | ||
| For OpenShift payload components contributors can advertise the existence of an extension binary | ||
| by adding information (the imagestream tag for the OCP payload component and the path to the binary | ||
| within their image) to a simple registry datastructure in github.com/openshift/origin. |
There was a problem hiding this comment.
Alternative: could we store this info in the release image? (The kind that shows up in oc adm release info -ojson).
This would let us register things more dynamically, and also supply more metadata like which suites it has. Maybe get rid of the info subcommand.
There was a problem hiding this comment.
maybe; would require updates to oc adm release new
sosiouxme
left a comment
sosiouxme
left a comment
There was a problem hiding this comment.
mostly typos, maybe a few bits of substance
dgoodwin
left a comment
dgoodwin
left a comment
There was a problem hiding this comment.
Just a few thoughts, looks quite exciting.
|
|
||
| Component authors may choose to reduce the number of tests run for non-default | ||
| configuration profiles, focusing only on tests likeliest to fail based on the | ||
| configuration change, in order to reduce overall execution time. |
There was a problem hiding this comment.
Could we get an example on this one, I couldn't come up with a use case immediately.
There was a problem hiding this comment.
Let's say you want to test a more verbose logging level configuration option in your CRD. You may have hundreds of tests that fully exercise your component in the 'default' configuration, but it is overkill to run them all again simply to verify that debug log statements are being emitted by your pod when verbose logging is enabled.
So you expose an extension configuration for the verbose logging level, and output only one test when asked for a list in that configuration. All that test does is read the component's pod logs and makes sure that it sees debug output.
If you expose a configuration that disables http/1, you might just want to run a test that verifies an http/1 connection is rejected and an http/2 connection is accepted. If you want to test branding, you might just want to verify that the HTML you scrape contains the newly configured name. If you expose a threshold, you might just want to test that a single expected alert is firing after configuring it.
I can add these to the doc if you buy the premise.
| # If applying the configuration implies a disruption, inform | ||
| # openshift-tests, so that it can be accounted for in overall | ||
| # disruption reporting. | ||
| "disruption": "1m", |
There was a problem hiding this comment.
This makes me a little nervous if it's happening in other repos and we didn't have good visibility into people abusing this because a problem popped up. Probably not a core concern for this enhancement though.
| # testing logic. This allows component readiness | ||
| # to display the human-readable version of the test | ||
| # name while considering test runs across name changes. | ||
| "originalName": "security version compliance", |
There was a problem hiding this comment.
I really like baking this into the component repos, might get us more renames as very very few people go to the mapping repo.
It would be neat if something could comment on PRs where we see added+removed tests that "if this was a rename, please do ...". That would be relatively often a rename. Problem for another day though.
| # before the test was run. | ||
| "component": "default", | ||
| } | ||
| }, |
There was a problem hiding this comment.
Environment is a little unusual in the results for every test, that seems like mostly a characteristic of a job and a lot of duplication?
There was a problem hiding this comment.
There are fixed aspects of the environment, like GCP vs AWS, but the enhancement suggests that environment include component configuration information -- a single job being able to apply and test multiple different configuration options. Imagine an operator being able to cycle through several of its typical configurations, running the same tests or tests specific to those configurations, during the execution of a single job. That configuration is relevant to the outcome of a test and Component Readiness must be able to differentiate the same test name running in one configuration vs another.
A next question would be why we would store those static environmental aspects in the aggregated results file alongside each test. My hope there is that the results files can begin to stand alone. You can just push the file content into a database and you know everything you need to know from the resulting DB. You don't need parse prowjob job names, for example, to derive additional context about how the test was run. Many tools can ingest a comprehensive file like this directly, so our options for analysis expand. Imagine wanting to move to a new database or use local tooling to analyze the data. With a comprehensive file format, we just ingest the file into our target analysis tool -- no custom logic like what we have in the cloud function required to pull bits and pieces from multiple artifacts.
| Note that `run-test` will blindly execute tests in the list as quickly as possible, | ||
| in parallel, without consideration for system resources or parallelism constraints |
There was a problem hiding this comment.
Just a note, OTE doesn't do parallel execution yet. There's some oddities about how we invoke ginkgo tests today. I think I just haven't found all the things I need mutexes for yet.
I assume that's why origin shells out to execute every test
There was a problem hiding this comment.
You can resolve this one, OTE executes in parallel. Ginkgo has a mutex to force serial execution but other frameworks would be parallelized.
jupierce
force-pushed
the
openshift-tests-extension
branch
3 times, most recently
from
cdb1d75 to
ac860f4
Compare
stbenjam
left a comment
stbenjam
left a comment
There was a problem hiding this comment.
Just a couple of comments. The changes to details are good
| # If a test name is updated at any time in the future, | ||
| # originalName must report the original name of the | ||
| # testing logic. This allows component readiness | ||
| # to display the human-readable version of the test | ||
| # name while considering test runs across name changes. | ||
| "originalName": "security version compliance", |
There was a problem hiding this comment.
In the current version I have this as otherNames so we could have the full history of a test's name.
originalName could work, but it must be included in the ExtensionTestResult, and have this data make its way to a column in the junit table. We'd then need to update ci-test-mapping to look at this column when considering the test ID, which should work fine for both the old way (a rename map in the ci-test-mapping repo) and the extension way (stable original name).
I wouldn't expect anyone including component readiness to group by otherNames (or even originalName), but rather on the test ID from a join on the ci-test-mapping table. We need to be backwards compatible with the universe today, and previous openshift releases without extension test binaries, which means continuing to use the mapping table.
jupierce
force-pushed
the
openshift-tests-extension
branch
from
76ccbf2 to
cc85139
Compare
|
Inactive enhancement proposals go stale after 28d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Mark the proposal as fresh by commenting If this proposal is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
|
Stale enhancement proposals rot after 7d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Mark the proposal as fresh by commenting If this proposal is safe to close now please do so with /lifecycle rotten |
openshift-ci
bot
added
lifecycle/rotten
|
/remove-lifecycle rotten |
openshift-ci
bot
removed
the
lifecycle/rotten
|
@jupierce: The DetailsIn 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 kubernetes-sigs/prow repository. |
| // omitting an option for --component, unless instructed otherwise, will "just | ||
| // work". | ||
| componentRegistry := e.NewRegistry() | ||
| componentRegistry.Register(componentRegistry) |
There was a problem hiding this comment.
@jupierce is it componentRegistry.Register(componentExtension)? thanks
There was a problem hiding this comment.
It is! Nice catch.
I'll correct this, but for implementers, better information on the state of the API can be found here:
https://docs.google.com/document/d/1cFZj9QdzW8hbHc3H0Nce-2xrJMtpDJrwAse9H7hLiWk/edit?tab=t.0#heading=h.66y4kqbj468a is the source for details on the API. https://github.com/openshift-eng/openshift-tests-extension/blob/main/cmd/example-tests/main.go provides an example.
|
Stale enhancement proposals rot after 7d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Mark the proposal as fresh by commenting If this proposal is safe to close now please do so with /lifecycle rotten |
openshift-ci
bot
added
lifecycle/rotten
|
/remove-lifecycle rotten |
|
Rotten enhancement proposals close after 7d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Reopen the proposal by commenting /close |
|
@openshift-bot: Closed this PR. DetailsIn 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 kubernetes-sigs/prow repository. |
jupierce
force-pushed
the
openshift-tests-extension
branch
from
c2c0d43 to
7956d27
Compare
openshift-merge-robot
added
the
needs-rebase
jupierce
force-pushed
the
openshift-tests-extension
branch
from
7956d27 to
a4f0b97
Compare
openshift-merge-robot
removed
the
needs-rebase
jupierce
removed
the
lifecycle/rotten
jupierce
force-pushed
the
openshift-tests-extension
branch
3 times, most recently
from
e2df6ea to
7993a48
Compare
jupierce
force-pushed
the
openshift-tests-extension
branch
from
7993a48 to
a64a410
Compare
|
@jupierce: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/lgtm |
|
/approve |
openshift-ci
bot
added
the
approved
|
huh. I wonder how and why this repo's config doesn't do the normal self approver thing /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, jupierce The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
12 participants
No description provided.